Commit Graph

4 Commits

Author SHA1 Message Date
Jeremy Daer 0141eae018 Address review: assert uploaded bytes in disk update test
direct_upload_url_for now returns [blob, url] so the authenticated update
test can assert blob.download matches the uploaded payload, not just the
204 status — guarding against a write landing at the wrong key or with the
wrong content. Both update call sites destructure the tuple.
2026-06-15 13:24:38 -07:00
Jeremy Daer c49c41a0d7 Address review: cover DiskController#update in tests
Add integration coverage for the disk service PUT now that the initializer
relies on it being both session-gated and CSRF-exempt:

- unauthenticated PUT -> redirected to login (write blocked)
- authenticated PUT with forgery protection enabled and no authenticity
  token -> 204 (proves the signed-token service PUT stays CSRF-exempt and
  guards against the concern's protect_from_forgery re-arming it)
2026-06-15 13:18:40 -07:00
Jeremy Daer 49f06d0b22 Address review: keep disk PUT CSRF-exempt, use intent helpers
- Including Authentication re-arms protect_from_forgery on DiskController.
  Active Storage's direct-upload service PUT (#update) sends only signed
  service headers and no CSRF token, so a real authenticated upload would
  422 storing bytes. Re-exempt #update from forgery protection; the signed
  URL token and session check still gate the write.
- Swap the raw skip_before_action for the Authentication concern's
  intent-revealing allow_unauthenticated_access / allow_bot_access helpers
  on #show, matching the rest of the app.
- Scope the test's ActiveStorage::Current.url_options override to a
  set { } block so it can't leak thread-local state into later tests.
2026-06-15 13:10:59 -07:00
Jeremy Daer ed5a172871 Require authentication for ActiveStorage direct-upload write endpoints
ActiveStorage's direct-upload endpoints ship unauthenticated by Rails
default: ActiveStorage::DirectUploadsController and DiskController inherit
from ActionController::Base, so they bypass the app's Authentication
concern. That leaves the write path open — anyone could mint blob records
and PUT bytes to local disk storage.

Campfire never uses direct upload for legitimate attachments. Those flow
through MessagesController#create (already authenticated), and Trix file
drops are disabled in the composer. Gating the write path is therefore
pure defense-in-depth with no functional cost.

Require an authenticated session on the two write actions
(DirectUploadsController#create and DiskController#update) by including the
existing Authentication concern. Blob serving stays public —
DiskController#show keeps its auth skip, and the Blobs/Representations
controllers are untouched — so message attachments and the account logo
keep loading. Because these controllers live in ActiveStorage::Engine and
only see the engine's url helpers, also include the application route
helpers so the concern can redirect to new_session_url on failure (302,
write blocked).
2026-06-15 13:00:29 -07:00