-
Notifications
You must be signed in to change notification settings - Fork 854
fix(oauth): redact public authentication errors #1842
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.
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
Changes from all commits
8a86aae
446d365
02acbc6
5eb5563
c8ec047
e298b2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -292,12 +292,38 @@ export class UnsupportedOAuthProviderError extends Error { | |
| } | ||
|
|
||
| export class OAuthLoginRequiredError extends Error { | ||
| readonly provider: string; | ||
|
|
||
| constructor(provider: string) { | ||
| super(`Not logged in to ${provider}. Run: ocx login ${provider}`); | ||
| this.name = "OAuthLoginRequiredError"; | ||
| this.provider = provider; | ||
| } | ||
| } | ||
|
|
||
| export class OAuthProviderPublicationError extends Error { | ||
| constructor() { | ||
| super("OAuth credential was saved, but the provider entry was not written. Resolve the account namespace collision, then retry login."); | ||
| this.name = "OAuthProviderPublicationError"; | ||
| } | ||
| } | ||
|
|
||
| /** Project arbitrary OAuth failures onto the small, stable public error vocabulary. */ | ||
| export function publicOAuthAuthenticationErrorMessage(error: unknown): string { | ||
| if (error instanceof OAuthMutationBusyError) { | ||
| return error.message === "OAuth mutation queue wait timed out" | ||
| ? "OAuth mutation queue wait timed out" | ||
| : "OAuth mutation queue is busy"; | ||
| } | ||
| if ( | ||
| (error instanceof OAuthLoginRequiredError && isOAuthProvider(error.provider)) | ||
| || error instanceof OAuthProviderPublicationError | ||
| || error instanceof OAuthTokenRefreshBusyError | ||
| || error instanceof OAuthTokenRefreshStaleError | ||
| ) return error.message; | ||
| return "OAuth authentication failed. Check the OpenCodex account status and retry."; | ||
|
luvs01 marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When reauthenticating an existing account succeeds with a different identity—or when a legacy account lacks verifiable identity— AGENTS.md reference: src/AGENTS.md:L17-L17 Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| function accessSnapshot(provider: string, accountId: string, cred: OAuthCredentials): OAuthAccessSnapshot { | ||
| const storedKiroRouting = { | ||
| ...(cred.kiro?.profileArn ? { profileArn: cred.kiro.profileArn } : {}), | ||
|
|
@@ -1136,10 +1162,7 @@ export async function runLogin( | |
| provider, | ||
| ); | ||
| if (lateCollision) { | ||
| throw new Error( | ||
| `${lateCollision}. The credential for "${provider}" was saved, but the provider entry was not written. ` | ||
| + "Rename the account selector, then re-run the login.", | ||
| ); | ||
| throw new OAuthProviderPublicationError(); | ||
| } | ||
| upsertOAuthProvider(latestConfig, provider); | ||
| saveLatestConfig(latestConfig); | ||
|
|
@@ -1381,7 +1404,16 @@ export async function startLoginFlow( | |
| onManualCodeInput: (expectedState?: string) => waitForManualLoginCode(provider, abort.signal, expectedState), | ||
| signal: abort.signal, | ||
| }; | ||
| const abandonIfNotOwner = (error?: unknown): boolean => { | ||
| if (loginAbort.get(provider) === abort) return false; | ||
| if (!urlResolved) reject(error ?? new Error("OAuth login was superseded")); | ||
| return true; | ||
| }; | ||
| const settle = async (error?: unknown): Promise<void> => { | ||
| // Cancellation deletes this controller and records its own terminal result. A late provider | ||
| // rejection (or an older flow settling after a replacement starts) must not overwrite that | ||
| // state or delete the replacement flow's controller/manual-code slot. | ||
| if (abandonIfNotOwner(error)) return; | ||
| let finalError = error; | ||
| try { | ||
| await lifecycle?.onSettled?.(); | ||
|
|
@@ -1390,6 +1422,7 @@ export async function startLoginFlow( | |
| // runtime config. For an already-failed login, keep the original recovery error. | ||
| if (finalError === undefined) finalError = settleError; | ||
| } | ||
| if (abandonIfNotOwner(finalError)) return; | ||
| if (finalError === undefined) { | ||
| loginAbort.delete(provider); | ||
| clearManualCodeSlot(provider); | ||
|
|
@@ -1403,7 +1436,7 @@ export async function startLoginFlow( | |
| const e = finalError; | ||
| loginAbort.delete(provider); | ||
| clearManualCodeSlot(provider); | ||
| const msg = e instanceof Error ? e.message : String(e); | ||
| const msg = publicOAuthAuthenticationErrorMessage(e); | ||
|
luvs01 marked this conversation as resolved.
|
||
| loginState.set(provider, { done: true, error: msg }); | ||
| if (!urlResolved) reject(e); | ||
| }; | ||
|
|
@@ -1414,9 +1447,10 @@ export async function startLoginFlow( | |
| (e: unknown) => settle(e), | ||
| ).catch((e: unknown) => { | ||
| // settle catches lifecycle failures, so this is only a defensive promise-boundary guard. | ||
| if (abandonIfNotOwner(e)) return; | ||
| loginAbort.delete(provider); | ||
| clearManualCodeSlot(provider); | ||
| const msg = e instanceof Error ? e.message : String(e); | ||
| const msg = publicOAuthAuthenticationErrorMessage(e); | ||
| loginState.set(provider, { done: true, error: msg }); | ||
| if (!urlResolved) reject(e); | ||
| }); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.