docs(04): add code review report
This commit is contained in:
Jakub Zych
139
.planning/phases/04-cli-scaffolding-i18n-and-mail/04-REVIEW.md
Normal file
139
.planning/phases/04-cli-scaffolding-i18n-and-mail/04-REVIEW.md
Normal file
@@ -0,0 +1,139 @@
|
|||||||
|
---
|
||||||
|
status: issues
|
||||||
|
phase: 04-cli-scaffolding-i18n-and-mail
|
||||||
|
depth: standard
|
||||||
|
files_reviewed: 36
|
||||||
|
files_reviewed_list:
|
||||||
|
- bonfire/command.go
|
||||||
|
- bonfire/root.go
|
||||||
|
- cmd/summer/main.go
|
||||||
|
- cmd/summer/main_test.go
|
||||||
|
- examples/hello/config/mail.yaml
|
||||||
|
- examples/hello/hello_test.go
|
||||||
|
- examples/hello/plugins/base/lang/en/lang.yaml
|
||||||
|
- examples/hello/plugins/base/lang/pl/lang.yaml
|
||||||
|
- examples/hello/plugins/base/plugin.go
|
||||||
|
- examples/hello/plugins/base/views/mail/hello-en.htm
|
||||||
|
- examples/hello/plugins/base/views/mail/hello.htm
|
||||||
|
- examples/hello/plugins/base/views/mail/layouts/hello.htm
|
||||||
|
- internal/build/artifact.go
|
||||||
|
- internal/build/build.go
|
||||||
|
- internal/build/build_test.go
|
||||||
|
- internal/build/leaf.go
|
||||||
|
- internal/build/registry.go
|
||||||
|
- internal/build/scaffold.go
|
||||||
|
- internal/build/stubs/artifacts.tmpl
|
||||||
|
- internal/build/stubs/plugin.tmpl
|
||||||
|
- internal/build/stubs/registry.tmpl
|
||||||
|
- pact/capabilities.go
|
||||||
|
- party/registry.go
|
||||||
|
- party/registry_test.go
|
||||||
|
- phrasebook/loader.go
|
||||||
|
- phrasebook/translator.go
|
||||||
|
- phrasebook/translator_test.go
|
||||||
|
- postcard/assets/default.htm
|
||||||
|
- postcard/drivers.go
|
||||||
|
- postcard/mailer.go
|
||||||
|
- postcard/mailer_test.go
|
||||||
|
- postcard/mailpit_test.go
|
||||||
|
- postcard/smtp_test.go
|
||||||
|
- postcard/templates.go
|
||||||
|
- postcard/templates_test.go
|
||||||
|
- scripts/check-phase4.sh
|
||||||
|
findings:
|
||||||
|
critical: 0
|
||||||
|
warning: 4
|
||||||
|
info: 3
|
||||||
|
total: 7
|
||||||
|
reviewed: 2026-09-18T12:15:00Z
|
||||||
|
---
|
||||||
|
|
||||||
|
# Phase 4: Code Review Report
|
||||||
|
|
||||||
|
**Reviewed:** 2026-09-18T12:15:00Z
|
||||||
|
**Depth:** standard
|
||||||
|
**Files Reviewed:** 36
|
||||||
|
**Status:** issues
|
||||||
|
|
||||||
|
## Summary
|
||||||
|
|
||||||
|
Scaffolding, phrasebook, and postcard are a real boot path: `make:*` writes Winter-shaped leaves and a temp+rename `registry.gen.go`, `summer build` runs a models-leaf import check before `go build`, `party.Activate` publishes a translator and mailer then fails Boot on missing mail files, and Send renders Markdown through `html/template` + goldmark (unsafe HTML off) with CR/LF header rejection and mandatory TLS unless `none` is set. String Vars cannot drop raw `<script>` into HTML; mail dotted names reject `..` / `/` / `\`; lang paths must be exactly `lang/<locale>/<group>.yaml`; SMTP dial-fail tests do not echo `secretpass`.
|
||||||
|
|
||||||
|
The defects that matter are enforcement holes, not missing features. D-11 is prefix-matching and non-recursive, and it silently skips plugins whose replace path is outside the app root. Concurrent `make:*` can drop registry entries. Locale catalog keys keep the directory spelling (`pt_BR` vs `pt-BR`). Mail still executes caller `Vars` as `html/template` data, so a `template.HTML` value bypasses the escaping the tests cover, and the regex post-check does not catch `onclick=` or `<img onerror>`.
|
||||||
|
|
||||||
|
No path-traversal read of host files was found on the Send or Get paths (lookups are in-memory after boot). `scripts/check-phase4.sh` does not leak a background process.
|
||||||
|
|
||||||
|
## Warnings
|
||||||
|
|
||||||
|
### WR-01: Models leaf check is bypassable
|
||||||
|
|
||||||
|
**File:** `internal/build/leaf.go:22-80`
|
||||||
|
**Issue:** D-11 requires `summer build` to fail when `models/` imports a sibling package. `inspectModelsImports` only reads immediate `models/*.go` (subdirectories are skipped), only compares the import string to `module/{classes,controllers,console,jobs,middleware,updates}`, and `checkModelsLeaf` `continue`s when `localModuleDir` is outside the app root. Three bypasses:
|
||||||
|
|
||||||
|
1. `models/internal/hook.go` (or any nested package) importing `module/classes` is never parsed.
|
||||||
|
2. `import "alias.example/classes"` plus `replace alias.example/classes => ./classes` does not match the module-prefix test.
|
||||||
|
3. A plugin whose `go.mod` replace is an absolute path outside `appDir` is not checked at all.
|
||||||
|
|
||||||
|
The happy-path test writes `models/bad.go` under a plugin inside the copied hello app, so none of these are covered.
|
||||||
|
|
||||||
|
**Fix:** Walk `models/` recursively for `.go` files; resolve each import against the plugin directory (or reject any import whose cleaned replace target is a sibling leaf); return an error instead of `continue` when a manifest plugin cannot be located inside the app root.
|
||||||
|
|
||||||
|
```go
|
||||||
|
if !underRoot(appDir, pluginDir) {
|
||||||
|
return fmt.Errorf("build: plugin %s: models leaf check skipped; %s is outside the app root", p.ID, pluginDir)
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
### WR-02: Concurrent `make:*` can lose registry entries
|
||||||
|
|
||||||
|
**File:** `internal/build/registry.go:80-111`, `internal/build/artifact.go:467-477,510-538`, `internal/build/scaffold.go:249-256`
|
||||||
|
**Issue:** `writeRegistry` is atomic for one writer (temp file + `Rename`). The surrounding protocol is not. `rejectDuplicateFile` / `nextMigrationFile` are stat-then-write; `refreshRegistry` scans then writes with no file lock. Two overlapping `make:model` / `make:migration` calls can both pass the existence check, both write artifacts, then the earlier scan can rename a registry that omits the later file. `go mod tidy` on the same `go.mod` races the same way. Agent-driven parallel `make:*` is in scope for this repo's loop.
|
||||||
|
|
||||||
|
**Fix:** Hold an exclusive flock on `registry.gen.go` (or a `make.lock` in the plugin dir) from duplicate checks through `refreshRegistry` + tidy. Open artifact files with `O_CREATE|O_EXCL`. After the lock, rescan immediately before writing the registry.
|
||||||
|
|
||||||
|
### WR-03: Locale catalog keys are not canonicalized
|
||||||
|
|
||||||
|
**File:** `phrasebook/loader.go:243-263`, `phrasebook/translator.go:189-214`
|
||||||
|
**Issue:** Load stores messages under the directory name (`pt_BR`, `en-us`). `fallbackChain` adds the requested string, then `language.Parse(requested).String()` (typically `pt-BR` / `en-US`) and parents, then the configured fallback. D-04's `pl-PL` → `pl` case works because files live under `pl`. A Winter/Laravel `lang/pt_BR/` tree is not found for a `pt-BR` request (and the reverse misses if files use hyphens and the request uses underscores). Map lookup is also case-sensitive (`en-us` vs `en-US`). Missing keys then return the raw key instead of the translation that was loaded.
|
||||||
|
|
||||||
|
The hyphen parent walk itself is correct; the catalog key is what never matches.
|
||||||
|
|
||||||
|
**Fix:** On Load, key the catalog with a canonical form (`strings.ReplaceAll(locale, "_", "-")` then `language.Parse` / `tag.String()`, plus the original spelling). Use the same function for `GetIn` / `ChoiceIn` so request, parent, and file locale collapse to one chain.
|
||||||
|
|
||||||
|
### WR-04: Caller `Vars` can bypass HTML escaping; `validateHTML` is not a real sanitizer
|
||||||
|
|
||||||
|
**File:** `postcard/templates.go:336-346,367-377,399-409`, `postcard/mailer.go:16-23`
|
||||||
|
**Issue:** Plan T-04-08 / D-07: goldmark stays safe-mode and trusted `template.HTML` is only for layout-injected, already-rendered content. `render` passes `msg.Vars` (`map[string]any`) straight into `execHTML` for subject and Markdown body. A plugin that puts `template.HTML`, `template.URL`, or `template.JS` in `Vars` skips escaping. Layout then injects the Markdown HTML as `template.HTML`.
|
||||||
|
|
||||||
|
The regex net does not close that hole: `rawUnsafeTag` ignores `<img>` / `<svg>`; `eventHandler` requires whitespace around `=` so `onclick=alert(1)` and `<svg/onload=…>` do not match. String Vars are covered by tests; typed HTML is not. Collection invitations and other user-authored fields will flow through this API.
|
||||||
|
|
||||||
|
**Fix:** Coerce `Vars` to strings (or reject `html/template` typed values) before `execHTML`. Keep `template.HTML` / `template.CSS` only on the layout data built inside `render`. Replace the regex with a real HTML policy (or drop the claim that `validateHTML` is the safety net).
|
||||||
|
|
||||||
|
## Info
|
||||||
|
|
||||||
|
### IN-01: SMTP driver errors are wrapped without redaction
|
||||||
|
|
||||||
|
**File:** `postcard/drivers.go:163-165`, `postcard/mailer.go:79-81`
|
||||||
|
**Issue:** `DialAndSendWithContext` is returned as `fmt.Errorf("smtp: %w", err)` and then `postcard: %w`. Dial-to-port-1 tests assert `secretpass` is absent. AUTH / server-response failures are not tested; go-mail can include SMTP reply text. Log driver is clean (no password field, no HTML body).
|
||||||
|
|
||||||
|
**Fix:** Map driver errors to a stable `postcard: smtp send failed` (optionally `errors.Is` for tests) and log the verbose error at debug without username/password.
|
||||||
|
|
||||||
|
### IN-02: Any extra file in `LangFS` fails plugin boot
|
||||||
|
|
||||||
|
**File:** `phrasebook/loader.go:80-107,243-263`
|
||||||
|
**Issue:** `WalkDir` plus fail-closed `parseLangPath` treats `lang/en/lang.yaml.bak`, `README.md`, or a nested `lang/en/nested/x.yaml` as a hard Load error naming the plugin. That is safe against traversal (`../` cleans to a non-`lang/a/b.yaml` path). It also means `//go:embed .` or an editor backup in `lang/` cannot boot. Mail registration only reads declared names, which is the better shape.
|
||||||
|
|
||||||
|
**Fix:** Skip non-matching paths (or only `fs.Glob` `lang/*/*.yaml`) instead of failing the catalog.
|
||||||
|
|
||||||
|
### IN-03: `data:` is banned in all rendered HTML
|
||||||
|
|
||||||
|
**File:** `postcard/templates.go:25,406-408`
|
||||||
|
**Issue:** `(javascript|vbscript|data):` rejects every data URI, including `data:image/png;base64,…` used as inline mail images, and also fails a send whose body merely mentions `javascript:`. Conservative for v1; it is not XSS by itself.
|
||||||
|
|
||||||
|
**Fix:** Restrict the scheme check to URL attributes (`href` / `src` / `xlink:href`) and allow `data:image/*`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
_Reviewed: 2026-09-18T12:15:00Z_
|
||||||
|
_Reviewer: gsd-code-reviewer_
|
||||||
|
_Depth: standard_
|
||||||
Reference in New Issue
Block a user