From 7c4fca92ecbfa4c0bde7ca8846b319d839b04c99 Mon Sep 17 00:00:00 2001
From: Jakub Zych <jakub@keios.eu>
Date: Tue, 6 Oct 2026 19:28:21 +0200
Subject: [PATCH] docs(15): add code review report

---
 .../phases/15-journal-plugin/15-REVIEW.md     | 216 ++++++++++++++++++
 1 file changed, 216 insertions(+)
 create mode 100644 .planning/phases/15-journal-plugin/15-REVIEW.md

diff --git a/.planning/phases/15-journal-plugin/15-REVIEW.md b/.planning/phases/15-journal-plugin/15-REVIEW.md
new file mode 100644
index 0000000..b2c1dab
--- /dev/null
+++ b/.planning/phases/15-journal-plugin/15-REVIEW.md
@@ -0,0 +1,216 @@
+---
+phase: 15-journal-plugin
+reviewed: 2026-10-06T17:27:26Z
+depth: standard
+files_reviewed: 62
+files_reviewed_list:
+  - ../sm-grzybyfunkcjonalne-app/.gitmodules
+  - ../sm-grzybyfunkcjonalne-app/boot_test.go
+  - ../sm-grzybyfunkcjonalne-app/config/http.yaml
+  - ../sm-grzybyfunkcjonalne-app/go.mod
+  - ../sm-grzybyfunkcjonalne-app/go.work
+  - ../sm-grzybyfunkcjonalne-app/plugins.gen.go
+  - ../sm-grzybyfunkcjonalne-app/summer.yaml
+  - ../sm-journal-plugin/README.md
+  - ../sm-journal-plugin/admin.go
+  - ../sm-journal-plugin/admin_harness_test.go
+  - ../sm-journal-plugin/admin_navigation.go
+  - ../sm-journal-plugin/admin_permissions.go
+  - ../sm-journal-plugin/assets/images/journal-icon.svg
+  - ../sm-journal-plugin/classes/format_html.go
+  - ../sm-journal-plugin/classes/format_html_test.go
+  - ../sm-journal-plugin/console/columns.go
+  - ../sm-journal-plugin/console/export_posts.go
+  - ../sm-journal-plugin/console/import_posts.go
+  - ../sm-journal-plugin/controllers/api/auth.go
+  - ../sm-journal-plugin/controllers/api/media.go
+  - ../sm-journal-plugin/controllers/api/media_test.go
+  - ../sm-journal-plugin/controllers/api/posts.go
+  - ../sm-journal-plugin/controllers/api/posts_helpers.go
+  - ../sm-journal-plugin/controllers/api/posts_search.go
+  - ../sm-journal-plugin/controllers/api/posts_test.go
+  - ../sm-journal-plugin/controllers/api/rss.go
+  - ../sm-journal-plugin/controllers/categories.go
+  - ../sm-journal-plugin/controllers/posts.go
+  - ../sm-journal-plugin/controllers/tags.go
+  - ../sm-journal-plugin/go.mod
+  - ../sm-journal-plugin/integration_test.go
+  - ../sm-journal-plugin/journal_api_matrix_test.go
+  - ../sm-journal-plugin/journal_api_task3_test.go
+  - ../sm-journal-plugin/journal_api_writes_test.go
+  - ../sm-journal-plugin/journal_public_list_smoke_test.go
+  - ../sm-journal-plugin/lang/en/lang.yaml
+  - ../sm-journal-plugin/lang/pl/lang.yaml
+  - ../sm-journal-plugin/models/category.go
+  - ../sm-journal-plugin/models/fillable_test.go
+  - ../sm-journal-plugin/models/post.go
+  - ../sm-journal-plugin/models/post/fields.yaml
+  - ../sm-journal-plugin/models/post_search.go
+  - ../sm-journal-plugin/models/post_translatable_smoke_test.go
+  - ../sm-journal-plugin/models/search_gate.go
+  - ../sm-journal-plugin/models/settings.go
+  - ../sm-journal-plugin/models/tag.go
+  - ../sm-journal-plugin/models/translatable_test.go
+  - ../sm-journal-plugin/plugin.go
+  - ../sm-journal-plugin/plugin_test.go
+  - ../sm-journal-plugin/posts_admin_smoke_test.go
+  - ../sm-journal-plugin/routes.go
+  - ../sm-journal-plugin/search.go
+  - ../sm-journal-plugin/search_test.go
+  - ../sm-journal-plugin/updates/202610060001_create_golem15_journal_posts.go
+  - ../sm-journal-plugin/updates/202610060002_create_golem15_journal_categories.go
+  - ../sm-journal-plugin/updates/202610060003_create_golem15_journal_tags.go
+  - ../sm-journal-plugin/updates/202610060004_create_golem15_journal_posts_categories.go
+  - ../sm-journal-plugin/updates/202610060005_create_golem15_journal_posts_tags.go
+  - ../sm-journal-plugin/updates/202610060006_create_golem15_journal_settings.go
+  - ../sm-journal-plugin/updates/202610060007_add_author_slug_to_backend_users.go
+  - ../sm-journal-plugin/updates/migrations_test.go
+  - ../sm-journal-plugin/updates/postgres_test.go
+  - scripts/check-phase15.sh
+findings:
+  critical: 2
+  warning: 6
+  info: 4
+  total: 12
+status: 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)`:
+
+```go
+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`:
+
+```go
+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 `Count`s 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:
+
+```go
+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` `Create`s 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 `NewWriter`s 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_