---
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_