13 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 15-journal-plugin | 2026-10-06T17:27:26Z | standard | 62 |
|
|
issues_found |
Phase 15: Code Review Report
Reviewed: 2026-10-06T17:27:26Z Depth: standard Files Reviewed: 62 Status: issues_found
Summary
Standard-depth review of Phase 15 Journal plugin sources from 15-01 through 15-04 SUMMARY key-files. Production code lives in sm-journal-plugin (HEAD d09edde) and the sm-grzybyfunkcjonalne-app proof host, plus scripts/check-phase15.sh. Planning artifacts (15-SECURITY-REVIEW.md, 15-VALIDATION.md) were excluded.
Public GET auth, JOURNAL-005 draft 404, backend JWT writes, fillable allow-lists, parameterized search ILIKE, and media folder confinement under /journal/ are sound. Two ship-blocking issues remain: the Posts toolbar import/export actions write and read arbitrary filesystem paths from client JSON, and translations.content_html bypasses FormatHTML, defeating the XSS gate that main content goes through.
Narrative Findings (AI reviewer)
Critical Issues
CR-01: Admin toolbar import/export accepts unsandboxed filesystem paths
File: ../sm-journal-plugin/console/import_posts.go:340-357
Issue: IOPayloadPath copies the client JSON path field unchanged. Posts admin actions runExportPosts / runImportPosts pass that string into ExportToDir / ImportFromDir, which call os.MkdirAll, os.WriteFile, os.Stat, and os.ReadFile with no root confinement, allow-list, or .. rejection. Export also does filepath.Join(dir, slug+".json"); admin slugs are not constrained by slugPattern, so a slug such as foo/../../../tmp/x is cleaned by filepath.Join to a path outside the intended directory. Any backend principal with golem15.journal.access_import_export can therefore read arbitrary *.json trees and write JSON (and create directories) as the server user. CLI --path is operator-local; the same helper on an HTTP admin action is path traversal.
Fix: Confine toolbar paths to a storage subdirectory (for example storage/journal-export), reject absolute paths and .. after filepath.Clean, and sanitize export filenames to filepath.Base(slug):
func confinedExportDir(raw, fallback string) (string, error) {
root, err := filepath.Abs("storage/app")
if err != nil {
return "", err
}
candidate := filepath.Clean(filepath.Join(root, fallback))
if strings.TrimSpace(raw) != "" {
candidate = filepath.Clean(filepath.Join(root, raw))
}
rel, err := filepath.Rel(root, candidate)
if err != nil || strings.HasPrefix(rel, "..") {
return "", fmt.Errorf("journal: export path is outside storage")
}
return candidate, nil
}
Do not pass the raw payload path to os.WriteFile. Keep unrestricted --path on the CLI command only.
CR-02: translations.content_html bypasses FormatHTML (stored XSS)
File: ../sm-journal-plugin/controllers/api/posts_helpers.go:762-774
Issue: Main content is always passed through classes.FormatHTML before persist (store/update and admin preparePostWrite). syncTranslations does that only when content is non-empty and content_html is empty; if the writer sends translations.pl.content_html, the value is stored via translate.SetTranslated with no rejectUnsafe gate. Console import syncTranslations (console/import_posts.go:309-330) writes content_html from JSON as-is. Public GET /_journal/api/v1/posts/{slug} returns those attributes in translations (translationsOf). That defeats the JOURNAL-003/004 control: an editor (or anyone who can import) can persist <script>, event handlers, or javascript: URLs that the markdown path would reject, and the public API will serve them to readers.
Fix: Never persist client-supplied content_html. Derive it with FormatHTML from translated content, or run rejectUnsafe on any HTML before SetTranslated:
if content := asString(attrs["content"]); strings.TrimSpace(content) != "" {
html, err := classes.FormatHTML(content)
if err != nil {
return err
}
attrs["content_html"] = html
} else {
delete(attrs, "content_html")
}
Apply the same rule in console.syncTranslations.
Warnings
WR-01: rss_enabled is ignored; disabling RSS still serves the feed
File: ../sm-journal-plugin/controllers/api/rss.go:24-38
Issue: Settings load RSSEnabled, and lang/en/lang.yaml says a disabled feed returns 404. RSS never reads settings.RSSEnabled. After First fails it even defaults RSSEnabled: true and then proceeds to list published posts. An administrator who turns the feed off still exposes titles, excerpts, and (if rss_include_content) full HTML at GET /_journal/api/v1/rss.
Fix: Return 404 when !settings.RSSEnabled after a successful settings load (keep the well-formed empty document only when the settings row is missing, if that remains D-14).
WR-02: Typesense search double-paginates and reports the wrong total
File: ../sm-journal-plugin/controllers/api/posts.go:79-107
Issue: searchWithEngine already asks Typesense for Page/PerPage and returns that page's IDs. Index then Counts those IDs (so total is at most the page size) and applies Offset((page-1)*perPage).Limit(perPage) again. Page 1 looks short; page 2 of a Typesense-backed search returns an empty list. Default search_use_typesense is off, so this is latent until the setting is enabled.
Fix: When the engine is used, take total from the Typesense found count and do not apply a second SQL offset. Filter id IN ? with the engine IDs and Limit(perPage) only (offset 0), or request an unpaged ID set from the engine and paginate in SQL once.
WR-03: Public search trusts a stale process-global gate instead of settings
File: ../sm-journal-plugin/controllers/api/posts_search.go:14-27
Issue: searchWithEngine returns immediately when models.SearchGateEnabled() is false. That atomic is written from the beachcomber index gate (search.go:19-22), not from this request. The function then loads golem15_journal_settings for weights but never inspects settings.SearchUseTypesense. After boot, search stays on SQL until some save runs the gate; after an admin turns Typesense off, search can keep hitting Typesense until the next index callback.
Fix: Decide from the settings row (or call the same gate function) on every search, matching ShouldBeSearchable's fail-closed default:
var settings models.Settings
if err := db.First(&settings, 1).Error; err != nil || !settings.SearchUseTypesense {
return nil, false
}
WR-04: Featured-image write routes skip access_posts
File: ../sm-journal-plugin/controllers/api/posts.go:322-342
Issue: Store/Update/Destroy call requireAccessPosts then CanEdit. UploadFeaturedImage and DeleteFeaturedImage only require a backend JWT and post.CanEdit. CanEdit is true for the owner or access_other_posts, with no access_posts check. A backend user who lost journal access, or who holds access_other_posts without access_posts, can still attach or delete public featured images.
Fix: Call requireAccessPosts (and keep CanEdit) in both featured-image handlers, matching TestJournalFeaturedImageUnauthenticated's permission story for authenticated non-editors.
WR-05: Post create/update persists the row before associations
File: ../sm-journal-plugin/controllers/api/posts.go:221-228
Issue: Store Creates the post, then applyPostAssocs writes categories, tags, and translations. If assoc writes fail, the handler returns 500 with an orphaned post (and tags resolveTagIDs may already have created). Update has the same split. Import CLI wraps work in a transaction; the HTTP API does not.
Fix: Wrap Create/Save + applyPostAssocs in db.WithContext(r.Context()).Transaction(...).
WR-06: uniqueMediaKey overwrites the original object after 999 collisions
File: ../sm-journal-plugin/controllers/api/media.go:122-132
Issue: If cover.png through cover-999.png already exist, the loop returns the original pathName. The handler then NewWriters that key and overwrites the existing blob. Unlikely, but it is silent data loss on a public media path.
Fix: Return an error (422/500) when no free key is found; do not reuse pathName.
Info
IN-01: FormatHTML XSS tests do not require rejection
File: ../sm-journal-plugin/classes/format_html_test.go:46-55
Issue: Cases pass when err == nil as long as the needle is absent from the output. Goldmark's default HTML escaping can hide a dead rejectUnsafe gate. The comment claims script/iframe/javascript must fail the reject gate; the assertion does not require err != nil.
Fix: For the unsafe cases, if err == nil { t.Fatalf(...) }.
IN-02: TestMediaFolderPatternRejectsDotDot cannot fail if ../ is allowed
File: ../sm-journal-plugin/controllers/api/media_test.go:24-28
Issue: The fatal condition is MatchString(folder) && !strings.Contains(folder, ".."). For fixture ../etc, Contains("..") is true, so a pattern that incorrectly matches still does not fail the test.
Fix: if mediaFolderPattern.MatchString("../etc") { t.Fatal(...) }.
IN-03: Phrasebook still documents PHP artisan scout import
File: ../sm-journal-plugin/lang/en/lang.yaml:31
Issue: search_use_typesense_comment tells operators to run php artisan scout:import "Golem15\Journal\Models\Post". That command does not exist on the Go host and will confuse operators when they enable Typesense.
Fix: Replace with the SummerCMS reindex command once it exists, or drop the artisan sentence.
IN-04: RSS channel links fall back to the request Host header
File: ../sm-journal-plugin/controllers/api/rss.go:122-133
Issue: When config key url is empty, requestOrigin uses r.Host (and TLS on the immediate connection). A Host-header attacker can mint feed <link>/<guid> values pointing at another origin. Harmless if every host sets url; the proof-host path should keep that key set.
Fix: Require url (or a dedicated rss.site_url) and refuse to emit item links without it.
Reviewed: 2026-10-06T17:27:26Z Reviewer: the agent (gsd-code-reviewer) Depth: standard