fix(middleware): exempt PUT /books/{id}/content from 1MB MaxBytes cap (bookshelf-t582g.4.3) #1246
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-t582g.4.3"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
isUploadPathininternal/middleware/max_bytes.goonly exemptedPOST /books/{id}/filesfrom the 1MB body cap. The replace-content route (PUT /books/{id}/content) was not exempt, so any real book file (>1MB) was rejected with 413 before reaching the handler's own 2GB cap.PUT /books/{id}/contentto the upload path exemption. The handler already applieshttp.MaxBytesReader(w, r.Body, maxUploadBodyBytes)+ the service'sio.LimitReader, so the middleware exemption is safe.MaxBytes→MethodOverridechain returns 200.app.New()middleware stack reaches the handler and returns 200.Test plan
go test ./internal/middleware/...— 325 specs, all green, 100% coveragego test ./internal/...— all internal packages passgo build -tags e2e ./e2e/...— e2e compilesgolangci-lint run ./internal/middleware/... ./e2e/api/...— 0 issuesCloses bead bookshelf-t582g.4.3 on merge.
The replace-content route streams a raw body directly into the existing book file, bypassing the middleware's 1MB cap entirely before the handler's own 2GB cap could apply — causing any real book file (>1MB) to 413. Fix: extend isUploadPath in max_bytes.go to also exempt PUT /books/{id}/content (alongside the existing POST .../files exemption). The handler already applies its own http.MaxBytesReader + service-level io.LimitReader cap (2 GB), so the middleware exemption is correct. Tests: - Unit (body_cap_chain_test.go): RED→GREEN test confirming a >1MB raw PUT through the full MaxBytes→MethodOverride chain returns 200, not 413. - e2e (journey_3_bookdrop_ingest_test.go): full-stack assertion that a >1MB PUT through the real wired app.New() stack reaches the handler. Closes bead bookshelf-t582g.4.3 on merge. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Security Review — PR #1234
Reviewed the diff (removes an inert
main:has(.book-show){max-width:none}rule + corrects a stale CSS comment instatic/css/main.css).Verification performed:
max-widthis set onmainanywhere else inmain.css, andmax-width:noneis the CSS initial value, so removal changes nothing rendered.max-widthis a pure layout property — notcontain,overflow, visibility,clip, or any containment/isolation control. No security-relevant affordance is dropped..reader-viewercarriesmax-width:700px; the previously-named.reader-columnhas no max-width (flex:1). The corrected reference points at the element that actually constrains width.No findings.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Security Review — PR #1246 (exempt PUT /books/{id}/content from 1 MB MaxBytes cap)
Scope:
internal/middleware/max_bytes.goexemption + wiring,internal/books/replace_content_{handler,service,store}.go, test additions.Adversarial focus verification
(1) Exemption is NARROW — confirmed.
isUploadPathnow returns true forPUT+ prefix/books/+ suffix/content(in addition to the existingPOST .../files). Enumerated every non-GET route ending in/contentunder/books/: the only match is the single registered routePUT /books/{id}/content(internal/books/routes.go:175). The other/contentroute isGET .../file/{fileID}/content— GET is never body-capped and doesn't match the PUT check. No other PUT/POST/PATCH/DELETE route ends in/content, so an attacker cannot steer a huge body to a different endpoint via this suffix. Exemption is path-based, not Content-Type-based, so a forgedmultipart/form-dataheader on any other route is still capped.(1b) Method-override cannot widen the exemption — confirmed. Chain order is
MaxBytes -> CSRF -> MethodOverride -> mux(internal/app/app.go:940-943). MaxBytes evaluatesisUploadPathwhile the method is still the wire method (POST), before_method=PUTrewriting; a forgedPOST ...?_method=PUTto a non-/contentpath is capped at 1 MB. No bypass.(2) Exempted route still has a REAL upper bound — confirmed. Not unbounded. Two independent layers cap it at 2 GB: the handler wraps the body with
http.MaxBytesReader(w, r.Body, maxUploadBodyBytes)wheremaxUploadBodyBytes = MaxUploadBytes + 4096andMaxUploadBytes = 2 << 30(replace_content_handler.go:77,upload_service.go:34), and the service independently enforces it withio.LimitReader(src, limit+1)+ a written-count check returningErrFileTooLarge(replace_content_service.go:150-163). Streamed via temp+rename (no full buffering in memory) -> no memory/disk DoS.(3) Auth/ownership gating — confirmed. Route is gated by
g.EditMetadata(PermissionEditMetadata) (routes.go:175).userIDis session-sourced (userIDFromRequest = d.ExtractUser(r).ID,wire.go:640), never request-supplied. The file lookupGetBookFileForReplacescopes by user viaJOIN user_library_mapping ulm ON ulm.library_id = b.library_id AND ulm.user_id = ?(replace_content_store.go), so a user cannot overwrite a file in a library they can't access; a miss returnssql.ErrNoRows -> ErrNotFound(404, no existence leak). Path-containment check rejectsfile_sub_pathtraversal outside the library root.Other notes
package middleware_test,package api_test); the e2e spec carries the required cannot-test-at-unit-level justification and asserts the exemption end-to-end throughapp.New().internal/dbtest/dbtest.gochange (bounded 30s drop-timeout +ctxpropagation on cleanup) is a test-harness resilience fix, unrelated to the exemption; no security impact.No security findings.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
8517df536ca3c007688c