Skip to content

fix(guard): delete an access-control list that nothing enforced - #3876

Open
ntdatt812 wants to merge 1 commit into
decolua:masterfrom
ntdatt812:fix/dead-protected-api-paths
Open

ntdatt812 wants to merge 1 commit into
decolua:masterfrom
ntdatt812:fix/dead-protected-api-paths

Conversation

@ntdatt812

Copy link
Copy Markdown
Contributor

PROTECTED_API_PATHS lists seventeen /api prefixes under this comment:

// Require auth, but allow through if requireLogin is disabled
const PROTECTED_API_PATHS = [
  "/api/settings", "/api/keys", "/api/providers", …
];

It appears exactly once in the repository — its own declaration:

$ grep -rn "PROTECTED_API_PATHS" src/ tests/
src/dashboardGuard.js:51:const PROTECTED_API_PATHS = [

Nothing reads it.

Why that is worse than not having it

The name is the hazard. A list shaped like an access-control policy invites the next person to protect a route by adding a line to it — and the line does nothing. Nobody is protected less today, but the next person to trust it would be.

The rule it describes is real

It is the deny-by-default arm for /api/* in proxy(): anything not on the public allow-list needs a CLI token or an authenticated session, and isAuthenticated() returns true when requireLogin is off — which is exactly the escape hatch the comment meant. That arm covers every /api path, not the seventeen named ones, which is why deleting the list changes no answer.

Held by tests rather than by that argument

tests/unit/dashboard-guard-api-deny-by-default.test.js runs over the exact paths the dead list named:

  • each is rejected unauthenticated
  • so is a child route under each
  • so is a path invented today — the rule is the /api/ prefix, not membership of any list
  • the requireLogin=false hatch still opens
  • /api/settings/require-login stays public even though /api/settings is not

One expectation had to be corrected while writing them, and it is worth keeping: /api/mcp/ answers 403, not 401, because a stricter local-only gate runs first. Asserting 401 everywhere would have recorded that stricter answer as a failure — so the child-route case accepts either, and a separate test pins the 403 specifically.

Verification

  • new suite — 37 passed
  • existing dashboard-guard.test.js — unchanged, still passing
  • npx next build — ✓ compiled successfully

`PROTECTED_API_PATHS` listed seventeen /api prefixes under the comment
"Require auth, but allow through if requireLogin is disabled". It appears
exactly once in the repository: its own declaration. Nothing reads it.

That is worse than not having it. A list shaped like a policy invites the
next person to protect a route by adding a line to it, and the line does
nothing. The name is the whole hazard -- it reads as the thing that
enforces auth on those paths.

The rule it describes is real, and is the deny-by-default arm for `/api/*`
in proxy(): anything not on the public allow-list needs a CLI token or an
authenticated session, and isAuthenticated() returns true when
requireLogin is off, which is the escape hatch the comment meant. That arm
covers every /api path, not the seventeen named ones, so deleting the list
changes no answer.

Rather than delete it on that argument alone, the rule is now held by
tests over the exact paths the list named: each is rejected
unauthenticated, so is a child route, so is a path invented today, the
requireLogin=false hatch still opens, and /api/settings/require-login stays
public even though /api/settings is not.

One expectation had to be corrected while writing them: /api/mcp/ answers
403, not 401, because a stricter local-only gate runs first. Asserting 401
everywhere would have recorded that stricter answer as a failure, so the
child-route case accepts either and a separate test pins the 403.

37 tests pass; the existing dashboard-guard suite is unchanged; next build
compiles.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant