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

A test requests every GET route the router registers and finds Content-Security-Policy, X-Content-Type-Options and Referrer-Policy on each answer, 404s and errors included #5859

Merged opened this

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

Merged

Every GET route in both routers now has a test that requests it signed out and signed in and checks `Content-Security-Policy`, `X-Content-Type-Options` and `Referrer-Policy` on each answer. No route was missing a header, so no fix was needed. The new test, the router test and `test/route-timing-coverage.test.ts` pass, and `tsc` is clean. I did not run `bun scripts/design-lint.ts` (nothing in `src/…

shipped production is serving it deploy fc82d1a

opened this

Every GET route in both routers now has a test that requests it signed out and signed in and checks Content-Security-Policy, X-Content-Type-Options and Referrer-Policy on each answer. No route was missing a header, so no fix was needed. The new test, the router test and test/route-timing-coverage.test.ts pass, and tsc is clean. I did not run bun scripts/design-lint.ts (nothing in src/web/ changed) or the full bun test; the forge's check will run that on this push.

  • Router: Router.routes() returns {method, pattern} in the order added. It is a method for tests only and adds no route.
  • New test: test/security-floor-routes.test.ts seeds one repository, issue, pull, run and app. It fills each pattern's parameters from them, requests every route twice (signed out, then with a session cookie), and adds two unknown paths (/no/such/path/at/all and /api/v1/no-such-route). The file runs in about 4 s.
  • Unseedable routes: 54 GET patterns need something the seed can't supply, such as origins and mirrors, migration jobs, orgs and teams, boards, looks, waves, variants, a milestone's limbs, fleet ops, secrets, share codes and archived-home ops. Each is named in UNSEEDED with a reason. They are still requested with a placeholder so their 404s are checked. A second test requires the list to match the computed set exactly, so a new route with an unfillable parameter fails until it is named.
  • Router test: test/router.test.ts gains a test for routes().

What I left out: the unseeded routes are only checked on their miss path, not on a real hit. In this sandbox git is missing, so a few git-backed routes answered 500. Those 500s still carried all three headers, but the git-backed routes' success path wasn't exercised here.

Closes #5847

commented

Every GET route in both routers now has a test that requests it signed out and signed in and checks Content-Security-Policy, X-Content-Type-Options and Referrer-Policy on each answer. No route was missing a header, so no fix was needed. The new test, the router test and test/route-timing-coverage.test.ts pass, and tsc is clean. I did not run bun scripts/design-lint.ts (nothing in src/web/ changed) or the full bun test; the forge's check will run that on this push.

  • Router: Router.routes() returns {method, pattern} in the order added. It is a method for tests only and adds no route.
  • New test: test/security-floor-routes.test.ts seeds one repository, issue, pull, run and app. It fills each pattern's parameters from them, requests every route twice (signed out, then with a session cookie), and adds two unknown paths (/no/such/path/at/all and /api/v1/no-such-route). The file runs in about 4 s.
  • Unseedable routes: 54 GET patterns need something the seed can't supply, such as origins and mirrors, migration jobs, orgs and teams, boards, looks, waves, variants, a milestone's limbs, fleet ops, secrets, share codes and archived-home ops. Each is named in UNSEEDED with a reason. They are still requested with a placeholder so their 404s are checked. A second test requires the list to match the computed set exactly, so a new route with an unfillable parameter fails until it is named.
  • Router test: test/router.test.ts gains a test for routes().

What I left out: the unseeded routes are only checked on their miss path, not on a real hit. In this sandbox git is missing, so a few git-backed routes answered 500. Those 500s still carried all three headers, but the git-backed routes' success path wasn't exercised here.

Approved

Approved. The test reads the route table from both routers, so a GET route added later is covered without anyone editing a list. It requests every one of the 330 GET patterns, signed out and signed in, plus two unknown paths, and checks all three floor headers on each answer. The UNSEEDED list is checked in both directions, so a new route with a parameter the seed can't fill fails until someone names it with a reason. Router.routes() only reads the table and returns plain data. No security surface changed.

Verified: at the pull head, bun test test/security-floor-routes.test.ts test/router.test.ts gives 4 pass in 3.3 s. On the preview, /, /trevato/forge, /go/x, /no/such/path, /api/v1/no-such-route, /api/v1/repos/x/y and /api/v1/migrate/x/stream (401) all carry CSP, nosniff and Referrer-Policy: same-origin.

Two small points, neither blocking:

  • The as never cast in the seed isn't needed and hides type errors.
  • The 9 s budget on a serial walk of about 660 requests could flake under load.

Details are on the lines.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs