Harden blob serving and pagination URL generation - #3040
Conversation
There was a problem hiding this comment.
Pull request overview
Hardens pagination URL generation and Active Storage blob serving against a same-origin XSS chain.
Changes:
- Filters URL-generation controls from pagination parameters.
- Restricts unattached blobs to same-account users.
- Forces scriptable MIME types to download as binary.
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.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
app/helpers/pagination_helper.rb |
Filters pagination URL parameters. |
lib/rails_ext/active_storage_authorization.rb |
Hardens blob authorization and MIME handling. |
test/helpers/pagination_helper_test.rb |
Covers malicious pagination parameters. |
test/integration/pagination_blob_html_xss_test.rb |
Exercises the cross-account blob XSS chain. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| URL_FOR_CONTROL_OPTIONS = %i[ | ||
| script_name host protocol port subdomain domain tld_length | ||
| trailing_slash only_path relative_url_root anchor params _recall | ||
| ].freeze |
There was a problem hiding this comment.
Fixed original_script_name — added it to URL_FOR_CONTROL_OPTIONS (RouteSet#url_for prepends it to script_name before route generation, same vector as script_name) with a regression test that fails without the filter.
On use_route: I tested it against the real pagination path (a valid named route, filter removed) and it does not rewrite the generated href. Pagination always forwards the router-set controller/action, which a query string cannot override, and explicit controller/action take precedence over use_route in url_for generation — so the named route is never selected. No reachable failure here, so I have not filtered it. Leaving this thread open for a human to confirm that call.
On the allowlist suggestion (nest query params under params:): that would break parameterized routes here. Pagination must keep path segments like board_id at top level so url_for can regenerate the current route (e.g. /public/boards/the-board/columns/...); nesting them under params: pushes them into the query string and the route no longer resolves. The set of control options url_for strips is finite and enumerable, so the denylist is the fitting instrument.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab976a32f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef11d22a4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| URL_FOR_CONTROL_OPTIONS = %i[ | ||
| script_name original_script_name host protocol port subdomain domain | ||
| tld_length trailing_slash only_path relative_url_root anchor params | ||
| _recall |
There was a problem hiding this comment.
use_route before URL generation
When a pagination request includes use_route, RouteSet#url_for consumes it outside RESERVED_OPTIONS to select a named route rather than treating it as a query parameter. An attacker can therefore supply a blob-proxy route name together with its signed blob ID and filename parameters, causing automatic pagination to target that route despite the new blocklist; add use_route to the filtered control options so this path-retargeting boundary is actually closed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Traced this against the actual call path and it doesn't reproduce. The helper builds the href with the view's url_for, which always merges url_options — including _recall (the current request's path_parameters). With _recall present, url_for regenerates the current route and ignores use_route: supplying use_route=rails_service_blob_proxy&signed_id=…&filename=… (or even a nonexistent route name) still produces /public/boards/:board_id/columns/stream?…, not the blob-proxy path — the route name falls through. A bare Rails.application.routes.url_helpers.url_for (no recall) does retarget, but that isn't the path pagination takes.
_recall is attacker-uncontrollable here: it's already in the stripped URL_FOR_CONTROL_OPTIONS, and the trusted value comes from the view context. So use_route can't select a foreign route through this helper. script_name/original_script_name are different — they're prepended to the generated path regardless of recall, which is why those are stripped. Leaving use_route out of the denylist; happy to revisit if a call site reaches url_for without recall.
| def media_type_for_serving | ||
| content_type.to_s.split(";").first.to_s.strip.downcase |
There was a problem hiding this comment.
When a direct upload declares a malformed type such as text/html,foo, this normalization leaves it unchanged, so neither this list nor Active Storage's exact binary list recognizes it. Turbo's frame-response HTML check accepts a Content-Type beginning with text/html followed by a word boundary (including a comma), and an attachment disposition does not prevent a fetched frame response from being parsed, so a publicly attached blob—or an unattached blob requested by another user in the same account—can still execute the payload; normalize or conservatively reject non-canonical media types rather than splitting only on semicolons.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a99d0f1. media_type_for_serving now drops everything from the first ;, ,, or whitespace, so text/html,foo (and other non-canonical forms) normalize to text/html, hit the dangerous-inline list, and are forced to application/octet-stream.
Added an integration regression that uploads an unattached blob declaring content_type: "text/html,foo" and asserts it's served as octet-stream with an attachment disposition. Verified it fails without the normalization change (the blob is served inline as text/html,foo).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
test/integration/pagination_blob_html_xss_test.rb:35
- This request still uses the integration default
/37signalsprefix (test/test_helper.rb:78), but Mike only belongs to Initech (test/fixtures/users.yml:39-44).Current.useris therefore nil and the test passes onuser.present?, without exercising the newaccount_id == user.account_idcheck. Send the blob URL through the Initech prefix so this is genuinely an authenticated cross-account authorization test.
sign_in_as :mike
get rails_storage_proxy_path(blob)
assert_response :forbidden
| end | ||
|
|
||
| def forcibly_serve_as_binary? | ||
| super || dangerous_inline_media_type? |
There was a problem hiding this comment.
The normalized essence is only used for the dangerous-inline check (HTML/XML/SVG — the scriptable types this fix targets), and it now catches the parameterized/malformed forms of those via media_type_for_serving. super still compares the original content_type against Active Storage's configured content_types_to_serve_as_binary, and that comparison is purely additive: it can only add force-binary cases, never undo one our check already made.
The gap described — a non-dangerous type that's in AS's configured binary list only in a parameterized form (e.g. application/zip; x) being served inline — is Active Storage's own exact-string matching, pre-dates this PR, and isn't a scripting/XSS vector (those types aren't executed inline). Normalizing for AS's full configured binary list is a separate, non-security change I'd rather not fold into this fix. The scriptable types, which are the ones that matter for #3943339, are all covered.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| URL_FOR_CONTROL_OPTIONS = %i[ | ||
| script_name original_script_name host protocol port subdomain domain | ||
| tld_length trailing_slash only_path relative_url_root anchor params | ||
| _recall | ||
| ].freeze |
There was a problem hiding this comment.
Checked this against the actual call path and it doesn't reach a retarget. path_params only supplies values for a named route's dynamic segments (unused entries discarded); pagination generates via controller/action, not a named route, so url_for discards it entirely. Even forcing it — path_params: { board_id: "rails/active_storage/blobs" } in the request — the href stays /public/boards/the-board/columns/stream?page=2: the trusted board_id segment (from the request's path parameters / url_for recall) wins, and the injected value is dropped, not forwarded as a query parameter.
This is the same category as use_route above — a url_for option that the trusted recalled path parameters neutralize at this call site. The denylist strips the class that retargets regardless of recall: script_name/original_script_name, which are prepended to the generated path, and those carry regression tests that fail without the filter. For the recall-neutralized options, adding an entry only grows the list with a test that passes with or without it — no reachable path covered. Leaving path_params out; happy to add it if you can show a request that actually moves the href.
Three defense-in-depth fixes closing a same-origin script-execution chain that let one authenticated user run JavaScript in another user's session via a crafted public-board URL. - Pagination links built the next-page URL by feeding every request parameter to url_for, which interprets reserved keys (script_name, host, only_path, ...) as URL-generation control options. A request-supplied script_name could rewrite the generated path onto an unrelated same-origin route. Forward only ordinary query params; strip url_for control options. - Active Storage authorized any blob with no attachments to any authenticated principal, so a deliberately-unattached blob was viewable across account boundaries. Scope the unattached-blob allowance (the transient pre-attachment window) to principals within the blob's own account. - Normalize the media type before the serve-as-binary check and force HTML/XML/SVG blobs to application/octet-stream, so a parameterized content type (e.g. text/html;charset=utf-8) can no longer slip past the exact-match binary list and be served inline as executable HTML. Adds regression coverage for each link.
url_for prepends original_script_name to script_name before route generation, exactly as it does script_name, so an attacker who slips it into the request query can rewrite the generated pagination path onto an arbitrary same-origin route (the parameter-injection boundary this change closes). Add it to URL_FOR_CONTROL_OPTIONS with regression coverage.
media_type_for_serving split only on ";", so a malformed declared type such as "text/html,foo" passed through unchanged — matching neither the dangerous-inline list nor Active Storage's exact binary list, so it was served inline while a client's text/html prefix check still treated it as HTML. Drop everything from the first ";", ",", or whitespace so the essence normalizes to text/html and is forced to binary (HackerOne #3943339).
The permit! fingerprint changed when it moved into forwardable_request_params. Also drop a stale Card::Entropic entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a99d0f1 to
41d29cd
Compare
flavorjones
left a comment
flavorjones
left a comment
There was a problem hiding this comment.
@jeremy text/x-html would get through this and still be served as HTML by turbo. The failing test would be:
test "unattached blob with an html media type outside the dangerous list is served as octet-stream" do
blob = create_unattached_html_blob(content_type: "text/x-html")
sign_in_as :david
get rails_storage_proxy_path(blob)
assert_response :success
assert_equal "application/octet-stream", response.media_type
endExpected: "application/octet-stream"
Actual: "text/x-html"
Turbo parses a frame response as HTML whenever its content type matches FetchResponse#isHTML, which is /^(?:text\/([^\s;,]+\b)?html|application\/xhtml\+xml)\b/. Any text/<prefix>html matches.
On attach, Marcel only overrides the declared type when it recognizes the bytes. This payload is a bare <turbo-frame> fragment with no HTML signature, so text/x-html survives identification and an attached card image serves it too.
One solution would be to copy Turbo's regular expression:
TURBO_HTML_MEDIA_TYPE = %r{\A(?:text/(?:[^\s;,]+\b)?html|application/xhtml\+xml)\b}
def dangerous_inline_media_type?
media_type_for_serving.match?(TURBO_HTML_MEDIA_TYPE) ||
ActiveStorage::DANGEROUS_INLINE_MEDIA_TYPES.include?(media_type_for_serving)
endbut that feels gross and prone to rot.
Another idea is to use Mime::Type#html?, which is (symbol == :html) || @string.include?("html") and so is broader than the regex (e.g. text/xhtml):
def dangerous_inline_media_type?
media_type = Mime::Type.lookup(media_type_for_serving)
media_type&.html? ||
ActiveStorage::DANGEROUS_INLINE_MEDIA_TYPES.include?(media_type_for_serving)
endI'm not sure which one is better, probably the second if we can convince ourselves it's sufficient.
Summary
Closes a same-origin script-execution chain (HackerOne #3943339) where one
authenticated user could run JavaScript in another user's session when the
victim opened a single crafted public-board URL, then mint and exfiltrate a
victim Identity access token. Reproduced end-to-end and fixed with
defense-in-depth on three independent links — any one of which breaks the
chain.
The chain
Pagination URL generation forwarded url_for control options.
pagination_linkbuilt the next-page URL fromparams.permit!.to_h, whichis fed straight to
url_for.url_fortreats reserved keys (script_name,host,only_path, …) as URL-generation options, so a request-suppliedscript_namerewrote the beginning of the generated path onto anothersame-origin route (an Active Storage blob-proxy path), and an encoded
#truncated the rest into a fragment.
Automatic pagination fetches that URL into a live Turbo frame with no
click. The public column stream uses
with_automatic_pagination; itsIntersectionObserver loads the poisoned link into a native
<turbo-frame src>, whose FrameRenderer executes<script>with a copied CSP nonce.Active Storage authorized unattached blobs to anyone. The blob
authorization helper returned
... || attachments.none?, so adeliberately-unattached blob was viewable by any authenticated principal —
across account boundaries.
Parameterized MIME slipped past the binary list. Active Storage's
serve-as-binary check is an exact-string match;
text/html;charset=utf-8is not in the default list, so the attacker's HTML was served inline as
text/htmland parsed by Turbo as a frame.Downstream: the same-origin script mints an Identity write token from
/my/access_tokens.jsonand exfiltrates it. This endpoint is intentionallyreachable; the fix breaks the chain upstream so the script never runs.
Fixes
app/helpers/pagination_helper.rb— forward only ordinary query paramsinto
url_for; strip the url_for control options (script_name,host,port,only_path,anchor, …). Routing keys (controller/action) comefrom routing, not the query string, and are left in place.
lib/rails_ext/active_storage_authorization.rb(authorization) — scopethe unattached-blob allowance (which exists for the transient window between
direct upload and attachment) to principals within the blob's own account.
Cross-account access to an unattached blob now falls through to
forbidden.lib/rails_ext/active_storage_authorization.rb(serving) — normalize themedia type before the serve-as-binary comparison and force HTML/XML/SVG
blobs to
application/octet-stream, so a parameterized content type can nolonger be served inline as executable markup.
Tests
test/integration/pagination_blob_html_xss_test.rb— a cross-accountunattached HTML blob is forbidden; an in-account HTML blob is served as
octet-stream (attachment), never executable
text/html; documents that thetoken-mint endpoint returns a raw identity-scoped write token.
test/helpers/pagination_helper_test.rb— the next-page link ignoresscript_name/host/only_pathwhile preserving legitimate filter params,and pins that automatic pagination selects the script-executing native
Turbo-frame branch.
Each new test was confirmed to fail before its corresponding fix. The existing
Active Storage authorization suite (52 tests) and the helpers/public-controller
suites remain green.
Notes for reviewers
account_id(NOT NULL, defaulted toCurrent.account), so theauthorization scoping uses account membership. There is no per-blob
identity creator column; account scoping closes the cross-account vector.
image/svg+xml) to binarytoo — inline SVG from the app origin is a scriptable XSS vector. If any flow
relies on inline SVG rendering, flag it.
script-src-absent origin (structural, larger change). Not done here; worth
considering as a follow-up.