fix(clerk-js): Re-throw network errors in FraudProtection #7254
Conversation
🦋 Changeset detectedLatest commit: c1acc32 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughIntroduces an early-throw path in FraudProtection.execute to immediately surface runtime network errors (code === 'network_error') so cache fallback can run; keeps existing captcha handling for other errors and adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FraudProtection as FraudProtection.execute
participant API
participant Cache as Cache Fallback
Client->>FraudProtection: invoke with callback
FraudProtection->>API: perform API request
alt API returns network runtime error (code='network_error')
API-->>FraudProtection: error (network_error)
rect rgb(255, 244, 230)
Note over FraudProtection: NEW early-throw path — bypass captcha
FraudProtection->>Cache: throw to trigger cache fallback
end
Cache-->>Client: fallback response
else API returns other error
API-->>FraudProtection: error (non-network)
rect rgb(230, 245, 255)
Note over FraudProtection: Existing captcha flow
FraudProtection->>FraudProtection: inflight/captcha challenge & token submission
FraudProtection->>API: replay original callback with token
API-->>FraudProtection: response
FraudProtection-->>Client: return response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
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 |
|
The app crashed when starting offline with __experimental_resourceCache enabled due to three interconnected issues:
How It Manifested:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/clerk-js/src/core/clerk.ts (1)
2758-2770: Add a more specific error guard to prevent masking unexpected errors.The broadened fallback condition correctly handles network errors currently (which escape as generic Errors from
clerkNetworkError), but the!isClerkRuntimeError(err)check is overly permissive and could silently suppress unexpected exceptions in the future.Narrow the condition to explicitly check for network error characteristics:
const shouldFallback = this.shouldFallbackToCachedResources() && ((isClerkRuntimeError(err) && err.code === 'network_error') || (err instanceof Error && err.message?.includes('Network error')));This prevents masking programming errors, validation failures, or other unexpected exceptions while still catching the network errors that escape as generic Errors.
🧹 Nitpick comments (1)
packages/clerk-js/src/core/clerk.ts (1)
2743-2746: Consider simplifying the return statement.The intermediate variable
hasCacheFunctiondoesn't add clarity or enable reuse. You could simplify to:private shouldFallbackToCachedResources = (): boolean => { - const hasCacheFunction = !!this.__internal_getCachedResources; - return hasCacheFunction; + return !!this.__internal_getCachedResources; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/clerk-js/src/core/clerk.ts(2 hunks)packages/clerk-js/src/core/resources/Environment.ts(1 hunks)packages/clerk-js/src/core/resources/ProtectConfig.ts(1 hunks)packages/expo/src/cache/dummy-data/environment-resource.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/clerk-js/src/core/clerk.ts (1)
packages/shared/src/errors/helpers.ts (1)
isClerkRuntimeError(83-85)
packages/clerk-js/src/core/resources/Environment.ts (3)
packages/clerk-js/src/core/resources/CommerceSettings.ts (1)
CommerceSettings(8-55)packages/clerk-js/src/core/resources/APIKeySettings.ts (1)
APIKeySettings(8-35)packages/clerk-js/src/core/resources/ProtectConfig.ts (1)
ProtectConfig(10-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
packages/clerk-js/src/core/resources/ProtectConfig.ts (1)
36-36: LGTM! Guard ensures consistent snapshot structure.Defaulting
loadersto an empty array when undefined ensures the serialized snapshot always has a defined array, preventing potential null/undefined issues downstream.packages/clerk-js/src/core/resources/Environment.ts (1)
56-58: LGTM! Explicit null handling improves consistency.Using the nullish coalescing operator to pass
nullinstead ofundefinedto constructors ensures explicit handling of missing fields. The constructors (CommerceSettings, APIKeySettings, ProtectConfig) all properly handlenullparameters in theirfromJSONmethods.packages/expo/src/cache/dummy-data/environment-resource.ts (1)
234-260: LGTM! Dummy data expanded to match new environment shape.The additions align with the environment resource changes:
commerce_settingswith detailed billing configuration matches CommerceSettings structureprotect_config.loadersinitialized as empty array matches the guard added in ProtectConfigapi_keys_settingscorresponds to APIKeySettingsThis ensures test coverage for the expanded environment configuration.
chriscanin
changed the title
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
|
If |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/fix-offline-mode-resource-cache.md (1)
6-6: Clarify the changeset description wording.The phrase "broadening error handling to trigger cache fallback for non-network errors" is confusing because the code actually checks FOR network errors (including generic Error instances with network-related messages). Consider revising to something like:
"Fix offline mode crash when using
__experimental_resourceCacheby handling missingprotect_configfield from FAPI response and broadening network error detection to trigger cache fallback for network-like errors beyond just ClerkRuntimeError instances."-Fix offline mode crash when using `__experimental_resourceCache` by handling missing `protect_config` field from FAPI response and broadening error handling to trigger cache fallback for non-network errors. +Fix offline mode crash when using `__experimental_resourceCache` by handling missing `protect_config` field from FAPI response and broadening network error detection to trigger cache fallback for network-like errors beyond just ClerkRuntimeError instances.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/fix-offline-mode-resource-cache.md(1 hunks)packages/clerk-js/src/core/clerk.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/clerk-js/src/core/clerk.ts (1)
packages/shared/src/errors/helpers.ts (2)
isNetworkError(43-47)isClerkRuntimeError(83-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
packages/clerk-js/src/core/clerk.ts (1)
2757-2761: The review comment is incorrect.The inline network error check at lines 2757-2759 is not code duplication; it's more comprehensive than the existing
isNetworkErrorhelper. The helper only inspects message/name text patterns and cannot detectClerkRuntimeErrorobjects by theircodeproperty. The inline check handles two distinct error patterns:
isClerkRuntimeError(err) && err.code === 'network_error'— catches errors thrown fromBase.tswhich may have generic messages like "Failed to fetch" that don't contain "network" text. The helper cannot detect these via message inspection alone.err.message?.includes('Network error at')— catches errors fromclerkNetworkError()infapiClient.ts.The helper would only catch pattern 2 (after normalizing spaces: "networkerrorat..." includes "networkerror"), but not pattern 1. Therefore, consolidating to use only the helper would remove critical error detection logic.
Likely an incorrect or invalid review comment.
dstaley
changed the title
dstaley
changed the title
| this.commerceSettings = new CommerceSettings(data.commerce_settings); | ||
| this.apiKeysSettings = new APIKeySettings(data.api_keys_settings); | ||
| this.protectConfig = new ProtectConfig(data.protect_config); | ||
| this.protectConfig = new ProtectConfig(data.protect_config ?? null); |
There was a problem hiding this comment.
for this one the ProtectConfig constructor expects: ProtectConfigJSON | ProtectConfigJSONSnapshot | null, not
undefined, or we could add | null | undefined to the ProtectConfig constructor, but it will receive undefined if we are not connected
When Online (Normal Case):
- FAPI returns environment data
- protect_config field is missing from FAPI response (not in
the Go struct) (can we alter this? should we? it didn't seem like this was the right approach). - data.protect_config = undefined
- new ProtectConfig(undefined) is called
- Constructor accepts | undefined, so no type error
- fromJSON checks if (!data) which catches undefined
- Returns early with default empty ProtectConfig (id: '',
loaders: undefined) - When serialized to cache: loaders ?? [] ensures it becomes []
not undefined
When Offline (Cache Case):
- Load from cache (DUMMY data or previously cached data)
- Cached data has protect_config: { object: 'protect_config',
id: '', loaders: [] } - data.protect_config = { object: 'protect_config', id: '',
loaders: [] } - new ProtectConfig({ ... }) is called with actual data
- fromJSON processes it normally
- Everything works
Looking into this. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/shared/src/types/json.ts (1)
85-95: Clarify whetherprotect_configis always present or truly optional on the wireChanging
protect_configtoProtectConfigJSON | nullmodels responses that always include the field but sometimes set it tonull. The reported issue, however, mentions FAPI responses whereprotect_configis missing entirely, which this interface still treats as a contract violation.If FAPI will be updated to always send
protect_config(possiblynull), this type is correct and keeps the DTO shape strict. If omission remains valid, consider reflecting that here instead:- protect_config: ProtectConfigJSON | null; + protect_config?: ProtectConfigJSON | null;Either way, it would be good to explicitly confirm and document the intended contract for this field so the JSON types and backend stay in sync.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/clerk-js/src/core/resources/Environment.ts(0 hunks)packages/shared/src/types/json.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/clerk-js/src/core/resources/Environment.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/shared/src/types/json.ts (1)
packages/shared/src/types/protectConfig.ts (1)
ProtectConfigJSON(12-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
chriscanin
force-pushed
the
chris/mobile-322-the-customer-reports-that-offline-mode-stopped-working-after
branch
from
f8c1123 to
8a6baf9
Compare
|
When the app starts offline, Clerk attempts to fetch environment and client data from FAPI, but the network requests fail, throwing a ClerkRuntimeError with code: network_error. This error gets caught by FraudProtection.execute(), which wraps all FAPI requests to handle captcha challenges. The fraud protection code checks if it's a ClerkAPIResponseError (which network errors are classified as) and then tries to access e.errors[0]?.code to determine if it's a captcha challenge. We added a simple check in FraudProtection.execute() at: Mobile is now working with this change. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/clerk-js/src/core/fraudProtection.ts (1)
48-52: Excellent fix for preventing TypeError that masked network errors.This correctly addresses the offline crash by re-throwing network errors before accessing
e.errors[0], which would throw a TypeError whene.errorsis undefined. The early throw preserves the original network error, allowingclerk.tsto recognize it and trigger cache fallback.The logic is sound:
- Checks occur after confirming
eis a ClerkAPIResponseError- Type guard ensures safe access to
e.code- Network errors bypass captcha logic entirely, as intended
Optional defensive improvement:
Consider making line 54 more defensive as well:
- if (e.errors[0]?.code !== 'requires_captcha') { + if (e.errors?.[0]?.code !== 'requires_captcha') {This would provide belt-and-suspenders protection against any other error types without an
errorsarray, though with your current fix this scenario should not occur.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/clerk-js/src/core/fraudProtection.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/clerk-js/src/core/fraudProtection.ts (1)
packages/shared/src/errors/helpers.ts (1)
isClerkRuntimeError(83-85)
🔇 Additional comments (1)
packages/clerk-js/src/core/fraudProtection.ts (1)
1-1: LGTM! Import addition is correct.The
isClerkRuntimeErrorimport is properly sourced from@clerk/shared/errorand used as a type guard in the network error check below.
dstaley
changed the title
dstaley
changed the title
brkalow
deleted the
chris/mobile-322-the-customer-reports-that-offline-mode-stopped-working-after
branch
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.