fix: support Entra common issuer discovery for OIDC#1340
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:
📝 WalkthroughWalkthroughThis PR implements OIDC (OpenID Connect) authentication support with Microsoft Entra ID multi-tenant issuer compatibility. It introduces OIDC discovery and verification utilities, updates authentication configuration and routes to support OAuth2 flows with fallback session claims, adds integration tests, and extends documentation with Entra ID configuration guidance. Changes
Sequence DiagramssequenceDiagram
participant Client
participant App as App<br/>/auth/login
participant OidcProvider as OIDC Provider
participant AppCallback as App<br/>/auth/callback
Client->>App: GET /auth/login
App->>OidcProvider: Fetch discovery document
OidcProvider-->>App: Discovery metadata + JWKS URI
App->>OidcProvider: Fetch JWKS
OidcProvider-->>App: Public key set
App->>OidcProvider: Redirect to authorization endpoint
OidcProvider-->>Client: 302 to provider login
Client->>OidcProvider: Complete authentication
OidcProvider->>AppCallback: Redirect with auth code
AppCallback->>OidcProvider: Exchange code for ID token
OidcProvider-->>AppCallback: ID token (RS256 signed)
AppCallback->>AppCallback: Verify token signature against JWKS
AppCallback->>AppCallback: Validate issuer, audience, expiry
AppCallback->>Client: 302 + Set-Cookie auth session
Client->>Client: Store session cookie
sequenceDiagram
participant Client
participant AppGate as Auth Gate<br/>Middleware
participant SessionProvider as Session<br/>Provider
participant FallbackResolver as Fallback Claims<br/>Resolver
Client->>AppGate: Request with session cookie
AppGate->>SessionProvider: Validate primary session
alt Primary session valid
SessionProvider-->>AppGate: AuthSessionClaims
AppGate->>AppGate: Set authSubject/authClaims
AppGate->>Client: Continue to next handler
else Primary session invalid or missing
SessionProvider-->>AppGate: null
AppGate->>FallbackResolver: resolveFallbackClaims(request)
alt Fallback claims available
FallbackResolver-->>AppGate: AuthSessionClaims (fallback)
AppGate->>AppGate: Set authSubject/authClaims from fallback
AppGate->>Client: Continue to next handler
else No fallback claims
FallbackResolver-->>AppGate: null
AppGate->>AppGate: Log unauthorized
AppGate->>Client: 401/Redirect
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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:XL
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
🤖 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/config.ts`:
- Around line 224-270: The resolveDiscovery caching can race if multiple callers
trigger the branch before discoveryPromise is set; fix by assigning
discoveryPromise to a Promise synchronously (wrap the fetch in an
immediately-invoked async function) so the in-flight promise is created
atomically, and keep the existing catch handler to clear discoveryPromise and
jwks on error. Update the logic in createOidcUserInfoResolver’s resolveDiscovery
(the discoveryPromise assignment) to set discoveryPromise = (async () =>
fetchOidcDiscoveryDocument(options.discoveryUrl,
options.expectedIssuer))().catch((error: unknown) => { discoveryPromise =
undefined; jwks = undefined; throw error; }); so the promise is always created
immediately and subsequent callers see the same promise.
- Around line 362-377: The advanced cookie config currently sets
useSecureCookies: false which forces cookies to be non-Secure and overrides
defaultCookieAttributes; change this so cookies are Secure in production by
either removing useSecureCookies or setting useSecureCookies: true, and ensure
defaultCookieAttributes.secure is set appropriately (e.g., secure: env.NODE_ENV
=== "production" or secure: true when using custom names); update the
advanced.cookies.session_token (authSessionCookieName) handling to keep custom
name but not disable the Secure flag so Rackula middleware can read the cookie
while still enforcing HTTPS in production.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
api/.env.exampleapi/src/app.tsapi/src/auth-logger.test.tsapi/src/auth/config.tsapi/src/oidc.integration.test.tsdocs/deployment/AUTHENTICATION.md
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| advanced: { | ||
| // Keep custom cookie names exact (without auto __Secure- prefixing) so | ||
| // Rackula's CSRF/auth middleware can reliably read the configured cookie name. | ||
| useSecureCookies: false, | ||
| cookies: { | ||
| session_token: { | ||
| name: authSessionCookieName, | ||
| }, | ||
| }, | ||
| defaultCookieAttributes: { | ||
| httpOnly: true, // Prevent XSS access to cookie | ||
| sameSite: "lax", // CSRF protection | ||
| secure: process.env.NODE_ENV === "production", // HTTPS only in production | ||
| secure: env.NODE_ENV === "production", // HTTPS only in production | ||
| // domain: '.racku.la' // Uncomment if using subdomains | ||
| }, |
There was a problem hiding this comment.
Suggestion: The secure flag for the Better Auth session cookie ignores the RACKULA_AUTH_SESSION_COOKIE_SECURE environment override and is always derived from NODE_ENV, which can diverge from security.ts's authSessionCookieSecure setting; this leads to inconsistent Secure attributes between cookies issued by Better Auth and those issued by your own auth middleware (e.g. on /auth/check and /auth/logout), and in configurations that explicitly disable Secure in production the refresh/logout routes will downgrade a previously secure cookie to a non-secure one. [logic error]
Severity Level: Major ⚠️
- ❌ Secure session cookie can be downgraded on auth-check/logout.
- ⚠️ Better Auth and middleware disagree on Secure cookie policy.
- ⚠️ RACKULA_AUTH_SESSION_COOKIE_SECURE override not honored by Better Auth.| advanced: { | |
| // Keep custom cookie names exact (without auto __Secure- prefixing) so | |
| // Rackula's CSRF/auth middleware can reliably read the configured cookie name. | |
| useSecureCookies: false, | |
| cookies: { | |
| session_token: { | |
| name: authSessionCookieName, | |
| }, | |
| }, | |
| defaultCookieAttributes: { | |
| httpOnly: true, // Prevent XSS access to cookie | |
| sameSite: "lax", // CSRF protection | |
| secure: process.env.NODE_ENV === "production", // HTTPS only in production | |
| secure: env.NODE_ENV === "production", // HTTPS only in production | |
| // domain: '.racku.la' // Uncomment if using subdomains | |
| }, | |
| advanced: { | |
| // Keep custom cookie names exact (without auto __Secure- prefixing) so | |
| // Rackula's CSRF/auth middleware can reliably read the configured cookie name. | |
| useSecureCookies: false, | |
| cookies: { | |
| session_token: { | |
| name: authSessionCookieName, | |
| }, | |
| }, | |
| defaultCookieAttributes: { | |
| httpOnly: true, // Prevent XSS access to cookie | |
| sameSite: "lax", // CSRF protection | |
| secure: | |
| (() => { | |
| const override = readEnv(env, "RACKULA_AUTH_SESSION_COOKIE_SECURE"); | |
| if (!override) { | |
| return env.NODE_ENV === "production"; | |
| } | |
| const normalized = override.toLowerCase(); | |
| if (normalized === "true") { | |
| return true; | |
| } | |
| if (normalized === "false") { | |
| return false; | |
| } | |
| return env.NODE_ENV === "production"; | |
| })(), // Aligns with auth cookie secure flag configuration | |
| // domain: '.racku.la' // Uncomment if using subdomains | |
| }, | |
| }, |
Steps of Reproduction ✅
1. Configure the API with authentication enabled and conflicting cookie settings by
starting the app via `createApp(env)` in `api/src/app.ts:78` using an environment map
where `NODE_ENV="production"` and `RACKULA_AUTH_SESSION_COOKIE_SECURE="false"` (or
`"true"` with `NODE_ENV!="production"`). This mirrors documented override usage in
`docs/deployment/AUTHENTICATION.md:233-237` and `api/README.md:259-261`.
2. When `createApp` runs, it calls `resolveApiSecurityConfig(env)` in
`api/src/security.ts:933`, which computes `authSessionCookieSecure` using
`parseOptionalBoolean("RACKULA_AUTH_SESSION_COOKIE_SECURE",
env.RACKULA_AUTH_SESSION_COOKIE_SECURE, isProduction)` at `security.ts:1022-1025`. With
the override set to `"false"` and `NODE_ENV="production"`, `authSessionCookieSecure`
becomes `false`.
3. Still in `createApp`, Better Auth is instantiated as `const auth =
securityConfig.authSessionSecret ? createAuth(securityConfig.authSessionSecret, env) :
undefined;` at `api/src/app.ts:112-114`. Inside `createAuth` in
`api/src/auth/config.ts:293-381`, the `advanced.defaultCookieAttributes.secure` flag for
Better Auth is hard-coded to `env.NODE_ENV === "production"` at `config.ts:371-375`,
ignoring `RACKULA_AUTH_SESSION_COOKIE_SECURE`, so Better Auth will set `Secure` on its
session cookie whenever `NODE_ENV` is `"production"`.
4. Trigger a login via the Better Auth compatibility route `GET /auth/login`, wired in
`createApp` at `api/src/app.ts:281-287`. Better Auth (via `createAuth`) issues a
`Set-Cookie` header for the session cookie `rackula_auth_session` (name wired through
`advanced.cookies.session_token.name` at `auth/config.ts:366-370`) with `Secure` present
because `env.NODE_ENV === "production"` even though `authSessionCookieSecure` is `false`.
5. With the session established, call `GET /auth/check` (or `/api/auth/check`), which is
registered in `api/src/app.ts:283-289`. The handler `authCheckRouteHandler` at
`app.ts:229-259` reads the session cookie and then calls
`createRefreshedAuthSessionCookieHeader(claims, authSessionConfig)` at `app.ts:250-253`.
That function in `security.ts:841-885` ultimately uses `buildSessionCookieHeader` at
`security.ts:474-501`, which inspects `securityConfig.authSessionCookieSecure` at
`security.ts:496-498` to decide whether to append `Secure`. Because
`authSessionCookieSecure` was resolved to `false` in step 2, the refreshed `Set-Cookie`
header omits `Secure`, overwriting the previously `Secure` Better Auth cookie with a
non-secure cookie of the same name.
6. Observe in the HTTP response from `/auth/check` (or `/auth/logout`, which uses
`createExpiredAuthSessionCookieHeader` at `security.ts:809-830` via `app.ts:273-277`) that
the `Set-Cookie` header for `rackula_auth_session` lacks the `Secure` attribute even
though the original login cookie from Better Auth had `Secure`. This demonstrates that the
Better Auth configuration in `auth/config.ts` is not honoring the
`RACKULA_AUTH_SESSION_COOKIE_SECURE` override and can downgrade a previously secure
session cookie when the override disagrees with `NODE_ENV`, creating inconsistent cookie
security between Better Auth and the custom auth middleware.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** api/src/auth/config.ts
**Line:** 362:376
**Comment:**
*Logic Error: The `secure` flag for the Better Auth session cookie ignores the `RACKULA_AUTH_SESSION_COOKIE_SECURE` environment override and is always derived from `NODE_ENV`, which can diverge from `security.ts`'s `authSessionCookieSecure` setting; this leads to inconsistent `Secure` attributes between cookies issued by Better Auth and those issued by your own auth middleware (e.g. on `/auth/check` and `/auth/logout`), and in configurations that explicitly disable `Secure` in production the refresh/logout routes will downgrade a previously secure cookie to a non-secure one.
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. |
|
Addressed all three review items in follow-up commit
Validation:
|
|
@CodeAnt-AI review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
Question: review Answer: What this PR does (high level)
Positive points
Concrete review items, suggestions, and potential issues
Actionable recommendations (prioritized)
Overall impression
If you want, I can:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 154-162: The returned fallback session object currently hardcodes
role: "admin" (in the function that builds the fallback OIDC session mapping
where the object with keys sub, sid, iat, exp, idleExp, generation, role is
returned); add a brief explanatory comment immediately above this return that
states this is an MVP-only hardcoded admin role (temporary until role
configuration is added) and mention that future work will make the role
configurable via authSessionConfig or the OIDC claims; optionally annotate which
config key (authSessionConfig.authSessionGeneration) will be used when
implementing configurability so reviewers can find the intended extension point.
In `@api/src/auth/config.ts`:
- Line 5: The EnvMap type is duplicated in config.ts; instead of redefining it,
import the existing EnvMap from api/src/security.ts to avoid duplication and
keep types consistent—update config.ts to remove the type alias and add an
import for EnvMap from the module that exports it (referencing the EnvMap symbol
and the security.ts module) and adjust any usages in functions or exports in
config.ts to use the imported type.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
api/src/app.tsapi/src/auth/config.tsapi/src/oidc.integration.test.tsapi/src/security.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 148-152: When computing fallbackSubject (the expression using
session.user.email?.trim() || session.user.id?.trim() || "oidc-user"), add a
warning log when both email and id are missing and the code is about to use the
generic "oidc-user" value; locate the fallbackSubject assignment and insert a
single warning via the existing logger (e.g., processLogger or logger) that
includes enough non-sensitive context (e.g., session.id or request id) to aid
debugging, ensuring you do not log raw tokens or sensitive user data.
In `@api/src/auth/config.ts`:
- Around line 164-202: fetchOidcDiscoveryDocument currently calls
fetch(discoveryUrl, ...) without a timeout which can hang; wrap the fetch in an
AbortController (or use AbortSignal.timeout) with a sensible timeout value (e.g.
configurable constant) passed to fetch, pass controller.signal, and ensure you
call controller.abort() on timeout; update fetch error handling in
fetchOidcDiscoveryDocument to catch an AbortError and throw a clear
timeout-specific error (while keeping existing non-OK response and JSON parsing
checks), referencing discoveryUrl, expectedIssuer, normalizeIssuerUrl, and
parseAbsoluteUrl to locate the function to modify.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
api/src/app.tsapi/src/auth/config.tsapi/src/oidc.integration.test.tsapi/src/security.ts
- Import EnvMap from security.ts instead of redefining in config.ts - Add 10s timeout to OIDC discovery fetch via AbortSignal.timeout - Log warning when OIDC session falls back to generic "oidc-user" subject - Document MVP-only hardcoded admin role in fallback session mapping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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/app.ts`:
- Around line 155-164: The check "if (!fallbackSubject)" is dead because
fallbackSubject is always truthy due to the "|| 'oidc-user'" default; remove the
unreachable conditional or change the fallback logic so fallbackSubject can be
null/undefined when both session.user.email and session.user.id are missing.
Specifically, update the code around the fallbackSubject variable in app.ts (the
declaration and subsequent "if (!fallbackSubject)" block) by either deleting the
unreachable if block or by deriving fallbackSubject without the forced
"'oidc-user'" default and handling the null case explicitly where needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 141-142: The current guard uses falsy checks on the parsed epoch
values (if (!issuedAt || !expiresAt || expiresAt <= issuedAt)) which incorrectly
treats valid 0 timestamps as invalid; change the condition to explicit null
checks (e.g. issuedAt === null || expiresAt === null || expiresAt <= issuedAt)
so only null results from toEpochSeconds() are rejected while allowing 0, and
keep the same return null behavior when the check fails.
- Around line 82-90: The current code in the readSetCookieHeaders path uses a
defensive fallback that reads headers.get("set-cookie") after checking
withGetSetCookie.getSetCookie, which is unnecessary and can collapse multiple
cookies; update the implementation to assume Bun >= 1.0.0 and simply return
headers.getSetCookie() (or [] when headers is undefined). Remove the
withGetSetCookie cast, the conditional that checks getSetCookie, and the
rawSetCookie fallback; ensure the function (readSetCookieHeaders) first returns
[] if headers is falsy and otherwise returns headers.getSetCookie().
| const withGetSetCookie = headers as Headers & { | ||
| getSetCookie?: () => string[]; | ||
| }; | ||
| if (typeof withGetSetCookie.getSetCookie === "function") { | ||
| return withGetSetCookie.getSetCookie(); | ||
| } | ||
|
|
||
| const rawSetCookie = headers.get("set-cookie"); | ||
| return rawSetCookie ? [rawSetCookie] : []; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js Fetch API Headers getSetCookie() vs get() Set-Cookie multiple cookies
💡 Result:
-
headers.get('set-cookie')returns one value (a single string), because the FetchHeadersmodel historically can’t represent multipleSet-Cookieheader fields (andSet-Cookieis also treated specially/forbidden in browser-exposed responses). (fetch.spec.whatwg.org)- Even if multiple cookies were sent, you must not reliably split that string on commas (cookie
Expires=contains commas).
- Even if multiple cookies were sent, you must not reliably split that string on commas (cookie
-
headers.getSetCookie()(Node/server-focused) returns an array of allSet-Cookieheader values, preserving each cookie header separately (e.g.["a=1; ...", "b=2; ..."]). (developer.mozilla.org) -
If you’re using
node-fetch(not Node’s built-infetch), the common way to get all cookies isres.headers.raw()['set-cookie'](array). (stackoverflow.com)
Citations:
- 1: https://fetch.spec.whatwg.org/?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Headers/getSetCookie?utm_source=openai
- 3: https://stackoverflow.com/questions/53826792/how-to-get-all-set-cookie-headers-from-node-fetch?utm_source=openai
🌐 Web query:
Bun Fetch Headers getSetCookie() multiple Set-Cookie handling
💡 Result:
In Bun (server-side), don’t use headers.get("set-cookie") to read cookies when a response can contain multiple Set-Cookie headers—get() historically can’t represent them correctly because Set-Cookie cannot be safely comma-joined.
Use Headers.getSetCookie(), which returns an array of all Set-Cookie header values:
const res = await fetch(url);
const setCookies: string[] = res.headers.getSetCookie();This method exists specifically to handle “multiple Set-Cookie headers” properly. [1]
Sending multiple Set-Cookie headers in Bun
If you’re constructing a response and need multiple cookies, emit multiple header values (not a single comma-joined string). Bun’s CookieMap can produce the correct array form via toSetCookieHeaders():
import { CookieMap } from "bun";
const cookies = new CookieMap();
cookies.set("a", "1");
cookies.set("b", "2");
return new Response("ok", {
headers: {
"Set-Cookie": cookies.toSetCookieHeaders(), // string[]
},
});toSetCookieHeaders() explicitly returns a string[] intended to be used as multiple Set-Cookie header values. [2]
Note: In browsers, Set-Cookie is not exposed to frontend JS via Fetch/Headers; getSetCookie() is mainly for server runtimes. [1]
References: [1] MDN Headers.getSetCookie() docs, [2] Bun CookieMap.toSetCookieHeaders() docs.
🌐 Web query:
HTTP Set-Cookie headers multiple values get() method collapse
💡 Result:
Many HTTP client/server “header maps” treat repeated headers as a single header and collapse them (often by joining values with ,) when you call something like get("Set-Cookie"). That does not work for cookies:
Set-Cookieis special: to set multiple cookies, a response must send multipleSet-Cookieheader fields, not a single comma-separated value. Servers are advised not to combine multipleSet-Cookieheaders because commas are meaningful in cookie syntax (notably inExpires=), so combining can change semantics. [1]- In browsers (Fetch/XHR): you generally cannot read
Set-Cookieat all—Fetch defines it as a forbidden response header name, soresponse.headers.get("set-cookie")won’t give you the cookies. The browser handles cookie storage internally. [2] - In server-side Fetch (and some runtimes): use the newer
Headers.getSetCookie()API to retrieve allSet-Cookieheader values as an array, avoiding the collapse problem. [3]
Practical fixes (depending on environment):
- Don’t combine cookies into one header; emit multiple
Set-Cookielines. [1] - If you’re consuming responses in a server environment, prefer
getSetCookie()(or a runtime-specific “raw headers” API) instead ofget(). [3]
Sources:
[1] RFC 6265 (inline errata) guidance not to combine multiple Set-Cookie headers https://www.rfc-editor.org/rfc/inline-errata/rfc6265.html
[2] MDN: Set-Cookie is not accessible to frontend JS (forbidden response header) https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie
[3] MDN: Headers.getSetCookie() returns all Set-Cookie values (intended for server environments) https://developer.mozilla.org/en-US/docs/Web/API/Headers/getSetCookie
🏁 Script executed:
cat -n api/src/app.ts | head -100 | tail -30Repository: RackulaLives/Rackula
Length of output: 946
🏁 Script executed:
# Check for Node version requirements or runtime targets
fd -e json -e ts -e md | xargs grep -l "engines\|@types/node\|node-version\|Node.js" | head -5 | xargs cat | head -50Repository: RackulaLives/Rackula
Length of output: 2269
🏁 Script executed:
# Check imports and context around the problematic code
cat -n api/src/app.ts | head -95 | tail -20Repository: RackulaLives/Rackula
Length of output: 691
🏁 Script executed:
# Check package.json for Node version and dependencies
fd -name package.json | xargs cat | head -60Repository: RackulaLives/Rackula
Length of output: 294
🏁 Script executed:
# Check if there are comments or documentation about this fallback
grep -B5 -A5 "readSetCookieHeaders\|getSetCookie" api/src/app.tsRepository: RackulaLives/Rackula
Length of output: 746
🏁 Script executed:
# Check what runtime/framework is being used (looks like Hono based on Context)
rg "import.*Context|from.*hono" api/src/app.ts | head -10Repository: RackulaLives/Rackula
Length of output: 223
🏁 Script executed:
cat api/package.jsonRepository: RackulaLives/Rackula
Length of output: 582
🏁 Script executed:
# Check if there's any documentation or comments about Node version support
grep -r "node\|version\|engine" api/package.json | head -20Repository: RackulaLives/Rackula
Length of output: 100
🏁 Script executed:
# Check when getSetCookie was added and what Node versions support it
web_search "Node.js getSetCookie() available version added"Repository: RackulaLives/Rackula
Length of output: 114
🌐 Web query:
Bun Fetch Headers getSetCookie() available since version
💡 Result:
Bun doesn’t appear to document a specific Bun release where Headers.prototype.getSetCookie() was introduced. Bun states it implements the standard Headers Web API (used by fetch) but doesn’t provide per-method “since version” details. [1]
Headers.getSetCookie() itself is a Fetch-standard method (server/runtime-oriented) and is listed by MDN as a “Baseline 2023” API. [2]
Practical approach in Bun: feature-detect and/or pin a minimum Bun version in your project:
const supported = typeof new Headers().getSetCookie === "function";Sources: [1] [2]
🌐 Web query:
Bun Headers API getSetCookie() support all versions
💡 Result:
Headers.prototype.getSetCookie() (Fetch standard) is supported in Bun starting with Bun v1.0.0 (2023-09-08), so it’s available across all Bun 1.x releases (1.0, 1.1, 1.2, 1.3, etc.). It is not guaranteed in pre-1.0 Bun builds. [1][2]
Refs:
[1] api.Headers.getSetCookie compatibility data (shows Bun v1.0.0+) (bcd-training.deno.dev)
[2] MDN: Headers.getSetCookie() description/spec context (developer.mozilla.org)
Citations:
- 1: https://bcd-training.deno.dev/feature?id=api.Headers.getSetCookie&utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Headers/getSetCookie?utm_source=openai
Remove unnecessary fallback that assumes getSetCookie() unavailability.
The fallback to headers.get("set-cookie") is unreachable given the Bun >= 1.0.0 requirement in engines (where getSetCookie() is available). If executed in an older or non-standard runtime, the branch would collapse multiple cookies into a single value, breaking auth/session handling. Since the target runtime guarantees getSetCookie() support, simplify this to remove the defensive pattern:
function readSetCookieHeaders(headers: Headers | undefined): string[] {
if (!headers) {
return [];
}
return headers.getSetCookie();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/app.ts` around lines 82 - 90, The current code in the
readSetCookieHeaders path uses a defensive fallback that reads
headers.get("set-cookie") after checking withGetSetCookie.getSetCookie, which is
unnecessary and can collapse multiple cookies; update the implementation to
assume Bun >= 1.0.0 and simply return headers.getSetCookie() (or [] when headers
is undefined). Remove the withGetSetCookie cast, the conditional that checks
getSetCookie, and the rawSetCookie fallback; ensure the function
(readSetCookieHeaders) first returns [] if headers is falsy and otherwise
returns headers.getSetCookie().
| if (!issuedAt || !expiresAt || expiresAt <= issuedAt) { | ||
| return null; |
There was a problem hiding this comment.
Use explicit null checks for parsed epoch values.
toEpochSeconds() returns null for invalid values; using !issuedAt / !expiresAt also rejects valid 0 timestamps.
🛠️ Proposed fix
- if (!issuedAt || !expiresAt || expiresAt <= issuedAt) {
+ if (issuedAt === null || expiresAt === null || expiresAt <= issuedAt) {
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!issuedAt || !expiresAt || expiresAt <= issuedAt) { | |
| return null; | |
| if (issuedAt === null || expiresAt === null || expiresAt <= issuedAt) { | |
| return null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/app.ts` around lines 141 - 142, The current guard uses falsy checks
on the parsed epoch values (if (!issuedAt || !expiresAt || expiresAt <=
issuedAt)) which incorrectly treats valid 0 timestamps as invalid; change the
condition to explicit null checks (e.g. issuedAt === null || expiresAt === null
|| expiresAt <= issuedAt) so only null results from toEpochSeconds() are
rejected while allowing 0, and keep the same return null behavior when the check
fails.
Co-authored-by: codeant-ai[bot] <151821869+codeant-ai[bot]@users.noreply.github.com>
- Import EnvMap from security.ts instead of redefining in config.ts - Add 10s timeout to OIDC discovery fetch via AbortSignal.timeout - Log warning when OIDC session falls back to generic "oidc-user" subject - Document MVP-only hardcoded admin role in fallback session mapping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The || "oidc-user" default guarantees fallbackSubject is always truthy, making the if (!fallbackSubject) guard unreachable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ggfevans
force-pushed
the
fix/1335-entra-common-support
branch
from
7824a83 to
9a5c514
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:XXL
Sequence DiagramShows the main successful OIDC login path where a "common" Entra issuer is accepted even when discovery returns a tenant-specific issuer, the ID token audience is validated, a session cookie is created, and subsequent auth checks use the signed cookie or fallback Better Auth session mapping (with idle-timeout enforcement). sequenceDiagram
participant User
participant App
participant BetterAuth
participant OIDC
User->>App: GET /auth/login (next=/dashboard)
App->>BetterAuth: signInWithOAuth2(providerId=oidc, callbackURL)
BetterAuth->>OIDC: Fetch discovery & token exchange (discovery returns tenant-specific issuer)
OIDC-->>BetterAuth: jwks + id_token (tenant issuer)
BetterAuth-->>App: return id_token & set-cookie for provider flow
App->>App: validate id_token (issuer accepted for Entra common→tenant match, audience == client_id)
App-->>User: Set rackula_auth_session cookie and redirect to next
User->>App: GET /auth/check (with cookies)
alt Signed session cookie present & valid
App-->>User: 204 (refresh cookie if needed)
else Fallback session available from BetterAuth API
App->>BetterAuth: getSession(headers) => mapFallbackSessionClaims (use persisted created/expires to enforce idle-timeout)
alt Fallback valid
App-->>User: 204 (no silent extension)
else
App-->>User: 401 Unauthorized
end
end
Generated by CodeAnt AI |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/src/app.ts (1)
77-91: 🧹 Nitpick | 🔵 TrivialSimplify
readSetCookieHeadersto usegetSetCookie()directly.Since the project targets Bun >= 1.0.0 where
Headers.getSetCookie()is available, the fallback toheaders.get("set-cookie")is unnecessary and could incorrectly collapse multiple cookies into a single string if executed.♻️ Proposed simplification
function readSetCookieHeaders(headers: Headers | undefined): string[] { if (!headers) { return []; } - - const withGetSetCookie = headers as Headers & { - getSetCookie?: () => string[]; - }; - if (typeof withGetSetCookie.getSetCookie === "function") { - return withGetSetCookie.getSetCookie(); - } - - const rawSetCookie = headers.get("set-cookie"); - return rawSetCookie ? [rawSetCookie] : []; + return headers.getSetCookie(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/app.ts` around lines 77 - 91, The readSetCookieHeaders function currently falls back to headers.get("set-cookie") which can collapse multiple cookies; since Bun >= 1.0.0 provides Headers.getSetCookie(), simplify the function (readSetCookieHeaders) to check for headers presence and directly call and return headers.getSetCookie(), returning an empty array if headers is undefined; remove the custom type cast and the get("set-cookie") fallback so multiple Set-Cookie values are preserved.
🤖 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/oidc.integration.test.ts`:
- Around line 32-45: The readSetCookies function can be simplified to call
Headers.getSetCookie() directly: replace the current implementation (including
the cast to withGetSetCookie, try/catch, and the headers.get("set-cookie")
fallback) with a single direct call to headers.getSetCookie() and return its
result (ensuring the return type is string[]); update the function
signature/typing only if necessary to reflect that getSetCookie exists on
Headers in Bun >=1.0.0.
---
Duplicate comments:
In `@api/src/app.ts`:
- Around line 77-91: The readSetCookieHeaders function currently falls back to
headers.get("set-cookie") which can collapse multiple cookies; since Bun >=
1.0.0 provides Headers.getSetCookie(), simplify the function
(readSetCookieHeaders) to check for headers presence and directly call and
return headers.getSetCookie(), returning an empty array if headers is undefined;
remove the custom type cast and the get("set-cookie") fallback so multiple
Set-Cookie values are preserved.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
api/.env.exampleapi/src/app.tsapi/src/auth/config.tsapi/src/oidc.integration.test.tsapi/src/security.tsdocs/deployment/AUTHENTICATION.md
| function readSetCookies(headers: Headers): string[] { | ||
| const withGetSetCookie = headers as Headers & { getSetCookie: () => string[] }; | ||
| try { | ||
| const setCookies = withGetSetCookie.getSetCookie(); | ||
| if (Array.isArray(setCookies)) { | ||
| return setCookies; | ||
| } | ||
| } catch { | ||
| // Fall through to standard header handling. | ||
| } | ||
|
|
||
| const raw = headers.get("set-cookie"); | ||
| return raw ? [raw] : []; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify readSetCookies to use getSetCookie() directly.
Since the project targets Bun >= 1.0.0 where Headers.getSetCookie() is available, the fallback to headers.get("set-cookie") is unnecessary. The fallback branch could also incorrectly handle multiple cookies by collapsing them into a single string.
♻️ Proposed simplification
function readSetCookies(headers: Headers): string[] {
- const withGetSetCookie = headers as Headers & { getSetCookie: () => string[] };
- try {
- const setCookies = withGetSetCookie.getSetCookie();
- if (Array.isArray(setCookies)) {
- return setCookies;
- }
- } catch {
- // Fall through to standard header handling.
- }
-
- const raw = headers.get("set-cookie");
- return raw ? [raw] : [];
+ return headers.getSetCookie();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/oidc.integration.test.ts` around lines 32 - 45, The readSetCookies
function can be simplified to call Headers.getSetCookie() directly: replace the
current implementation (including the cast to withGetSetCookie, try/catch, and
the headers.get("set-cookie") fallback) with a single direct call to
headers.getSetCookie() and return its result (ensuring the return type is
string[]); update the function signature/typing only if necessary to reflect
that getSetCookie exists on Headers in Bun >=1.0.0.
|
CodeAnt AI Incremental review completed. |
1 participant
User description
Summary
/auth/loginand/auth/callbackcompatibility routes with Better Auth generic OAuth2 flowTesting
Closes #1335
CodeAnt-AI Description
Support Microsoft Entra 'common' OIDC discovery, restore legacy auth routes, and enforce fallback session idle timeout
What Changed
Impact
✅ Fewer failed Entra logins✅ Clearer callback rejection when client ID is wrong✅ Fewer replayable fallback sessions after logout💡 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.