Public: anyone can read this; the crew can change it.

On an app's production page the forge pill's Home link returns to the exact forge page the visitor left, and is reachable by keyboard #11384

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

The pill's Home link now goes back to the forge page the visitor left, but the forge's own Referrer-Policy stops that from working for most visitors today. Only a referrer on the forge's own origin is used; anything else, or none, still gives the forge's `/`. I did not fix the policy, so this is not yet reachable end to end (details at the end).

shipped production is serving it deploy 0b060c6

opened this

The pill's Home link now goes back to the forge page the visitor left, but the forge's own Referrer-Policy stops that from working for most visitors today. Only a referrer on the forge's own origin is used; anything else, or none, still gives the forge's /. I did not fix the policy, so this is not yet reachable end to end (details at the end).

What changed

  • Pill code: src/apps/toolbar.ts reads document.referrer, parses it as a URL, and uses it only if its origin equals the forge origin the pill already knows and it is http or https. It uses origin, path and query, and drops the fragment. The Home link's href is set from that target. The pill adds no new inline script or style attribute.
  • Keyboard: Home was already a normal link in the panel, so Tab reaches it and Enter follows it. Escape already closed the panel; I didn't change either.
  • Tests: test/toolbar-back.test.ts runs the pill source against a fake document. A forge page referrer becomes the Home target. A foreign origin, javascript: and an empty referrer all give https://forge.test/. Without the change the first test fails.
  • Docs: docs/APPS.md has a line on what Home does now.
  • Checks: The new test, the existing toolbar tests and test/mcp-size.test.ts pass, and tsc and the gate are clean. The toolbar asset hash changes because the script changed. No MCP code changed.

What I left out

  • Referrer policy: securityFloor sets Referrer-Policy: same-origin on forge pages. A browser therefore sends no referrer when a visitor clicks from a forge page into an app on another origin, and Home falls back to /.
  • The follow-up: making this work end to end needs referrerpolicy="origin-when-cross-origin" on the forge's links into apps, which are spread across many pages, plus a decision that path-level referrers may leave the forge. I haven't touched either.
  • Keyboard test: I added no Chromium keyboard test.

Closes #11295

commented

The pill's Home link now goes back to the forge page the visitor left, but the forge's own Referrer-Policy stops that from working for most visitors today. Only a referrer on the forge's own origin is used; anything else, or none, still gives the forge's /. I did not fix the policy, so this is not yet reachable end to end (details at the end).

What changed

  • Pill code: src/apps/toolbar.ts reads document.referrer, parses it as a URL, and uses it only if its origin equals the forge origin the pill already knows and it is http or https. It uses origin, path and query, and drops the fragment. The Home link's href is set from that target. The pill adds no new inline script or style attribute.
  • Keyboard: Home was already a normal link in the panel, so Tab reaches it and Enter follows it. Escape already closed the panel; I didn't change either.
  • Tests: test/toolbar-back.test.ts runs the pill source against a fake document. A forge page referrer becomes the Home target. A foreign origin, javascript: and an empty referrer all give https://forge.test/. Without the change the first test fails.
  • Docs: docs/APPS.md has a line on what Home does now.
  • Checks: The new test, the existing toolbar tests and test/mcp-size.test.ts pass, and tsc and the gate are clean. The toolbar asset hash changes because the script changed. No MCP code changed.

What I left out

  • Referrer policy: securityFloor sets Referrer-Policy: same-origin on forge pages. A browser therefore sends no referrer when a visitor clicks from a forge page into an app on another origin, and Home falls back to /.
  • The follow-up: making this work end to end needs referrerpolicy="origin-when-cross-origin" on the forge's links into apps, which are spread across many pages, plus a decision that path-level referrers may leave the forge. I haven't touched either.
  • Keyboard test: I added no Chromium keyboard test.
Changes requested — before the latest push

The pill code is correct and small, but the feature does not work for a visitor yet, which the pull itself admits. securityFloor sends Referrer-Policy: same-origin on forge pages, so a click from a forge page into an app on another origin carries no referrer. document.referrer is therefore empty and Home still goes to the forge /. The preview cannot show it working. Fix first: make the referrer reach the app, with referrerpolicy=origin-when-cross-origin (or a deliberate policy change in securityFloor) on the forge links into apps, and say in the pull that path-level referrers may leave the forge. A reviewer cannot approve a Home link that never takes anyone back. Also: (1) the test swallows every exception from the pill (catch {}), so a crash before the Home link is built reads as a pass of the fallback assertions; assert the link exists. (2) Nothing covers Tab/Enter/Escape (box 3); at minimum assert the Home link is an anchor with an href and the panel keydown handler for Escape. (3) The docs line in APPS.md is one very long line; wrap it like the paragraph around it. (4) Direction is pipeline first; keep this single-purpose and small.

commented

The forge's Open the app redirect now lets the forge page's address through to the app, so the pill's Home link has a referrer to return to. I haven't seen it work in a browser. The new test only checks the redirect's header, and the sandbox can't open a browser or click through. tsc, the gate, the design lint and the toolbar and apps-api tests pass. The tests I ran don't cover the pill's Tab/Enter behaviour in a browser (Keyboard test below).

What changed

  • Referrer fix. GET /:owner/:name/app/:env/open (appOpen) is the single redirect behind every "Open the app" link, and it now answers with Referrer-Policy: no-referrer-when-downgrade. I did not change securityFloor.
  • Policy choice. I did not use origin-when-cross-origin as suggested. Across origins it sends only the origin, so the page's path would never reach the app and Home would still go to /. Because of that, the app now receives the forge page's full address, path and query, over https only. Any app the visitor opens through that redirect can read which forge page they came from, including private repo names in the path. The pill drops the fragment, but the app's server still sees it.
  • Test for the fix. In test/apps-api.test.ts, the redirect test asserts a 302 with that policy, and it fails without the change.
  • Test crash handling. In test/toolbar-back.test.ts, the client no longer swallows exceptions. The helper asserts the Home link was built and has an href.
  • Keyboard test. I added only the source-level check you minimum-asked for: Home is an anchor, and the Escape handler is present. A real Tab/Enter/Escape test belongs in toolbar-keyboard.test.ts (headless Chromium), which I did not extend.
  • Docs. The docs/APPS.md line is wrapped. It says Home only returns to the forge page when the redirect lets the referrer through, otherwise it goes to /.

Left out

  • Other app links. Links that point straight at an app, bypassing the redirect, are unchanged and still send no referrer. These are talk_build's productionUrl/previewUrl links and the "Open on its own" link in talk_preview. Several of them also carry rel="noreferrer", so each needs its own look.

To try

  1. On a repo's page, press Open the app.
  2. On the app's page, open the pill's panel. Home should return to the repo page.
Approved

Earlier asks: the redirect now sends the full forge address (no-referrer-when-downgrade, tested at the redirect), the toolbar test no longer swallows crashes, and the docs line says when Home returns to the page. All met. Home is a plain anchor, so Tab and Enter work with JavaScript on or off; Escape already closed the panel. Only same-origin http(s) referrers are used and the fragment is dropped. No inline script or style attribute was added.

Next two things:

  1. Non-blocking: add a real Tab/Enter/Escape case to toolbar-keyboard.test.ts. Box 3 is currently held by a source-level check only.
  2. Non-blocking: the app now sees the full path of the forge page, including private repo names, for any app opened through the redirect. The pull and docs state this. Consider an issue to decide whether private repos should fall back to origin-only.
Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs