Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an OIDC service worker and trusted-domains files, moves token injection to the worker and into build via an Changes
Sequence DiagramsequenceDiagram
participant Client as Client App
participant SW as OIDC Service Worker
participant API as Backend API
participant OIDC as OIDC Server
Client->>SW: postMessage(init config / code_verifier / login callback)
OIDC->>SW: token response (access_token / id_token / dpop_nonce)
SW->>SW: store tokens, nonce, session state
Client->>API: fetch request (no Authorization header)
SW->>SW: validate trusted domain, generate DPoP proof if required
SW->>API: forward request with injected Authorization and DPoP
API->>SW: response (may include token placeholders)
SW->>SW: rewrite/hide token fields in response body
SW->>Client: deliver rewritten response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 6
🧹 Nitpick comments (2)
package.json (1)
13-13: Consider runningoidc-genindev/turbotoo.
next devserves files frompublic/as-is, andpublic/OidcServiceWorker.jsdoesimportScripts("OidcTrustedDomains.js")at registration time (seepublic/OidcServiceWorker.jsline 504). If a developer hasn't manually generated thepublic/assets, SW registration will fail in dev. Given that you already deletedpublic/OidcTrustedDomains.jsafter generation (the template is processed at runtime byenvsubstin the container), local dev also needs a strategy (e.g.,copytrustedpulling frompublic/local/OidcTrustedDomains.js) wired intodev. The existingcopytrustedscript looks like it was intended for this but isn't invoked anywhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 13, The dev setup fails to register the service worker when public/OidcTrustedDomains.js is missing; update package.json scripts so the dev and turbo flows run the trusted-domains generation or fallback copy before starting (e.g., ensure "dev" and any turbo pipeline script invoke "npm run oidc-gen" or "npm run copytrusted" similar to "build" which runs oidc-gen), and keep the copytrusted script wired in to populate public/local/OidcTrustedDomains.js for local development so public/OidcServiceWorker.js can importScripts("OidcTrustedDomains.js") successfully; modify the "dev"/turbo script entries to run the generation/copy step prior to starting Next.js.public/OidcServiceWorker.js (1)
1-1: Avoid checking in the generated service worker — it will drift from the installed library version.This file is produced by
node ./node_modules/@axa-fr/oidc-client/bin/copy-service-worker-files.mjs public(the newoidc-genscript). According to the upstream library, the SW must always match the installed library version; the upstream guidance is to regenerate it on every install. Having both the checked-in copy and a regenerated one duringbuildcreates two problems:
npm installdoes not update it (nopostinstallis defined), so dev builds (next dev) use whatever snapshot is in git; if you bump@axa-fr/react-oidc, page JS and SW can disagree untilnpm run buildis run.- Reviewers lose the ability to tell “intentional fork of the SW” from “stale generated file” — right now nothing signals which it is.
Recommended: add
public/OidcServiceWorker.jsandpublic/OidcServiceWorker.js.mapto.gitignore, add apostinstall(or aprepare) that runsoidc-gen, and keep only the.tmplchecked in. If you intentionally need to patch the SW, fork it into a separate file the build copies over the generated one, so the patch is explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/OidcServiceWorker.js` at line 1, Remove the generated public/OidcServiceWorker.js and public/OidcServiceWorker.js.map from version control and add those filenames to .gitignore; add a postinstall (or prepare) npm script that runs the upstream generator (node ./node_modules/@axa-fr/oidc-client/bin/copy-service-worker-files.mjs public or your oidc-gen wrapper) so the service worker is regenerated on install; keep only the source template (e.g., OidcServiceWorker.js.tmpl) checked in and, if you need intentional local patches, create a clearly named forked file that the build explicitly copies over the generated file instead of editing the generated output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 9: The "oidc-gen" npm script is non-portable and brittle: replace the
direct "./node_modules/@axa-fr/oidc-client/..." invocation and the plain "rm
public/OidcTrustedDomains.js" with a cross-platform, non-failing approach and
declare the missing package as a devDependency; specifically, add
"@axa-fr/oidc-client" to devDependencies (match the version used by
`@axa-fr/react-oidc`) and add "rimraf" to devDependencies, then update the
"oidc-gen" script to call the oidc client via npx (or node -e) and remove the
target file with rimraf (or a node unlink that ignores ENOENT) so removing
public/OidcTrustedDomains.js never fails on Windows or when the file is absent.
In `@public/OidcServiceWorker.js`:
- Line 504: The service worker importScripts call in OidcServiceWorker.js
(importScripts("OidcTrustedDomains.js")) will throw during next dev because
OidcTrustedDomains.js is only generated at container start; update the dev
workflow so the file always exists locally by wiring the existing copytrusted
step (or an equivalent that generates public/OidcTrustedDomains.js) into the
dev/turbo npm script pipeline (package.json) so running dev runs copytrusted
before starting the dev server; ensure oidc-gen no longer deletes the file
during local dev and document the dependency in package.json so
importScripts("OidcTrustedDomains.js") succeeds when registering the service
worker.
- Around line 492-502: The current code calls
trustedTypes.createPolicy("default", ...) which can conflict with other code
that also registers a "default" policy and cause the service worker to fail to
activate; change the policy registration to use a unique name (e.g.,
"oidc-sw-policy") and wrap the createPolicy call in a try/catch around the
trustedTypes.createPolicy("oidc-sw-policy", { createScriptURL: function(url) {
... } }) call so any "already defined" error is swallowed or logged instead of
throwing; keep the same URL whitelist check against scriptFilename inside
createScriptURL and only register when typeof trustedTypes !== "undefined" &&
typeof trustedTypes.createPolicy === "function".
In `@public/OidcTrustedDomains.js.tmpl`:
- Line 5: The template renders an empty NETBIRD_MGMT_GRPC_API_ENDPOINT into the
default allowlist which becomes the regex caret (^) and matches everything; fix
by preventing empty entries: update public/OidcTrustedDomains.js.tmpl to filter
out empty/blank env vars when building the default array (so
NETBIRD_MGMT_GRPC_API_ENDPOINT is omitted if unset) and/or add the same
required-check for NETBIRD_MGMT_GRPC_API_ENDPOINT in docker/init_react_envs.sh
(mirror the NETBIRD_MGMT_API_ENDPOINT guard); this ensures getDomains doesn't
produce a "" element that causes checkDomain and getCurrentDatabaseDomain to
compile /^/ and leak tokens.
In `@src/utils/api.tsx`:
- Around line 88-95: The polling loop in isIdTokenExpired captures the
render-time idToken and repeatedly calls isExpired(idToken), so it never sees a
refreshed token; update the loop to read the latest token each iteration (e.g.,
store the hook/getter result in a mutable ref or call the id token getter inside
the while) and call isExpired with that fresh value, or trigger a token refresh
and await a re-render before re-checking; ensure you keep the same
attempts/sleep logic but replace uses of the stale idToken with the up-to-date
value inside the loop so isExpired evaluates the current token each time.
- Around line 100-116: Change OIDCProvider.tsx so the service worker is not
enabled for idToken-only deployments: in the OIDCProvider component where you
set service_worker_only and service_worker_relative_url, check
config.tokenSource and if it equals "idtoken" do not set/omit those props (or
set them false/undefined). Update the code path that currently unconditionally
sets service_worker_only: true and service_worker_relative_url to skip/disable
the service worker when config.tokenSource === "idtoken" so fetches using the
idToken header are not intercepted and overwritten by the service worker.
---
Nitpick comments:
In `@package.json`:
- Line 13: The dev setup fails to register the service worker when
public/OidcTrustedDomains.js is missing; update package.json scripts so the dev
and turbo flows run the trusted-domains generation or fallback copy before
starting (e.g., ensure "dev" and any turbo pipeline script invoke "npm run
oidc-gen" or "npm run copytrusted" similar to "build" which runs oidc-gen), and
keep the copytrusted script wired in to populate
public/local/OidcTrustedDomains.js for local development so
public/OidcServiceWorker.js can importScripts("OidcTrustedDomains.js")
successfully; modify the "dev"/turbo script entries to run the generation/copy
step prior to starting Next.js.
In `@public/OidcServiceWorker.js`:
- Line 1: Remove the generated public/OidcServiceWorker.js and
public/OidcServiceWorker.js.map from version control and add those filenames to
.gitignore; add a postinstall (or prepare) npm script that runs the upstream
generator (node
./node_modules/@axa-fr/oidc-client/bin/copy-service-worker-files.mjs public or
your oidc-gen wrapper) so the service worker is regenerated on install; keep
only the source template (e.g., OidcServiceWorker.js.tmpl) checked in and, if
you need intentional local patches, create a clearly named forked file that the
build explicitly copies over the generated file instead of editing the generated
output.
🪄 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: d750d0df-c98a-4078-89f1-313bc104d75f
📒 Files selected for processing (5)
package.jsonpublic/OidcServiceWorker.jspublic/OidcTrustedDomains.js.tmplsrc/auth/OIDCProvider.tsxsrc/utils/api.tsx
heisbrot
reviewed
Apr 24, 2026
| //service_worker_relative_url: "/OidcServiceWorker.js", | ||
| service_worker_only: false, | ||
| service_worker_relative_url: "/OidcServiceWorker.js", | ||
| service_worker_only: true, |
Contributor
There was a problem hiding this comment.
I think we can also leave service_worker_only at false, this looks to me like a fallback when service worker fails. Can you verify that?
Also an ENV would be good to disable it.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auth/OIDCProvider.tsx`:
- Around line 104-110: The current handlers onSessionLost and
onLogoutFromAnotherTab redirect unconditionally to "/", losing the user's
pre-logout path so SecureProvider.login later redirects back to root; update
these handlers to preserve and restore the current path (e.g., capture
window.location.pathname+search into sessionStorage or append it as a query
param before redirecting) and ensure post-login logic in SecureProvider uses
that stored path, or simply re-enable the SessionLost component
(sessionLostComponent={SessionLost}) and remove the commented-out line so the
existing SessionLost flow (which routes to /peers) is used; also remove the dead
commented code if you opt for the sessionStorage/query approach.
In `@src/utils/api.tsx`:
- Around line 111-141: nativeFetch currently returns handleErrors(...) when
isIdTokenExpired() is true, but handleErrors (from useApiErrorHandling) resolves
to login()/Promise<void> or rejects and not a Fetch Response, causing callers
like apiRequest to treat the result as a Response (accessing res.ok) and crash;
change the expired-token branch in nativeFetch to reject a proper ErrorResponse
(e.g., return Promise.reject({ code: 401, message: "token expired" })) instead
of calling handleErrors, so the existing promise rejection flow in apiRequest
and SWR consumers handles the error consistently; keep references to
isIdTokenExpired, nativeFetch, handleErrors, apiRequest and getCurrentIdToken to
locate and update the code path.
🪄 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: bdc9d8ea-3478-4225-864b-3394e75b973a
📒 Files selected for processing (8)
config.jsondocker/init_react_envs.shpackage.jsonpublic/OidcTrustedDomains.jspublic/OidcTrustedDomains.js.tmplsrc/auth/OIDCProvider.tsxsrc/utils/api.tsxsrc/utils/config.ts
✅ Files skipped from review due to trivial changes (1)
- config.json
🚧 Files skipped from review as they are similar to previous changes (1)
- public/OidcTrustedDomains.js.tmpl
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/api.tsx`:
- Around line 124-131: The wrapper currently overwrites all headers, dropping
caller-provided ones; update the fetch call to merge headers instead of
replacing them: build a merged headers object/Headers instance by combining
headers from init.headers (and if input is a Request, its headers) with the
defaults {"Content-Type":"application/json", "Accept":"application/json",
"Authorization": `Bearer ${getCurrentIdToken() ?? idToken}`} and only add
defaults if those keys are not already present; then pass that merged headers
into fetch so existing custom headers from callers are preserved (use spread or
Headers API to normalize before calling fetch).
🪄 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: 34d7b7c7-d6b6-46ee-9fd5-61cd8e1e8c81
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonsrc/auth/OIDCProvider.tsxsrc/auth/SecureProvider.tsxsrc/auth/sessionRedirect.tssrc/utils/api.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/utils/api.tsx (1)
135-144:⚠️ Potential issue | 🟡 MinorCaller-provided headers are dropped in the
oidcFetchbranch.The idToken branch at line 127 preserves
init?.headersvia spread, but this branch overwrites headers entirely. This creates inconsistent behavior: custom headers passed by callers will work whentokenSource === "idtoken"but be silently lost otherwise.🔧 Proposed fix to preserve caller headers
// Default access token path: the service worker intercepts the request and // injects the Authorization header without exposing the token to page JS. return oidcFetch(input, { ...init, headers: { + ...(init?.headers || {}), "Content-Type": "application/json", Accept: "application/json", }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/api.tsx` around lines 135 - 144, The oidcFetch branch currently overwrites caller-provided headers, dropping init?.headers; update the return that calls oidcFetch(input, {...}) so headers are merged instead of replaced (preserve init?.headers while still supplying defaults). Specifically, in the oidcFetch branch of the function that uses input and init, build headers by including the default "Content-Type" and "Accept" and then spreading ...(init?.headers || {}) so callers can override or add headers; keep the rest of the init spread as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/utils/api.tsx`:
- Around line 135-144: The oidcFetch branch currently overwrites caller-provided
headers, dropping init?.headers; update the return that calls oidcFetch(input,
{...}) so headers are merged instead of replaced (preserve init?.headers while
still supplying defaults). Specifically, in the oidcFetch branch of the function
that uses input and init, build headers by including the default "Content-Type"
and "Accept" and then spreading ...(init?.headers || {}) so callers can override
or add headers; keep the rest of the init spread as-is.
7 tasks
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
2 participants
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.
Issue ticket number and link
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Security Improvements
New Features
Improvements