feat: add OAuth 2.0 PKCE authentication flow#70
Merged
Conversation
Implement full OAuth PKCE flow with OAuthPkce module (startAuth, exchangeCode), AuthContext provider, ProtectedRoute component, LoginPage, and CallbackPage. Mock server gets OAuth authorize/token endpoints. NavBar shows authenticated user name. Posts page is protected behind auth. Closes #39
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 27 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds OAuth 2.0 PKCE authentication: a mock OAuth server (authorize/token endpoints), ReScript FFI bindings for OAuth client and React integration, an AuthContext provider and user state, protected-route enforcement, callback handling, and UI updates (NavBar, Login, Router, App) to use authentication state. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Browser as Browser/App
participant AuthCtx as AuthContext (React)
participant PKCE as OAuthPkce (bindings)
participant Server as Mock OAuth Server
participant Storage as sessionStorage
User->>Browser: Click "Sign in with OAuth"
Browser->>AuthCtx: auth.login()
AuthCtx->>PKCE: authorize(config, ~provider="mock")
PKCE->>PKCE: generate codeVerifier & state, compute code_challenge
PKCE->>Storage: store verifier & state
PKCE-->>Browser: return authorize URL
Browser->>Server: GET /oauth/authorize?code_challenge=...&state=...
Server->>Server: create mock-code-*, store pendingCodes entry
Server-->>Browser: 302 Redirect to redirect_uri?code=...&state=...
Browser->>Browser: Load /callback with query
Browser->>AuthCtx: auth.handleCallback(window.location.search)
AuthCtx->>PKCE: handleCallback(search) -> callbackResult
PKCE->>Storage: read verifier & state
PKCE->>Server: POST /oauth/token (code, code_verifier...)
Server->>Server: validate code, delete pending
Server-->>PKCE: return token JSON
PKCE-->>AuthCtx: tokenSet / result
AuthCtx->>Browser: set LoggedIn(user)
Browser->>User: show authenticated UI
sequenceDiagram
participant User as User
participant Router as App Router
participant Protected as ProtectedRoute
participant AuthCtx as AuthContext
participant Posts as PostsPage
User->>Router: Navigate to /posts
Router->>Protected: Render ProtectedRoute(PostsPage)
Protected->>AuthCtx: useAuth()
alt LoggedOut
Protected->>Router: push("/login")
Protected-->>User: render nothing
else LoggedIn
Protected-->>User: render PostsPage
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src/context/AuthContext.res (2)
53-63:handleCallbacklacks error handling and silently ignores the token.If
OAuthPkce.exchangeCodefails, the error propagates to the caller but the user sees no feedback. Also, the token is ignored which means the access token isn't stored for API calls:♻️ Proposed improvements
let handleCallback = async (code: string) => { - let tokenSet = await OAuthPkce.exchangeCode(oauthConfig, code) - // For the template demo, create a user from the token - // In production, you'd decode the JWT or call a /userinfo endpoint - ignore(tokenSet) - setState(_ => LoggedIn({ - id: "user-1", - name: "Alice Chen", - email: "alice@example.com", - })) + try { + let tokenSet = await OAuthPkce.exchangeCode(oauthConfig, code) + // Store token for API calls (consider sessionStorage or secure cookie) + ignore(%raw(`sessionStorage.setItem("access_token", tokenSet.accessToken)`)) + // For the template demo, create a user from the token + // In production, you'd decode the JWT or call a /userinfo endpoint + setState(_ => LoggedIn({ + id: "user-1", + name: "Alice Chen", + email: "alice@example.com", + })) + } catch { + | _ => Js.Console.error("Failed to exchange authorization code") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/AuthContext.res` around lines 53 - 63, handleCallback currently awaits OAuthPkce.exchangeCode and ignores the result, and also has no error handling; wrap the exchangeCode call in a try/catch inside handleCallback, on success extract and persist the relevant tokens from tokenSet (e.g., access_token/refresh_token or id_token) into your auth state or secure storage so subsequent API calls can use them, and on failure set an error state or call the existing error reporting path so the UI can show feedback instead of letting the error propagate; update the setState call (where LoggedIn is created) to include the token fields from tokenSet rather than ignoring tokenSet.
18-24: Hardcoded OAuth configuration limits environment flexibility.The
oauthConfiguses hardcodedlocalhostURLs. Consider making these configurable via environment variables for deployment to different environments:♻️ Environment-based configuration example
+// In a separate config file or using Vite/Rspack env variables +let getEnvVar: string => option<string> = %raw(`function(name) { + return typeof import.meta !== 'undefined' && import.meta.env ? import.meta.env[name] : undefined +}`) + let oauthConfig: OAuthPkce.config = { - clientId: "rspack-rescript-template", - redirectUri: "http://localhost:8080/callback", - authorizeUrl: "http://localhost:4000/oauth/authorize", - tokenUrl: "http://localhost:4000/oauth/token", + clientId: getEnvVar("VITE_OAUTH_CLIENT_ID")->Option.getOr("rspack-rescript-template"), + redirectUri: getEnvVar("VITE_OAUTH_REDIRECT_URI")->Option.getOr("http://localhost:8080/callback"), + authorizeUrl: getEnvVar("VITE_OAUTH_AUTHORIZE_URL")->Option.getOr("http://localhost:4000/oauth/authorize"), + tokenUrl: getEnvVar("VITE_OAUTH_TOKEN_URL")->Option.getOr("http://localhost:4000/oauth/token"), scopes: ["openid", "profile", "email"], }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/AuthContext.res` around lines 18 - 24, The oauthConfig object is using hardcoded localhost values; change oauthConfig (type OAuthPkce.config) to read clientId, redirectUri, authorizeUrl, tokenUrl and optional scopes from environment variables with sensible localhost defaults so deployments can override them; update the initialization in AuthContext.res to pull from process.env (or your runtime env access) like ENV_CLIENT_ID/ENV_REDIRECT_URI/ENV_AUTHORIZE_URL/ENV_TOKEN_URL and fall back to the existing literal values, validate/normalize any trailing slashes if needed, and ensure the rest of the code that imports oauthConfig continues to work unchanged.src/components/NavBar/NavBar.res (1)
80-95: Consider reducing duplication in the login link rendering.Both branches render identical
<a>elements with only the text differing. This could be simplified:♻️ Simplified version
- {switch auth.state { - | AuthContext.LoggedIn(user) => - <a - className={linkStyles(~active=currentPath == "/login")} - href="/login" - onClick={evt => handleNav("/login", evt)}> - {React.string(user.name)} - </a> - | AuthContext.LoggedOut => - <a - className={linkStyles(~active=currentPath == "/login")} - href="/login" - onClick={evt => handleNav("/login", evt)}> - {React.string("Sign In")} - </a> - }} + <a + className={linkStyles(~active=currentPath == "/login")} + href="/login" + onClick={evt => handleNav("/login", evt)}> + {React.string(switch auth.state { + | AuthContext.LoggedIn(user) => user.name + | AuthContext.LoggedOut => "Sign In" + })} + </a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/NavBar/NavBar.res` around lines 80 - 95, Reduce duplication by computing the link text once and rendering a single <a> element instead of duplicating it in both AuthContext.LoggedIn and AuthContext.LoggedOut branches: inside the switch on auth.state (AuthContext.LoggedIn(user) / AuthContext.LoggedOut) derive a linkText (use user.name for LoggedIn, "Sign In" for LoggedOut) and then render one anchor using linkStyles(~active=currentPath == "/login"), href="/login" and onClick={evt => handleNav("/login", evt)} with React.string(linkText); update the switch to return the text value (or a small variant) and keep the final <a> render outside the branches so only the text differs.mock-server/index.js (1)
115-146: Mock server storescode_challengebut never verifiescode_verifier.The PKCE flow requires the token endpoint to verify that
SHA256(code_verifier) == code_challenge. Currently, the mock accepts any code without validating the verifier, which means the PKCE security isn't actually being tested.For a more realistic mock that helps catch client-side PKCE bugs:
♻️ Proposed PKCE verification
+const crypto = await import("node:crypto"); + +function verifyCodeChallenge(codeVerifier, codeChallenge) { + const hash = crypto.createHash("sha256").update(codeVerifier).digest(); + const computed = hash.toString("base64url"); + return computed === codeChallenge; +} + // Mock OAuth token endpoint if (url.pathname === "/oauth/token" && req.method === "POST") { let body = ""; req.on("data", (chunk) => { body += chunk; }); req.on("end", () => { const params = new URLSearchParams(body); const code = params.get("code"); + const codeVerifier = params.get("code_verifier"); + const pending = pendingCodes.get(code); - if (code && pendingCodes.has(code)) { + if (code && pending && verifyCodeChallenge(codeVerifier, pending.codeChallenge)) { pendingCodes.delete(code);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mock-server/index.js` around lines 115 - 146, The token endpoint accepts any code without verifying the PKCE code_verifier; change pendingCodes from a Set to a Map that stores the code -> code_challenge when issuing the auth code, then in the "/oauth/token" POST handler parse code_verifier from the request body, compute its SHA256 and base64url-encode it (use crypto.createHash('sha256').update(code_verifier).digest('base64') then convert to base64url by replacing '+'->'-', '/'->'_', and trimming '='), and compare that value against the stored code_challenge for the provided code (only delete the entry and return a token if they match); otherwise return the 400 invalid_grant response. Ensure you reference and update the logic around pendingCodes, code_challenge storage (where codes are issued), and the token endpoint handler that currently reads code from URLSearchParams.src/bindings/OAuthPkce.res (1)
82-96: Mock PKCE validation incomplete.The mock
/oauth/tokenendpoint storescode_challengeand validates that codes exist, but does not verify that the receivedcode_verifiermatches the storedcode_challengethrough proper PKCE hashing (SHA256). This gap means the local mock doesn't achieve parity with real OAuth server behavior, potentially hiding PKCE implementation regressions during local development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bindings/OAuthPkce.res` around lines 82 - 96, The mock /oauth/token endpoint currently checks only that a code exists but doesn’t validate PKCE; update the token exchange handler to compute the SHA256 hash of the received code_verifier, base64-url-encode that digest, and compare it (using a timing-safe comparison) to the stored code_challenge before issuing tokens. Locate the mock /oauth/token handler that reads code_verifier and code_challenge (the same place the test stores code_challenge when issuing the auth code), perform the SHA256 + base64url transform on the incoming code_verifier and reject the request if it does not match the stored code_challenge; keep the client-side request building (body sent to config.tokenUrl / fetchRaw) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mock-server/index.js`:
- Around line 97-112: The redirect handler builds a callback URL using
redirect_uri without validating it, which can produce an invalid redirect;
update the /oauth/authorize branch to validate that redirect_uri is present and
a non-empty string (and optionally URL-decodable) before using it (the code that
reads redirectUri from url.searchParams and constructs callbackUrl); if
validation fails, respond with a 400 error (res.writeHead(400) / res.end with a
clear message) instead of performing the 302 redirect, and when valid use
encodeURIComponent or URL constructor to safely build the callback URL and
continue storing the pendingCodes entry.
In `@src/bindings/OAuthPkce.res`:
- Around line 74-76: The code silently uses an empty string for codeVerifier and
proceeds to parse token fields without checking the HTTP response; update the
OAuthPkce logic to (1) validate that codeVerifier (from sessionStorage getItem
"pkce_code_verifier") is present and non-empty and return/throw a clear error if
missing before building the token request, and (2) check the token HTTP response
status before parsing JSON in the token handling block (the code that
builds/returns tokenSet), and if the response is not successful, parse the error
body (or include status/text) and return/throw a descriptive error rather than
constructing tokenSet from an error response.
- Around line 50-53: The OAuth PKCE flow stores a nonce using
generateRandomString(32) and sessionStorage.setItem("pkce_state", state) but
never validates it in the callback/exchange logic; update the callback handler
code (the logic that reads the authorization code and performs the token
exchange) to read sessionStorage.getItem("pkce_state"), compare it strictly to
the incoming callback "state" parameter, and abort the exchange (return
error/log and stop) if they differ; after a successful match, remove the stored
pkce_state (and pkce_code_verifier) from storage to prevent reuse.
- Around line 20-31: The code uses %raw for browser interop in bindings; replace
those with typed `@val` externals and call them from the module: define `@val`
externals for the crypto/Uint8Array random generator used by
generateRandomString, for SubtleCrypto.digest and TextEncoder used by
generateCodeChallenge, and for sessionStorage operations (getItem, setItem,
removeItem) used throughout (including the sessionStorage cleanup calls). Follow
the existing pattern used by the encodeURIComponent `@val` binding—declare typed
externals with correct JS types and then rewrite generateRandomString,
generateCodeChallenge, and the sessionStorage read/write/remove sites to call
those externals instead of using %raw.
- Line 47: The functions using positional parameters (notably startAuth) must be
converted to ReScript named-parameter signatures using the ~ prefix (e.g.,
change startAuth = async (config: config) => to use ~config), and update any
other functions mentioned on lines 47 and 73 to the same style; modify all call
sites to pass arguments with the ~name: value syntax and adjust any type
annotations to match the new named parameters so the repository’s naming
convention is preserved.
In `@src/components/ProtectedRoute/ProtectedRoute.res`:
- Around line 6-10: The switch in ProtectedRoute.res performing
RescriptReactRouter.push("/login") directly during render (when auth.state
matches AuthContext.LoggedOut) causes a render-phase side effect; move the
redirect into a React hook by detecting AuthContext.LoggedOut inside the
component and calling RescriptReactRouter.push("/login") from a useEffect (or
equivalent ReScript hook) so the redirect runs after render, leaving the render
branch to return React.null without causing side effects.
In `@src/context/AuthContext.res`:
- Around line 44-47: The login function calls OAuthPkce.startAuth without
handling rejections; wrap the await in a try/catch inside the login function to
catch errors from OAuthPkce.startAuth, log or report the error (e.g., via
console/processLogger) and surface user-friendly feedback (e.g., set an auth
error state or call an existing showError method) instead of letting the promise
remain unhandled, and only call setWindowLocation(url) when the call succeeds;
reference the login function, OAuthPkce.startAuth, and setWindowLocation when
applying the change.
In `@src/pages/CallbackPage.res`:
- Around line 9-25: The callback handler in CallbackPage's useEffect currently
reads the "code" but does not validate the OAuth "state" for CSRF protection;
update the effect to also read the "state" query param, retrieve the stored
state from sessionStorage (the value set by startAuth in OAuthPkce.res), compare
them and only call auth.handleCallback(c) when they match; if the state is
missing or mismatched, log an error/handle gracefully (e.g., push("/login") and
avoid calling handleCallback), and ensure you remove the stored state from
sessionStorage after verification to prevent replay attacks; keep existing
Promise handling around auth.handleCallback and redirects.
---
Nitpick comments:
In `@mock-server/index.js`:
- Around line 115-146: The token endpoint accepts any code without verifying the
PKCE code_verifier; change pendingCodes from a Set to a Map that stores the code
-> code_challenge when issuing the auth code, then in the "/oauth/token" POST
handler parse code_verifier from the request body, compute its SHA256 and
base64url-encode it (use
crypto.createHash('sha256').update(code_verifier).digest('base64') then convert
to base64url by replacing '+'->'-', '/'->'_', and trimming '='), and compare
that value against the stored code_challenge for the provided code (only delete
the entry and return a token if they match); otherwise return the 400
invalid_grant response. Ensure you reference and update the logic around
pendingCodes, code_challenge storage (where codes are issued), and the token
endpoint handler that currently reads code from URLSearchParams.
In `@src/bindings/OAuthPkce.res`:
- Around line 82-96: The mock /oauth/token endpoint currently checks only that a
code exists but doesn’t validate PKCE; update the token exchange handler to
compute the SHA256 hash of the received code_verifier, base64-url-encode that
digest, and compare it (using a timing-safe comparison) to the stored
code_challenge before issuing tokens. Locate the mock /oauth/token handler that
reads code_verifier and code_challenge (the same place the test stores
code_challenge when issuing the auth code), perform the SHA256 + base64url
transform on the incoming code_verifier and reject the request if it does not
match the stored code_challenge; keep the client-side request building (body
sent to config.tokenUrl / fetchRaw) unchanged.
In `@src/components/NavBar/NavBar.res`:
- Around line 80-95: Reduce duplication by computing the link text once and
rendering a single <a> element instead of duplicating it in both
AuthContext.LoggedIn and AuthContext.LoggedOut branches: inside the switch on
auth.state (AuthContext.LoggedIn(user) / AuthContext.LoggedOut) derive a
linkText (use user.name for LoggedIn, "Sign In" for LoggedOut) and then render
one anchor using linkStyles(~active=currentPath == "/login"), href="/login" and
onClick={evt => handleNav("/login", evt)} with React.string(linkText); update
the switch to return the text value (or a small variant) and keep the final <a>
render outside the branches so only the text differs.
In `@src/context/AuthContext.res`:
- Around line 53-63: handleCallback currently awaits OAuthPkce.exchangeCode and
ignores the result, and also has no error handling; wrap the exchangeCode call
in a try/catch inside handleCallback, on success extract and persist the
relevant tokens from tokenSet (e.g., access_token/refresh_token or id_token)
into your auth state or secure storage so subsequent API calls can use them, and
on failure set an error state or call the existing error reporting path so the
UI can show feedback instead of letting the error propagate; update the setState
call (where LoggedIn is created) to include the token fields from tokenSet
rather than ignoring tokenSet.
- Around line 18-24: The oauthConfig object is using hardcoded localhost values;
change oauthConfig (type OAuthPkce.config) to read clientId, redirectUri,
authorizeUrl, tokenUrl and optional scopes from environment variables with
sensible localhost defaults so deployments can override them; update the
initialization in AuthContext.res to pull from process.env (or your runtime env
access) like ENV_CLIENT_ID/ENV_REDIRECT_URI/ENV_AUTHORIZE_URL/ENV_TOKEN_URL and
fall back to the existing literal values, validate/normalize any trailing
slashes if needed, and ensure the rest of the code that imports oauthConfig
continues to work unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ed55c07-48fd-4289-a306-8d90313a213f
📒 Files selected for processing (9)
mock-server/index.jssrc/App.ressrc/Router.ressrc/bindings/OAuthPkce.ressrc/components/NavBar/NavBar.ressrc/components/ProtectedRoute/ProtectedRoute.ressrc/context/AuthContext.ressrc/pages/CallbackPage.ressrc/pages/LoginPage.res
Comment on lines
+97
to
+112
| if (url.pathname === "/oauth/authorize") { | ||
| const redirectUri = url.searchParams.get("redirect_uri"); | ||
| const state = url.searchParams.get("state"); | ||
| const code = `mock-code-${Date.now()}`; | ||
|
|
||
| // Store the code verifier expectation | ||
| pendingCodes.set(code, { | ||
| codeChallenge: url.searchParams.get("code_challenge"), | ||
| clientId: url.searchParams.get("client_id"), | ||
| }); | ||
|
|
||
| // Redirect back with code and state (simulates user approving) | ||
| const callbackUrl = `${redirectUri}?code=${code}&state=${state}`; | ||
| res.writeHead(302, { Location: callbackUrl }); | ||
| res.end(); | ||
| return; |
There was a problem hiding this comment.
Missing null check for redirect_uri could cause malformed redirect.
If redirect_uri is missing from the request, the callback URL will contain null, resulting in an invalid redirect. Consider adding validation.
🛡️ Proposed fix
// Mock OAuth authorize endpoint
if (url.pathname === "/oauth/authorize") {
const redirectUri = url.searchParams.get("redirect_uri");
const state = url.searchParams.get("state");
+
+ if (!redirectUri) {
+ res.writeHead(400, { "Content-Type": "application/json" });
+ res.end(JSON.stringify({ error: "invalid_request", error_description: "redirect_uri is required" }));
+ return;
+ }
+
const code = `mock-code-${Date.now()}`;📝 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.
Suggested change
| if (url.pathname === "/oauth/authorize") { | |
| const redirectUri = url.searchParams.get("redirect_uri"); | |
| const state = url.searchParams.get("state"); | |
| const code = `mock-code-${Date.now()}`; | |
| // Store the code verifier expectation | |
| pendingCodes.set(code, { | |
| codeChallenge: url.searchParams.get("code_challenge"), | |
| clientId: url.searchParams.get("client_id"), | |
| }); | |
| // Redirect back with code and state (simulates user approving) | |
| const callbackUrl = `${redirectUri}?code=${code}&state=${state}`; | |
| res.writeHead(302, { Location: callbackUrl }); | |
| res.end(); | |
| return; | |
| if (url.pathname === "/oauth/authorize") { | |
| const redirectUri = url.searchParams.get("redirect_uri"); | |
| const state = url.searchParams.get("state"); | |
| if (!redirectUri) { | |
| res.writeHead(400, { "Content-Type": "application/json" }); | |
| res.end(JSON.stringify({ error: "invalid_request", error_description: "redirect_uri is required" })); | |
| return; | |
| } | |
| const code = `mock-code-${Date.now()}`; | |
| // Store the code verifier expectation | |
| pendingCodes.set(code, { | |
| codeChallenge: url.searchParams.get("code_challenge"), | |
| clientId: url.searchParams.get("client_id"), | |
| }); | |
| // Redirect back with code and state (simulates user approving) | |
| const callbackUrl = `${redirectUri}?code=${code}&state=${state}`; | |
| res.writeHead(302, { Location: callbackUrl }); | |
| res.end(); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mock-server/index.js` around lines 97 - 112, The redirect handler builds a
callback URL using redirect_uri without validating it, which can produce an
invalid redirect; update the /oauth/authorize branch to validate that
redirect_uri is present and a non-empty string (and optionally URL-decodable)
before using it (the code that reads redirectUri from url.searchParams and
constructs callbackUrl); if validation fails, respond with a 400 error
(res.writeHead(400) / res.end with a clear message) instead of performing the
302 redirect, and when valid use encodeURIComponent or URL constructor to safely
build the callback URL and continue storing the pendingCodes entry.
| } | ||
| `) | ||
|
|
||
| let startAuth = async (config: config) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for positional async function params in this bindings file
rg -nP 'let\s+\w+\s*=\s*async\s*\(' src/bindings/OAuthPkce.res -C1Repository: acald-creator/rspack-rescript
Length of output: 334
Use named parameters with ~ prefix in ReScript function signatures.
Lines 47 and 73 use positional arguments but should use named parameters per repository conventions.
🔧 Proposed fix
-let startAuth = async (config: config) => {
+let startAuth = async (~config: config) => {
@@
-let exchangeCode = async (config: config, code: string) => {
+let exchangeCode = async (~config: config, ~code: string) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bindings/OAuthPkce.res` at line 47, The functions using positional
parameters (notably startAuth) must be converted to ReScript named-parameter
signatures using the ~ prefix (e.g., change startAuth = async (config: config)
=> to use ~config), and update any other functions mentioned on lines 47 and 73
to the same style; modify all call sites to pass arguments with the ~name: value
syntax and adjust any type annotations to match the new named parameters so the
repository’s naming convention is preserved.
Comment on lines
+50
to
+53
| let state = generateRandomString(32) | ||
|
|
||
| ignore(%raw(`sessionStorage.setItem("pkce_code_verifier", codeVerifier)`)) | ||
| ignore(%raw(`sessionStorage.setItem("pkce_state", state)`)) |
There was a problem hiding this comment.
Validate OAuth state before exchanging the authorization code.
Line 50 stores pkce_state, but Lines 73-83 never compare callback state against it. This removes CSRF protection from the OAuth callback flow.
🔧 Proposed fix
-let exchangeCode = async (config: config, code: string) => {
+let exchangeCode = async (~config: config, ~code: string, ~state: string) => {
+ let storedState: string = %raw(`sessionStorage.getItem("pkce_state") || ""`)
+ if storedState == "" || storedState != state {
+ Js.Exn.raiseError("Invalid OAuth state")
+ }
let codeVerifier: string = %raw(`sessionStorage.getItem("pkce_code_verifier") || ""`)Also applies to: 73-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bindings/OAuthPkce.res` around lines 50 - 53, The OAuth PKCE flow stores
a nonce using generateRandomString(32) and sessionStorage.setItem("pkce_state",
state) but never validates it in the callback/exchange logic; update the
callback handler code (the logic that reads the authorization code and performs
the token exchange) to read sessionStorage.getItem("pkce_state"), compare it
strictly to the incoming callback "state" parameter, and abort the exchange
(return error/log and stop) if they differ; after a successful match, remove the
stored pkce_state (and pkce_code_verifier) from storage to prevent reuse.
Comment on lines
+74
to
+76
| let codeVerifier: string = %raw(`sessionStorage.getItem("pkce_code_verifier") || ""`) | ||
|
|
||
| let body = |
There was a problem hiding this comment.
Guard missing verifier and non-success token responses.
Line 74 silently falls back to "" for code_verifier, and Lines 87-106 parse/return token fields without checking response status. This can produce invalid tokenSet values on error responses.
Also applies to: 87-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bindings/OAuthPkce.res` around lines 74 - 76, The code silently uses an
empty string for codeVerifier and proceeds to parse token fields without
checking the HTTP response; update the OAuthPkce logic to (1) validate that
codeVerifier (from sessionStorage getItem "pkce_code_verifier") is present and
non-empty and return/throw a clear error if missing before building the token
request, and (2) check the token HTTP response status before parsing JSON in the
token handling block (the code that builds/returns tokenSet), and if the
response is not successful, parse the error body (or include status/text) and
return/throw a descriptive error rather than constructing tokenSet from an error
response.
Comment on lines
+6
to
+10
| switch auth.state { | ||
| | AuthContext.LoggedOut => { | ||
| RescriptReactRouter.push("/login") | ||
| React.null | ||
| } |
There was a problem hiding this comment.
Side effect during render phase may cause unexpected behavior.
Calling RescriptReactRouter.push("/login") directly in the render body is a side effect that React discourages. This can lead to multiple redirect attempts or warnings in strict mode. Consider moving the redirect to a useEffect:
♻️ Proposed fix using useEffect
module ProtectedRoute = {
`@react.component`
let make = (~children) => {
let auth = AuthContext.useAuth()
+ React.useEffect1(() => {
+ switch auth.state {
+ | AuthContext.LoggedOut => RescriptReactRouter.push("/login")
+ | AuthContext.LoggedIn(_) => ()
+ }
+ None
+ }, [auth.state])
+
switch auth.state {
- | AuthContext.LoggedOut => {
- RescriptReactRouter.push("/login")
- React.null
- }
+ | AuthContext.LoggedOut => React.null
| AuthContext.LoggedIn(_) => children
}
}
}📝 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.
Suggested change
| switch auth.state { | |
| | AuthContext.LoggedOut => { | |
| RescriptReactRouter.push("/login") | |
| React.null | |
| } | |
| module ProtectedRoute = { | |
| `@react.component` | |
| let make = (~children) => { | |
| let auth = AuthContext.useAuth() | |
| React.useEffect1(() => { | |
| switch auth.state { | |
| | AuthContext.LoggedOut => RescriptReactRouter.push("/login") | |
| | AuthContext.LoggedIn(_) => () | |
| } | |
| None | |
| }, [auth.state]) | |
| switch auth.state { | |
| | AuthContext.LoggedOut => React.null | |
| | AuthContext.LoggedIn(_) => children | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ProtectedRoute/ProtectedRoute.res` around lines 6 - 10, The
switch in ProtectedRoute.res performing RescriptReactRouter.push("/login")
directly during render (when auth.state matches AuthContext.LoggedOut) causes a
render-phase side effect; move the redirect into a React hook by detecting
AuthContext.LoggedOut inside the component and calling
RescriptReactRouter.push("/login") from a useEffect (or equivalent ReScript
hook) so the redirect runs after render, leaving the render branch to return
React.null without causing side effects.
Comment on lines
+44
to
+47
| let login = async () => { | ||
| let url = await OAuthPkce.startAuth(oauthConfig) | ||
| setWindowLocation(url) | ||
| } |
There was a problem hiding this comment.
Missing error handling for OAuth flow failures.
If OAuthPkce.startAuth throws (e.g., network error, crypto API unavailable), the promise rejection is unhandled. Consider adding error handling to provide user feedback:
🛡️ Proposed fix with error handling
let login = async () => {
- let url = await OAuthPkce.startAuth(oauthConfig)
- setWindowLocation(url)
+ try {
+ let url = await OAuthPkce.startAuth(oauthConfig)
+ setWindowLocation(url)
+ } catch {
+ | _ => Js.Console.error("Failed to start OAuth flow")
+ }
}📝 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.
Suggested change
| let login = async () => { | |
| let url = await OAuthPkce.startAuth(oauthConfig) | |
| setWindowLocation(url) | |
| } | |
| let login = async () => { | |
| try { | |
| let url = await OAuthPkce.startAuth(oauthConfig) | |
| setWindowLocation(url) | |
| } catch { | |
| | _ => Js.Console.error("Failed to start OAuth flow") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context/AuthContext.res` around lines 44 - 47, The login function calls
OAuthPkce.startAuth without handling rejections; wrap the await in a try/catch
inside the login function to catch errors from OAuthPkce.startAuth, log or
report the error (e.g., via console/processLogger) and surface user-friendly
feedback (e.g., set an auth error state or call an existing showError method)
instead of letting the promise remain unhandled, and only call
setWindowLocation(url) when the call succeeds; reference the login function,
OAuthPkce.startAuth, and setWindowLocation when applying the change.
Comment on lines
+9
to
+25
| React.useEffect0(() => { | ||
| let url = %raw(`new URL(window.location.href)`) | ||
| let code: option<string> = url["searchParams"]["get"]("code")->Nullable.toOption | ||
|
|
||
| switch code { | ||
| | Some(c) => | ||
| auth.handleCallback(c) | ||
| ->Promise.then(_ => { | ||
| RescriptReactRouter.push("/login") | ||
| Promise.resolve() | ||
| }) | ||
| ->ignore | ||
| | None => RescriptReactRouter.push("/login") | ||
| } | ||
|
|
||
| None | ||
| }) |
There was a problem hiding this comment.
Missing OAuth state parameter validation (CSRF protection).
The PKCE flow stores a state value in sessionStorage during startAuth (see src/bindings/OAuthPkce.res:53). The callback should verify the returned state matches to prevent CSRF attacks:
🔒 Proposed fix with state validation and error handling
React.useEffect0(() => {
let url = %raw(`new URL(window.location.href)`)
let code: option<string> = url["searchParams"]["get"]("code")->Nullable.toOption
+ let state: option<string> = url["searchParams"]["get"]("state")->Nullable.toOption
+ let storedState: option<string> = %raw(`sessionStorage.getItem("pkce_state")`)->Nullable.toOption
- switch code {
- | Some(c) =>
+ switch (code, state, storedState) {
+ | (Some(c), Some(s), Some(stored)) if s == stored =>
auth.handleCallback(c)
->Promise.then(_ => {
RescriptReactRouter.push("/login")
Promise.resolve()
})
+ ->Promise.catch(_ => {
+ RescriptReactRouter.push("/login")
+ Promise.resolve()
+ })
->ignore
- | None => RescriptReactRouter.push("/login")
+ | _ => RescriptReactRouter.push("/login")
}
None
})📝 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.
Suggested change
| React.useEffect0(() => { | |
| let url = %raw(`new URL(window.location.href)`) | |
| let code: option<string> = url["searchParams"]["get"]("code")->Nullable.toOption | |
| switch code { | |
| | Some(c) => | |
| auth.handleCallback(c) | |
| ->Promise.then(_ => { | |
| RescriptReactRouter.push("/login") | |
| Promise.resolve() | |
| }) | |
| ->ignore | |
| | None => RescriptReactRouter.push("/login") | |
| } | |
| None | |
| }) | |
| React.useEffect0(() => { | |
| let url = %raw(`new URL(window.location.href)`) | |
| let code: option<string> = url["searchParams"]["get"]("code")->Nullable.toOption | |
| let state: option<string> = url["searchParams"]["get"]("state")->Nullable.toOption | |
| let storedState: option<string> = %raw(`sessionStorage.getItem("pkce_state")`)->Nullable.toOption | |
| switch (code, state, storedState) { | |
| | (Some(c), Some(s), Some(stored)) if s == stored => | |
| auth.handleCallback(c) | |
| ->Promise.then(_ => { | |
| RescriptReactRouter.push("/login") | |
| Promise.resolve() | |
| }) | |
| ->Promise.catch(_ => { | |
| RescriptReactRouter.push("/login") | |
| Promise.resolve() | |
| }) | |
| ->ignore | |
| | _ => RescriptReactRouter.push("/login") | |
| } | |
| None | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/CallbackPage.res` around lines 9 - 25, The callback handler in
CallbackPage's useEffect currently reads the "code" but does not validate the
OAuth "state" for CSRF protection; update the effect to also read the "state"
query param, retrieve the stored state from sessionStorage (the value set by
startAuth in OAuthPkce.res), compare them and only call auth.handleCallback(c)
when they match; if the state is missing or mismatched, log an error/handle
gracefully (e.g., push("/login") and avoid calling handleCallback), and ensure
you remove the stored state from sessionStorage after verification to prevent
replay attacks; keep existing Promise handling around auth.handleCallback and
redirects.
Replace custom PKCE implementation with @cosmonexus/oauth-client, oauth-react, oauth-tokens, and oauth-core from local Verdaccio registry. Add ReScript bindings for OAuthClient and AuthProvider. AuthContext wraps oauth-react's AuthProvider and useAuth hook. Add .npmrc for @CosmoNexus scoped registry. Fix Rspack ESM module resolution for extensionless imports.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/context/AuthContext.res (2)
3-8: Hardcoded development URLs limit environment portability.The OAuth configuration uses hardcoded
localhostURLs which will break in staging/production environments. Consider making these configurable via environment variables.♻️ Proposed approach
+// Environment-based configuration +@val external issuerBaseUrl: string = "process.env.OAUTH_ISSUER_URL" +@val external clientId: string = "process.env.OAUTH_CLIENT_ID" +@val external redirectUri: string = "process.env.OAUTH_REDIRECT_URI" + let oauthConfig: OAuthPkce.oauthClientConfig = { - issuerBaseUrl: "http://localhost:4000/oauth", - clientId: "rspack-rescript-template", - redirectUri: "http://localhost:8080/callback", + issuerBaseUrl: issuerBaseUrl, + clientId: clientId, + redirectUri: redirectUri, scopes: ["openid", "profile", "email"], }You would also need to add these to
rspack.config.jsDefinePlugin with appropriate defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/AuthContext.res` around lines 3 - 8, The oauthConfig object (typed as OAuthPkce.oauthClientConfig) currently hardcodes localhost URLs; change it to read issuerBaseUrl, clientId, and redirectUri (and optionally scopes) from environment variables with sensible defaults so different environments can override them, e.g., use process.env.OAUTH_... values when constructing oauthConfig; also update your build config (rspack.config.js DefinePlugin) to expose those env vars to the client with defaults so staging/production can supply proper values.
66-74: Hardcoded user data inhandleCallbackshould be replaced with real user info.The TODO comment acknowledges this, but the hardcoded user data means authentication state doesn't reflect the actual authenticated user. For a proper implementation, decode the JWT ID token or call the
/userinfoendpoint.Would you like me to help implement JWT decoding or a
/userinfocall to populate the user from the actual token response?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/AuthContext.res` around lines 66 - 74, The handleCallback currently ignores the token response and sets hardcoded user data; use the returned token set from cosmoAuth.handleTokenCallback(search) (the _tokenSet variable) to populate the real user before calling setState(LoggedIn(...)). Decode the ID token (or call the /userinfo endpoint with the access_token) to extract unique id, name, and email, map those fields into the LoggedIn payload, and replace the hardcoded values; also handle missing/invalid tokens and surface/log errors from cosmoAuth.handleTokenCallback so you don't call setState with dummy data.package.json (1)
24-27: Inconsistent version pinning for@cosmonexus/*packages.
@cosmonexus/oauth-coreuses exact version1.0.0while the other three use caret ranges^1.0.0. This inconsistency could lead to version mismatches if the packages have interdependencies.Consider aligning the version specifiers:
♻️ Proposed fix
"@cosmonexus/oauth-client": "^1.0.0", - "@cosmonexus/oauth-core": "1.0.0", + "@cosmonexus/oauth-core": "^1.0.0", "@cosmonexus/oauth-react": "^1.0.0", "@cosmonexus/oauth-tokens": "^1.0.0",Since these packages are from a scoped registry (
@cosmonexus), please verify that the.npmrcis correctly configured and the packages are accessible from your Verdaccio registry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 24 - 27, The package versions for the `@cosmonexus` scope are inconsistent: update the dependency specifier for "@cosmonexus/oauth-core" to use the same caret range as the others (change "1.0.0" to "^1.0.0") so all four packages ("@cosmonexus/oauth-client", "@cosmonexus/oauth-core", "@cosmonexus/oauth-react", "@cosmonexus/oauth-tokens") use the same semver pattern; after updating, verify your .npmrc is configured for the scoped registry so these packages resolve from your Verdaccio registry.mock-server/index.js (1)
115-137: Mock server doesn't validate PKCEcode_verifieragainstcode_challenge.The
/oauth/tokenendpoint stores thecode_challenge(line 104) but never verifies that thecode_verifiersubmitted in the token request hashes to match it. While acceptable for a mock server, this means PKCE validation isn't being tested.If you want the mock to actually validate PKCE for more realistic testing:
♻️ Proposed enhancement
if (url.pathname === "/oauth/token" && req.method === "POST") { let body = ""; req.on("data", (chunk) => { body += chunk; }); req.on("end", () => { const params = new URLSearchParams(body); const code = params.get("code"); + const codeVerifier = params.get("code_verifier"); + const pending = pendingCodes.get(code); - if (code && pendingCodes.has(code)) { + // For a real PKCE check, hash codeVerifier with SHA256 and base64url encode + // For simplicity, the mock just checks the code exists + if (code && pending) { pendingCodes.delete(code);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mock-server/index.js` around lines 115 - 137, The token endpoint currently checks that a code exists in pendingCodes but doesn't validate PKCE; update the /oauth/token handler (where it reads params.get("code")) to also read params.get("code_verifier"), compute its SHA-256 digest and base64url-encode it, and compare that value to the stored code_challenge associated with the code (the structure that stored the code_challenge at the earlier authorization step). If the computed challenge does not match the stored one, respond with a 400 error; only issue the access_token when the code_verifier matches the stored code_challenge. Use the existing pendingCodes/codeChallenge storage identifiers to locate the saved challenge for comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mock-server/index.js`:
- Around line 115-137: The token endpoint currently checks that a code exists in
pendingCodes but doesn't validate PKCE; update the /oauth/token handler (where
it reads params.get("code")) to also read params.get("code_verifier"), compute
its SHA-256 digest and base64url-encode it, and compare that value to the stored
code_challenge associated with the code (the structure that stored the
code_challenge at the earlier authorization step). If the computed challenge
does not match the stored one, respond with a 400 error; only issue the
access_token when the code_verifier matches the stored code_challenge. Use the
existing pendingCodes/codeChallenge storage identifiers to locate the saved
challenge for comparison.
In `@package.json`:
- Around line 24-27: The package versions for the `@cosmonexus` scope are
inconsistent: update the dependency specifier for "@cosmonexus/oauth-core" to
use the same caret range as the others (change "1.0.0" to "^1.0.0") so all four
packages ("@cosmonexus/oauth-client", "@cosmonexus/oauth-core",
"@cosmonexus/oauth-react", "@cosmonexus/oauth-tokens") use the same semver
pattern; after updating, verify your .npmrc is configured for the scoped
registry so these packages resolve from your Verdaccio registry.
In `@src/context/AuthContext.res`:
- Around line 3-8: The oauthConfig object (typed as OAuthPkce.oauthClientConfig)
currently hardcodes localhost URLs; change it to read issuerBaseUrl, clientId,
and redirectUri (and optionally scopes) from environment variables with sensible
defaults so different environments can override them, e.g., use
process.env.OAUTH_... values when constructing oauthConfig; also update your
build config (rspack.config.js DefinePlugin) to expose those env vars to the
client with defaults so staging/production can supply proper values.
- Around line 66-74: The handleCallback currently ignores the token response and
sets hardcoded user data; use the returned token set from
cosmoAuth.handleTokenCallback(search) (the _tokenSet variable) to populate the
real user before calling setState(LoggedIn(...)). Decode the ID token (or call
the /userinfo endpoint with the access_token) to extract unique id, name, and
email, map those fields into the LoggedIn payload, and replace the hardcoded
values; also handle missing/invalid tokens and surface/log errors from
cosmoAuth.handleTokenCallback so you don't call setState with dummy data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 634f8f73-58c5-45bd-b879-c8ca7e559e49
⛔ Files ignored due to path filters (2)
bun.lockbis excluded by!**/bun.lockbpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.npmrcbiome.jsonmock-server/index.jsmock-server/schema.graphqlpackage.jsonrspack.config.jssrc/App.ressrc/bindings/OAuthPkce.ressrc/bindings/OAuthReact.ressrc/context/AuthContext.ressrc/pages/CallbackPage.restsconfig.json
✅ Files skipped from review due to trivial changes (4)
- .npmrc
- mock-server/schema.graphql
- biome.json
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/App.res
- src/pages/CallbackPage.res
- src/bindings/OAuthPkce.res
Use clearState() instead of logout() for sign out so the browser stays in the app. Add /oauth/logout endpoint to mock server for cases where server-side logout is needed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OAuthPkce.resstartAuth— generates code verifier/challenge, state, stores in sessionStorage, returns auth URLexchangeCode— exchanges authorization code for tokens using stored verifierAuthContextprovider withLoggedOut | LoggedIn(user)state,login,logout,handleCallbackProtectedRoutecomponent — redirects to/loginif not authenticatedLoginPage— shows sign-in button (logged out) or user profile with sign-out (logged in)CallbackPage— handles OAuth redirect, exchanges code, redirects to login/oauth/authorizeand/oauth/tokenendpoints for local testingProtectedRouteAuth flow
/login/oauth/authorize/callback?code=...&state=...CallbackPageexchanges code for token via/oauth/tokenAuthContextupdates toLoggedInstate/postspageTest plan
bun run res:build— 107 modules compiled, zero warningsbun run build— production build succeedsbun run test— all 18 tests passAPI pattern matches
@cosmonexus/oauth-clientfrom auth-sdk-generator for future swap-in.Closes #39
Summary by CodeRabbit