Skip to content

fix: Correctly handle all isAuthenticationErrorData cases #1573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gjtorikian merged 3 commits into from
Apr 24, 2026
Merged

fix: Correctly handle all isAuthenticationErrorData cases #1573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14a7c23
fix: Correctly handle all `isAuthenticationErrorData` cases
davidcornu Apr 24, 2026
a573a19
fix: Make `AuthenticationErrorData` type-safe for both response shapes
gjtorikian Apr 24, 2026
18a7ec4
fix: Narrow `AuthenticationErrorData` to match actual API shapes
gjtorikian Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 50 additions & 6 deletions src/common/exceptions/authentication.exception.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,22 @@ export type AuthenticationErrorCode =
| 'mfa_verification'
| 'sso_required';

export interface AuthenticationErrorData extends WorkOSErrorData {
code: AuthenticationErrorCode;
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;
});

Comment on lines +15 to +30
Copy link
Copy Markdown
Contributor Author

@davidcornu davidcornu Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    });

Copy link
Copy Markdown
Contributor

@gjtorikian gjtorikian Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it 👍

const AUTHENTICATION_ERROR_CODES: ReadonlySet<string> = new Set<string>([
'email_verification_required',
'organization_selection_required',
Expand All @@ -29,24 +37,60 @@ const AUTHENTICATION_ERROR_CODES: ReadonlySet<string> = new Set<string>([
'sso_required',
]);

export function isAuthenticationErrorData(
function parseAuthenticationErrorCode(
value: unknown,
): AuthenticationErrorCode | undefined {
if (typeof value !== 'string') {
return;
}

if (!AUTHENTICATION_ERROR_CODES.has(value)) {
return;
}

return value as AuthenticationErrorCode;
}

function getAuthenticationErrorCode(
data: AuthenticationErrorData,
): AuthenticationErrorCode;
function getAuthenticationErrorCode(
data: WorkOSErrorData,
): data is AuthenticationErrorData {
): AuthenticationErrorCode | undefined;
function getAuthenticationErrorCode(
data: WorkOSErrorData,
): AuthenticationErrorCode | undefined {
return (
typeof data.code === 'string' && AUTHENTICATION_ERROR_CODES.has(data.code)
parseAuthenticationErrorCode(data.code) ??
parseAuthenticationErrorCode(data.error)
);
}

export function isAuthenticationErrorData(
data: WorkOSErrorData,
): data is AuthenticationErrorData {
return getAuthenticationErrorCode(data) !== undefined;
}

export class AuthenticationException extends GenericServerException {
readonly name = 'AuthenticationException';
override readonly code: AuthenticationErrorCode;
Copy link
Copy Markdown
Contributor Author

@davidcornu davidcornu Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely a better user experience.

readonly pendingAuthenticationToken: string | undefined;

constructor(
status: number,
readonly rawData: AuthenticationErrorData,
requestID: string,
) {
super(status, rawData.message, rawData, requestID);
const code = getAuthenticationErrorCode(rawData);

super(
status,
rawData.message ?? (rawData.error_description as string | undefined),
rawData,
requestID,
);
this.code = code;
this.pendingAuthenticationToken = rawData.pending_authentication_token;
}
}
29 changes: 29 additions & 0 deletions src/workos.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import fetch from 'jest-fetch-mock';
import { fetchOnce, fetchHeaders, fetchBody } from './common/utils/test-utils';
import {
ApiKeyRequiredException,
AuthenticationException,
GenericServerException,
NotFoundException,
OauthException,
Expand Down Expand Up @@ -355,6 +356,34 @@ describe('WorkOS', () => {
),
);
});

it('throws an AuthenticationException for known authentication errors', async () => {
const rawData = {
error: 'sso_required',
error_description:
'User must authenticate using one of the matching connections.',
email: 'user@example.com',
connection_ids: ['conn_123'],
};

fetchOnce(rawData, {
status: 400,
headers: { 'X-Request-ID': 'a-request-id' },
});

const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU');
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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').

const request = workos.post('/path', {});

await expect(request).rejects.toBeInstanceOf(AuthenticationException);
await expect(request).rejects.toMatchObject({
code: 'sso_required',
message:
'User must authenticate using one of the matching connections.',
name: 'AuthenticationException',
rawData,
status: 400,
});
});
});

describe('when the api responses with a 429', () => {
Expand Down
6 changes: 3 additions & 3 deletions src/workos.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,9 @@ export class WorkOS {
);
}
default: {
if (error || errorDescription) {
if (isAuthenticationErrorData(data)) {
throw new AuthenticationException(status, data, requestID);
} else if (error || errorDescription) {
Comment thread
greptile-apps[bot] marked this conversation as resolved.
throw new OauthException(
status,
requestID,
Expand All @@ -500,8 +502,6 @@ export class WorkOS {
message,
requestID,
});
} else if (isAuthenticationErrorData(data)) {
throw new AuthenticationException(status, data, requestID);
} else {
throw new GenericServerException(
status,
Expand Down
Loading