[auth-unkey] Expand fail-closed resolver coverage#41
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Unkey bearer resolver now treats verification exceptions as unauthenticated and can report them through an optional callback. Tests expand coverage for invalid codes, malformed tokens, tenant scoping, and metadata preservation. ChangesFail-closed verification error handling
Estimated code review effort: 3 (Moderate) | ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
No new issues found.
TL;DR — Wraps client.keys.verifyKey() in try/catch inside @dsar/auth-unkey so a thrown Unkey verification error fails closed (resolver returns undefined) instead of bubbling out, and backfills test coverage for invalid-result codes, provider throws, malformed-token pass-through, and tenant metadata preservation.
Key changes
- Fail closed on
verifyKeythrow — extract averifyUnkeyToken()helper that returnsundefinedwhenverifyKeyrejects, and widenisValidResultto acceptundefined. - Table-driven invalid-result coverage —
INVALID_VERIFY_RESULTSexercisesEXPIRED,REVOKED,MALFORMED, andRATE_LIMITEDvalid: falseshapes. - Malformed-token pass-through assertion — documents that the resolver delegates malformed-looking tokens to Unkey rather than short-circuiting locally.
- Tenant metadata preservation test — sanity check that tenant-scoped metadata flows into the resolved identity.
Summary ｜ 2 files ｜ 1 commit ｜ base: main ← fix/17-auth-unkey-test-coverage
Fail-closed on provider throw
Before: A thrown error from
client.keys.verifyKey()escapedmakeUnkeyBearerResolver()and surfaced as a provider error to callers.
After: Thrown verification errors are caught inverifyUnkeyToken()and converted toundefined, matching the behavior forvalid: falseresults.
The contract narrows in the safe direction: callers no longer have to distinguish "Unkey threw" from "key rejected" — both surface as unauthenticated. The trade is that infra-level Unkey outages become observationally indistinguishable from real auth rejections at this layer; if observability hooks land later, this catch block is the place to emit them.
packages/auth-unkey/src/index.ts
Expanded fail-closed test coverage
Before: Only one invalid-result shape and no provider-throw case were exercised.
After:it.eachtables cover four invalid-result codes plus a dedicated rejection test, and malformed-token inputs are asserted to round-trip to Unkey unchanged.
The new createInvalidVerifyKey / createRecordingInvalidVerifyKey helpers keep the table-driven tests compact, and the tenant-metadata test pins down the happy-path mapping for tenant-scoped identities.
packages/auth-unkey/test/index.test.ts
Claude Opus ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/auth-unkey/src/index.ts`:
- Around line 194-197: The catch block that swallows provider failures for
client.keys.verifyKey(verifyInput) should capture the thrown error (use catch
(err)), emit an observability signal (e.g., increment a metric like
"unkey.verify.failure" and/or call a structured logger such as
processLogger.warn or metricsClient.increment) including context (verifyInput
identifier or relevant metadata) and the error details, and then continue to
return undefined to preserve the fail-closed behavior; update the catch in the
function that returns UnkeyVerifyResultShape to perform these actions before
returning undefined.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59a6c455-64ec-4d16-ba13-e4c06ca71c1f
📒 Files selected for processing (2)
packages/auth-unkey/src/index.tspackages/auth-unkey/test/index.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/auth-unkey/src/index.tspackages/auth-unkey/test/index.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types instead of relying on Oxlint + Oxfmt to catch naming issues
Add comments for complex logic, but prefer self-documenting code
Files:
packages/auth-unkey/src/index.tspackages/auth-unkey/test/index.test.ts
**/index.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid barrel files (index files that re-export everything)
Files:
packages/auth-unkey/src/index.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/auth-unkey/test/index.test.ts
🔇 Additional comments (1)
packages/auth-unkey/test/index.test.ts (1)
8-35: LGTM!Also applies to: 78-92, 134-147, 337-421
|
I kept this scoped to #17’s fail-closed requirement. The resolver now treats thrown Unkey verification errors the same as invalid verification results by returning |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — one new test in packages/auth-unkey/test/index.test.ts exercising tenant-mismatch rejection via a custom mapIdentity. No source changes since the prior review.
- Added tenant-mismatch rejection test — uses
verifyAdminKey(resolvestenantId: "tenant-1") with amapRequestedTenantIdentitymapper that returnsnullwhen thex-requested-tenant-idheader disagrees, asserting the resolver short-circuits toundefined. Closes a gap around custommapIdentityreturningnull.
Claude Opus ｜ 𝕏
|
Maintainer review: reviewed the diff for supply-chain safety (no dependency, lockfile, or workflow changes; no exec/network beyond the resolver under test) and behavior. The fail-closed wrapping of |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the new commit adds an optional onVerifyError observability hook to the fail-closed Unkey resolver, directly resolving the "errors swallowed silently" tradeoff flagged in prior review context.
- Added
onVerifyErrorconfig hook —UnkeyBearerResolverConfiggains an optionalonVerifyError?: (error: unknown) => void, invoked insideverifyUnkeyToken's catch block before returningundefined. The resolver still fails closed; the hook only lets hosts log or emit metrics for thrown verification failures. - Fires on throw only — the hook is intentionally not called for
valid: falseresults, keeping normal auth rejections distinct from provider errors. The doc comment on the type reflects this. - Added coverage — a test asserts the hook receives the thrown error while the resolver still resolves to
undefined. - Bumped changeset
patch→minor— correct semver for the new public API surface.
Claude Opus ｜ 𝕏
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)
packages/auth-unkey/src/index.ts (1)
188-201: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProtect fail-closed behavior when
onVerifyErrorthrows.If the
onVerifyErrorcallback throws synchronously (e.g., a metrics SDK is also down), the exception propagates out of the catch block beforereturn undefinedis reached, breaking the fail-closed guarantee this PR establishes. The resolver would throw instead of returningundefined, potentially surfacing a 500 instead of an unauthenticated response.🔒️ Proposed fix
} catch (error) { - onVerifyError?.(error); + try { + onVerifyError?.(error); + } catch { + // Swallow callback errors to preserve fail-closed behavior + } return undefined; }🤖 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/auth-unkey/src/index.ts` around lines 188 - 201, The fail-closed path in verifyUnkeyToken is broken if onVerifyError throws inside the catch block, because the exception escapes before undefined is returned. Update verifyUnkeyToken in index.ts so the onVerifyError callback is invoked safely inside its own protected handling, preserving the current behavior of returning undefined whenever client.keys.verifyKey fails. Keep the fix localized to verifyUnkeyToken and ensure any callback failure cannot turn the resolver into a throw.
🤖 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.
Outside diff comments:
In `@packages/auth-unkey/src/index.ts`:
- Around line 188-201: The fail-closed path in verifyUnkeyToken is broken if
onVerifyError throws inside the catch block, because the exception escapes
before undefined is returned. Update verifyUnkeyToken in index.ts so the
onVerifyError callback is invoked safely inside its own protected handling,
preserving the current behavior of returning undefined whenever
client.keys.verifyKey fails. Keep the fix localized to verifyUnkeyToken and
ensure any callback failure cannot turn the resolver into a throw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a38d310-3590-4287-9324-c9629a4cf220
📒 Files selected for processing (4)
.changeset/auth-unkey-fail-closed.mdpackages/auth-unkey/src/index.tspackages/auth-unkey/src/types.tspackages/auth-unkey/test/index.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/auth-unkey/src/types.tspackages/auth-unkey/src/index.tspackages/auth-unkey/test/index.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types instead of relying on Oxlint + Oxfmt to catch naming issues
Add comments for complex logic, but prefer self-documenting code
Files:
packages/auth-unkey/src/types.tspackages/auth-unkey/src/index.tspackages/auth-unkey/test/index.test.ts
**/index.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid barrel files (index files that re-export everything)
Files:
packages/auth-unkey/src/index.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/auth-unkey/test/index.test.ts
🪛 markdownlint-cli2 (0.22.1)
.changeset/auth-unkey-fail-closed.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (5)
packages/auth-unkey/src/types.ts (1)
54-61: LGTM!packages/auth-unkey/src/index.ts (1)
219-223: LGTM!packages/auth-unkey/test/index.test.ts (2)
390-412: LGTM!Also applies to: 414-435, 437-458, 460-480
352-369: 📐 Maintainability & Code QualityTest arrays cover the required cases.
.changeset/auth-unkey-fail-closed.md (1)
1-5: LGTM!
2 participants
Linked Issue
Closes #17
Description
This expands
@dsar/auth-unkeycoverage around invalid and failing Unkey verification paths.Before this change, invalid Unkey verification results returned
undefined, but a thrown Unkey verification error escaped frommakeUnkeyBearerResolver(). That meant an unreachable or failing Unkey client could bubble a provider error instead of failing closed as unauthenticated.This change wraps the Unkey
verifyKeycall and returnsundefinedwhen verification throws, keeping thrown provider failures consistent with invalid/rejected key results.The tests cover:
Type of Change
Test Coverage
Targeted tests passed:
Latest targeted local verification: