fix(frontend): /api-docs is public, and the allowlist is derived from app/ - #471
Conversation
…from app/ The API reference page shipped correct in every respect except one: `PUBLIC_ROUTES` in `auth-provider.tsx` did not list it, so the provider mounted in the root layout redirected every visitor to `/login` — the opposite of the page's whole purpose, which is that an integrator can read the contract before they have credentials. Nothing caught it, and the reason is worth recording. The page's own tests render the component directly, so they never mount the provider. The route-coverage test asserts the backend serves the document. And the deployment probe was HTTP-level, where the redirect is invisible: the server returns 200 and the bounce happens in the browser. Three guards, none of them positioned to see it. So the fix is not only the missing entry. `auth-provider.test.tsx` now DERIVES the expected set by reading `app/` — a top-level route directory that is neither the dashboard group nor `/login` is public by construction and must appear in `PUBLIC_ROUTES` — plus a second test asserting that every route the list names does not in fact redirect. Both directions, so neither a new public page nor a stale entry can pass silently. Mutation-checked: with `/api-docs` removed the first test fails with `app/api-docs/ renders outside the dashboard group but is not in PUBLIC_ROUTES`. `fonts/` is excluded from the scan as an asset directory, not a route. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6df1452be1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const discovered = readdirSync(join(process.cwd(), "app"), { withFileTypes: true }) | ||
| .filter((e) => e.isDirectory() && !AUTHENTICATED.has(e.name) && e.name !== "fonts") | ||
| .map((e) => `/${e.name}`); |
There was a problem hiding this comment.
This treats every top-level directory name as a literal URL segment, which is not true for App Router route groups or dynamic segments. For example, adding a public page at app/(public)/help/page.tsx makes the test demand /(public) while never checking the actual /help URL, so /help can still be redirected to login despite this regression guard passing after the bogus entry is added. Derive routable paths recursively or exercise actual public URLs instead of mapping directory names directly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in da3af09 — you're right, and the failure mode you describe is worse than a false negative: the bogus /(public) entry would have made the guard pass while /help still redirected, so the regression test would have stepped over the exact defect it exists to catch.
The walk now recurses for real page files and resolves URLs the way the router does — a route group contributes no segment, (dashboard) marks everything beneath it authenticated however deeply nested, and a dynamic segment is skipped because matching is prefix-based, so its static ancestor is what has to be listed.
Two things the fix pulled in. isPublicRoute is now extracted and exported from the provider, so the test asserts against the real matching rather than a copy that would only agree with itself. And there's a second assertion for the converse — nothing in the allowlist may exempt the authenticated group — because a lone / entry would satisfy the coverage direction while unlocking the whole app.
Mutation-checked, including your case specifically. With app/(public)/help/page.tsx present:
AssertionError: app serves /help outside (dashboard) but it is not public: expected false to be true
and adding /programs to the allowlist:
AssertionError: /programs is inside (dashboard) but PUBLIC_ROUTES exempts it: expected true to be false
…c allowlist Review (Codex P2) caught that the guard treated every top-level directory name as a URL segment, which the App Router does not. A public page at `app/(public)/help/page.tsx` would have made the test demand a nonexistent `/(public)` entry while never checking `/help` — so the bogus entry satisfies the guard and `/help` still redirects to login. The regression test would have passed over the very defect it exists to catch, one layer removed from the original bug. The walk now recurses for real `page` files and resolves URLs the way the router does: a route group contributes no segment, `(dashboard)` marks everything beneath it authenticated however deeply nested, and a dynamic segment is skipped because prefix matching means its static ancestor is what must be listed. Two further changes the fix made worth making. `isPublicRoute` is extracted and exported, so the test asserts against the provider's own matching instead of a reimplementation that would agree with itself. And a second assertion covers the converse — nothing in the allowlist may exempt the authenticated group — because a single `/` entry would satisfy the coverage direction while unlocking the whole app. Mutation-checked both ways, and the route-group case explicitly: with `app/(public)/help/page.tsx` present the guard fails with `app serves /help outside (dashboard) but it is not public`, naming the real URL; adding `/programs` to the allowlist fails with `/programs is inside (dashboard) but PUBLIC_ROUTES exempts it`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
The defect
https://twh.os.mieweb.org/api-docsredirects to/login. The page is public by design — anintegrator should be able to read the API contract before they have credentials, which is most of what
that page is for — but
PUBLIC_ROUTESincomponents/auth-provider.tsxlisted only/and/sandbox, and the provider is mounted in the root layout.Why nothing caught it
Three guards exist near this and none was positioned to see it:
ApiReferencedirectly, so the provider never mounts.openapi.test.tsasserts the backend serves the document, which it does.curl, and the redirect is client-side — the server returns 200.This is the vacuous-guard shape again: coverage that reads as present because each guard is true about
the thing it watches, while the failure lives between them.
The fix
/api-docsadded toPUBLIC_ROUTES, and the list is no longer trusted to be maintained by hand.auth-provider.test.tsxnow derives the expected set from the filesystem — a top-level directory underapp/that is neither(dashboard)norloginis public by construction and must appear in the list —and a second test walks the list asserting none of its entries redirects. Both directions, so a new
public page and a stale entry each fail loudly.
Mutation-checked: removing
/api-docsfails withapp/api-docs/ renders outside the dashboard group but is not in PUBLIC_ROUTES.Verification
npx vitest run components/__tests__/auth-provider.test.tsx→ 12 passnpm run lint→ 0 errors (1 pre-existing warning intest/mocks/)npm run build→ clean🤖 Generated with Claude Code