fix: Correctly handle all isAuthenticationErrorData cases#1573
fix: Correctly handle all isAuthenticationErrorData cases#1573gjtorikian merged 3 commits intoworkos:mainfrom
isAuthenticationErrorData cases#1573Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReorders HTTP error handling to detect authentication-error payloads first, refactors authentication error types/guards to accept Changes
Possibly related PRs
🚥 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)
Comment |
Greptile SummaryThis PR fixes a misclassification bug where Confidence Score: 5/5Safe to merge — the fix is minimal, well-typed, and covered by a new regression test. The root cause is clearly identified and the fix is a one-line reordering backed by a discriminated union and proper overloads. No P0 or P1 issues found; all three changed files are consistent with each other. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[HTTP Response - default status] --> B{isAuthenticationErrorData?}
B -->|"data.code or data.error in AUTH_CODES"| C[throw AuthenticationException\ncode = getAuthenticationErrorCode\nmessage = data.message ?? data.error_description]
B -->|No| D{error or errorDescription?}
D -->|Yes| E[throw OauthException]
D -->|No| F{code and errors?}
F -->|Yes| G[throw BadRequestException]
F -->|No| H[throw GenericServerException]
style C fill:#d4edda,stroke:#28a745
style E fill:#fff3cd,stroke:#ffc107
Reviews (4): Last reviewed commit: "fix: Narrow `AuthenticationErrorData` to..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
davidcornu
force-pushed
the
authentication-error-precedence
branch
from
2a69959 to
14a7c23
Compare
Review NotesNice fix — the reordering in 1. Type guard unsoundness on
|
|
Thanks for the review @gjtorikian. For context - I have no attachment to my current implementation, just figured I'd put together a quick fix as I've unfortunately become all-too-familiar with this part of the SDK over the last few months and the upgrade to v9 caused production errors for us (we got a
Good point. Thanks for checking the source!
This doesn't make for a great experience for downstream users as they now have to care about whether
thoughts?
Sure thing. Surprised this wasn't flagged on the original PR. |
The API returns authentication errors in two shapes: 403s use `code` + `message`, while 400s (e.g. `sso_required`) use `error` + `error_description`. The previous type declared `code` as non-optional, so TypeScript consumers got `undefined` at runtime with no compiler warning after the type guard narrowed. Model the two shapes as a discriminated union and add a guaranteed `AuthenticationException.code` accessor that normalizes both into a single `AuthenticationErrorCode`. Adds a regression test with the actual `sso_required` payload shape.
|
Yeah, I do agree that users shouldn't have to check both As I took notes across the PR and the spec I realized it would probably just be easier for me to push the changes here, so. Consumers can just use I'm sort of speed running this (it's late on a Friday!), but the changes I made are, at least, technically accurate. Let me know if they solve your use case and I can push a release. |
![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 the current code and only fix it if needed.
Inline comments:
In `@src/workos.spec.ts`:
- Line 374: Replace the realistic API key literal passed into the WorkOS
constructor with the standard test placeholder 'n' to avoid secret-scanner
churn; locate the instantiation "new
WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU')" in the test (the WorkOS constructor
call) and change the argument to 'n' so the line reads new WorkOS('n').
🪄 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 Plus
Run ID: 7d6faad5-cb72-4eee-bc04-8470e5a1a2f8
📒 Files selected for processing (2)
src/common/exceptions/authentication.exception.tssrc/workos.spec.ts
| headers: { 'X-Request-ID': 'a-request-id' }, | ||
| }); | ||
|
|
||
| const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU'); |
There was a problem hiding this comment.
Use the standard test API key placeholder on Line 374.
Please replace the realistic-looking key with 'n' to match suite convention and avoid secret-scanner churn in tests.
🔧 Suggested change
- const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU');
+ const workos = new WorkOS('n');Based on learnings: in workos-node TypeScript tests, new WorkOS('n') is the established placeholder-key convention and deviations should be flagged.
📝 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.
| const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU'); | |
| const workos = new WorkOS('n'); |
🧰 Tools
🪛 Betterleaks (1.1.2)
[high] 374-374: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
(stripe-access-token)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/workos.spec.ts` at line 374, Replace the realistic API key literal passed
into the WorkOS constructor with the standard test placeholder 'n' to avoid
secret-scanner churn; locate the instantiation "new
WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU')" in the test (the WorkOS constructor
call) and change the argument to 'n' so the line reads new WorkOS('n').
No rush. Solved my issue by reverting some of our v9 changes and adding a code comment that points to this PR 😬. |
| interface BaseAuthenticationErrorData extends WorkOSErrorData { | ||
| error?: string; | ||
| error_description?: string; | ||
| pending_authentication_token?: string; | ||
| user?: UserResponse; | ||
| organizations?: Array<{ id: string; name: string }>; | ||
| connection_ids?: string[]; | ||
| } | ||
|
|
||
| export type AuthenticationErrorData = | ||
| | (BaseAuthenticationErrorData & { | ||
| code: AuthenticationErrorCode; | ||
| }) | ||
| | (BaseAuthenticationErrorData & { | ||
| code?: AuthenticationErrorCode; | ||
| error: AuthenticationErrorCode; | ||
| }); | ||
|
|
There was a problem hiding this comment.
Returning AuthenticationErrorCode isn't actually correct (and might lead to downstream users implementing a lot more cases than actually exist).
If sso_required is the only case where error and error_description are present then maybe this can be a bit more specific?
interface BaseAuthenticationErrorData extends WorkOSErrorData {
pending_authentication_token?: string;
user?: UserResponse;
organizations?: Array<{ id: string; name: string }>;
connection_ids?: string[];
}
export type AuthenticationErrorData =
| (BaseAuthenticationErrorData & {
code: Exclude<AuthenticationErrorCode, "sso_required">;
})
| (BaseAuthenticationErrorData & {
error: "sso_required";
error_description: string;
});
|
|
||
| export class AuthenticationException extends GenericServerException { | ||
| readonly name = 'AuthenticationException'; | ||
| override readonly code: AuthenticationErrorCode; |
There was a problem hiding this comment.
This is definitely a better user experience.
Only `sso_required` uses the OAuth-style `error`/`error_description` response shape. Typing `error` as the full `AuthenticationErrorCode` union misleads downstream users into handling cases that don't exist. Addresses PR feedback from @davidcornu.
Co-authored-by: Garen J. Torikian <gjtorikian@users.noreply.github.com>
|
Thanks @gjtorikian ❤️ |
2 participants
Description
#1561 added a new
AuthenticationExceptiontype to handle the following cases:workos-node/src/common/exceptions/authentication.exception.ts
Lines 7 to 13 in 96cf545
However, because the error handling logic first checks whether the payload has an
errororerror_descriptionfieldworkos-node/src/workos.ts
Lines 486 to 487 in 96cf545
sso_requiredis being treated as anOauthExceptioninstead of aAuthenticationException(see https://workos.com/docs/reference/authkit/authentication-errors#sso-required-error).Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.
Summary by CodeRabbit