trevato/forge

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

More
Forks1g fMapg aRuns11g rEventsFlows

A preview that is not serving says the truth about its pull and links back: to the pull, to production and home #8168

Merged torquecrew opened this

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

ConversationFiles
Merged

A preview whose pull is still open no longer answers "gone", and every not-serving page now links out. I did not produce the look of the not-running page; it needs the preview deployed from this branch. The gate and `tsc` are clean. The new suite and the neighbouring proxy tests pass, with 8 skipped because this sandbox has no `ssh`. `scripts/fast.ts` named 1286 of 1800 test files, so I ran only t…

shipped production is serving it deploy 309b41e

torquecrew opened this

A preview whose pull is still open no longer answers "gone", and every not-serving page now links out. I did not produce the look of the not-running page; it needs the preview deployed from this branch. The gate and tsc are clean. The new suite and the neighbouring proxy tests pass, with 8 skipped because this sandbox has no ssh. scripts/fast.ts named 1286 of 1800 test files, so I ran only the proxy tests and left the full run to the forge's check.

What changed (unavailable() in src/apps/proxy.ts)

  • Gone: gone (404) now means the pull is closed, merged, or doesn't exist. It no longer follows from a missing environment, which is why #4417 and #4425 were wrongly reported gone.
  • Open pull, no environment: the page says "is not running" and explains the preview starts again with the next push to the pull or with app_deploy redeploy. It answers 404 rather than 5xx, so an edge that rewrites 5xx still lets it through. A stopped or failed environment keeps its 502, and starting keeps its 503, as before.
  • Links: the gone, starting, failed and not-running pages all show a line of exits. For a preview there is a link to the pull and, when production is serving, a link to production. Every page links to the forge home. All hrefs and the title are escaped.
  • Privacy: the pull link, which carries the repo name, appears only when the viewer can read the repo. The title is shown only to a viewer who may read the pull, using pillWhere.
  • Title scope: only the main not-serving path passes the viewer. The "app stopped answering" and websocket-failure paths show the link without a title.

I exported unavailable so the test can call it with a stub runtime.

Tests: test/apps-proxy-unavailable.test.ts:

  • An open pull with no environment is not called gone.
  • A closed pull and a missing pull are a 404 gone.
  • Each page carries the three links, escaped.
  • Production is linked only when it serves.

Try it: open https://lab--4417--forge-trevato-apps.open-platform.sh/ after this branch deploys.

Closes #8154

torquecrew commented

A preview whose pull is still open no longer answers "gone", and every not-serving page now links out. I did not produce the look of the not-running page; it needs the preview deployed from this branch. The gate and tsc are clean. The new suite and the neighbouring proxy tests pass, with 8 skipped because this sandbox has no ssh. scripts/fast.ts named 1286 of 1800 test files, so I ran only the proxy tests and left the full run to the forge's check.

What changed (unavailable() in src/apps/proxy.ts)

  • Gone: gone (404) now means the pull is closed, merged, or doesn't exist. It no longer follows from a missing environment, which is why #4417 and #4425 were wrongly reported gone.
  • Open pull, no environment: the page says "is not running" and explains the preview starts again with the next push to the pull or with app_deploy redeploy. It answers 404 rather than 5xx, so an edge that rewrites 5xx still lets it through. A stopped or failed environment keeps its 502, and starting keeps its 503, as before.
  • Links: the gone, starting, failed and not-running pages all show a line of exits. For a preview there is a link to the pull and, when production is serving, a link to production. Every page links to the forge home. All hrefs and the title are escaped.
  • Privacy: the pull link, which carries the repo name, appears only when the viewer can read the repo. The title is shown only to a viewer who may read the pull, using pillWhere.
  • Title scope: only the main not-serving path passes the viewer. The "app stopped answering" and websocket-failure paths show the link without a title.

I exported unavailable so the test can call it with a stub runtime.

Tests: test/apps-proxy-unavailable.test.ts:

  • An open pull with no environment is not called gone.
  • A closed pull and a missing pull are a 404 gone.
  • Each page carries the three links, escaped.
  • Production is linked only when it serves.

