217 lines
13 KiB
Markdown
217 lines
13 KiB
Markdown
---
|
|
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_
|