fix: restore deny-by-default auth gate (#1333)#1337
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTests now assert explicit HTTP status codes for auth flows (401/204), and security middleware adds an exported Changes
Sequence Diagram(s)(omitted — changes are limited to tests and middleware constants/lookup and do not introduce a multi-component sequential flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
bot
added
the
size:L
Nitpicks 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/auth-logger.test.ts (1)
248-275: 🧹 Nitpick | 🔵 TrivialAssert HTTP contract, not only log side effects.
These tests should also assert response status (
204for valid check,401for invalid check) to cover user-facing behavior and error-path semantics.Suggested test tightening
it("does not log auth.session.invalid on valid auth check", async () => { const app = createApp(buildAuthEnabledEnv()); - await app.request("/auth/check", { + const response = await app.request("/auth/check", { headers: { Cookie: buildAuthCookie({ sid: "check-success" }), Origin: "https://rack.example.com", }, }); + expect(response.status).toBe(204); const authEvents = extractAuthEvents(writeSpy); expect(authEvents.some((e) => e.event === "auth.session.invalid")).toBe( false, ); }); it("logs auth.session.invalid on invalid auth check", async () => { const app = createApp(buildAuthEnabledEnv()); - await app.request("/auth/check"); + const response = await app.request("/auth/check"); + expect(response.status).toBe(401); const authEvents = extractAuthEvents(writeSpy); expect(authEvents.some((e) => e.event === "auth.session.invalid")).toBe( true, ); });As per coding guidelines "Test only high-value behavior: ... user-facing behavior ... error paths and edge cases, and integration between components."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/auth-logger.test.ts` around lines 248 - 275, The tests currently only assert log side-effects; update the two specs ("does not log auth.session.invalid on valid auth check" and "logs auth.session.invalid on invalid auth check") to also assert the HTTP response status returned by the handler when calling app.request("/auth/check")—for the valid case (when Cookie is set via buildAuthCookie({ sid: "check-success" })) assert status is 204, and for the invalid case (no Cookie) assert status is 401; use the response from createApp(buildAuthEnabledEnv()).request(...) and keep the existing extractAuthEvents(writeSpy) assertions intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/app.ts`:
- Around line 132-140: The authLoginRouteHandler currently rebuilds the redirect
to "/api/auth/sign-in/social" but drops the incoming "next" query parameter;
update authLoginRouteHandler to read the original request URL's search params
(new URL(c.req.url)) and, if a "next" value exists, set that on
loginTarget.searchParams (in addition to setting "provider" to "oidc") so the
redirect preserves the post-login return target; keep using
c.redirect(`${loginTarget.pathname}${loginTarget.search}`) to perform the
redirect.
- Around line 89-97: Add the missing logout endpoints to the auth gate allowlist
so anonymous POSTs to logout aren't blocked: update the AUTH_PUBLIC_PATHS Set
(in api/src/security.ts) to include "/auth/logout" and "/api/auth/logout"
alongside existing public paths, and add/update tests to assert that
unauthenticated requests to POST /auth/logout and POST /api/auth/logout reach
the handler (are allowed) to prevent regressions; ensure the change integrates
with createAuthGateMiddleware and any tests referencing AUTH_PUBLIC_PATHS.
---
Outside diff comments:
In `@api/src/auth-logger.test.ts`:
- Around line 248-275: The tests currently only assert log side-effects; update
the two specs ("does not log auth.session.invalid on valid auth check" and "logs
auth.session.invalid on invalid auth check") to also assert the HTTP response
status returned by the handler when calling app.request("/auth/check")—for the
valid case (when Cookie is set via buildAuthCookie({ sid: "check-success" }))
assert status is 204, and for the invalid case (no Cookie) assert status is 401;
use the response from createApp(buildAuthEnabledEnv()).request(...) and keep the
existing extractAuthEvents(writeSpy) assertions intact.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
api/src/app.ts
Outdated
| const authLoginRouteHandler = (c: Context<AppEnv>) => { | ||
| if (!oidcApiAvailable) { | ||
| return authUnavailableRouteHandler(c); | ||
| } | ||
|
|
||
| const loginTarget = new URL("/api/auth/sign-in/social", c.req.url); | ||
| loginTarget.searchParams.set("provider", "oidc"); | ||
| return c.redirect(`${loginTarget.pathname}${loginTarget.search}`); | ||
| }; | ||
|
|
||
| const authCallbackRouteHandler = (c: Context<AppEnv>) => { | ||
| if (!oidcApiAvailable) { | ||
| return authUnavailableRouteHandler(c); | ||
| } | ||
|
|
||
| const requestUrl = new URL(c.req.url); | ||
| const callbackTarget = new URL("/api/auth/callback/oidc", c.req.url); | ||
| callbackTarget.search = requestUrl.search; | ||
| return c.redirect(`${callbackTarget.pathname}${callbackTarget.search}`); |
There was a problem hiding this comment.
Suggestion: The auth gate middleware runs on all /api/* routes, so the current /auth/login and /auth/callback handlers redirect unauthenticated users to /api/auth/sign-in/social and /api/auth/callback/oidc, which are not whitelisted as public auth paths; as a result, the auth gate intercepts and returns 401 before the Better Auth handler sees the request, breaking the OIDC login and callback flows when an auth provider is configured. To fix this, have the compatibility handlers call the Better Auth auth.handler directly with a constructed internal Request to the appropriate /api/auth/* endpoint, instead of issuing an HTTP redirect that re-enters the Hono pipeline and hits the auth gate. [logic error]
Severity Level: Critical 🚨
- ❌ OIDC login via /auth/login fails; user cannot authenticate.
- ❌ Better Auth callback /auth/callback blocked; sessions never established.
- ⚠️ Admin-protected layout and asset writes unusable without authentication.| const authLoginRouteHandler = (c: Context<AppEnv>) => { | |
| if (!oidcApiAvailable) { | |
| return authUnavailableRouteHandler(c); | |
| } | |
| const loginTarget = new URL("/api/auth/sign-in/social", c.req.url); | |
| loginTarget.searchParams.set("provider", "oidc"); | |
| return c.redirect(`${loginTarget.pathname}${loginTarget.search}`); | |
| }; | |
| const authCallbackRouteHandler = (c: Context<AppEnv>) => { | |
| if (!oidcApiAvailable) { | |
| return authUnavailableRouteHandler(c); | |
| } | |
| const requestUrl = new URL(c.req.url); | |
| const callbackTarget = new URL("/api/auth/callback/oidc", c.req.url); | |
| callbackTarget.search = requestUrl.search; | |
| return c.redirect(`${callbackTarget.pathname}${callbackTarget.search}`); | |
| const authLoginRouteHandler = (c: Context<AppEnv>) => { | |
| if (!oidcApiAvailable || !auth) { | |
| return authUnavailableRouteHandler(c); | |
| } | |
| const loginTarget = new URL("/api/auth/sign-in/social", c.req.url); | |
| loginTarget.searchParams.set("provider", "oidc"); | |
| const forwardedRequest = new Request(loginTarget.toString(), { | |
| method: c.req.method, | |
| headers: c.req.raw.headers, | |
| }); | |
| return auth.handler(forwardedRequest); | |
| }; | |
| const authCallbackRouteHandler = (c: Context<AppEnv>) => { | |
| if (!oidcApiAvailable || !auth) { | |
| return authUnavailableRouteHandler(c); | |
| } | |
| const requestUrl = new URL(c.req.url); | |
| const callbackTarget = new URL("/api/auth/callback/oidc", c.req.url); | |
| callbackTarget.search = requestUrl.search; | |
| const forwardedRequest = new Request(callbackTarget.toString(), { | |
| method: c.req.method, | |
| headers: c.req.raw.headers, | |
| }); | |
| return auth.handler(forwardedRequest); | |
| }; |
Steps of Reproduction ✅
1. Configure the API with auth enabled and OIDC set up so that `createAuth()` installs the
`genericOAuth` plugin (`api/src/auth/config.ts:30-52`), and start the server via
`createApp()` (`api/src/app.ts:40-87`), ensuring `securityConfig.authEnabled === true`.
2. Note that `createAuthGateMiddleware()` is mounted globally for all routes when auth is
enabled (`api/src/app.ts:89-96`), and it treats only specific paths as public via
`AUTH_PUBLIC_PATHS` (`api/src/security.ts:72-81`), which include `/auth/login`,
`/auth/callback`, `/auth/check`, `/api/auth/login`, `/api/auth/callback`,
`/api/auth/check` but not `/api/auth/sign-in/social` or `/api/auth/callback/oidc`.
3. Trigger login by issuing `GET /auth/login` in a browser or HTTP client; this hits
`authLoginRouteHandler` defined at `api/src/app.ts:132-140`, which, when
`oidcApiAvailable` is true (`api/src/app.ts:109-120`), constructs a URL
`/api/auth/sign-in/social` and responds with an HTTP redirect (`c.redirect(...)` at
`api/src/app.ts:137-139`).
4. The browser follows the redirect to `GET /api/auth/sign-in/social`; this request again
enters the Hono app and passes through `createAuthGateMiddleware`
(`api/src/security.ts:1076-1134`): since the pathname `/api/auth/sign-in/social` is an API
path (`isApiRequestPath` true, `api/src/security.ts:349-352`) but not an auth public path
(`isAuthPublicPath` false, `api/src/security.ts:355-357`), the middleware attempts to
resolve a session (`resolveAuthenticatedSessionClaims`, `api/src/security.ts:896-924`),
finds no valid cookie, logs `auth.session.invalid`, and immediately returns a 401 JSON
response (`api/src/security.ts:1119-1130`) instead of calling `next()`.
5. Because the auth gate returns 401, the Better Auth handler mounted at `/api/auth/*`
(`createAuthHandler(auth)` at `api/src/app.ts:216-220`) never sees the
`/api/auth/sign-in/social` request, so the OIDC sign-in flow cannot start and the user
cannot be redirected to the identity provider.
6. Similarly, on OIDC callback, the identity provider redirects the user to
`/auth/callback?...`; this hits `authCallbackRouteHandler` (`api/src/app.ts:142-150`),
which redirects to `/api/auth/callback/oidc?...`, but that redirected request is again
intercepted by `createAuthGateMiddleware` as a non-public API path and returns 401 before
reaching Better Auth, preventing session establishment after login.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** api/src/app.ts
**Line:** 132:150
**Comment:**
*Logic Error: The auth gate middleware runs on all `/api/*` routes, so the current `/auth/login` and `/auth/callback` handlers redirect unauthenticated users to `/api/auth/sign-in/social` and `/api/auth/callback/oidc`, which are not whitelisted as public auth paths; as a result, the auth gate intercepts and returns 401 before the Better Auth handler sees the request, breaking the OIDC login and callback flows when an auth provider is configured. To fix this, have the compatibility handlers call the Better Auth `auth.handler` directly with a constructed internal Request to the appropriate `/api/auth/*` endpoint, instead of issuing an HTTP redirect that re-enters the Hono pipeline and hits the auth gate.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/auth-logger.test.ts (1)
190-196:⚠️ Potential issue | 🟡 MinorAssert the auth-gate HTTP status in this integration test.
This test currently proves logging behavior, but it does not verify the gate outcome. Add an explicit
401assertion so the test fails if the gate stops denying anonymous API requests.Suggested patch
- await app.request("/api/layouts"); + const response = await app.request("/api/layouts"); + expect(response.status).toBe(401); const authEvents = extractAuthEvents(writeSpy);As per coding guidelines:
**/*.test.{ts,tsx}should validate high-value user-facing/error-path integration behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/auth-logger.test.ts` around lines 190 - 196, The test currently only checks logs via extractAuthEvents(writeSpy) after calling app.request("/api/layouts"); capture the HTTP response from that call (e.g., const res = await app.request("/api/layouts")) and add an assertion that res.status (or res.statusCode) equals 401 to ensure the auth gate actually denies anonymous requests; keep the existing log assertions (extractAuthEvents(writeSpy)) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/src/auth-logger.test.ts`:
- Around line 190-196: The test currently only checks logs via
extractAuthEvents(writeSpy) after calling app.request("/api/layouts"); capture
the HTTP response from that call (e.g., const res = await
app.request("/api/layouts")) and add an assertion that res.status (or
res.statusCode) equals 401 to ensure the auth gate actually denies anonymous
requests; keep the existing log assertions (extractAuthEvents(writeSpy)) intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/auth-logger.test.ts`:
- Around line 295-302: This test duplicates the JSON-line parsing/filtering
logic already implemented by extractAuthEvents; replace the ad-hoc authLines
construction that parses allOutput into JSON and filters event
startsWith("auth.") with a call to extractAuthEvents(allOutput) (or otherwise
delegate to extractAuthEvents) so the test uses the same narrowing/parsing
behavior; update any assertions that reference authLines to use the returned
value from extractAuthEvents.
- Add /auth/logout and /api/auth/logout to AUTH_PUBLIC_PATHS so anonymous logout calls are not blocked by the auth gate - Create separate CSRF_EXEMPT_AUTH_PATHS set so logout still gets CSRF protection (state-changing POST must validate origin) - Forward login/callback requests internally via auth.handler() instead of HTTP redirects that get intercepted by the auth gate - Preserve incoming `next` query param on /auth/login with open redirect protection (relative paths only, no protocol-relative) - Assert HTTP status codes (204/401) in auth-logger check tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ggfevans
force-pushed
the
fix/1333-restore-auth-gate
branch
from
e164fd0 to
54caa81
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:M
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/security.ts`:
- Around line 70-75: Add a JSDoc block above the exported constant
STATE_CHANGING_METHODS that documents it as a public API: describe that it is a
Set of HTTP methods considered to change server state (POST, PUT, PATCH,
DELETE), its intended usage (e.g., to gate CSRF/validation logic or middleware
checks), and the fact that it is exported and immutable; include `@constant`,
`@type` {Set<string>} and an example of typical usage or mention that methods are
uppercase strings so callers should normalize input.
Add JSDoc for STATE_CHANGING_METHODS constant and refactor token leakage test to reuse extractAuthEvents helper instead of inline reimplementation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
User description
Summary
/auth/login,/auth/callback,/auth/check,/auth/logout) and API aliases for compatibilityTest Plan
cd api && bun test src/security.test.ts src/auth-logger.test.tsnpm run lintnpm run test:runnpm run buildcodeant static-analysis --uncommitted --fail-on INFOcodeant security-analysis --uncommitted --fail-on INFOcodeant secrets --uncommitted --fail-on allcoderabbit --prompt-only --type uncommittedCloses #1333
CodeAnt-AI Description
Restore deny-by-default auth gate and public auth endpoints; ensure CSRF and auth logging behave correctly
What Changed
Impact
✅ Deny anonymous requests (401) when auth is enabled✅ Logout keeps CSRF protection while auth bootstrap endpoints bypass CSRF✅ Clearer auth check outcomes (204/401) and consistent auth event logging💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.