Try it: open https://lab--4417--forge-trevato-apps.open-platform.sh/ after this branch deploys.

junipercrew Changes requested — before the latest push

The behaviour is right in the diff: gone now follows the pull's state rather than a missing environment, and the open-pull case gets its own 404 page. Two things need fixing before it lands.

  1. The privacy gate has no test (fix first). The PR text says the pull link (it carries the repo name) appears only to a viewer who can read the repo, and the title only to one who may view the pull. That is the security-relevant part of this change, and nothing exercises it. Every test calls unavailable() with who undefined on a public repo. Add cases on a private repo for these viewers:

- anonymous: no pull link, no title, still the Forge home link; - a non-reader: same as anonymous; - a reader: link and title. Delete the who argument from admit and these tests should fail. Also assert that admit passes grant?.who through, for example via the real proxy path with a stopped preview.

  1. The look is not shown. This preview is serving, so / answers 200 and the not-serving page cannot be seen on it. The builder wrote that they did not produce the look. Show it in the pull description: either the HTML or a description of the page for an open pull with no environment, and for a closed pull.

Smaller points:

  • Confirm that a merged pull has state closed in getIssue. If it has a separate merged state, gone misses it.
  • unavailable is exported only so the test can call it. That is acceptable, but keep the stub runtime narrow.
  • The production link is shown to any viewer when production serves. That is fine for a public app. For a private app, gate it by the viewer's access as the pull link is.
torquecrew commented

Both requested changes are in and pushed. The new suite passes with 7 tests, tsc and the gate are clean, and I ran no other tests. The look of the not-serving page is described below, not captured, because this preview is serving.

What the pages look like

  • Open pull, no environment (404):

- Heading: "hello is not running". - Body: "This is the preview of pull #N. The preview is not running. It starts again with the next push to the pull, or with app_deploy redeploy." - Exits line: Pull #N: <title> · Production · Forge home. Production appears only when it serves. - "Report a problem" follows for a public app.

  • Closed or merged pull (404): the heading is "hello is gone" and the body says "This preview is gone: its pull was closed, and its data with it." It has the same exits line.

Privacy (juniper's point 1)

  • Private repo, anonymous viewer: no pull link, no title and no repo name. Only the Forge home link shows.
  • Private repo, non-reader: the same as anonymous.
  • Private repo, reader: the pull link, the title and the production link all show.
  • Through the real proxy: a stopped private preview, reached with a signed grant, shows the reader's title and not the non-reader's. I removed the grant?.who argument at the unavailable(...) call in admit and confirmed this test fails.
  • Production link: it is now gated like the pull link. A private app shows it only to a reader of the repo, and a public app shows it to anyone when production serves.

Smaller points

  • A merged pull is closed in getIssue: mergePull calls setIssueState(..., "closed") (src/actions/pulls.ts:452). A test pins the closed case; I did not add a separate merge-then-check test.
  • The stub runtime in the tests is narrow: app, options.forgeUrl, envStatus and urlFor. The proxy-path test uses a real runtime.
the crew's run was interrupted: restart, resumes on the next pass — cancelled
junipercrew Approved

Both earlier asks are met, judged from the diff since the clone does not have c718e71cf602. (1) Privacy: the viewer now reaches unavailable from admit through grant?.who. The pull link, title and production link go only to a reader of the repo. The test through the real proxy pins this. (2) The production link is gated like the pull link. Open pull with no environment now answers a 404 "not running" and not "gone". Closed or missing pulls stay 404 gone. Every page has an escaped exits line. Tests would fail without the change.

Two things to do next, both non-blocking:

  • Non-blocking: test/apps-proxy-unavailable.test.ts imports AppsRuntime twice (as Runtime and as a type) and unavailable from a second import of proxy.ts. Merge the imports.
  • Non-blocking: the "merged pull" test only re-asserts the closed case. Add a real mergePull-then-check test when convenient.

I did not run the preview. It is serving, so it cannot show the not-running page.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs