fix: pass port and https flag to WorkOS SDK when base URL requires it#140
fix: pass port and https flag to WorkOS SDK when base URL requires it#140nicknisi merged 1 commit intoworkos:mainfrom
Conversation
📝 WalkthroughWalkthroughThe WorkOS SDK initialization in ChangesWorkOS SDK Configuration
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Greptile SummaryThis PR fixes SDK initialization in The fix is minimal and correct: Confidence Score: 5/5Safe to merge — minimal, targeted fix with no regressions on the standard HTTPS path. The change is a 4-line addition that correctly conditions port and https options on the parsed URL. Default ports (80/443) are not overridden because url.port returns an empty string for them. https: false is only applied when the user explicitly configured an http: URL, matching user intent. No existing behavior is altered for the production HTTPS path. No files require special attention. Important Files Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| const url = new URL(base); | ||
| const sdk = new WorkOS(key, { | ||
| apiHostname: url.hostname, | ||
| ...(url.port && { port: Number(url.port) }), | ||
| ...(url.protocol === 'http:' && { https: false }), | ||
| }); |
There was a problem hiding this comment.
Validate base format before using new URL to avoid opaque runtime failures.
At Line 63, new URL(base) assumes base is a full http(s) URL. Because endpoint values are user-provided and currently unvalidated upstream, malformed or shorthand inputs can throw/misparse and later fail as generic fetch failed. Add protocol/hostname validation with a clear error message at this boundary.
Suggested fix
- const url = new URL(base);
+ let url: URL;
+ try {
+ url = new URL(base);
+ } catch {
+ throw new Error(`Invalid API base URL "${base}". Expected a full URL like "https://api.workos.com" or "http://localhost:4100".`);
+ }
+
+ if (!['http:', 'https:'].includes(url.protocol) || !url.hostname) {
+ throw new Error(`Invalid API base URL "${base}". Only http/https URLs with a hostname are supported.`);
+ }
+
const sdk = new WorkOS(key, {
apiHostname: url.hostname,
...(url.port && { port: Number(url.port) }),
...(url.protocol === 'http:' && { https: false }),
});📝 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 url = new URL(base); | |
| const sdk = new WorkOS(key, { | |
| apiHostname: url.hostname, | |
| ...(url.port && { port: Number(url.port) }), | |
| ...(url.protocol === 'http:' && { https: false }), | |
| }); | |
| let url: URL; | |
| try { | |
| url = new URL(base); | |
| } catch { | |
| throw new Error(`Invalid API base URL "${base}". Expected a full URL like "https://api.workos.com" or "http://localhost:4100".`); | |
| } | |
| if (!['http:', 'https:'].includes(url.protocol) || !url.hostname) { | |
| throw new Error(`Invalid API base URL "${base}". Only http/https URLs with a hostname are supported.`); | |
| } | |
| const sdk = new WorkOS(key, { | |
| apiHostname: url.hostname, | |
| ...(url.port && { port: Number(url.port) }), | |
| ...(url.protocol === 'http:' && { https: false }), | |
| }); |
There was a problem hiding this comment.
it's a pre-existing issue of the code, it currently doesn't handle it either and just crashes with ERR_INVALID_URL
There was a problem hiding this comment.
@Langleu, that's a fair point — the previous new URL(base).hostname call had the exact same behaviour, so this PR doesn't regress anything there. I'll withdraw the concern for this PR.
Would you like me to open a follow-up GitHub issue to track adding proper URL validation (with a user-friendly error message) as a future improvement? It'd be a clean, standalone change that won't block this fix.
There was a problem hiding this comment.
so it's already letting a user know that's not what is expected.
I think the better fix would be to adjust whatever is expected by the CLI in the first place vs just Custom API endpoint.
There was a problem hiding this comment.
🧩 Analysis chain
Good point — improving the prompt/placeholder/description at the input level is a cleaner UX fix. Let me find where that prompt lives in the codebase.
🏁 Script executed:
#!/bin/bash
rg -rn "Custom API endpoint" --type ts -C5
rg -rn "Custom API endpoint" --type js -C5
rg -rn "baseUrl\|base_url\|apiEndpoint\|endpoint" --type ts -n -C3 | grep -i "prompt\|input\|question\|placeholder\|label" | head -40
Length of output: 1284
🏁 Script executed:
#!/bin/bash
# Find env command file
fd -e ts env src/commands/
# Look at the full env command
cat src/commands/env.ts 2>/dev/null || fd -e ts -e js "env" src/commands/ --exec cat {}
# Check bin.ts around endpoint option
rg -n "endpoint" src/bin.ts -C3
Length of output: 8944
@Langleu, agreed — improving the CLI-level description is the right place for this. Looking at the code, there are two spots that could be made more explicit:
-
src/bin.tsLine 369 — the--endpointflag description is just'Custom API endpoint', which doesn't hint at the expected format:// before .option('endpoint', { type: 'string', describe: 'Custom API endpoint' }) // after .option('endpoint', { type: 'string', describe: 'Custom API endpoint URL, e.g. http://localhost:4100' })
-
src/utils/help-json.ts— the endpoint description there also appears to be a stale/truncated artifact ('n URL'), which would be worth fixing alongside. -
Interactive
env addflow insrc/commands/env.ts— the interactive wizard currently doesn't prompt forendpointat all (only name, type, and API key). If the target audience for this is local emulator users who runworkos env addinteractively, it may be worth adding an optional prompt there too, with placeholder text showing the expected format:const endpointResult = await clack.text({ message: 'Custom API endpoint URL (optional, e.g. http://localhost:4100)', placeholder: 'Leave blank for default (https://api.workos.com)', }); if (!clack.isCancel(endpointResult) && endpointResult) { endpoint = endpointResult; }
Would you like me to open a follow-up issue to track the interactive-prompt gap and the help-json.ts description fix?
|
Generally interested what the plans are further for this cli / emulator topic? I'd like to utilize the emulator topic for local development in dev environments as well as for temporary preview environments. |
|
Thanks for this, I'll take a look today. Yes! The plan is to support this specifically to make it easier to test in offline/ephemeral environments. It's kind of stealth-shipped right now while we work out all the issues and ensure stability. |
|
Thanks, I appreciate it! Nice, makes sense. I only stumbled upon the emulator by accident after I read through the whole README, as Some sort of stickiness would be nice for the IDs, to have proper drop-in, but it might as well be on me to properly use |
2 participants
I've been playing around a bit with the WorkOS emulator that's included in the CLI and started to grow on it, but it wasn't possible to run the CLI easily against the emulator.
Emulator runs by default on port
4100.Using the cli with the env above would result for any command in
since it would try to talk to
localhost:443as the 443 is populated by the SDK, probably also had some issues with protocol.The description of
endpointdoes not suggest anything against my understanding of the value.Tested it locally:
Probably could take this even further and say that by default it runs against itself but that's maybe room for improvement from maintainer site.
Summary by CodeRabbit