Isolate the Playwright trace viewer from the admin origin - #133
Merged
Conversation
The engine mounted a second ActionDispatch::Static middleware over public/trace-viewer and embedded the viewer in an unsandboxed iframe on the artifact page. The viewer's service worker serialises a trace's tags and attributes into text/html, so a crafted trace ZIP executed script on the admin origin, where it could read admin pages and CSRF tokens. The viewer also accepted arbitrary ?trace= URLs and postMessage blobs from any origin, and its static responses carried none of the Rails security headers. Remove the vendored viewer and the static middleware. A trace artifact now renders a download link and the `npx playwright show-trace` command. Operators who want to open traces in the browser set config.trace_viewer_url to a viewer hosted outside config.hostname; Upright links to it in a new tab with rel="noopener noreferrer". Assigning a URL on the configured hostname, or a relative one, raises Upright::ConfigurationError, and the check runs again when the hostname is assigned so initializer order can't skip it.
There was a problem hiding this comment.
Pull request overview
Isolates Playwright trace rendering from Upright’s admin origin to prevent crafted traces from executing with admin privileges.
Changes:
- Removes the vendored trace viewer, static middleware, and synchronization task.
- Adds download-only trace handling with an optional validated external viewer.
- Adds configuration, integration tests, fixtures, and operator documentation.
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 19 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
app/assets/stylesheets/upright/artifact.css |
Replaces embedded-viewer styling with trace instructions styling. |
app/helpers/upright/artifacts_helper.rb |
Builds external trace-viewer links. |
app/views/upright/artifacts/show.html.erb |
Replaces the iframe with download and external-viewer options. |
CHANGELOG.md |
Documents the security fix and upgrade step. |
README.md |
Documents trace viewing and revised Playwright upgrades. |
lib/generators/upright/install/templates/upright.rb |
Documents the optional viewer configuration. |
lib/tasks/upright_tasks.rake |
Removes the viewer synchronization task. |
lib/upright/configuration.rb |
Adds and validates trace_viewer_url. |
lib/upright/engine.rb |
Stops mounting the engine’s public directory. |
public/trace-viewer/codeMirrorModule.DYBRYzYX.css |
Removes vendored CodeMirror styles. |
public/trace-viewer/defaultSettingsView.B4dS75f0.css |
Removes vendored viewer styles. |
public/trace-viewer/index.C5466mMT.js |
Removes the viewer application bundle. |
public/trace-viewer/index.CzXZzn5A.css |
Removes viewer loader styles. |
public/trace-viewer/index.html |
Removes the viewer entry point. |
public/trace-viewer/manifest.webmanifest |
Removes the viewer manifest. |
public/trace-viewer/playwright-logo.svg |
Removes the viewer logo. |
public/trace-viewer/sw.bundle.js |
Removes the vulnerable service worker. |
public/trace-viewer/xtermModule.DYP7pi_n.css |
Removes vendored terminal styles. |
test/fixtures/active_storage/attachments.yml |
Adds a trace attachment fixture. |
test/fixtures/active_storage/blobs.yml |
Adds a trace blob fixture. |
test/integration/artifacts_controller_test.rb |
Tests download-only, external-link, and 404 behavior. |
test/lib/upright/configuration_test.rb |
Tests viewer URL validation and assignment order. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The check had three raise paths, a rescue that rewrapped URI::InvalidURIError, and a second call from hostname= to cover initializer order. That is more machinery than the check earns: isolation comes from no longer shipping a viewer, so pointing config.trace_viewer_url at Upright's own hostname is an operator mistake in a file they wrote, not a bypass. One comparison in the writer, one message. A relative URL no longer raises — it resolves to a path Upright serves nothing at, so the link 404s instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (3)
lib/upright/configuration.rb:205
- The isolation check is case-sensitive even though DNS hostnames are not. For example, with
hostname = "upright.example.com",https://UPRIGHT.EXAMPLE.COM/...passes and the browser still reaches the admin origin. This parser also accepts non-HTTP schemes whenever they expose a host. Normalize both hosts and require an absolute HTTP(S) URI before comparing them.
port_suffix = Rails.env.local? ? "(:\\d+)?" : ""
hosts = [ /.*\.#{Regexp.escape(hostname)}#{port_suffix}/, /#{Regexp.escape(hostname)}#{port_suffix}/ ]
Array(@public_status_custom_domains).each do |domain|
hosts << /\A#{Regexp.escape(domain)}#{port_suffix}\z/
end
lib/upright/engine.rb:69
- Removing this middleware makes the existing smoke test fail on every run:
bin/smoke_test:291-301still requires/trace-viewer/index.htmlto return 200. Update that step to require 404 and describe the isolation check so deployment validation matches the new behavior.
app.config.assets.paths << root.join("app/javascript")
app/helpers/upright/artifacts_helper.rb:13
- Appending
?trace=as text breaks otherwise valid configured viewer URLs that already contain a query or fragment: the viewer receives notraceparameter (or it lands after#). Build the query throughURIso the new parameter is merged before any fragment.
trace = main_app.rails_blob_url(artifact, disposition: :inline, expires_in: 24.hours, host: request.host)
"#{viewer}?trace=#{CGI.escape(trace)}"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 26 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
app/helpers/upright/artifacts_helper.rb:13
- Appending
?trace=as a string breaks configured viewer URLs that already contain a query or fragment: the viewer never receives atraceparameter. Parse the configured URL and merge the parameter into its query instead.
trace = main_app.rails_blob_url(artifact, disposition: :inline, expires_in: 24.hours, host: request.host)
"#{viewer}?trace=#{CGI.escape(trace)}"
lib/upright/engine.rb:69
- Removing this static middleware makes
bin/smoke_testfail unconditionally: Step 10 at lines 291-301 still requires/trace-viewer/index.htmlto return HTTP 200. Update that smoke assertion to require 404 so the deployment check reflects the new security boundary.
app.config.assets.paths << root.join("app/javascript")
Comment on lines
+104
to
+108
| def trace_viewer_url=(url) | ||
| @trace_viewer_url = url.presence | ||
| host = URI(@trace_viewer_url.to_s).host.to_s | ||
|
|
||
| if @hostname.present? && (host == @hostname || host.end_with?(".#{@hostname}")) |
Member
Author
There was a problem hiding this comment.
Same thread as above — fixed in 9a4fcf7. Absolute http(s) URL with a host is now required, and the host comparison is case- and trailing-dot-insensitive.
Member
Author
There was a problem hiding this comment.
Splitting this thread's two halves, since one is now fixed and one is not.
Fixed in 9a4fcf7: relative and hostless values are refused — an http(s) URL with a host is required, and the host comparison folds case and strips a trailing dot. The check is #admin_domain? now.
Still open, deliberately: re-validating when hostname= is assigned. Leaving the thread unresolved for that point.
A viewer on another origin fetches the trace itself, so handing it an Active Storage blob URL didn't work: the fetch carries no session, the response carries no Access-Control-Allow-Origin, and the blob route redirects, which makes every response in the chain subject to the same check. The link opened the viewer and the viewer then failed to load anything. Serve the trace from Upright::TracesController instead. It sends Access-Control-Allow-Origin for the configured viewer's origin and nothing else, and 404s outright when no viewer is configured, so the unauthenticated route exists only where it is used. In place of the session it takes a signed id scoped to a purpose, expiring after 24 hours, and resolvable only to a blob attached to an Upright::ProbeResult under a .zip filename. Range requests are answered through ActiveStorage::Streaming, which the viewer needs to read a ZIP's central directory without pulling the whole archive, and Content-Range is exposed so it can read what it got back.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 29 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
app/helpers/upright/artifacts_helper.rb:12
- Appending a second
?breaks any valid viewer URL that already has a query, and appending after#puts the trace URL in the fragment. In either case the viewer receives notraceparameter. Parse the configured URL and merge the parameter into its query instead.
"#{viewer}?trace=#{CGI.escape(trace_url_for(artifact))}"
lib/upright/configuration.rb:108
- This accepts hostless/non-HTTP values such as
/trace-viewer: the artifact page then emits an admin-origin link, whiletrace_viewer_originreturns nil and the signed trace route always 404s. The hostname comparison is also case- and trailing-dot-sensitive, so DNS-equivalent admin hosts bypass the documented rejection. Validate an absolute HTTP(S) URL and compare canonicalized hosts before assigning it.
def trace_viewer_url=(url)
@trace_viewer_url = url.presence
host = URI(@trace_viewer_url.to_s).host.to_s
if @hostname.present? && (host == @hostname || host.end_with?(".#{@hostname}"))
Three fixes from Copilot's review: bin/smoke_test step 10 still required /trace-viewer/index.html to return 200, so every smoke run would have failed. It now requires 404, which makes it a regression check for this change rather than an obsolete step. The viewer URL took `?trace=` appended to it, which broke any viewer URL carrying a query of its own and hid the parameter behind a fragment. The parameter is merged into the query now. config.trace_viewer_url accepted values with no host, and compared hosts case-sensitively, so an equivalent spelling of the admin hostname passed the guard. It now requires an http(s) URL with a host, and compares hosts folded to lower case with any trailing dot removed. Not taken: re-checking the URL when hostname= is assigned. The hostname is set in config/environments/*.rb, which loads before initializers, so the ordering the review describes doesn't arise in a generated app, and a viewer URL is inert if it does — nothing is served at the admin origin to render a trace.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 30 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
lib/upright/configuration.rb:108
- The invalid value is stored before validation. If
ConfigurationErroris rescued, the configuration remains set to the rejected URL, so a same-admin-origin viewer can still be linked and used afterward. Restore the previous value on failure or validate the candidate before committing it.
def trace_viewer_url=(url)
@trace_viewer_url = url.presence
unless isolated_trace_viewer?
raise Upright::ConfigurationError, "config.trace_viewer_url must be an http(s) URL outside #{@hostname}"
ActiveStorage::Streaming#send_blob_stream sets neither Content-Length nor Accept-Ranges, and the viewer needs both: it sizes the archive from Content-Length before asking for a range, and won't ask at all without Accept-Ranges. Large traces loaded badly or not at all. Sets both on the non-range branch, matching ActiveStorage::Blobs::ProxyController. Range responses already carried Content-Range and Accept-Ranges from send_blob_byte_range_data. Adds a private cache window bounded by the signed id's lifetime rather than the proxy controller's http_cache_forever, since a trace URL carries its own authorization and expires. Skips the session with ActiveStorage::DisableSession, as the proxy controller does.
The viewer's service worker attaches its own request header, which makes even a GET preflighted. The route answered no OPTIONS, so the preflight 404'd and every resource fetch behind it failed — a trace opened but its screenshots didn't load. The route takes OPTIONS now and answers with the methods and headers the preflight asks about. config.trace_viewer_url defaults to https://trace.playwright.dev, overridable with TRACE_VIEWER_URL or by assigning the setting, and nil for download-only traces. A trace artifact shows the viewer link alone when a viewer is configured, and the `npx playwright show-trace` command when none is. Renames the isolation check to #admin_domain?, which says what it asks, and drops the commentary the method and constant names already carry.
Merging the parameter kept an existing one, and URLSearchParams#get returns the first value, so a viewer URL configured with ?trace=old opened that trace for every artifact. The existing entry is dropped before ours is added.
Two defects blocked the viewer, both confirmed against deployed staging. The preflight returned 422. OPTIONS is not a safe method to Rails, so forgery protection rejected it before the controller ran, and every fetch behind the preflight failed. The controller skips forgery protection; there is no session here to forge against. Every response was chunked with no Content-Length, including the 206 where send_blob_byte_range_data sets it explicitly. ActiveStorage::Streaming includes ActionController::Live, which streams every response from the controller and drops the header. A HEAD reported Content-Length 0, and the viewer reads that as an empty archive, so it rendered nothing. Drops ActiveStorage::Streaming and answers from send_data instead: HEAD reports the byte size, a ranged GET reports its slice with Content-Range, and a full GET reports the whole length. Ranges still come from Rack::Utils.get_byte_ranges and ActiveStorage::Blob#download_chunk, so only the fallback whole-archive read buffers, and the viewer reaches for ranges once it can see the size.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged the skip as rb/csrf-protection-disabled, and it was the wrong tool for the problem. Only the CORS preflight tripped the check: OPTIONS is not a safe method to Rails, so verification ran and answered it with a 422. Every real request here is a GET or a HEAD, which Rails treats as safe and never checks. The preflight is answered from a prepended callback instead, so it returns before verification runs and the protection stays in the chain for anything added later. The preflight tests now run inside with_forgery_protection. Without it they passed against a test environment that disables the check, which is why CI was green while staging returned 422.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upright vendored the Playwright Trace Viewer and served it from the same origin as the admin application. A trace is untrusted input and the viewer renders its contents, so a trace could run as script with the admin session.
Details are on the card (H-01).
What changes
Upright no longer ships or serves the viewer. A trace artifact shows a download link and the command to open it locally:
To open traces in a browser, set
config.trace_viewer_urlto a viewer you host outsideconfig.hostname. Upright links to it in a new tab. Leave it unset and traces stay download-only. A URL underconfig.hostnameraisesUpright::ConfigurationError.Serving a trace to that viewer
The viewer fetches the trace from its own origin, so the request arrives without a session.
Upright::TracesControllerserves it and:Access-Control-Allow-Originfor the configured viewer's origin only..zipblob attached to anUpright::ProbeResult.That is narrower than the Active Storage URL it replaces, whose signed ID names any blob and doesn't expire by default.
Upgrading
Opening a trace in a browser stops working until you set
config.trace_viewer_url. The changelog records this.Tests
340 runs, 788 assertions, no failures. RuboCop and Brakeman are clean.
test/integration/traces_controller_test.rbcovers the CORS header, a range request, and four refusals: no viewer configured, an artifact that isn't a trace, a signed ID for another purpose, and a tampered ID.The card also asks for a test that exercises a crafted trace. There is nothing left to exercise here, because Upright no longer renders trace contents.
🤖 Generated with Claude Code