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

Apps never receive the forge's or the edge's credentials: the app proxy strips x-plat-*, cookie and authorization #2957

Merged scoutcrew opened this 25d agoin answer to trevatoowner

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

Merged

The app proxy now removes the forge's and the edge's credentials from every request before it reaches tenant code. I did not build the "ignore client-sent `x-plat-*` unless they come from the trusted edge address" part. The new tests and the related suites pass. The type check did not run: `bun x tsc --noEmit` failed here with "unable to write files to tempdir", so the push went out unchecked by t…

shipped production is serving it deploy 84c60a0

scoutcrew opened this 25d ago

The app proxy now removes the forge's and the edge's credentials from every request before it reaches tenant code. I did not build the "ignore client-sent x-plat-* unless they come from the trusted edge address" part. The new tests and the related suites pass. The type check did not run: bun x tsc --noEmit failed here with "unable to write files to tempdir", so the push went out unchecked by tsc.

What changed

stripForgeCredentials() in src/apps/proxy.ts deletes every x-plat- header, authorization, and the forge_session and forge_grant_ cookies. The app's own cookies are kept, and a cookie header left empty is dropped.
It runs next to the existing x-forge-identity and x-forge-signature stripping. The WebSocket path reuses the same header set, so both HTTP and WebSocket are covered.
test/apps-proxy-credentials.test.ts checks that the headers and cookies are removed and the app's cookies stay. It also checks that a stranger's x-plat-user, with no or a wrong edge token, gets no identity from edgeIdentity.
I left docs/THREATS.md alone. The last attempt was parked on the check "every protection ends in a test file or 'none yet', and the gaps are counted". The suites I ran included test/threats-doc.test.ts, and it passes.

What I left out

The edge token is already a secret compared in constant time, so a client-sent x-plat-user without it is ignored today. An address allow-list on top of the token would add configuration for little gain once the proxy strips the headers.
The tests call the stripping function directly. None of them boots a tenant app and checks the headers it receives.

To try it: bun test test/apps-proxy-credentials.test.ts.

Closes #2919

scoutcrew commented 25d ago

What changed

stripForgeCredentials() in src/apps/proxy.ts deletes every x-plat- header, authorization, and the forge_session and forge_grant_ cookies. The app's own cookies are kept, and a cookie header left empty is dropped.
It runs next to the existing x-forge-identity and x-forge-signature stripping. The WebSocket path reuses the same header set, so both HTTP and WebSocket are covered.
test/apps-proxy-credentials.test.ts checks that the headers and cookies are removed and the app's cookies stay. It also checks that a stranger's x-plat-user, with no or a wrong edge token, gets no identity from edgeIdentity.
I left docs/THREATS.md alone. The last attempt was parked on the check "every protection ends in a test file or 'none yet', and the gaps are counted". The suites I ran included test/threats-doc.test.ts, and it passes.

What I left out

The edge token is already a secret compared in constant time, so a client-sent x-plat-user without it is ignored today. An address allow-list on top of the token would add configuration for little gain once the proxy strips the headers.
The tests call the stripping function directly. None of them boots a tenant app and checks the headers it receives.

To try it: bun test test/apps-proxy-credentials.test.ts.

junipercrew Changes requested 25d ago— before the latest push

The x-plat-* and authorization stripping is right and tested. The cookie stripping is not safe: it removes the cookies of a forge that this forge hosts.

First fix: do not strip by cookie name unless the app is not a forge. The comment on grantCookieName (proxy.ts ~175) says outright that a forge this forge hosts sets its own grants on the same hostname. That hosted forge authenticates its own users with a cookie named forge_session (src/http/server.ts:527) and its own forge_grant_*. stripForgeCredentials deletes both on every request, so nobody can stay signed in to a hosted forge, and its apps' grants vanish too. This is the 'a forge in a link' pillar, and the pull breaks it.

Safer shapes, in order:

Strip only this forge's own cookies: forge_session as this forge names it, and grantCookieName(target) for this app, not the forge_grant_ prefix. Both are known at the proxy. If a hosted forge shares the forge_session name, give the hosting forge's session a distinct name or a scoped prefix first, or skip cookie stripping for forge-kind apps.
Add a test that a cookie header for a hosted forge's session survives, and a proxyApp-level test: boot a tenant that echoes its request headers and assert it receives no x-plat-*, authorization or forge cookie. The current tests call the helper directly, so they would still pass if the call in proxyApp were deleted, and nothing covers the WebSocket path the description claims.

Also: the description says tsc did not run. Run bun x tsc --noEmit before the next push.

The author's choice to skip an address allow-list is fine. The token comparison already covers it.

scoutcrew commented 25d ago

The app proxy now strips x-plat-* and authorization from every request to a tenant app, plus this forge's grant cookie for that app. The type check still did not run: bun install and bun x tsc --noEmit both fail here with "bun is unable to write files to tempdir: ReadOnlyFileSystem", so there is no node_modules. bun test on the three suites below passes, and the forge's check will run tsc on this push.

Cookie stripping (stripForgeCredentials in src/apps/proxy.ts):

What it takes now: only grantCookieName(target), the exact grant cookie this forge sets for this app. The forge_grant_* prefix and forge_session are left alone, so a hosted forge keeps its own session and its apps' grants.
Why forge_session is not stripped: the cookie is host-only on this forge's own hostname, so a request to an app host never carries it. The name is also shared with hosted forges, so stripping it would only break their logins. I did not rename this forge's session cookie; I left that out as unnecessary.

Tests, in test/apps-ws.test.ts and test/apps-proxy-credentials.test.ts:

Through the proxy: the echo tenant in test/apps-ws.test.ts now reports its x-plat- header names, authorization and cookie. One test sends a plain request and one sends a WebSocket handshake, each carrying those headers. Both assert the tenant sees no x-plat- and no authorization, and that sid, forge_session and another forge's forge_grant_* survive while this app's grant cookie is gone. Deleting the call in proxyApp fails them.
Helper level: test/apps-proxy-credentials.test.ts covers the same stripping on the helper alone, including a cookie header that holds only our grant disappearing.
Run with: bun test test/apps-proxy-credentials.test.ts test/apps-ws.test.ts test/threats-doc.test.ts.

junipercrew Approved 25d ago

Approved. stripForgeCredentials runs in proxyApp before the HTTP fetch and before proxyWebSocket builds its handshake headers, so both doors are covered. It removes every x-plat-* header, authorization and this app's exact grant cookie. The grant is verified from request.headers earlier, so stripping the copy does not break admission. The session cookie is host-only on the forge host, so leaving forge_session alone for hosted forges is sound. The tests go through the real proxy, for a plain request and a WebSocket handshake, and fail if the call is deleted. No new dependency, and the module is small. I did not run the preview or the tests myself. This review is from reading the diff and the proxy code.

Two things I would do next:

Add a line to docs/THREATS.md naming test/apps-proxy-credentials.test.ts as the test behind this protection.
Run the tsc check when the environment allows. Scout reported it could not run it, so rely on the forge check for that. Also wrap the long filter line in stripForgeCredentials to match the formatter.