Add additional parameter support#88
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThe PR refactors ChangesOAuth Sign-In API Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review — Add additional parameter support
Summary: Adds a new positional signInWithOAuth(provider, options) signature with provider-specific additionalParams, keeps the old object-form as a deprecated overload, and bumps @insforge/shared-schemas to pick up the OAuthInitRequest string-catchall that makes the passthrough params type-safe.
Requirements context
No matching spec/plan found under /docs/superpowers/ (the only documents there cover functions-in-process-dispatch, unrelated to OAuth). Assessed against the PR title/body and the existing code. PR body is empty — a one-line description of intent/backcompat would help future readers.
Verification performed in a clean checkout of the head commit: npx tsc --noEmit passes, and the new src/modules/auth/__tests__/auth.test.ts (3 tests) passes.
Findings
Critical
(none)
The most security-relevant aspect — whether caller-supplied additionalParams can override the PKCE code_challenge or redirect_uri — is handled correctly. In src/modules/auth/auth.ts:285-289 the spread is placed first so the explicit redirect_uri/code_challenge always win, and additionalParams values are appended via URLSearchParams (src/lib/http-client.ts:180-182), so they are URL-encoded and cannot inject extra parameters. This is directly covered by the "does not let additionalParams override OAuth init fields" test — good defensive test.
Suggestion
- Functionality —
redirectTois now required; behavior change for callers that omitted it.src/modules/auth/auth.ts:285-289always setsredirect_uri: signInOptions.redirectTo, whereas the previous code only set it when truthy (if (redirectTo) params.redirect_uri = redirectTo). The newOAuthSignInOptions.redirectTo(auth.ts:50) is a requiredstring, so TS callers are protected, but a plain-JS caller using the deprecated form and omittingredirectTowould now send the literalredirect_uri=undefined(buildUrl appends the raw value). Since the server schema requiresredirect_urithis is likely acceptable, but consider guarding (if (signInOptions.redirectTo)) to preserve the old behavior, or call out the breaking change explicitly. - Software engineering — no test for the missing-options error branch. The runtime branch at
auth.ts:267-275returns a structuredINVALID_INPUTerror when called with a bare provider and no options. The overload signature makes this unreachable for TS users, but it's reachable from JS and currently untested. A small test would lock in that contract.
Information
- Functionality —
nullhandling in overload dispatch.auth.ts:263usestypeof providerOrOptions === 'object', so an accidentalsignInWithOAuth(null)is treated as legacy options, thenconst { provider } = signInOptionsthrows and is caught as a generic500 UNEXPECTED_ERRORrather than the cleaner400 INVALID_INPUT. Edge case only; fine to leave. - Documentation — reserved params. The README/SDK-REFERENCE describe
additionalParamsas "provider-specific OAuth params" but don't note thatredirect_uriandcode_challengeare reserved and cannot be overridden. A one-line note would set expectations. - Dependency bump.
@insforge/shared-schemas^1.1.53 → ^1.1.55(first-party, caret range consistent with repo convention) is what introduces thez.string()catchall onoAuthInitRequestSchema, enabling the typed passthrough. Confirmed against the published 1.1.55 type definitions. Looks correct and intentional.
Performance
No performance-relevant changes — same single GET to the OAuth init endpoint; the only addition is a shallow object spread per call.
Verdict
approved (informational; no blocking issues). Clean, well-tested change with good backward compatibility via the deprecated overload. The suggestions above are non-blocking. The human approver still gives the explicit GitHub approval.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/auth/__tests__/auth.test.ts`:
- Around line 6-20: The createJsonResponse helper currently sets clone: () =>
response which returns an inner object lacking its own clone, causing failures
on repeated cloning; update createJsonResponse so the returned Response object
is self-referential (the clone() implementation returns the outer object itself
that includes clone/text/json/headers), i.e., ensure the object returned by
createJsonResponse (not the inner `response` variable) is what clone() returns;
verify the returned object exposes json(), text(), headers, status, ok, and
clone() consistently.
- Around line 22-66: The test fails because signInWithOAuth() calls the PKCE
helpers generateCodeVerifier() and generateCodeChallenge() (in
src/modules/auth/helpers.ts called from src/modules/auth/auth.ts) which rely on
Web Crypto globals (crypto.getRandomValues, crypto.subtle.digest), TextEncoder
and btoa; CI may not provide these so the helpers throw and set a non-null
error. Fix the test by stubbing or polyfilling the PKCE dependency before
constructing Auth: either (A) mock/spy on generateCodeVerifier and
generateCodeChallenge to return deterministic values, or (B) set global test
shims for crypto.getRandomValues, crypto.subtle.digest, TextEncoder and btoa so
the real helpers succeed; ensure the stubs are applied prior to new Auth(...) /
auth.signInWithOAuth() calls so the HTTP mock receives the generated PKCE
params.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53c889ee-288d-4b62-aad3-273cbc565032
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
README.mdSDK-REFERENCE.mdintegration-tests/auth.test.tspackage.jsonsrc/modules/auth/__tests__/auth.test.tssrc/modules/auth/auth.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Fermionic-Lyu
force-pushed
the
codex/oauth-additional-params
branch
from
3837e16 to
595788a
Compare
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="package.json">
<violation number="1" location="package.json:3">
P2: Version bumped as patch (1.3.1) but PR introduces new features — should be a minor bump (1.4.0) per semver and repository conventions</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| { | ||
| "name": "@insforge/sdk", | ||
| "version": "1.3.0", | ||
| "version": "1.3.1", |
There was a problem hiding this comment.
P2: Version bumped as patch (1.3.1) but PR introduces new features — should be a minor bump (1.4.0) per semver and repository conventions
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At package.json, line 3:
<comment>Version bumped as patch (1.3.1) but PR introduces new features — should be a minor bump (1.4.0) per semver and repository conventions</comment>
<file context>
@@ -1,6 +1,6 @@
{
"name": "@insforge/sdk",
- "version": "1.3.0",
+ "version": "1.3.1",
"description": "Official JavaScript/TypeScript client for InsForge Backend-as-a-Service platform",
"main": "./dist/index.js",
</file context>
| "version": "1.3.1", | |
| "version": "1.4.0", |
2 participants
Note
Add
additionalParamssupport and provider-first signature toAuth.signInWithOAuthsignInWithOAuthin auth.ts: new preferred signature takesprovideras the first argument and an options object{ redirectTo, additionalParams, skipBrowserRedirect }as the second; the old object-wrapper signature is retained as deprecated.additionalParamsto forward provider-specific OAuth hints in the init request; server-owned fields (redirect_uri,code_challenge) always override any colliding keys inadditionalParams.INVALID_INPUTerror when options orredirectToare missing.redirectTois now required for both signatures at runtime; callers omitting it will receive an error instead of proceeding silently.Changes since #88 opened
package.jsonfrom 1.3.0 to 1.3.1 [4587eb8]Macroscope summarized 595788a.
Summary by CodeRabbit
New Features
additionalParamsoption to support provider-specific OAuth configuration hints during sign-in requests, with built-in protection against overriding server-managed fields.Documentation
Chores
Summary by cubic
Adds a provider-first
auth.signInWithOAuth(provider, options)withadditionalParamsfor provider-specific OAuth hints. EnforcesredirectTo, tightens runtime validation, and bumps@insforge/sdkto1.3.1.New Features
additionalParamsinto the OAuth init request; blocks overrides toredirect_uriandcode_challenge.INVALID_INPUTerror when options are missing orredirectTois not provided.@insforge/shared-schemasfor CJS and updates to^1.1.55.Migration
signInWithOAuth({ provider, ... })withsignInWithOAuth("google", { redirectTo, ... })(redirectTois required).additionalParams; the legacy signature still works but is deprecated.Written for commit 4587eb8. Summary will update on new commits.