refactor: add hardening to auth flows and jose operations#165
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors AuthConfig and cookie helpers, reworks OAuth callback and access-token error paths, tightens crypto and secret-entropy checks, adds JOSE asymmetric algorithm inference and API rename, updates error stack-trace handling, and adjusts tests and docs. ChangesCore + JOSE changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/jose.ts (1)
279-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a rejection handler to the eagerly-started JOSE init promise
In
packages/core/src/jose.ts(createJoseInstance()),const jose = (async () => { ... })()starts immediately, but there’s no attached rejection handler. IfgetSecrets()/key import fails before any method awaitsjose, the failure can surface as an unhandled rejection during construction.Suggested fix
const jose = (async () => { const { jwsSecret, jweSecret, jwtSecret } = await getSecrets(secretKey, salt, session) return { jwt: createJWT<DefaultUser>(jwtSecret), jws: createJWS<DefaultUser>(jwsSecret), jwe: createJWE<DefaultUser>(jweSecret), } })() + void jose.catch(() => {}) return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/jose.ts` around lines 279 - 287, The eagerly-started promise stored in the jose constant (created by the IIFE that calls getSecrets and returns { jwt: createJWT(...), jws: createJWS(...), jwe: createJWE(...) }) has no rejection handler; attach a .catch handler to that promise to consume/log the error and rethrow if appropriate (or otherwise surface it consistently) so unhandled-rejection warnings are avoided — use the jose constant itself and the getSecrets/createJWT/createJWS/createJWE symbols to locate the code and call your project logger (or console.error) inside the .catch before rethrowing.
🧹 Nitpick comments (2)
packages/core/src/shared/errors.ts (2)
11-16: ⚡ Quick winSimplify the type guard check.
The first check
typeof errorConstructor === "function"is redundant since the parameter typeErrorConstructoralready guarantees it's a constructor function. You can remove this check without losing safety.♻️ Simplified implementation
export const hasCaptureStackTrace = (errorConstructor: ErrorConstructor): errorConstructor is V8ErrorConstructor => { - return ( - typeof errorConstructor === "function" && - "captureStackTrace" in errorConstructor && - typeof (errorConstructor as any).captureStackTrace === "function" - ) + return "captureStackTrace" in errorConstructor && typeof (errorConstructor as any).captureStackTrace === "function" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/shared/errors.ts` around lines 11 - 16, The type-guard hasCaptureStackTrace contains a redundant typeof errorConstructor === "function" check; update the function (hasCaptureStackTrace) to remove that first check and only test that "captureStackTrace" is a property on errorConstructor and that (errorConstructor as any).captureStackTrace is a function, keeping the return type predicate : errorConstructor is V8ErrorConstructor unchanged so callers still get the narrowed type.
3-17: ⚡ Quick winExtract duplicated type guard to shared utility.
The
V8ErrorConstructorinterface andhasCaptureStackTracefunction are duplicated inpackages/jose/src/errors.ts. Consider extracting this to a shared utility module to maintain a single source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/shared/errors.ts` around lines 3 - 17, The V8ErrorConstructor interface and hasCaptureStackTrace type guard are duplicated (found in packages/core/src/shared/errors.ts and packages/jose/src/errors.ts); extract them into a single shared utility module (e.g., export interface V8ErrorConstructor and export const hasCaptureStackTrace from a new shared file) and replace the duplicates by importing these symbols where needed (update usages in functions/classes that reference V8ErrorConstructor and hasCaptureStackTrace to import from the new shared module), then remove the local duplicate definitions in both files so there is a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/`@types/config.ts:
- Around line 162-233: The union's discriminant prevents assigning a computed
boolean to trustedProxyHeaders; update the TrustedProxyHeadersConfig declaration
so trustedProxyHeaders is not a literal-only type (e.g., use
trustedProxyHeaders?: boolean instead of trustedProxyHeaders: true /
trustedProxyHeaders?: false) while keeping the trustedOrigins types
(TrustedOrigin[] | (request: Request) => Promise<TrustedOrigin[]> |
TrustedOrigin[] ) intact; this makes computed expressions like
process.env.AURA_AUTH_TRUSTED_PROXY_HEADERS === "true" type-check against
TrustedProxyHeadersConfig (symbol: TrustedProxyHeadersConfig, fields:
trustedProxyHeaders, trustedOrigins, types: TrustedOrigin, Request).
In `@packages/core/src/cookie.ts`:
- Around line 67-73: getExpiredCookie currently forces secure: true which can
prevent browsers from clearing non-secure cookies on HTTP; change the returned
object to preserve the incoming options' secure flag instead of forcing true
(i.e., set secure to options?.secure or leave it undefined when not provided)
while keeping expires: new Date(0) and maxAge: 0 so logout/cleanup works
correctly; update the function getExpiredCookie and its use of
SerializeOptions/options to reference the original secure value rather than
hardcoding true.
In `@packages/jose/CHANGELOG.md`:
- Line 19: The CHANGELOG line claiming "at least 3.5 bits of entropy per
character" is inconsistent with the implementation constant
MIN_SECRET_ENTROPY_PER_CHAR (value 4) in packages/jose/src/secret.ts; update the
CHANGELOG entry to state "at least 4 bits of entropy per character" (leave the
minimum total entropy 254 bits as-is) so the documentation matches the code.
In `@packages/jose/src/errors.ts`:
- Around line 28-30: The optional chaining after the type guard is redundant:
inside the constructor where hasCaptureStackTrace(Error) is checked, remove the
`?.` and call `Error.captureStackTrace(this, new.target)` directly; update the
block that currently reads `if (hasCaptureStackTrace(Error)) {
Error.captureStackTrace?.(this, new.target) }` to call
`Error.captureStackTrace(this, new.target)` so it matches the usage in the other
errors helper and relies on the type guard for safety.
---
Outside diff comments:
In `@packages/core/src/jose.ts`:
- Around line 279-287: The eagerly-started promise stored in the jose constant
(created by the IIFE that calls getSecrets and returns { jwt: createJWT(...),
jws: createJWS(...), jwe: createJWE(...) }) has no rejection handler; attach a
.catch handler to that promise to consume/log the error and rethrow if
appropriate (or otherwise surface it consistently) so unhandled-rejection
warnings are avoided — use the jose constant itself and the
getSecrets/createJWT/createJWS/createJWE symbols to locate the code and call
your project logger (or console.error) inside the .catch before rethrowing.
---
Nitpick comments:
In `@packages/core/src/shared/errors.ts`:
- Around line 11-16: The type-guard hasCaptureStackTrace contains a redundant
typeof errorConstructor === "function" check; update the function
(hasCaptureStackTrace) to remove that first check and only test that
"captureStackTrace" is a property on errorConstructor and that (errorConstructor
as any).captureStackTrace is a function, keeping the return type predicate :
errorConstructor is V8ErrorConstructor unchanged so callers still get the
narrowed type.
- Around line 3-17: The V8ErrorConstructor interface and hasCaptureStackTrace
type guard are duplicated (found in packages/core/src/shared/errors.ts and
packages/jose/src/errors.ts); extract them into a single shared utility module
(e.g., export interface V8ErrorConstructor and export const hasCaptureStackTrace
from a new shared file) and replace the duplicates by importing these symbols
where needed (update usages in functions/classes that reference
V8ErrorConstructor and hasCaptureStackTrace to import from the new shared
module), then remove the local duplicate definitions in both files so there is a
single source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 737d6758-dd1f-4338-a044-1b9e01a50e0f
📒 Files selected for processing (23)
packages/core/src/@types/config.tspackages/core/src/actions/callback/access-token.tspackages/core/src/actions/callback/callback.tspackages/core/src/actions/callback/userinfo.tspackages/core/src/actions/signIn/authorization.tspackages/core/src/api/getSession.tspackages/core/src/cookie.tspackages/core/src/jose.tspackages/core/src/session/cookie-manager.tspackages/core/src/session/stateless.tspackages/core/src/shared/crypto.tspackages/core/src/shared/env.tspackages/core/src/shared/errors.tspackages/core/src/shared/utils.tspackages/core/src/validator/registry.tspackages/core/test/actions/callback/access-token.test.tspackages/core/test/actions/callback/callback.test.tspackages/core/test/actions/session/session.test.tspackages/core/test/actions/signIn/authorization.test.tspackages/jose/CHANGELOG.mdpackages/jose/src/errors.tspackages/jose/src/secret.tspackages/jose/test/index.test.ts
💤 Files with no reviewable changes (3)
- packages/core/test/actions/callback/callback.test.ts
- packages/core/test/actions/session/session.test.ts
- packages/core/src/session/stateless.ts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/jose/src/encrypt.ts`:
- Around line 43-47: The alg/key-management inference currently only checks
isCryptoKey(secret) and secret.type, which misses RSA JWK objects produced by
createSecret; update the logic in the EncryptJWT creation and corresponding
decrypt flows (where isAsymmetricCryptoKey is computed) to detect RSA JWKs by
inspecting secret.kty === "RSA" and treating JWK public keys as asymmetric
encryptable (set alg: "RSA-OAEP-256") and JWK private keys as asymmetric
decryptable, or alternatively convert JWKs to CryptoKey via createSecret before
deciding; implement the detection in the same places that compute
isAsymmetricCryptoKey (references: isCryptoKey, createSecret, EncryptJWT,
setProtectedHeader/alg) and add a regression test that encrypts with an RSA
public JWK (without passing alg) and decrypts with the matching RSA private JWK
(without specifying keyManagementAlgorithms) asserting successful round-trip.
In `@packages/jose/src/sign.ts`:
- Around line 33-38: signJWS/verifyJWS currently force "RS256" for any
asymmetric CryptoKey causing wrong alg selection for RSA-PSS, ECDSA, EdDSA keys;
change the logic that computes the JWS alg/algorithms (the isAsymmetricCryptoKey
branch around createSecret/isAsymmetricCryptoKey and the setProtectedHeader call
in SignJWT) to derive the proper JWS alg from the CryptoKey.algorithm.name (or
the key's algorithm metadata) instead of defaulting to "RS256": map "RSA-PSS" →
PS256/PS384/PS512 (choose matching hash if available), "RSASSA-PKCS1-v1_5" →
RS*, "ECDSA" → ES*, and "EdDSA" → EdDSA, and apply that computed alg to
setProtectedHeader (and for verifyJWS set allowed algorithms accordingly when
options.algorithms is not provided); keep honoring an explicit
options.alg/options.algorithms if caller supplied one.
In `@packages/jose/test/encrypt.test.ts`:
- Line 135: The test uses an invalid JWE enc value "RSA-OAP-256" in the
contentEncryptionAlgorithms option, causing early validation failures; update
the contentEncryptionAlgorithms array to use a valid JWE content encryption
algorithm (e.g. "A128GCM") so the negative test exercises the intended enc
mismatch with dir (the test that encrypts with enc: "A256GCM"); locate the
contentEncryptionAlgorithms setting in the encrypt.test.ts negative tests and
replace "RSA-OAP-256" with a valid enc like "A128GCM".
In `@packages/jose/test/sign.test.ts`:
- Around line 42-56: Tests are failing due to using short literal secrets
("my-secret-key", "wrong-secret-key") which trigger the built-in secret-length
validation instead of exercising token-parsing/signature paths; update both
tests to supply secrets >=32 bytes (e.g., use getRandomBytes(32) or derive a
32-byte secret via createDeriveKey/getRandomBytes and pass that into createJWS)
so verifyJWS is called with a valid-length secret and the expectations exercise
invalid-token and invalid-signature behavior for signJWS, verifyJWS, createJWS,
createDeriveKey and getRandomBytes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2b4d967-b308-44a4-bf24-e272c1255544
📒 Files selected for processing (15)
packages/core/src/@types/config.tspackages/core/src/cookie.tspackages/core/src/shared/errors.tspackages/core/test/actions/signIn/authorization.test.tspackages/jose/CHANGELOG.mdpackages/jose/src/assert.tspackages/jose/src/encrypt.tspackages/jose/src/errors.tspackages/jose/src/index.tspackages/jose/src/sign.tspackages/jose/test/deriveKey.test.tspackages/jose/test/encrypt.test.tspackages/jose/test/index.test.tspackages/jose/test/secret.test.tspackages/jose/test/sign.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/jose/CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request introduces additional hardening across authentication flows in
@aura-stack/authand@aura-stack/joseto strengthen security guarantees, improve validation reliability, and reduce configuration risks.The update includes security-focused improvements, bug fixes, and cryptographic enhancements designed to improve both runtime safety and developer experience.
Bug Fixes
get-sessionresponsesSecurity Improvements
Features
Key Changes