Skip to content

Support session token authentication for direct uploads#2706

Open
rbarbosa wants to merge 1 commit intobasecamp:mainfrom
rbarbosa:pr/direct-uploads-session-auth
Open

Support session token authentication for direct uploads#2706
rbarbosa wants to merge 1 commit intobasecamp:mainfrom
rbarbosa:pr/direct-uploads-session-auth

Conversation

@rbarbosa
Copy link

@rbarbosa rbarbosa commented Mar 16, 2026

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

  • The Active Storage direct upload endpoint only skips CSRF protection for bearer token auth, causing 422 errors for native mobile apps using magic link (session-based) authentication
  • Broadens the skip_forgery_protection condition to also bypass CSRF for authenticated JSON requests
  • Adds test coverage for session-token direct uploads and cross-account authorization

Security considerations

This is safe because:

  • Still requires valid authentication (session or bearer token)
  • Only applies to JSON requests (web UI remains CSRF-protected)
  • CSRF attacks target browser ambient authority — mobile JSON API requests aren't vulnerable

Test plan

  • Existing bearer token tests pass
  • New test: session token + JSON → 201 success
  • New test: session token + wrong account → 403 forbidden

@flavorjones
Copy link
Member

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>
@rbarbosa rbarbosa force-pushed the pr/direct-uploads-session-auth branch from d67b43b to a070641 Compare March 18, 2026 21:43
Copilot AI review requested due to automatic review settings March 18, 2026 21:43
@rbarbosa
Copy link
Author

Thanks @flavorjones! Let me know if anything else is needed.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_protection for ActiveStorage::DirectUploadsController to 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.

Comment on lines +48 to +56
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?) }
Copy link
Contributor

@robzolkos robzolkos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants