Support session token authentication for direct uploads#2706
Support session token authentication for direct uploads#2706rbarbosa wants to merge 1 commit intobasecamp:mainfrom
Conversation
|
The security check can be ignored -- will go away if this branch is rebased onto main. |
The Active Storage direct upload endpoint only skips CSRF protection for bearer token authentication. Native mobile apps using magic link (session-based) authentication get 422 errors when uploading files, because they don't send CSRF tokens for JSON API requests. Broaden the skip_forgery_protection condition to also bypass CSRF for authenticated JSON requests, which is safe since the session token is already validated and CSRF attacks don't apply to non-browser API clients. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d67b43b to
a070641
Compare
|
Thanks @flavorjones! Let me know if anything else is needed. |
There was a problem hiding this comment.
Pull request overview
This PR updates ActiveStorage direct upload CSRF behavior so native/mobile clients using session-token (magic link) authentication can successfully create direct uploads via JSON, and adds integration tests to cover this flow and cross-account authorization.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Broaden
skip_forgery_protectionforActiveStorage::DirectUploadsControllerto include authenticated JSON requests (in addition to bearer token auth). - Add direct-upload tests for session-token JSON requests and cross-account access.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| config/initializers/active_storage.rb | Expands the CSRF-skip condition for direct uploads to support session-authenticated JSON clients. |
| test/controllers/active_storage/direct_uploads_controller_test.rb | Adds integration tests for session-token direct upload success and forbidden cross-account behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test "create with session token in another account is forbidden" do | ||
| sign_in_as :david | ||
|
|
||
| post rails_direct_uploads_path(script_name: "/#{ActiveRecord::FixtureSet.identify("initech")}"), | ||
| params: @blob_params, | ||
| as: :json | ||
|
|
||
| assert_response :forbidden | ||
| end |
| include Authentication | ||
| include Authorization | ||
| skip_forgery_protection if: :authenticate_by_bearer_token | ||
| skip_forgery_protection if: -> { authenticate_by_bearer_token || (authenticated? && request.format.json?) } |
robzolkos
left a comment
There was a problem hiding this comment.
I may be missing something, but I think this is very close and mostly needs a CSRF-enabled test to show the session-auth path is really covered.
The bit I’m unsure about is that this skip_forgery_protection condition depends on authenticated?, and I think that only gets established during require_authentication. My read is that the Active Storage forgery callback runs before that, so I’m not yet convinced a session-backed JSON direct upload will skip CSRF in practice.
Could you please add a test variant with forgery protection explicitly enabled? There’s a good example of the setup/teardown pattern in test/controllers/concerns/request_forgery_protection_test.rb, and the existing sign_in_as helper lives in test/test_helpers/session_test_helper.rb. If that passes for the signed-in session upload flow, this would increase confidence that this works as intended.
Context
I'm building a native iOS client that uses magic link (session token) authentication. Image and file uploads are core functionality, but since the direct upload endpoint only bypasses CSRF for bearer token auth, session-based clients get 422 errors. This has been a real blocker for me.
Summary
skip_forgery_protectioncondition to also bypass CSRF for authenticated JSON requestsSecurity considerations
This is safe because:
Test plan