docs(05-06): security review of fill and encryption boundaries
- Map T-05-01 through T-05-24 to a passing test or restated accept rationale - Enumerate every .Reveal() call site; none are on log or serialize paths - Confirm zero AutoMigrate production calls and no Association Mode pivot writes
This commit is contained in:
Jakub Zych
@@ -0,0 +1,103 @@
|
||||
# Phase 5 Security Review — fill and encryption boundaries
|
||||
|
||||
**Date:** 2026-09-18
|
||||
**Scope:** Plans 05-01 through 05-06. Mass-assignment (`lagoon.Fill`, write-service allow-lists) and encrypted-at-rest credentials (`lagoon.Encrypted`, `.Reveal()`).
|
||||
**Repos grepped:** `summercms.go` and `fonoteka.go` (excluding `.planning/` and `vendor/`).
|
||||
|
||||
This is the phase-close review CONTEXT.md requires. Every `T-05-*` ID from the five prior plans (and this plan's own register) is mapped below to a passing test or a restated accept/transfer rationale. Unmapped IDs are a review gap, not an accepted risk.
|
||||
|
||||
## Fill boundary
|
||||
|
||||
PHP's mass-assignment boundary is two-layered (D-05/D-06/D-07): model `Fillable()` is the backstop; write services pass a narrower list. `json.Unmarshal` into a GORM model is never used on a write path.
|
||||
|
||||
| Layer | Allow-list | Proven by |
|
||||
|-------|------------|-----------|
|
||||
| Framework | `lagoon.Fill(model, allowed, requested, production)` copies only allow-listed gorm column keys | `TestFillAllowList`, `FuzzFill` (20s, 0 crashers) |
|
||||
| Album | `AlbumFillFields` excludes `collection_id` and `market_price_source` | `TestSaveAlbumFillFieldsSubset`, `FuzzSaveAlbum` (20s, 0 crashers) |
|
||||
| Collection | `CollectionFillFields` is `name`+`description` (no `owner_id`/`public_token`/`kind`) | `FuzzSaveCollection` (20s, 0 crashers) |
|
||||
| Credentials | `Fillable()` minus owner FK and Encrypted columns (no dedicated write-service file) | `FuzzSaveUserAiCredential`, `FuzzSaveOrgAiCredential`, `FuzzSaveUserDiscogsCredential`, `FuzzSaveOrgDiscogsCredential` |
|
||||
|
||||
Non-fillable and unknown keys are dropped with no error. In non-production they log once per type+key (`TestFillDroppedKeyLogsOnce`); production is silent (`TestFillProductionSilent`).
|
||||
|
||||
## Encryption boundary
|
||||
|
||||
`lagoon.Encrypted` plaintext is unexported. The only greppable accessor is `.Reveal()`. `MarshalJSON`, `String`, and `GoString` always emit `[redacted]`. Live Scan/Value is AES-256-GCM only; `DecryptLaravelPayload` is cutover-import-only and is not on the Scan/Value path.
|
||||
|
||||
Boot fails loud on empty/short/undecodable `app.key` (`TestEncryptedLoadAppKeyRejectsInvalid`). Column keys are HKDF-SHA256 from `app.key` with info `summercms.lagoon.encrypted.v1` (stdlib `crypto/hkdf`, no `golang.org/x/crypto`).
|
||||
|
||||
### `.Reveal()` call sites
|
||||
|
||||
`rg -n '\.Reveal\(\)'` over both trees (excluding `.planning/`):
|
||||
|
||||
| File | Role | Logging / Serialize* / marshal path? |
|
||||
|------|------|--------------------------------------|
|
||||
| `summercms.go/lagoon/encrypted.go:42` | method definition | no |
|
||||
| `summercms.go/lagoon/encrypted_test.go` (4 call sites) | unit assertions for round-trip and rotation | no — test only |
|
||||
| `fonoteka.go/plugins/golem15/fonoteka/classes/credential_write_service_fuzz_test.go` (8 call sites) | fuzz assertions that ciphertext round-trips | no — test only |
|
||||
|
||||
Zero call sites in `classes/serialize.go`, `slog`/`log` statements, `String`/`GoString`/`MarshalJSON`, or any production handler. `Encrypted.String`/`GoString`/`MarshalJSON` return the redaction literal and never call `Reveal`.
|
||||
|
||||
### Hidden / `json:"-"`
|
||||
|
||||
`TestHiddenNeverMarshals` (lagoon fixture + both `models.All()` registries, 26 Fonoteka + 2 user = 28) populates every `Hidden()` column with a sentinel and asserts it is absent from `json.Marshal` output. Every `Hidden()` column is independently required to carry `json:"-"`. T-05-03's 05-01 accept ("no real model implements them yet") is closed by this test.
|
||||
|
||||
## Association Mode and AutoMigrate
|
||||
|
||||
`rg -n '\.Association\('` finds only a comment in `lagoon/relations.go` documenting that pivot writes must **not** use Association Mode. `syncArtists` is delete-then-`Create` (`TestAlbumArtistsOrderRoundTrip`).
|
||||
|
||||
`rg -n 'AutoMigrate' fonoteka.go/plugins summercms.go/lagoon` matches only:
|
||||
|
||||
- `lagoon/migrations_test.go` `TestNoAutoMigrate` (fails if production lagoon `.go` files contain the token)
|
||||
- `parity/migrate_test.go` `TestPluginMigrationsDoNotUseAutoMigrate` (walks `fonoteka.go/plugins`)
|
||||
|
||||
Zero production `AutoMigrate` calls. Zero-tolerance rule held across all 5 prior plans' new code.
|
||||
|
||||
## T-05-17 allow-list audit
|
||||
|
||||
`fonoteka.go/parity/schema_diff_test.go` `allowedDiffs` has three entries, each with an inline decision comment:
|
||||
|
||||
| Key | Comment names |
|
||||
|-----|----------------|
|
||||
| `golem15_fonoteka_settings` | RESEARCH Open Question 2 |
|
||||
| `users` | D-03 / Phase 7 AUTH-02, no `widen_users` |
|
||||
| `golem15_fonoteka_oauth_refresh_tokens.fk:...user_id...` | 05-03 extra FK, P3 D-17 freeze |
|
||||
|
||||
The map is not being used to hide accidental missing columns: column presence, unique constraints, and non-allow-listed FKs still fail-hard (`TestSchemaMatchesPHPSnapshot`).
|
||||
|
||||
## Threat register
|
||||
|
||||
| Threat ID | Plan | Disposition | Evidence |
|
||||
|-----------|------|-------------|----------|
|
||||
| T-05-01 | 05-01 | mitigate | `TestFillAllowList`, `TestFillDroppedKeyNeverErrors`, `FuzzFill` — unknown/unlisted keys never set |
|
||||
| T-05-02 | 05-01 | mitigate | `TestParityCorpus` (Phase 3 genres fixture through `jwt.auth` + `inv.must-change-password`); plugin restructure did not rename the middleware (`plugin.go` still registers `inv.must-change-password`) |
|
||||
| T-05-03 | 05-01 | mitigate (was accept until models existed) | `lagoon.TestHiddenNeverMarshals` + `classes.TestHiddenNeverMarshals` over both `models.All()` (28 types). 05-01's accept rationale is superseded |
|
||||
| T-05-04 | 05-02 | mitigate | `TestSaveAlbumFillFieldsSubset`, `FuzzSaveAlbum` — `collection_id` / `market_price_source` stay at pre-fuzz values |
|
||||
| T-05-05 | 05-02 | mitigate | `TestAlbumArtistsOrderRoundTrip` — ordered pivot rewrite inside the SaveAlbum transaction |
|
||||
| T-05-06 | 05-02 | accept | Restated: table/column names are sourced from the model's own compiled `Rules()` map, never from request input; the value is parameterized |
|
||||
| T-05-07 | 05-02 | accept + proven | Restated: matches PHP's actual plain-unique constraint exactly (no partial index) per the D-02 audit. Proven by `TestCollectionPublicTokenDeleteThenRecreate` (soft-delete still unique-violates; hard-delete frees the token) |
|
||||
| T-05-08 | 05-03 | mitigate | `TestEncryptedRedacts`, `TestHiddenNeverMarshals`, credential `json:"-"` + `Hidden()`; `.Reveal()` grep above |
|
||||
| T-05-09 | 05-03 | mitigate | `TestEncryptedPreviousKeysFallback` — versioned ciphertext plus `app.previous_keys` decrypt-only |
|
||||
| T-05-10 | 05-03 | mitigate | `TestEncryptedLoadAppKeyRejectsInvalid` — empty/short/undecodable `app.key` fails, no default |
|
||||
| T-05-11 | 05-03 | accept | Restated: uses the Go 1.27 **standard library** `crypto/hkdf` package — no new dependency is introduced (this corrects an earlier draft of this plan that proposed `golang.org/x/crypto/hkdf`; Go 1.24+ ships HKDF in stdlib, so CLAUDE.md's stdlib-first rule applies directly and there is nothing for a package-legitimacy checkpoint to review) |
|
||||
| T-05-12 | 05-03 | accept | Restated: matches PHP's own design; raw tokens are never persisted anywhere, hashed only, consistent with the `Hidden()` backstop. `TestHiddenNeverMarshals` covers `token_hash` / `code_hash` / `client_secret_hash` |
|
||||
| T-05-13 | 05-04 | mitigate | `TestStaticHandler` — exact-match partition+disk_name only; `..` / extra slashes 404 |
|
||||
| T-05-14 | 05-04 | mitigate | `TestFileLifecycle`, `TestAttachSmoke` — rows deleted in-tx, blobs only after commit via `DeleteKeys` |
|
||||
| T-05-15 | 05-04 | accept | Restated: both `[OK]` in RESEARCH.md's Package Legitimacy Audit; no blocking human-verify checkpoint required |
|
||||
| T-05-16 | 05-04 | accept | Restated: this phase's `StaticHandler` is not yet wired into any route (HTTP-route boundary of this phase); the `is_public` gate is Phase 6/12's job when the handler is actually mounted — flagged here so it is not forgotten at wiring time |
|
||||
| T-05-17 | 05-05 | mitigate | `allowedDiffs` entries each have an inline decision comment (audit in this document); `TestSchemaMatchesPHPSnapshot` |
|
||||
| T-05-18 | 05-05 | accept | Restated: deliberate parity match with PHP (RESEARCH.md Open Question 3); documented in the migration's own comment so it is never "fixed" by a later, uninformed change |
|
||||
| T-05-19 | 05-05 | accept | Restated: test-only fixture plugin, not loaded by production `app/app.go`; gated behind `models.Album` type-assertion so it cannot fire for any other model; not wired into any HTTP path this phase. `TestCrossPluginCallback` |
|
||||
| T-05-20 | 05-06 | accept | Restated: 20s minimum fuzz time per target plus explicit adversarial seed corpus entries (`collection_id`, `market_price_source`, `organisation_id` overrides) named directly in the task rather than left to chance discovery |
|
||||
| T-05-21 | 05-06 | mitigate | `TestHiddenNeverMarshals` asserts `models.All()` counts (26+2) so a silently skipped model fails |
|
||||
| T-05-22 | 05-06 | mitigate | This document maps T-05-01 through T-05-24 by name |
|
||||
| T-05-23 | 05-02 | mitigate | `TestSaveAlbumMoneyNormalization` — blank/non-numeric → NULL, `"12.55555"` → `"12.5556"`, no panic |
|
||||
| T-05-24 | 05-02 | mitigate | `TestCollectionBeforeDeleteCascadesAlbums`, `TestWithSoftDeleteCascadeRollback` — cascade shares the parent delete transaction |
|
||||
|
||||
Highest-numbered ID across the five prior plans is T-05-24 (05-02). T-05-20..22 belong to this closing plan. None left unmapped.
|
||||
|
||||
## Residual / transferred risk
|
||||
|
||||
- HTTP-level DTO fuzz over every write endpoint remains Phase 12 (D-07, second half of criterion 3).
|
||||
- `StaticHandler` `is_public` gate remains Phase 6/12 (T-05-16).
|
||||
- Laravel CBC decrypt is Phase 15 cutover only (D-10).
|
||||
- Organisation membership/roles and `widen_users` remain Phase 7 AUTH-02 (T-05-17 users gap).
|
||||
Reference in New Issue
Block a user