Skip to content

Serve the framed Prometheus and Alertmanager pages from the app subdomain - #148

Merged
lewispb merged 1 commit into
mainfrom
framed-pages-outside-proxy-gate
Sep 7, 2026
Merged

lewispb merged 1 commit into
mainfrom
framed-pages-outside-proxy-gate

Conversation

@lewispb

@lewispb lewispb commented Sep 7, 2026

Copy link
Copy Markdown
Member

https://ams.37upright-staging.com/framed/prometheus and the same page in development return 403 on a plain visit.

Cause. The framed pages are the show action of the proxy controllers, so Upright::ProxyAuthentication's Fetch-Metadata gate runs on them, and it refuses Sec-Fetch-Site: same-site. That is how a browser reports a navigation from app.<host> to ams.<host>, and the header makes exactly that navigation: Upright::Current.site falls back to the host's own site, so on the app subdomain the "Prometheus" link points at ams.<host>/framed/prometheus. The sign-in redirect back from the app subdomain fails the same way, as does a request with no Fetch-Metadata headers at all.

Fix. The gate applies to the proxy action only. The framed page forwards nothing to an upstream; it renders an iframe and stays on the ordinary session path. The proxy behind the iframe is unchanged and is still refused for same-site requests.

Tests. For each page: served to a session arriving same-site, still redirects to sign-in without a session, and the proxy path itself is still refused same-site.

Independent of #147, which does not touch this behaviour.

Copilot AI balanced review requested due to automatic review settings September 7, 2026 08:13
…main

The framed pages share a controller with the proxies, so the proxy's
Fetch-Metadata gate ran on them and refused Sec-Fetch-Site: same-site. That is
how a browser reports a navigation from the app subdomain to a site subdomain,
and the header links there from every app page: Upright::Current.site falls
back to the host's own site, so on the app subdomain the Prometheus link points
at that site's /framed/prometheus. The result was a 403 on /framed/prometheus
and /framed/alertmanager. The pages forward nothing; the gate now applies to
the proxy action only.
@lewispb
lewispb force-pushed the framed-pages-outside-proxy-gate branch from 2ac5bd7 to 120aa42 Compare September 7, 2026 08:14
@lewispb lewispb changed the title Serve the framed Prometheus and Alertmanager pages after a same-site redirect Serve the framed Prometheus and Alertmanager pages from the app subdomain Sep 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The narrowly scoped callback change preserves proxy protections and has appropriate regression coverage.

Pull request overview

Scopes Fetch-Metadata protection to upstream proxy requests while allowing authenticated framed admin pages after same-site redirects.

Changes:

  • Exempts framed show actions from the proxy gate.
  • Adds regression tests and documents the fix.

[!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.

File summaries
File Description
app/controllers/concerns/upright/proxy_authentication.rb Exempts framed pages from Fetch-Metadata filtering.
test/integration/prometheus_proxy_controller_test.rb Covers framed-page access and proxy rejection.
test/integration/alertmanager_proxy_controller_test.rb Covers framed-page access and proxy rejection.
CHANGELOG.md Documents the resolved 403 behavior.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@lewispb
lewispb merged commit e749783 into main Sep 7, 2026
8 checks passed
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.

2 participants