fix(clerk-js): Handle removing origin in non-browser environments #6737
Conversation
🦋 Changeset detectedLatest commit: a78ffd7 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 |
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant U as stripOrigin(url)
participant E as Environment
C->>U: call with url string
U->>E: check if window.location exists
alt window.location defined
U->>U: parse URL and compare origin to window.location.origin
alt origins match
U-->>C: return path+query+hash (origin stripped)
else
U-->>C: return original absolute URL
end
else window.location undefined
U-->>C: return input unchanged
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (25)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@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: |
| // In non-browser environments `window.location.origin` might not be available | ||
| // if not polyfilled, so we can't construct a URL object with the `url` string | ||
| if (typeof window.location === 'undefined' && typeof url === 'string') { | ||
| return url; | ||
| } |
There was a problem hiding this comment.
❓ should we instead handle this in toURL() where the direct window.location.origin usage is?
There was a problem hiding this comment.
That was my initial thought, but I think stripOrigin is better suited for making sure origin is accessible, and as it accepts both URL and string, essentially the code looks like this:
- if you somehow managed to create a
URLobject, I'll pass it down totoURL - if you passed me a string, and I cannot safely detect
originto remove it, I'll just return the string
Happy to change this if you think moving it to toURL is easier to understand though.
There was a problem hiding this comment.
@bratsos I don't think this covers the case of passing a fully qualified URL as a string, e.g.
stripOrigin("https://clerk.com/blog") // -> /blog
In this case, I would still expect it to strip the origin. toURL() only uses window.location.origin as the fallback.
There was a problem hiding this comment.
In this case, if location is available (essentially in all browser environments) this will still work. It won't work if we detect that location is no available, so we're in an exotic runtime that isn't polyfilled.
The thing is that toURL uses URL directly, and even though it's polyfilled in react native (source), the implementation is non-standard - and for example .origin which is accessed by stripOrigin is not even implemented. So we cannot really assume that if URL exists, new URL('https://clerk.com/blog', undefined) would work as expected. I think it's too late if we reach to toURL when we detect that something is wrong in the runtime APIs.
An even better solution would probably be to try and detect if the required APIs are available, and work as expected, or detect the runtime and decide what to do based on that, but I think that's an overkill as in most cases you're either in a browser/polyfilled* environment (so window.location and URL exist and we can safely use them), or not, so in that case we cannot do much on a plain string, unless we want to implement a pathname parser in-house.
*: If you polyfilled the environment in a weird way this can also throw us off.
WDYT?
There was a problem hiding this comment.
Makes sense! Maybe we can adjust the JSDoc comment here to make it explicit that origin won't be stripped in certain non-browser environments.
| export function stripOrigin(url: URL | string): string { | ||
| // In non-browser environments `window.location.origin` might not be available | ||
| // if not polyfilled, so we can't construct a URL object with the `url` string | ||
| if (typeof window.location === 'undefined' && typeof url === 'string') { |
There was a problem hiding this comment.
Sorry if Im missing something obvious, but would this be safer?
| if (typeof window.location === 'undefined' && typeof url === 'string') { | |
| if ((typeof window === 'undefined' || !window.location) && typeof url === 'string') { |
There was a problem hiding this comment.
This is intentional as we're expecting that at least a window global object will be somehow declared. If it's not I prefer to get an error here, plus we'll have many other places that would break as well. WDYT?
Edit: Happy to make this more defensive though if we think it's valuable
There was a problem hiding this comment.
window is defined in react-native 😬
See my comment here https://github.com/clerk/javascript/pull/4095/files#r1742755342
![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
🧹 Nitpick comments (2)
packages/clerk-js/src/utils/url.ts (1)
160-162: Optional: maketoURLsafe in non-browser environments or document it as web-only.
toURLunconditionally readswindow.location.origin. Either guard like above or annotate as browser-only and keep it out of RN/SSR paths. This reduces future regressions similar to the one you’re fixing.If you prefer to harden it now:
export function toURL(url: string | URL): URL { - return new URL(url.toString(), window.location.origin); + const base = + typeof window !== 'undefined' && !!window.location && typeof window.location.origin === 'string' + ? window.location.origin + : DUMMY_URL_BASE; + return new URL(url.toString(), base); }packages/clerk-js/src/utils/__tests__/url.spec.ts (1)
646-673: Stubbingwindow.locationviadefinePropertyis brittle; prefer stubbing the global or add a case forURLinputs.Directly redefining
window.locationcan fail depending on the test environment/property descriptors. Usevi.stubGlobalor guard with try/catch. Also add a test forstripOrigin(new URL(...))withwindow/locationunavailable to lock the fix.Suggested test adjustments:
describe('stripOrigin(url)', () => { it('should strip origin when window.location is available', () => { - const originalLocation = window.location; - Object.defineProperty(window, 'location', { - value: { origin: 'https://example.com' }, - writable: true, - }); + const originalLocation = window.location; + // Prefer stubbing the global to avoid non-configurable descriptor issues + vi.stubGlobal('window', { ...window, location: { origin: 'https://example.com' } } as any); expect(stripOrigin('https://example.com/test?param=1')).toBe('/test?param=1'); expect(stripOrigin('/test')).toBe('/test'); - Object.defineProperty(window, 'location', { value: originalLocation }); + // Restore + vi.stubGlobal('window', { ...window, location: originalLocation } as any); }); it('should handle undefined window.location gracefully', () => { - const originalLocation = window.location; - Object.defineProperty(window, 'location', { - value: undefined, - writable: true, - }); + const originalLocation = window.location; + vi.stubGlobal('window', { ...window, location: undefined } as any); expect(() => stripOrigin('/test')).not.toThrow(); expect(stripOrigin('/test')).toBe('/test'); expect(stripOrigin('https://example.com/test')).toBe('https://example.com/test'); - Object.defineProperty(window, 'location', { value: originalLocation }); + vi.stubGlobal('window', { ...window, location: originalLocation } as any); }); + + it('should return href unchanged for URL inputs when window/location is unavailable', () => { + const originalWindow = globalThis.window; + vi.stubGlobal('window', undefined as any); + expect(stripOrigin(new URL('https://example.com/test'))).toBe('https://example.com/test'); + vi.stubGlobal('window', originalWindow as any); + }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- 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/utils/__tests__/url.spec.ts(2 hunks)packages/clerk-js/src/utils/url.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/utils/url.tspackages/clerk-js/src/utils/__tests__/url.spec.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/utils/url.tspackages/clerk-js/src/utils/__tests__/url.spec.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/utils/url.tspackages/clerk-js/src/utils/__tests__/url.spec.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/utils/url.tspackages/clerk-js/src/utils/__tests__/url.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/utils/url.tspackages/clerk-js/src/utils/__tests__/url.spec.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/utils/url.tspackages/clerk-js/src/utils/__tests__/url.spec.ts
packages/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Unit tests should use Jest or Vitest as the test runner.
Files:
packages/clerk-js/src/utils/__tests__/url.spec.ts
packages/{clerk-js,elements,themes}/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Visual regression testing should be performed for UI components.
Files:
packages/clerk-js/src/utils/__tests__/url.spec.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/__tests__/**/*.{ts,tsx}: Create type-safe test builders/factories
Use branded types for test isolation
Implement proper mock types that match interfaces
Files:
packages/clerk-js/src/utils/__tests__/url.spec.ts
🧬 Code graph analysis (1)
packages/clerk-js/src/utils/__tests__/url.spec.ts (1)
packages/clerk-js/src/utils/url.ts (1)
stripOrigin(176-185)
⏰ 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). (6)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/clerk-js/src/utils/__tests__/url.spec.ts (1)
23-23: Import looks good.Including
stripOriginin the test surface is appropriate.
| export function stripOrigin(url: URL | string): string { | ||
| // In non-browser environments `window.location.origin` might not be available | ||
| // if not polyfilled, so we can't construct a URL object with the `url` string | ||
| if (typeof window.location === 'undefined' && typeof url === 'string') { | ||
| return url; | ||
| } | ||
|
|
||
| url = toURL(url); | ||
| return url.href.replace(url.origin, ''); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix: guard must not access window.location when window is undefined; also avoid toURL() (it dereferences window.location.origin).
Current check typeof window.location === 'undefined' will throw in SSR/Node where window is not defined. Additionally, the subsequent toURL(url) uses window.location.origin and can still throw in non-browser contexts or when url is a URL instance. Harden the function and remove the dependency on toURL() here.
Apply:
export function stripOrigin(url: URL | string): string {
- // In non-browser environments `window.location.origin` might not be available
- // if not polyfilled, so we can't construct a URL object with the `url` string
- if (typeof window.location === 'undefined' && typeof url === 'string') {
- return url;
- }
-
- url = toURL(url);
- return url.href.replace(url.origin, '');
+ // In non-browser environments `window` or `window.location` might not exist.
+ const hasWindowOrigin =
+ typeof window !== 'undefined' && !!window.location && typeof window.location.origin === 'string';
+ if (!hasWindowOrigin) {
+ // Without a reliable current origin, return input unchanged.
+ return typeof url === 'string' ? url : url.href;
+ }
+ const u = typeof url === 'string' ? new URL(url, window.location.origin) : url;
+ return u.href.replace(u.origin, '');
}📝 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.
| export function stripOrigin(url: URL | string): string { | |
| // In non-browser environments `window.location.origin` might not be available | |
| // if not polyfilled, so we can't construct a URL object with the `url` string | |
| if (typeof window.location === 'undefined' && typeof url === 'string') { | |
| return url; | |
| } | |
| url = toURL(url); | |
| return url.href.replace(url.origin, ''); | |
| } | |
| export function stripOrigin(url: URL | string): string { | |
| // In non-browser environments `window` or `window.location` might not exist. | |
| const hasWindowOrigin = | |
| typeof window !== 'undefined' && !!window.location && typeof window.location.origin === 'string'; | |
| if (!hasWindowOrigin) { | |
| // Without a reliable current origin, return input unchanged. | |
| return typeof url === 'string' ? url : url.href; | |
| } | |
| const u = typeof url === 'string' ? new URL(url, window.location.origin) : url; | |
| return u.href.replace(u.origin, ''); | |
| } |
🤖 Prompt for AI Agents
In packages/clerk-js/src/utils/url.ts around lines 176-185, the current guard
uses typeof window.location which throws when window is undefined and then calls
toURL() which itself accesses window.location.origin; change the logic to first
check typeof window !== 'undefined' before touching window at all, avoid calling
toURL(), and handle both URL and string inputs explicitly: if the input is a URL
instance use it directly; if it's a string and window is undefined simply return
the string unchanged; otherwise (in a browser or when window exists) construct a
URL safely using window.location.origin as the base (or a fallback base like
'http://localhost' if needed) and return href with origin stripped; ensure no
access to window.location happens unless typeof window !== 'undefined'.
…-read-property-origin-of-undefined
bratsos
deleted the
alexbratsos/user-3415-clerkclerk-expo-cannot-read-property-origin-of-undefined
branch
5 participants
Description
Users reported a
[TypeError: Cannot read property 'origin' of undefined]warning/error when callingsignOut()from theuseAuthhook in Expo applications. While thesignOutfunctionality worked correctly, this error was logged to the console.This issue started when we introduced the debugLogger in PR #6452, which attempts to strip the origin from the redirect URL before reporting it in debug logs.
Steps to reproduce
expo-router- it imports@expo/metro-runtimewhich sets up polyfills forwindow.location.originso you won't be able to see the erroruseAuth().signOut()[TypeError: Cannot read property 'origin' of undefined]packages/clerk-jsrunturbo build && yalc publish(requiresyalcinstalled globally)yalc add @clerk/clerk-jsChecklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit