|
|
|
|
@@ -0,0 +1,316 @@
|
|
|
|
|
---
|
|
|
|
|
phase: quick-261028-r16
|
|
|
|
|
plan: 01
|
|
|
|
|
type: execute
|
|
|
|
|
wave: 1
|
|
|
|
|
depends_on: []
|
|
|
|
|
files_modified:
|
|
|
|
|
- modules/lagoon/attach/store_test.go
|
|
|
|
|
- docs/services/storage.md
|
|
|
|
|
- docs/database/attachments.md
|
|
|
|
|
- modules/lagoon/transaction.go
|
|
|
|
|
- modules/lagoon/transaction_test.go
|
|
|
|
|
- modules/lagoon/relations.go
|
|
|
|
|
- modules/lagoon/README.md
|
|
|
|
|
- docs/database/transactions.md
|
|
|
|
|
autonomous: true
|
|
|
|
|
requirements:
|
|
|
|
|
- todo:2026-10-11-uploads-keep-client-extension-stored-xss
|
|
|
|
|
- todo:lagoon-readme-after-commit-callback-order
|
|
|
|
|
|
|
|
|
|
estimate:
|
|
|
|
|
tokens: 70000
|
|
|
|
|
raw_tokens: 70000
|
|
|
|
|
tasks: 2
|
|
|
|
|
confidence: low
|
|
|
|
|
|
|
|
|
|
must_haves:
|
|
|
|
|
truths:
|
|
|
|
|
- "A real PNG with <script> in a tEXt chunk passes attach.IsAllowedImage. Uploaded as x.html through attach.Store in image mode (html allowed), it is stored as <hex>.png and its URL ends in .png. file_name stays x.html, content_type is image/png and the stored bytes are unchanged. A Docker-free test also checks the IsAllowedImage + DiskExtension half"
|
|
|
|
|
- "attach.DiskExtension (phase 19.3, commit 2e22e9f) stays the single shared helper and attach.Store keeps naming blobs through it. No exported lagoon or attach identifier is added, removed or changed"
|
|
|
|
|
- "docs/services/storage.md's deploy note covers two cases. (1) A static server picks the Content-Type from the extension: set nosniff at server level, use an extension allow-list with its own types map, answer 404 for everything else, with a neutral nginx example. (2) A public S3 bucket or a CDN in front of it sends the Content-Type stored with the object, which is the sniffed type: use a separate origin, and set nosniff plus attachment disposition for non-images through the bucket/CDN header settings"
|
|
|
|
|
- "The lagoon.OnDatabase example in modules/lagoon/README.md registers its create callback After(\"gorm:create\").Before(\"gorm:commit_or_rollback_transaction\") and says why, matching docs/database/transactions.md"
|
|
|
|
|
- "The lagoon.RegisterJoinTable doc comment and its README row send pivot-ordered reads through an explicit join on the pivot table, and say that a Preload of the field cannot be ordered by a pivot column"
|
|
|
|
|
- "lagoon.AfterCommit logs a warning when a callback that runs after lagoon:after_commit on the same write registers work. Nothing else changes: the work still never runs, the write still succeeds, and the work is still buffered as before. Correctly ordered callbacks log nothing and behave exactly as before, including a callback between GORM's commit step and lagoon:after_commit and a later write on a reused chained statement"
|
|
|
|
|
- "go vet ./..., go test -short ./..., go test ./cmd/summer -run TestDocsTree and go run ./cmd/summer docs:build --check are green after each commit. The Docker-backed attach and lagoon tests named below pass"
|
|
|
|
|
artifacts:
|
|
|
|
|
- path: modules/lagoon/attach/store_test.go
|
|
|
|
|
provides: "pngWithScriptText fixture, TestPNGScriptChunkNaming (no Docker), TestStoreScriptChunkPNG (Postgres)"
|
|
|
|
|
contains: "TestStoreScriptChunkPNG"
|
|
|
|
|
- path: docs/services/storage.md
|
|
|
|
|
provides: "Deploy note split into static-server and public-bucket cases, with a neutral nginx allow-list example"
|
|
|
|
|
contains: "X-Content-Type-Options"
|
|
|
|
|
- path: modules/lagoon/transaction.go
|
|
|
|
|
provides: "statementFlushedKey marker set by flushStatementAfterCommit; AfterCommit warning guarded by transactionalHandle"
|
|
|
|
|
contains: "statementFlushedKey"
|
|
|
|
|
- path: modules/lagoon/transaction_test.go
|
|
|
|
|
provides: "late_callback_warns, callback_between_commit_and_flush_runs, reused_statement_no_false_warning subtests"
|
|
|
|
|
contains: "late_callback_warns"
|
|
|
|
|
- path: modules/lagoon/README.md
|
|
|
|
|
provides: "Corrected OnDatabase example, warning sentence, RegisterJoinTable row"
|
|
|
|
|
contains: "Before(\"gorm:commit_or_rollback_transaction\")"
|
|
|
|
|
- path: modules/lagoon/relations.go
|
|
|
|
|
provides: "Corrected RegisterJoinTable read guidance"
|
|
|
|
|
key_links:
|
|
|
|
|
- from: modules/lagoon/attach/store.go
|
|
|
|
|
to: modules/lagoon/attach/diskext.go
|
|
|
|
|
via: "newDiskName(DiskExtension(sniffed, ext)) in Store (unchanged; pinned by the new regression)"
|
|
|
|
|
pattern: "DiskExtension\\(sniffed, ext\\)"
|
|
|
|
|
- from: modules/lagoon/transaction.go flushStatementAfterCommit
|
|
|
|
|
to: modules/lagoon/transaction.go AfterCommit
|
|
|
|
|
via: "db.InstanceSet(statementFlushedKey, true) read back with db.InstanceGet and the !transactionalHandle(db) guard"
|
|
|
|
|
pattern: "statementFlushedKey"
|
|
|
|
|
- from: modules/lagoon/README.md
|
|
|
|
|
to: modules/lagoon/example_test.go#on-database
|
|
|
|
|
via: "Same registration chain as the tested docs region shown on docs/database/transactions.md"
|
|
|
|
|
pattern: "After\\(\"gorm:create\"\\)\\.Before\\(\"gorm:commit_or_rollback_transaction\"\\)"
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
<objective>
|
|
|
|
|
Close the two pending lagoon todos in the framework (summercms.go only).
|
|
|
|
|
|
|
|
|
|
1. Stored XSS through the client file extension (todo 2026-10-11, major). Live reads at planning time show that the framework part is already done. Phase 19.3 plan 03 (commit 2e22e9f, D-15 of 19.3-CONTEXT) added the shared helper `attach.DiskExtension` in `modules/lagoon/attach/diskext.go`. `attach.Store` names every blob through it (`store.go`: `newDiskName(DiskExtension(sniffed, ext))`), `file_name` keeps the client's name, and `TestDiskExtensionTable`, `TestStoreDiskExtensionSmoke` and `TestStoreDiskExtension` cover it. `attachments.md` and the lagoon README describe it, and `docs/services/storage.md` has a deploy note. Do NOT add a second helper or change attach's exported API. Two gaps remain:
|
|
|
|
|
- No test covers the exact exploit from the todo: a real, decodable PNG with `<script>` in a `tEXt` chunk going through image mode, where it passes `IsAllowedImage`.
|
|
|
|
|
- The deploy note claims that a CDN in front of the bucket "picks the Content-Type from the file extension". That is wrong for a public S3 bucket: S3 sends the Content-Type stored with the object, and `attach.Store` stores the sniffed type (19.3 deferred-items.md). The note also has no concrete web-server allow-list.
|
|
|
|
|
Task 1 closes both gaps.
|
|
|
|
|
2. lagoon README after-commit callback order (todo, priority high). The README's `lagoon.OnDatabase` example registers its callback off GORM's after-create step, which GORM sorts after `lagoon:after_commit`, so the buffered work silently never runs. The `RegisterJoinTable` doc comment still advises ordering a Preload by pivot columns, which fails. Task 2 fixes both texts and adds a log-only warning in `lagoon.AfterCommit`. The warning is small and safe: one marker in the existing flush callback, one guarded log line, and no change to what runs.
|
|
|
|
|
|
|
|
|
|
Decomposition: two independent auto tasks, two commits. They touch no common files, so either order works; run them in order. Tracer-first does not apply. These are two separate hardening fixes on paths that are already proven, so this plan behaves as `--no-tracer`, the same shape as earlier quick plans in this repo.
|
|
|
|
|
|
|
|
|
|
Not in scope (orchestrator records as follow-ups; do not edit sibling repos):
|
|
|
|
|
- `../fonoteka.go/plugins/golem15/fonoteka/controllers/api/collection_media_controller.go:251` (`storeUpload`) and `../fonoteka.go/plugins/golem15/fonoteka/classes/album_files.go:35` (`StorePublicFile`). Both append `filepath.Ext(<client name>)` to the disk name. Adoption is one line per site: sniff first, then use `attach.DiskExtension(contentType, filepath.Ext(name))`. That function already strips the dot, lower-cases the extension and drops MIME parameters. Note for that follow-up: an upload without an extension then gets `.bin`/`.txt` instead of no extension.
|
|
|
|
|
- Already safe, optional dedup only: `sm-feedback-plugin` `screenshotExtension` (classes/submission_store.go) and `sm-user-plugin` `avatarExt` (controllers/api_controller.go:1497). Both derive the extension from their own sniff tables.
|
|
|
|
|
- Behaviour follow-up, deferred: `attach.Store` writes the sniffed type as blob metadata. A public S3 bucket therefore serves an HTML body stored as `.txt` as `text/html`. The fix would be to write a non-rendering type, or `Content-Disposition: attachment`, for non-inline types. It changes Store's output, so it gets its own todo. This plan only documents the risk.
|
|
|
|
|
- The orchestrator's closing docs commit moves both todos to `.planning/todos/completed/`. The executor does not move them in the code commits.
|
|
|
|
|
|
|
|
|
|
Purpose: the exploit from the todo gets a test, the deploy docs stop giving wrong advice about bucket-served uploads, and a misordered after-commit callback becomes visible in the logs instead of silently doing nothing.
|
|
|
|
|
Output: two commits in summercms.go and 261028-r16-SUMMARY.md.
|
|
|
|
|
</objective>
|
|
|
|
|
|
|
|
|
|
<execution_context>
|
|
|
|
|
@~/.claude/msd-core/workflows/execute-plan.md
|
|
|
|
|
@~/.claude/msd-core/templates/summary.md
|
|
|
|
|
</execution_context>
|
|
|
|
|
|
|
|
|
|
<context>
|
|
|
|
|
@.planning/STATE.md
|
|
|
|
|
@./CLAUDE.md
|
|
|
|
|
@.planning/todos/pending/2026-10-11-uploads-keep-client-extension-stored-xss.md
|
|
|
|
|
@.planning/todos/pending/lagoon-readme-after-commit-callback-order.md
|
|
|
|
|
@.planning/phases/19.3-framework-transport-and-infrastructure-for-wavepath/deferred-items.md
|
|
|
|
|
|
|
|
|
|
<interfaces>
|
|
|
|
|
Existing code, verified at planning time. Nothing below changes signature.
|
|
|
|
|
|
|
|
|
|
modules/lagoon/attach/diskext.go
|
|
|
|
|
- func DiskExtension(contentType, clientExt string) string. For a specific sniffed type it returns the client's extension when that extension belongs to the type, else the type's canonical one (image/png gives png). For a generic type it keeps the client's extension unless that one is script-capable, which becomes txt for text/* and bin otherwise.
|
|
|
|
|
|
|
|
|
|
modules/lagoon/attach/store.go
|
|
|
|
|
- func Store(ctx, db *gorm.DB, bucket *blob.Bucket, in Upload, lim Limits) (*File, error)
|
|
|
|
|
- type Upload { FileName string; Body io.Reader; Public bool }
|
|
|
|
|
- type Limits { Extensions []string; MIMETypes []string; MaxBytes int64; Image bool }. In image mode the bytes must pass IsAllowedImage. An empty Extensions list means DefaultImageExtensions in image mode, else DefaultFileExtensions.
|
|
|
|
|
- writeBlob writes &blob.WriterOptions{ContentType: contentType}, where contentType is the sniffed type, or the type looked up from the extension when the sniff says only application/octet-stream.
|
|
|
|
|
- DefaultFileExtensions includes "txt".
|
|
|
|
|
|
|
|
|
|
modules/lagoon/attach/guard.go
|
|
|
|
|
- func IsAllowedImage(data []byte) bool: the bytes must sniff as an allowed image type, image.DecodeConfig must succeed with a positive size, and the size must stay under MaxImagePixels.
|
|
|
|
|
|
|
|
|
|
modules/lagoon/attach/store_test.go (package attach_test)
|
|
|
|
|
- smokePNG(t) []byte: png.Encode of a 4x3 RGBA image.
|
|
|
|
|
- attachGorm(t) *gorm.DB: testcontainers Postgres. Tests that use it skip under testing.Short().
|
|
|
|
|
- The TestStoreDiskExtensionSmoke pattern: lagoon.Migrate(gdb, nil), memblob.OpenBucket(nil), attach.Store(...).
|
|
|
|
|
|
|
|
|
|
modules/lagoon/transaction.go (package lagoon)
|
|
|
|
|
- const AfterCommitCallback = "lagoon:after_commit"; const statementBufferKey = "lagoon:after_commit"
|
|
|
|
|
- func AfterCommit(ctx, db *gorm.DB, fn). Branches, in order: (1) bufferFrom(ctx, db) finds a buffer, meaning the call is inside lagoon.Transaction; (2) db.InstanceGet("gorm:started_transaction") is set, meaning GORM opened an implicit transaction: the work is buffered under statementBufferKey; (3) transactionalHandle(db), meaning a foreign tx: warn and skip; (4) otherwise run the work now.
|
|
|
|
|
- func flushStatementAfterCommit(db *gorm.DB): registered After("gorm:commit_or_rollback_transaction") on create, update and delete by registerAfterCommit. It InstanceGets statementBufferKey and runs the buffered work when db.Error is nil.
|
|
|
|
|
- func transactionalHandle(db) bool: db.Statement.ConnPool implements gorm.TxCommitter.
|
|
|
|
|
- moduleLog = diary.For("lagoon"), with slog-style Warn(msg, key, value...). Tests capture it by swapping slog.SetDefault to a TextHandler at LevelWarn (see the plain_gorm_transaction_is_refused subtest).
|
|
|
|
|
|
|
|
|
|
GORM v1.31.2 callbacks/transaction.go, which the warning guard relies on:
|
|
|
|
|
- BeginTransaction sets db.Statement.ConnPool to the tx pool and InstanceSet("gorm:started_transaction", true).
|
|
|
|
|
- CommitOrRollbackTransaction commits or rolls back and then sets db.Statement.ConnPool = db.ConnPool, the pool rather than a TxCommitter. After the commit step, transactionalHandle(db) is therefore false on that statement.
|
|
|
|
|
- InstanceSet/InstanceGet key by fmt.Sprintf("%p", db.Statement)+key. A statement reused across writes on a chained handle (clone == 0, for example g.WithContext(ctx).Table("t")) keeps its Settings between writes.
|
|
|
|
|
|
|
|
|
|
modules/lagoon/transaction_test.go
|
|
|
|
|
- TestTransactionAfterCommit(t): db, _ := dedicatedDB(t, "lagoon_after_commit"); gdb, _ := Use(ctx, db). Table lagoon_ac_items (id SERIAL, name TEXT UNIQUE), model acItem{Name}. Also recorder{add, names} and committedCount(t, db, name).
|
|
|
|
|
- The implicit_single_statement subtest shows the pattern: a separate handle g2, _ := Use(ctx, db), then g2.Callback().Create().Before("gorm:create").Register("lagoon_test:after_commit", ...), whose callback calls AfterCommit(stmt.Statement.Context, stmt, ...).
|
|
|
|
|
|
|
|
|
|
modules/lagoon/example_test.go: region `docs:start on-database` (line ~587) holds the correct chain After("gorm:create").Before("gorm:commit_or_rollback_transaction") and is run by TestDocsOnDatabase.
|
|
|
|
|
</interfaces>
|
|
|
|
|
</context>
|
|
|
|
|
|
|
|
|
|
<tasks>
|
|
|
|
|
|
|
|
|
|
<task type="auto" tdd="true">
|
|
|
|
|
<name>Task 1: Pin the script-chunk PNG exploit through attach.Store and correct the uploads deploy note</name>
|
|
|
|
|
<files>modules/lagoon/attach/store_test.go, docs/services/storage.md, docs/database/attachments.md</files>
|
|
|
|
|
<read_first>
|
|
|
|
|
- modules/lagoon/attach/diskext.go (the helper; do not change it)
|
|
|
|
|
- modules/lagoon/attach/store.go lines 95-200 (Store, writeBlob, Limits defaults)
|
|
|
|
|
- modules/lagoon/attach/guard.go (IsAllowedImage)
|
|
|
|
|
- modules/lagoon/attach/store_test.go lines 1-60 and 286-420 (imports, smokePNG, attachGorm, TestStoreDiskExtensionSmoke, TestStoreDiskExtension)
|
|
|
|
|
- docs/services/storage.md lines 96-126 (built-in route, deploy note)
|
|
|
|
|
- docs/database/attachments.md lines 92-100 (disk-name paragraph)
|
|
|
|
|
- ../sm-grzybyfunkcjonalne-app/deploy/nginx/grzybyfunkcjonalne-staging.conf lines 209-231 (read-only: the tested nginx pattern. Copy its shape only; framework docs never name a consuming app)
|
|
|
|
|
</read_first>
|
|
|
|
|
<behavior>
|
|
|
|
|
- pngWithScriptText(t) returns a PNG that image/png decodes and attach.IsAllowedImage accepts, whose bytes contain the literal script tag in a tEXt chunk.
|
|
|
|
|
- http.DetectContentType on it gives image/png, and attach.DiskExtension(that type, "html") gives "png" (no Docker).
|
|
|
|
|
- attach.Store in image mode with Limits{Image: true, Extensions: png and html} and FileName "x.html" returns a File with DiskName ending ".png", URL() ending ".png", FileName "x.html" and ContentType "image/png". Reading back attach.BlobKey(DiskName) from the bucket returns exactly the uploaded bytes (Postgres, skips under -short).
|
|
|
|
|
</behavior>
|
|
|
|
|
<action>
|
|
|
|
|
Part A, the test. Per the stored-XSS todo's acceptance and 19.3 D-15, and with no production code change: the framework fix exists in diskext.go/store.go and stays as it is.
|
|
|
|
|
|
|
|
|
|
In modules/lagoon/attach/store_test.go (package attach_test), add a helper pngWithScriptText(t *testing.T) []byte. It starts from smokePNG(t). It splits after the 8-byte signature plus the IHDR chunk; the IHDR chunk is 25 bytes (4 length, 4 type, 13 data, 4 CRC), so the split is at offset 33. Assert that bytes 12..16 equal "IHDR" and t.Fatal otherwise. At the split, insert a tEXt chunk:
|
|
|
|
|
- a 4-byte big-endian data length (encoding/binary),
|
|
|
|
|
- the type "tEXt",
|
|
|
|
|
- the data: keyword "Comment", a NUL byte, then the text `<script>alert(document.cookie)</script>`,
|
|
|
|
|
- a 4-byte big-endian CRC computed by hash/crc32.ChecksumIEEE over type plus data.
|
|
|
|
|
Add encoding/binary, hash/crc32 and net/http to the imports.
|
|
|
|
|
|
|
|
|
|
Add TestPNGScriptChunkNaming. It is Docker-free and must not skip under -short. It checks:
|
|
|
|
|
- png.Decode of the fixture succeeds,
|
|
|
|
|
- attach.IsAllowedImage(fixture) is true,
|
|
|
|
|
- the fixture contains the script literal (bytes.Contains),
|
|
|
|
|
- http.DetectContentType(fixture) is "image/png",
|
|
|
|
|
- attach.DiskExtension of that type with client extension "html" is "png".
|
|
|
|
|
|
|
|
|
|
Add TestStoreScriptChunkPNG, modelled on TestStoreDiskExtensionSmoke. It skips under testing.Short() with the same message, and sets up attachGorm, lagoon.Migrate(gdb, nil) and memblob.OpenBucket(nil) with t.Cleanup Close. It calls attach.Store with Upload{FileName: "x.html", Body: bytes.NewReader(fixture), Public: true} and Limits{Image: true, Extensions: []string{"png", "html"}}. html is allowed on purpose, so the extension check is not what stops the attack. Then it asserts:
|
|
|
|
|
- the DiskName and URL() suffixes are ".png",
|
|
|
|
|
- FileName is "x.html" and ContentType is "image/png",
|
|
|
|
|
- bucket.ReadAll(ctx, attach.BlobKey(f.DiskName)) equals the fixture byte for byte. The payload is stored as uploaded; only its name makes it harmless.
|
|
|
|
|
Doc-comment both tests, naming the todo scenario (a write-scoped client uploads a valid image under an .html name).
|
|
|
|
|
|
|
|
|
|
Part B, the docs. Per the todo's defence-in-depth item and the 19.3 deferred-items note.
|
|
|
|
|
|
|
|
|
|
In docs/services/storage.md, rewrite the "Deploy note" blockquote at the end of the built-in route section as a short subsection or blockquote with two cases. Keep the existing sentence that attach.Store names each file after its sniffed content through attach.DiskExtension, so a PNG uploaded as x.html is stored as .png.
|
|
|
|
|
|
|
|
|
|
(1) A static server such as nginx serving the uploads directory. It picks the Content-Type from the extension and sends none of the route's headers. Give a neutral nginx example in a fenced nginx block with these parts:
|
|
|
|
|
- a server-level `add_header X-Content-Type-Options nosniff always;`, with a comment that an add_header inside a location replaces every add_header inherited from the server level,
|
|
|
|
|
- a case-insensitive regex location `~* ^/storage/app/uploads/public/.+\.(?:jpe?g|png|gif|webp)$` with `root /srv/acme`, its own `types` block that maps only those extensions (image/jpeg jpg jpeg, image/png png, image/gif gif, image/webp webp), and `default_type application/octet-stream`,
|
|
|
|
|
- a plain prefix `location /storage/` that answers `return 404;`, with a comment that it must stay a plain prefix, not `^~`, so that nginx tries the regex first.
|
|
|
|
|
Say in prose: extend the extension list only with types the application accepts (see attach.DefaultFileExtensions), and serve anything that is not a raster image as a download (Content-Disposition attachment) or from a separate origin, the way the built-in route does.
|
|
|
|
|
|
|
|
|
|
(2) A public S3 bucket, or a CDN that pulls from it, serving the files under storage.uploads.public_url. It does not look at the extension. It sends the Content-Type stored with the object, which attach.Store sets to the sniffed type. So an HTML body uploaded as note.txt (txt is in attach.DefaultFileExtensions) is stored as `<hex>.txt` but served as text/html. Serve such a bucket from its own origin, never the application's, and set X-Content-Type-Options nosniff and, for everything but raster images, Content-Disposition attachment through the bucket's or CDN's response-header settings.
|
|
|
|
|
|
|
|
|
|
Use only identifiers that exist: attach.Store, attach.DiskExtension, attach.DefaultFileExtensions, storage.uploads.public_url. The docs checker verifies the identifiers. It does not verify config keys, so check those by hand against the configuration rows already on the page.
|
|
|
|
|
|
|
|
|
|
In docs/database/attachments.md, line ~98, make the first sentence's reason precise: a static web server in front of the uploads directory picks the Content-Type from the extension. Add a short pointer that a public bucket sends the stored type instead, linking to the storage page's deploy note anchor. Use the anchor that `go run ./cmd/summer docs:build --check` accepts.
|
|
|
|
|
|
|
|
|
|
Commit only these three paths, staged by explicit path. Suggested message: "docs(storage): pin the script-chunk PNG upload and split the uploads deploy note (quick-261028-r16)". No co-author tag.
|
|
|
|
|
</action>
|
|
|
|
|
<verify>
|
|
|
|
|
<automated>go vet ./modules/lagoon/... && go test -count=1 -short -run 'TestPNGScriptChunkNaming|TestDiskExtensionTable' -v ./modules/lagoon/attach 2>&1 | grep -q -- '^--- PASS: TestPNGScriptChunkNaming' && go test -count=1 -run 'TestStoreScriptChunkPNG|TestStoreDiskExtension' ./modules/lagoon/attach && go test -short ./... && go test ./cmd/summer -run TestDocsTree && go run ./cmd/summer docs:build --check</automated>
|
|
|
|
|
</verify>
|
|
|
|
|
<acceptance_criteria>
|
|
|
|
|
- `grep -c 'func TestStoreScriptChunkPNG\|func TestPNGScriptChunkNaming\|func pngWithScriptText' modules/lagoon/attach/store_test.go` prints 3.
|
|
|
|
|
- The non-short run of TestStoreScriptChunkPNG reports PASS, not SKIP. If the Docker socket is denied, rerun through `sudo -u golem` as CLAUDE.md describes, and record in SUMMARY which way it ran.
|
|
|
|
|
- `grep -c 'nosniff always' docs/services/storage.md` is at least 1 and `grep -c 'Content-Type stored with the object' docs/services/storage.md` is at least 1 (or equivalent wording the executor records in SUMMARY).
|
|
|
|
|
- With SHA1 set to this task's own commit (from `git log -1 --format=%H` right after committing, before anything else lands), `git show --name-only --format= "$SHA1"` exits 0 and lists store_test.go as the only file under modules/lagoon/attach/: no production attach code changed.
|
|
|
|
|
- No consuming application name appears in the new docs text (grep the diff for fonoteka, grzyby, plytarium: 0 hits).
|
|
|
|
|
</acceptance_criteria>
|
|
|
|
|
<done>One commit. The exact todo exploit (a decodable PNG with a script tEXt chunk, uploaded in image mode as x.html) is pinned with and without Docker. The storage deploy note correctly separates static-server and public-bucket serving, with a working neutral nginx allow-list. attachments.md agrees with it. go vet, go test -short ./... and the docs checker are green.</done>
|
|
|
|
|
</task>
|
|
|
|
|
|
|
|
|
|
<task type="auto" tdd="true">
|
|
|
|
|
<name>Task 2: Warn on after-commit work registered after lagoon:after_commit; correct the OnDatabase example and the RegisterJoinTable read guidance</name>
|
|
|
|
|
<files>modules/lagoon/transaction.go, modules/lagoon/transaction_test.go, modules/lagoon/relations.go, modules/lagoon/README.md, docs/database/transactions.md</files>
|
|
|
|
|
<read_first>
|
|
|
|
|
- modules/lagoon/transaction.go (whole file, about 260 lines)
|
|
|
|
|
- modules/lagoon/transaction_test.go lines 1-60 and 195-260 (harness, implicit_single_statement, plain_gorm_transaction_is_refused log capture)
|
|
|
|
|
- modules/lagoon/relations.go lines 1-28
|
|
|
|
|
- modules/lagoon/README.md lines 25-40, 80-105, 125-135 and 190-196
|
|
|
|
|
- docs/database/transactions.md lines 66-92
|
|
|
|
|
- docs/database/relations.md lines 20-56 (already-correct pivot-order guidance to mirror)
|
|
|
|
|
- modules/lagoon/example_test.go lines 580-625 (on-database docs region and TestDocsOnDatabase)
|
|
|
|
|
- $(go list -m -f '{{.Dir}}' gorm.io/gorm)/callbacks/transaction.go (ConnPool reset after commit)
|
|
|
|
|
</read_first>
|
|
|
|
|
<behavior>
|
|
|
|
|
- late_callback_warns: on a fresh handle from Use(ctx, db), a Create callback registered After(AfterCommitCallback) calls AfterCommit while creating acItem "flush-late". The row is committed (committedCount 1), the recorded work never runs (recorder empty, the same as before this change), Create returns no error, and the captured Warn log contains "registered after lagoon:after_commit ran".
|
|
|
|
|
- callback_between_commit_and_flush_runs: a callback registered After("gorm:commit_or_rollback_transaction").Before(AfterCommitCallback), creating "flush-between", has its work run exactly once ([flush-between]) and logs no warning.
|
|
|
|
|
- reused_statement_no_false_warning: a correctly ordered callback (Before("gorm:create")) on q := g.WithContext(ctx).Table("lagoon_ac_items"), creating "flush-reuse-1" and then "flush-reuse-2" through the same q, records [flush-reuse-1 flush-reuse-2] and logs no warning. This pins the transactionalHandle guard against the marker left behind by the first write.
|
|
|
|
|
- All existing TestTransactionAfterCommit subtests and TestDocsOnDatabase stay green unchanged.
|
|
|
|
|
</behavior>
|
|
|
|
|
<action>
|
|
|
|
|
Code. Per the after-commit todo's suggested fix. The change is log-only and adds no exported identifiers.
|
|
|
|
|
|
|
|
|
|
In modules/lagoon/transaction.go:
|
|
|
|
|
- Add an unexported const statementFlushedKey = "lagoon:after_commit_flushed" next to statementBufferKey.
|
|
|
|
|
- In flushStatementAfterCommit, make the first statement db.InstanceSet(statementFlushedKey, true), before the buffer InstanceGet. Every write that passes through the flush then marks its statement. Extend the function's doc comment to say so.
|
|
|
|
|
- In AfterCommit's branch for a GORM-started transaction (gorm:started_transaction is set), before the buffering code, check two things: db.InstanceGet(statementFlushedKey) is present, and transactionalHandle(db) is false. Both together mean GORM's commit step already reset the statement's ConnPool to the pool and lagoon:after_commit already ran for this write. In that case call moduleLog.Warn with exactly the message "lagoon: after-commit callback registered after lagoon:after_commit ran; it will not run" and the key/value pair "hint", "register the GORM callback before gorm:commit_or_rollback_transaction".
|
|
|
|
|
- Then fall through to the existing buffering, unchanged. Do not return early and do not run the work. That keeps behaviour identical in every case, including the rare case of a callback sorted between commit and flush on a statement reused from an earlier write.
|
|
|
|
|
- Add a sentence to AfterCommit's doc comment. Called from a GORM callback that runs after AfterCommitCallback on the same write, the work is registered too late and never runs, and AfterCommit logs a warning. Register such callbacks Before("gorm:commit_or_rollback_transaction"). The transactionalHandle check keeps a statement reused across writes from warning for a correctly ordered callback in a later write.
|
|
|
|
|
|
|
|
|
|
In modules/lagoon/transaction_test.go, add the three subtests from <behavior> inside TestTransactionAfterCommit, after implicit_single_statement:
|
|
|
|
|
- Each uses its own handle from Use(ctx, db) so its callback stays off the shared gdb.
|
|
|
|
|
- Callback names: "lagoon_test:late", "lagoon_test:between", "lagoon_test:reuse".
|
|
|
|
|
- Row names: "flush-late", "flush-between", "flush-reuse-1", "flush-reuse-2".
|
|
|
|
|
- Capture logs exactly like plain_gorm_transaction_is_refused: swap slog.SetDefault to a TextHandler on a bytes.Buffer at LevelWarn and restore it with defer.
|
|
|
|
|
- Assert with strings.Contains on the substring "registered after lagoon:after_commit ran".
|
|
|
|
|
- If GORM itself rejects the second Create through the shared chained q (an error from GORM, not from lagoon), keep the other two subtests. Replace the reuse case with the closest GORM-supported reuse of one Statement, or drop it, and record the deviation and the reason in SUMMARY.
|
|
|
|
|
|
|
|
|
|
Texts.
|
|
|
|
|
- modules/lagoon/README.md, OnDatabase example (about line 89): change the registration chain to the one in example_test.go's on-database region, After("gorm:create").Before("gorm:commit_or_rollback_transaction"). The example currently chains off GORM's after-create callback.
|
|
|
|
|
- README, the lead-in sentence before that example (about line 84): add one sentence. Register the callback before GORM's gorm:commit_or_rollback_transaction step: lagoon runs a single-statement write's after-commit work from lagoon:after_commit right after that step, so work buffered by a callback GORM sorts later never runs, and lagoon.AfterCommit logs a warning for it. Link it to ../../docs/database/transactions.md#callbacks-registered-at-boot if the docs checker accepts README links; otherwise leave the link out.
|
|
|
|
|
- README, features line 29 (after-commit work): add that a callback which runs after lagoon:after_commit on the same write registers its work too late, the work never runs, and lagoon.AfterCommit logs a warning.
|
|
|
|
|
- README, API row for lagoon.RegisterJoinTable (about line 194): append that pivot-ordered reads join the pivot table explicitly, because a Preload of the field cannot be ordered by a pivot column.
|
|
|
|
|
- modules/lagoon/relations.go, RegisterJoinTable doc comment: replace the final sentence, which currently advises a Preload of the field plus an Order on the pivot's columns. The new text says the following. Reads in pivot order join the pivot table explicitly (Joins on the join table plus Order on its columns). A Preload of the field still loads the targets through the pivot model once RegisterJoinTable has run, but GORM issues that query without the join table, so ordering it by a pivot column fails with a missing FROM-clause entry. Keep the pivot-write contract paragraph as it is.
|
|
|
|
|
- docs/database/transactions.md, WARNING block (about line 86): keep its first sentence. Change the end of the second sentence so that it says the buffered work is never run, the write still succeeds, and lagoon logs a warning when that happens.
|
|
|
|
|
|
|
|
|
|
Commit only these five paths, staged by explicit path. Suggested message: "fix(lagoon): warn on after-commit work registered after lagoon:after_commit; correct the OnDatabase example and pivot read guidance (quick-261028-r16)". No co-author tag. Leave other sessions' STATE.md and state.json edits alone.
|
|
|
|
|
</action>
|
|
|
|
|
<verify>
|
|
|
|
|
<automated>go vet ./... && go test -count=1 -short ./modules/lagoon/... && go test -count=1 -run 'TestTransactionAfterCommit|TestDocsOnDatabase' -v ./modules/lagoon/ 2>&1 | grep -q -- '--- PASS: TestTransactionAfterCommit/late_callback_warns' && go test -count=1 -run 'TestTransactionAfterCommit|TestDocsOnDatabase' ./modules/lagoon/ && go test -short ./... && go test ./cmd/summer -run TestDocsTree && go run ./cmd/summer docs:build --check</automated>
|
|
|
|
|
</verify>
|
|
|
|
|
<acceptance_criteria>
|
|
|
|
|
- `grep -c 'gorm:after_create' modules/lagoon/README.md` prints 0.
|
|
|
|
|
- `grep -c 'After("gorm:create").Before("gorm:commit_or_rollback_transaction")' modules/lagoon/README.md` prints at least 1.
|
|
|
|
|
- `grep -c 'Preload(field)' modules/lagoon/relations.go` prints 0, and `grep -c -i 'join' modules/lagoon/relations.go` is at least 2.
|
|
|
|
|
- `grep -c 'statementFlushedKey' modules/lagoon/transaction.go` is at least 3 (declaration, set, read).
|
|
|
|
|
- `grep -c 'registered after lagoon:after_commit ran' modules/lagoon/transaction.go modules/lagoon/transaction_test.go` shows at least 1 in each file.
|
|
|
|
|
- The non-short run reports PASS for late_callback_warns, callback_between_commit_and_flush_runs and reused_statement_no_false_warning, unless the reuse case was replaced as described, and for every existing TestTransactionAfterCommit subtest and TestDocsOnDatabase. If the Docker socket is denied, rerun through `sudo -u golem`.
|
|
|
|
|
- No exported identifier is added or removed. Set SHA2 to this task's own commit (`git log -1 --format=%H` right after committing). Write `git show "$SHA2" -- modules/lagoon/` to a file in the session scratchpad and check that the command exited 0. Then `grep -cE '^[+-](func|type|var|const) [A-Z]'` on that file prints 0.
|
|
|
|
|
</acceptance_criteria>
|
|
|
|
|
<done>One commit. lagoon.AfterCommit warns, and only warns, when a callback that runs after lagoon:after_commit on the same write registers work; correct orderings and reused statements stay silent. The README example matches the tested docs region. The RegisterJoinTable comment, the README row and the transactions WARNING tell the truth. go vet, go test -short ./..., the Docker lagoon tests and the docs checker are green.</done>
|
|
|
|
|
</task>
|
|
|
|
|
|
|
|
|
|
</tasks>
|
|
|
|
|
|
|
|
|
|
<threat_model>
|
|
|
|
|
## Trust Boundaries
|
|
|
|
|
|
|
|
|
|
| Boundary | Description |
|
|
|
|
|
|----------|-------------|
|
|
|
|
|
| client upload → attach.Store | The client's file name and bytes are untrusted. The name's extension is checked against the allow-list, but it must not decide how the stored file is served |
|
|
|
|
|
| bucket/static server → browser | Whoever serves the uploads picks the Content-Type: nginx picks it from the extension, S3 sends the stored object's metadata. A rendering type on a shared origin is stored XSS |
|
|
|
|
|
| plugin GORM callback → lagoon after-commit buffer | Plugin code decides callback order. Misordered callbacks lose side effects (mail, jobs, broadcasts) without any trace |
|
|
|
|
|
|
|
|
|
|
## STRIDE Threat Register
|
|
|
|
|
|
|
|
|
|
| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan |
|
|
|
|
|
|-----------|----------|-----------|----------|-------------|-----------------|
|
|
|
|
|
| T-q261028r16-01 | Elevation of privilege | attach.Store disk name (valid PNG with script uploaded as x.html) | high | mitigate | Already mitigated by attach.DiskExtension in Store (19.3, 2e22e9f). Task 1 adds TestPNGScriptChunkNaming (no Docker) and TestStoreScriptChunkPNG (image mode, html allowed), which pin the exact todo exploit: the .png disk name, the .png URL and the bytes stored unchanged |
|
|
|
|
|
| T-q261028r16-02 | Elevation of privilege | Public S3 bucket or CDN serving the stored text/html metadata of a .txt upload | medium | transfer | Task 1 corrects the deploy note: serve a public bucket from its own origin, with nosniff and attachment disposition for non-images set through the bucket or CDN header settings. Changing Store's blob metadata changes behaviour, so it is deferred to its own todo (recorded by the orchestrator) |
|
|
|
|
|
| T-q261028r16-03 | Tampering | Static server serving the uploads directory by extension | medium | mitigate | Task 1 documents a neutral nginx pattern: server-level nosniff, a regex allow-list location with its own types map and default_type application/octet-stream, and a plain prefix 404 for everything else |
|
|
|
|
|
| T-q261028r16-04 | Repudiation | lagoon.AfterCommit buffering work after lagoon:after_commit already ran | medium | mitigate | Task 2 logs a warning with a fix hint. The README example and the transactions docs register before gorm:commit_or_rollback_transaction |
|
|
|
|
|
| T-q261028r16-05 | Denial of service | Warning noise or false positives on hot write paths | low | mitigate | The warning needs both the flushed marker and the statement's ConnPool no longer being a TxCommitter. reused_statement_no_false_warning and callback_between_commit_and_flush_runs pin the silent cases. Cost per write: one sync.Map store |
|
|
|
|
|
| T-q261028r16-SC | Tampering | npm/pip/cargo installs | low | accept | No new dependencies. Only the stdlib packages encoding/binary, hash/crc32 and net/http are added to a test file |
|
|
|
|
|
</threat_model>
|
|
|
|
|
|
|
|
|
|
<verification>
|
|
|
|
|
- After each of the two commits: go vet ./..., go test -short ./..., go test ./cmd/summer -run TestDocsTree and go run ./cmd/summer docs:build --check are green (CLAUDE.md rule).
|
|
|
|
|
- Docker-backed: go test -count=1 -run 'TestStoreScriptChunkPNG|TestStoreDiskExtension' ./modules/lagoon/attach and go test -count=1 -run 'TestTransactionAfterCommit|TestDocsOnDatabase' ./modules/lagoon/ pass (fall back to sudo -u golem when the socket is denied).
|
|
|
|
|
- No exported lagoon or attach identifier changes. No file outside summercms.go is edited; the sibling repos were only read at planning time.
|
|
|
|
|
- git log shows two commits without co-author tags, each staging only its own paths. Other sessions' STATE.md, state.json, G15OFFICE.md and .gitkeep files are left alone.
|
|
|
|
|
</verification>
|
|
|
|
|
|
|
|
|
|
<success_criteria>
|
|
|
|
|
- The stored-XSS todo's acceptance holds in the framework with a test of the exact exploit. The deploy docs give correct defence-in-depth advice for both static servers and public buckets.
|
|
|
|
|
- The after-commit todo's three items are done: README example, RegisterJoinTable comment and README line, and the AfterCommit warning, which is log-only and adds no API.
|
|
|
|
|
- The SUMMARY lists the follow-ups for the orchestrator: the fonoteka.go storeUpload (collection_media_controller.go:251) and StorePublicFile (album_files.go:35) adoption; the optional dedup in sm-feedback-plugin and sm-user-plugin; the deferred Store blob-metadata hardening for public buckets.
|
|
|
|
|
</success_criteria>
|
|
|
|
|
|
|
|
|
|
<output>
|
|
|
|
|
Create `.planning/quick/261028-r16-fix-the-two-lagoon-todos-derive-stored-u/261028-r16-SUMMARY.md` when done
|
|
|
|
|
</output>
|
Jakub Zych