Make Quickstart samples consistent#19
Conversation
📝 WalkthroughWalkthroughThis PR updates ThunderID SDK packages to support embedded sign-in/sign-up flows, seed flow metadata from server to client, add a UserAvatar component, simplify form validation, and fix SSR issues. It also refreshes quickstart samples across browser, Express, Next.js, Nuxt, React, and Vue, and narrows pnpm workspace package discovery. ChangesSDK Core Changes
Quickstart Samples Refresh
Workspace Config
Sequence Diagram(s)sequenceDiagram
participant SignIn
participant ThunderIDNextClient
participant SignInAction
SignIn->>ThunderIDNextClient: signIn(executionId payload)
ThunderIDNextClient-->>SignIn: EmbeddedSignInFlowResponse
SignIn->>SignInAction: submit assertion-based completion
SignInAction->>SignInAction: decode assertion and create session cookie
sequenceDiagram
participant ServerThunderIDProvider
participant ClientThunderIDProvider
participant FlowMetaProvider
ServerThunderIDProvider->>ClientThunderIDProvider: initialMeta
ClientThunderIDProvider->>FlowMetaProvider: initialMeta
FlowMetaProvider->>FlowMetaProvider: seed meta and skip first fetch
Related issues: None specified Related PRs: None specified Suggested labels: enhancement, samples, sdk Suggested reviewers: None specified Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
brionmario
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx (1)
220-220: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore inline server-field error rendering in the default sign-in UI
BaseSignInContentno longer consumesserverFieldErrors, so the built-in SignIn UI drops server-returned field errors instead of showing them next to the affected inputs.SignIn.tsxstill uses the array for render-prop consumers, so either wire the default renderer back up or remove the staleserverFieldErrorsplumbing from the default-UI path. Also update the stale “See BaseSignIn for the same pattern” comment inpackages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsx.🤖 Prompt for 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. In `@packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx` at line 220, The default sign-in UI is no longer rendering server-returned field errors because `BaseSignInContent` ignores `serverFieldErrors`, while `SignIn.tsx` still passes them through for the built-in flow. Wire `serverFieldErrors` back into `BaseSignInContent` so the affected inputs can show inline errors, or remove the unused plumbing from the default-UI path if it is no longer needed. Also update the stale “See BaseSignIn for the same pattern” comment in `BaseSignUp` to match the current implementation.
🧹 Nitpick comments (12)
samples/express/quickstart/index.mjs (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant path exclusion in config-notice gate.
express.static(Line 21) already serves/styles.cssbefore this middleware runs, so thereq.path !== '/styles.css'check at Line 82 is effectively dead logic. Harmless but worth a comment noting why it's there (e.g., in case the static mount changes).Also applies to: 81-86
🤖 Prompt for 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. In `@samples/express/quickstart/index.mjs` at line 21, The `req.path !== '/styles.css'` exclusion in the config-notice middleware is redundant because `app.use(express.static(...))` already serves that asset first, so this gate is effectively dead logic. Update the config-notice check to either remove the unnecessary `/styles.css` exclusion or add a brief comment in the middleware explaining that it is intentionally retained as a safeguard if the static mount order changes. Reference the `express.static` setup and the config-notice middleware so the intent stays clear.packages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsx (1)
486-490: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale cross-file comment reference.
The comment says "See BaseSignIn for the same pattern," but
BaseSignIn.tsxno longer implements server-field-error projection (thatuseEffectwas removed in this PR). Update the comment so it doesn't point future readers to logic that no longer exists.✏️ Proposed comment fix
/** - * Project server-side validation errors from the most recent flow response into the - * form's `errors` state. See BaseSignIn for the same pattern: first error per field - * wins, and the affected fields are marked touched so the error renders immediately. + * Project server-side validation errors from the most recent flow response into the + * form's `errors` state: first error per field wins, and the affected fields are + * marked touched so the error renders immediately. */🤖 Prompt for 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. In `@packages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsx` around lines 486 - 490, The inline documentation in BaseSignUp refers readers to a BaseSignIn pattern that no longer exists, so update the comment in BaseSignUp to remove the stale cross-file reference and describe the local server-side error projection behavior on its own. Keep the note aligned with the logic around projecting flow response errors into the form state, but avoid mentioning BaseSignIn unless that implementation is restored elsewhere.samples/react/quickstart/src/main.jsx (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the
.jsxextension for consistency.Other relative imports in this sample (e.g.,
./icons/ReactLogoinNav.jsx/ConfigNotice.jsx) omit the extension; ESLint'simport-x/extensionsalso flags this import.🔧 Proposed fix
-import ConfigNotice from './components/ConfigNotice.jsx' +import ConfigNotice from './components/ConfigNotice'🤖 Prompt for 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. In `@samples/react/quickstart/src/main.jsx` at line 5, The import in main.jsx uses a .jsx extension while the rest of the sample uses extensionless relative imports, so update the ConfigNotice import to match the existing style and satisfy import-x/extensions. Keep the change limited to the main.jsx import statement and preserve the same ConfigNotice symbol.Source: Linters/SAST tools
samples/react/quickstart/README.md (1)
33-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language to the fenced code block.
markdownlint flags the block at line 34 for missing a language identifier, unlike the sibling block above it (
```bash).📝 Proposed fix
2. Fill in your ThunderID credentials in `.env`, using the values you set in `thunderid-config/thunderid.env`: - ``` + ```bash VITE_THUNDERID_CLIENT_ID=REACT_QUICKSTART VITE_THUNDERID_BASE_URL=https://your-thunderid-instance ```🤖 Prompt for 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. In `@samples/react/quickstart/README.md` around lines 33 - 37, The README fenced block in the React quickstart lacks a language identifier, triggering markdownlint. Update the code fence in the quickstart instructions near the ThunderID credentials example to match the sibling fenced block style by adding an appropriate language tag to the existing fence so the block is explicitly labeled.Source: Linters/SAST tools
samples/browser/quickstart/src/style.css (1)
1081-1084: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFocus outline removed without a sufficiently distinct visible alternative.
outline: noneon:focusrelies solely onborder-colorchanging tovar(--blue)for focus visibility, which is a subtler cue than a full outline/box-shadow. Consider keeping a visible focus ring (e.g.,box-shadow) alongside the border-color change for keyboard users.♿ Suggested fix
.profile-dialog-field input:focus { outline: none; border-color: var(--blue); + box-shadow: 0 0 0 3px var(--blue-subtle); }🤖 Prompt for 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. In `@samples/browser/quickstart/src/style.css` around lines 1081 - 1084, The focus styling in the profile dialog input selector removes the outline without providing a clearly visible replacement, so update the .profile-dialog-field input:focus rule to keep a distinct focus indicator. Adjust the existing focus styles to preserve keyboard accessibility by adding a visible ring such as a box-shadow or equivalent alongside the border-color change, using the same .profile-dialog-field input:focus selector so the active state remains easy to locate.samples/browser/quickstart/src/components/profileDialog.js (2)
67-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModal dialog lacks Escape-key handling and focus management.
attachProfileDialogHandlerswires close/cancel/overlay-click but doesn't handle theEscapekey or move initial focus into the dialog (e.g., to the first input). For arole="dialog" aria-modal="true"element, keyboard users expect Escape to dismiss and focus to land inside on open.♿ Suggested addition
+ document.getElementById('profile-first-name')?.focus() + + const onKeydown = (e) => { + if (e.key === 'Escape') closeDialog() + } + document.addEventListener('keydown', onKeydown) + const closeDialog = () => { + document.removeEventListener('keydown', onKeydown) + overlay?.remove() + }🤖 Prompt for 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. In `@samples/browser/quickstart/src/components/profileDialog.js` around lines 67 - 112, `attachProfileDialogHandlers` currently closes the modal via close/cancel/overlay click only, but it needs keyboard and focus support for the dialog UX. Update the dialog setup to add an Escape-key listener that calls the existing `closeDialog` logic, and move initial focus into the dialog when it opens by focusing the first available input or the primary save control. Use the existing `overlay`, `closeDialog`, and `saveBtn` wiring in `attachProfileDialogHandlers` so the new behavior stays consistent with the current dialog lifecycle.
30-33: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAvatar URL rendered without protocol/allow-list validation.
avatarUrlis only HTML-escaped, not validated for scheme. Since the value is placed inside a quotedsrcattribute and quotes are escaped, attribute breakout isn't possible, so this is currently low-risk (imgdoesn't executejavascript:), but this pattern (also duplicated inhome.js) would benefit from restricting tohttp(s)schemes as defense-in-depth, especially since the sameescapeHtmlhelper is flagged by static analysis for hand-rolled escaping (CWE-79).🤖 Prompt for 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. In `@samples/browser/quickstart/src/components/profileDialog.js` around lines 30 - 33, The avatar rendering in profileDialog.js (and the matching logic in home.js) only HTML-escapes avatarUrl, but it should also validate the URL scheme before putting it into the img src attribute. Update getAvatarUrl/its callers so avatarUrl is accepted only when it uses an allow-listed http or https scheme, and fall back to initials otherwise. Keep escapeHtml for attribute safety, but apply the protocol check first so the avatarHtml construction cannot render unexpected schemes.packages/nuxt/src/runtime/composables/useThunderID.ts (1)
135-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew
any-typed response handling triggers ESLint errors.Static analysis flags unsafe member access/assignment on
res.dataand the synthesized response (no-unsafe-member-access,no-unsafe-assignment,no-explicit-any) for the newly added lines. Since the sign-in branch above already uses a similarly loose{data: any; success: boolean}shape, consider introducing a shared typed interface for the/api/auth/*fetch responses to satisfy the linter and improve type safety across both branches.♻️ Suggested typing
+interface AuthApiResponse<T = unknown> { + data: T & {afterSignInUrl?: string; afterSignUpUrl?: string}; + success: boolean; +} + - const res: {data: any; success: boolean} = await $fetch<{data: any; success: boolean}>('/api/auth/signup', { + const res: AuthApiResponse = await $fetch<AuthApiResponse>('/api/auth/signup', { body: {payload}, method: 'POST', });Also applies to: 144-146
🤖 Prompt for 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. In `@packages/nuxt/src/runtime/composables/useThunderID.ts` at line 135, The new `/api/auth/signup` and related auth fetch handling in `useThunderID` is using an `any` response shape that triggers unsafe ESLint rules. Introduce a shared typed interface or type alias for the `/api/auth/*` `$fetch` response and use it in both the sign-in and sign-up branches, replacing the inline `{data: any; success: boolean}` annotations so `res.data` is strongly typed and lint-safe.Source: Linters/SAST tools
samples/nuxt/quickstart/app/components/AppNav.vue (1)
19-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate logo/wordmark markup with
ConfigNotice.vue.The same SVG path and wordmark span is repeated verbatim in
samples/nuxt/quickstart/app/components/ConfigNotice.vue(lines 17-23). Extracting a sharedLogo.vuecomponent would avoid drift between the two.🤖 Prompt for 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. In `@samples/nuxt/quickstart/app/components/AppNav.vue` around lines 19 - 25, The logo and wordmark markup in AppNav.vue is duplicated in ConfigNotice.vue, so update AppNav to use a shared Logo component instead of repeating the SVG and span. Extract the repeated branding markup into a reusable component (for example, a logo/wordmark component) and replace both occurrences in AppNav and ConfigNotice with that component to keep them in sync.samples/nuxt/quickstart/thunderid-config/thunderid-config.yaml (1)
69-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winYAML template syntax will trip yamllint if linted directly.
The
{{- range .NUXT_QUICKSTART_REDIRECT_URIS}}/{{- end}}block is Go-template syntax meant to be rendered before YAML parsing, which is why yamllint flags line 75 as a syntax error. If this file (or its siblingthunderid-config.yamlfiles in other samples) is ever linted directly in CI, consider excludingthunderid-config/*.yamlfrom the yamllint config to avoid noisy false failures.🤖 Prompt for 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. In `@samples/nuxt/quickstart/thunderid-config/thunderid-config.yaml` around lines 69 - 85, The Go-template directives in the thunderid-config YAML are being linted as raw YAML, causing false yamllint failures. Update the yamllint setup to exclude the thunderid-config/*.yaml files (and any sibling thunderid-config.yaml templates in other samples) from direct linting so the unrendered template syntax in the redirectUris block is not parsed. Focus the change in the lint configuration rather than altering the template content itself.Source: Linters/SAST tools
samples/nextjs/quickstart/README.md (1)
38-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language to the fenced code block.
Line 40's fence is missing a language annotation, unlike the sibling
bashblocks elsewhere in this doc.Proposed fix
- ``` + ```bash NEXT_PUBLIC_THUNDERID_BASE_URL=https://localhost:8090🤖 Prompt for 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. In `@samples/nextjs/quickstart/README.md` around lines 38 - 48, The fenced credentials block in the quickstart README is missing a language annotation; update the Markdown fence in the numbered setup steps to use the same syntax style as the other bash examples. Adjust the code block around the ThunderID environment variables so it is explicitly labeled with bash, keeping the content unchanged.Source: Linters/SAST tools
packages/nextjs/src/server/ThunderIDProvider.tsx (1)
117-129: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnconditional flow-metadata fetch on every server render — no caching.
Unlike the user/profile fetches (gated by
signedIn),getFlowMetaruns on every render ofThunderIDServerProviderregardless of auth state, hitting the ThunderID backend for design-config/i18n on every single page load. Consider wrapping with a Next.js caching primitive (fetchwithnext: {revalidate}, orunstable_cache/React.cache) since design config/i18n bundles are unlikely to change per-request.🤖 Prompt for 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. In `@packages/nextjs/src/server/ThunderIDProvider.tsx` around lines 117 - 129, The getFlowMeta call in ThunderIDProvider is executed on every server render and should be cached instead of always hitting the backend. Update the ThunderIDServerProvider flow-metadata fetch path to use a Next.js caching primitive such as fetch with next.revalidate, unstable_cache, or React.cache around getFlowMeta so design config and i18n bundles are reused across requests. Keep the existing logger.warn error handling, but ensure the cached wrapper is applied at the getFlowMeta call site so repeated renders don’t refetch the same metadata.
🤖 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 `@packages/nextjs/src/server/actions/signInAction.ts`:
- Around line 113-140: The V2 assertion claims are being trusted after only
decoding, so validation must happen before any use of sub, organization, scope,
iat, or exp in signInAction. Update the flow around client.getDecodedIdToken and
SessionManager.createSessionToken to verify the assertion’s signature and
registered claims (issuer, audience, expiry, and any required nonce/session
binding) before deriving userIdFromToken, organizationId, and scopes. If
verification fails, stop the sign-in flow with an error instead of continuing
with the decoded payload.
In `@packages/nuxt/src/runtime/composables/useThunderID.ts`:
- Around line 134-149: Refresh the auth state after embedded sign-up completes
without redirect so useState('thunderid:auth') does not stay stale. In the
isEmbedded branch of useThunderID, after the /api/auth/signup fetch and before
returning the synthesized complete response, mirror the sign-in flow by
refetching /api/auth/session and updating the shared auth state before
completion is handled. Keep the fix localized to the sign-up completion path
that returns afterSignUpUrl and EmbeddedSignUpFlowStatus.Complete.
In `@samples/browser/quickstart/src/components/profileDialog.js`:
- Around line 5-27: Consolidate the duplicated user-display helpers by moving
escapeHtml, getAvatarUrl, and getInitials into a shared module such as
src/utils/userDisplay.js, then update profileDialog.js and home.js to import and
use those exported functions instead of keeping local copies. Keep the helper
names stable so both ProfileDialog and the home page continue to reference the
same implementations and any future escaping or avatar logic changes happen in
one place.
In `@samples/express/quickstart/index.mjs`:
- Around line 52-60: getSession and the async Express handlers that use it can
reject from SDK calls like isSignedIn, getAccessToken, and getUser without being
caught. Add a try/catch around getSession and the / and /token route logic, and
route failures through a shared error boundary or next(err) so Express 4 does
not leave rejected promises unhandled; use the existing getSession, /, and
/token handlers as the places to update.
In `@samples/express/quickstart/lib/auth.mjs`:
- Around line 24-34: The userinfo request in auth.mjs has no timeout, so
protected requests can hang indefinitely if ThunderID is slow or unresponsive.
Update the fetch call in the bearer-token verification flow to use an
AbortController-based timeout, and make sure the timeout is applied around the
existing response fetch in the userinfo handler. If the request is aborted,
return the same bad_gateway path with a clear timeout-related failure, and keep
the change localized to the auth verification logic.
In `@samples/express/quickstart/package.json`:
- Around line 5-8: The quickstart scripts use node --env-file in the
package.json scripts, but the documented runtime in the quickstart README is
Node.js 18+, which is incompatible. Update the quickstart to either raise the
documented Node requirement to a version that supports --env-file, or remove
--env-file from the start/dev scripts in the package.json so the sample works on
the stated Node version.
In `@samples/nextjs/quickstart/app/layout.tsx`:
- Around line 13-18: The REQUIRED_ENV_VARS list in app/layout.tsx is missing
NEXT_PUBLIC_THUNDERID_APPLICATION_ID, so the startup check can pass even though
the embedded sign-in flow still needs it. Update the REQUIRED_ENV_VARS constant
to include NEXT_PUBLIC_THUNDERID_APPLICATION_ID alongside the existing Thunder
ID env vars so ConfigNotice blocks startup until all required values are
present.
In `@samples/nuxt/quickstart/package.json`:
- Line 6: The dev script in the package.json sample uses a POSIX-only inline
environment assignment, so update the dev command to be cross-platform. Either
switch the TMPDIR setup in the dev script to a cross-env-based form or otherwise
make the script work in cmd.exe and other non-Unix shells, while keeping the
nuxt dev invocation in place.
In `@samples/vue/quickstart/README.md`:
- Around line 33-37: The fenced `.env` example in the Vue quickstart README is
missing a language identifier, triggering the MD040 lint warning. Update the
markdown code block in the ThunderID credentials section to specify the
appropriate language on the fence so the example is properly annotated.
---
Outside diff comments:
In `@packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx`:
- Line 220: The default sign-in UI is no longer rendering server-returned field
errors because `BaseSignInContent` ignores `serverFieldErrors`, while
`SignIn.tsx` still passes them through for the built-in flow. Wire
`serverFieldErrors` back into `BaseSignInContent` so the affected inputs can
show inline errors, or remove the unused plumbing from the default-UI path if it
is no longer needed. Also update the stale “See BaseSignIn for the same pattern”
comment in `BaseSignUp` to match the current implementation.
---
Nitpick comments:
In `@packages/nextjs/src/server/ThunderIDProvider.tsx`:
- Around line 117-129: The getFlowMeta call in ThunderIDProvider is executed on
every server render and should be cached instead of always hitting the backend.
Update the ThunderIDServerProvider flow-metadata fetch path to use a Next.js
caching primitive such as fetch with next.revalidate, unstable_cache, or
React.cache around getFlowMeta so design config and i18n bundles are reused
across requests. Keep the existing logger.warn error handling, but ensure the
cached wrapper is applied at the getFlowMeta call site so repeated renders don’t
refetch the same metadata.
In `@packages/nuxt/src/runtime/composables/useThunderID.ts`:
- Line 135: The new `/api/auth/signup` and related auth fetch handling in
`useThunderID` is using an `any` response shape that triggers unsafe ESLint
rules. Introduce a shared typed interface or type alias for the `/api/auth/*`
`$fetch` response and use it in both the sign-in and sign-up branches, replacing
the inline `{data: any; success: boolean}` annotations so `res.data` is strongly
typed and lint-safe.
In `@packages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsx`:
- Around line 486-490: The inline documentation in BaseSignUp refers readers to
a BaseSignIn pattern that no longer exists, so update the comment in BaseSignUp
to remove the stale cross-file reference and describe the local server-side
error projection behavior on its own. Keep the note aligned with the logic
around projecting flow response errors into the form state, but avoid mentioning
BaseSignIn unless that implementation is restored elsewhere.
In `@samples/browser/quickstart/src/components/profileDialog.js`:
- Around line 67-112: `attachProfileDialogHandlers` currently closes the modal
via close/cancel/overlay click only, but it needs keyboard and focus support for
the dialog UX. Update the dialog setup to add an Escape-key listener that calls
the existing `closeDialog` logic, and move initial focus into the dialog when it
opens by focusing the first available input or the primary save control. Use the
existing `overlay`, `closeDialog`, and `saveBtn` wiring in
`attachProfileDialogHandlers` so the new behavior stays consistent with the
current dialog lifecycle.
- Around line 30-33: The avatar rendering in profileDialog.js (and the matching
logic in home.js) only HTML-escapes avatarUrl, but it should also validate the
URL scheme before putting it into the img src attribute. Update getAvatarUrl/its
callers so avatarUrl is accepted only when it uses an allow-listed http or https
scheme, and fall back to initials otherwise. Keep escapeHtml for attribute
safety, but apply the protocol check first so the avatarHtml construction cannot
render unexpected schemes.
In `@samples/browser/quickstart/src/style.css`:
- Around line 1081-1084: The focus styling in the profile dialog input selector
removes the outline without providing a clearly visible replacement, so update
the .profile-dialog-field input:focus rule to keep a distinct focus indicator.
Adjust the existing focus styles to preserve keyboard accessibility by adding a
visible ring such as a box-shadow or equivalent alongside the border-color
change, using the same .profile-dialog-field input:focus selector so the active
state remains easy to locate.
In `@samples/express/quickstart/index.mjs`:
- Line 21: The `req.path !== '/styles.css'` exclusion in the config-notice
middleware is redundant because `app.use(express.static(...))` already serves
that asset first, so this gate is effectively dead logic. Update the
config-notice check to either remove the unnecessary `/styles.css` exclusion or
add a brief comment in the middleware explaining that it is intentionally
retained as a safeguard if the static mount order changes. Reference the
`express.static` setup and the config-notice middleware so the intent stays
clear.
In `@samples/nextjs/quickstart/README.md`:
- Around line 38-48: The fenced credentials block in the quickstart README is
missing a language annotation; update the Markdown fence in the numbered setup
steps to use the same syntax style as the other bash examples. Adjust the code
block around the ThunderID environment variables so it is explicitly labeled
with bash, keeping the content unchanged.
In `@samples/nuxt/quickstart/app/components/AppNav.vue`:
- Around line 19-25: The logo and wordmark markup in AppNav.vue is duplicated in
ConfigNotice.vue, so update AppNav to use a shared Logo component instead of
repeating the SVG and span. Extract the repeated branding markup into a reusable
component (for example, a logo/wordmark component) and replace both occurrences
in AppNav and ConfigNotice with that component to keep them in sync.
In `@samples/nuxt/quickstart/thunderid-config/thunderid-config.yaml`:
- Around line 69-85: The Go-template directives in the thunderid-config YAML are
being linted as raw YAML, causing false yamllint failures. Update the yamllint
setup to exclude the thunderid-config/*.yaml files (and any sibling
thunderid-config.yaml templates in other samples) from direct linting so the
unrendered template syntax in the redirectUris block is not parsed. Focus the
change in the lint configuration rather than altering the template content
itself.
In `@samples/react/quickstart/README.md`:
- Around line 33-37: The README fenced block in the React quickstart lacks a
language identifier, triggering markdownlint. Update the code fence in the
quickstart instructions near the ThunderID credentials example to match the
sibling fenced block style by adding an appropriate language tag to the existing
fence so the block is explicitly labeled.
In `@samples/react/quickstart/src/main.jsx`:
- Line 5: The import in main.jsx uses a .jsx extension while the rest of the
sample uses extensionless relative imports, so update the ConfigNotice import to
match the existing style and satisfy import-x/extensions. Keep the change
limited to the main.jsx import statement and preserve the same ConfigNotice
symbol.
🪄 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 Plus
Run ID: fffbfd90-71e5-4450-be33-c5205b12c6b6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (95)
packages/javascript/src/constants/OIDCRequestConstants.tspackages/javascript/src/constants/ScopeConstants.tspackages/nextjs/src/ThunderIDNextClient.tspackages/nextjs/src/client/components/presentation/SignIn/SignIn.tsxpackages/nextjs/src/client/components/presentation/SignUp/SignUp.tsxpackages/nextjs/src/client/components/presentation/UserAvatar/UserAvatar.tsxpackages/nextjs/src/client/contexts/ThunderID/ThunderIDProvider.tsxpackages/nextjs/src/client/index.tspackages/nextjs/src/server/ThunderIDProvider.tsxpackages/nextjs/src/server/actions/signInAction.tspackages/nuxt/src/runtime/composables/useThunderID.tspackages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsxpackages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsxpackages/react/src/contexts/FlowMeta/FlowMetaProvider.tsxpackages/react/src/index.tspackages/vue/src/components/auth/sign-up/BaseSignUp.tssamples/browser/quickstart/README.mdsamples/browser/quickstart/src/auth.jssamples/browser/quickstart/src/components/nav.jssamples/browser/quickstart/src/components/profileDialog.jssamples/browser/quickstart/src/main.jssamples/browser/quickstart/src/pages/home.jssamples/browser/quickstart/src/pages/profile.jssamples/browser/quickstart/src/style.csssamples/browser/quickstart/thunderid-config/thunderid-config.yamlsamples/browser/quickstart/thunderid-config/thunderid.envsamples/express/quickstart/README.mdsamples/express/quickstart/index.jssamples/express/quickstart/index.mjssamples/express/quickstart/lib/auth.mjssamples/express/quickstart/lib/expressLogo.mjssamples/express/quickstart/lib/layout.mjssamples/express/quickstart/lib/thunderMark.mjssamples/express/quickstart/package.jsonsamples/express/quickstart/postman/ThunderID-Express-Quickstart.postman_collection.jsonsamples/express/quickstart/public/styles.csssamples/express/quickstart/thunderid-config/thunderid-config.yamlsamples/express/quickstart/thunderid-config/thunderid.envsamples/nextjs/quickstart/.env.examplesamples/nextjs/quickstart/README.mdsamples/nextjs/quickstart/app/components/ConfigNotice.tsxsamples/nextjs/quickstart/app/components/HeroCtas.tsxsamples/nextjs/quickstart/app/components/Nav.tsxsamples/nextjs/quickstart/app/components/icons/NextLogo.tsxsamples/nextjs/quickstart/app/globals.csssamples/nextjs/quickstart/app/layout.tsxsamples/nextjs/quickstart/app/page.tsxsamples/nextjs/quickstart/app/profile/page.tsxsamples/nextjs/quickstart/app/signin/page.tsxsamples/nextjs/quickstart/app/signup/page.tsxsamples/nextjs/quickstart/thunderid-config/thunderid-config.yamlsamples/nextjs/quickstart/thunderid-config/thunderid.envsamples/node/quickstart/.env.examplesamples/node/quickstart/README.mdsamples/node/quickstart/index.jssamples/node/quickstart/package.jsonsamples/nuxt/quickstart/.env.examplesamples/nuxt/quickstart/.env.localsamples/nuxt/quickstart/README.mdsamples/nuxt/quickstart/app.vuesamples/nuxt/quickstart/app/app.vuesamples/nuxt/quickstart/app/assets/styles.csssamples/nuxt/quickstart/app/components/AppNav.vuesamples/nuxt/quickstart/app/components/ConfigNotice.vuesamples/nuxt/quickstart/app/pages/index.vuesamples/nuxt/quickstart/app/pages/signin.vuesamples/nuxt/quickstart/app/pages/signup.vuesamples/nuxt/quickstart/app/pages/token.vuesamples/nuxt/quickstart/nuxt.config.tssamples/nuxt/quickstart/package.jsonsamples/nuxt/quickstart/pages/index.vuesamples/nuxt/quickstart/pages/profile.vuesamples/nuxt/quickstart/thunderid-config/thunderid-config.yamlsamples/nuxt/quickstart/thunderid-config/thunderid.envsamples/react/quickstart/README.mdsamples/react/quickstart/src/App.csssamples/react/quickstart/src/App.jsxsamples/react/quickstart/src/components/ConfigNotice.jsxsamples/react/quickstart/src/components/HeroCtas.jsxsamples/react/quickstart/src/components/Nav.jsxsamples/react/quickstart/src/components/icons/ReactLogo.jsxsamples/react/quickstart/src/main.jsxsamples/react/quickstart/src/pages/HomePage.jsxsamples/react/quickstart/src/pages/ProfilePage.jsxsamples/react/quickstart/thunderid-config/thunderid-config.yamlsamples/react/quickstart/thunderid-config/thunderid.envsamples/vue/quickstart/README.mdsamples/vue/quickstart/src/App.vuesamples/vue/quickstart/src/components/ConfigNotice.vuesamples/vue/quickstart/src/components/Nav.vuesamples/vue/quickstart/src/pages/HomePage.vuesamples/vue/quickstart/src/pages/ProfilePage.vuesamples/vue/quickstart/src/style.csssamples/vue/quickstart/thunderid-config/thunderid-config.yamlsamples/vue/quickstart/thunderid-config/thunderid.env
💤 Files with no reviewable changes (15)
- samples/node/quickstart/.env.example
- samples/node/quickstart/package.json
- samples/nuxt/quickstart/.env.local
- samples/browser/quickstart/src/pages/profile.js
- samples/node/quickstart/README.md
- samples/express/quickstart/index.js
- samples/nextjs/quickstart/app/profile/page.tsx
- samples/vue/quickstart/src/pages/ProfilePage.vue
- samples/node/quickstart/index.js
- samples/nuxt/quickstart/pages/index.vue
- samples/react/quickstart/src/pages/ProfilePage.jsx
- samples/react/quickstart/src/App.jsx
- packages/javascript/src/constants/ScopeConstants.ts
- samples/nuxt/quickstart/pages/profile.vue
- samples/nuxt/quickstart/app.vue
| if (!response.assertion) { | ||
| throw new Error('[signInAction] Flow completed without an assertion.'); | ||
| } | ||
|
|
||
| const idToken: IdToken = await client.getDecodedIdToken(sessionId, response.assertion); | ||
| const userIdFromToken: string = (idToken.sub as string | undefined) ?? sessionId; | ||
| const organizationId: string | undefined = (idToken['user_org'] || idToken['organization_id']) as | ||
| | string | ||
| | undefined; | ||
| const scopes: string = (idToken['scope'] as string | undefined) ?? ''; | ||
| const iat: number = (idToken['iat'] as number | undefined) ?? Math.floor(Date.now() / 1000); | ||
| const exp: number = (idToken['exp'] as number | undefined) ?? iat + 3600; | ||
| const expiresIn: number = Math.max(exp - iat, 0); | ||
|
|
||
| const config: ThunderIDNextConfig = await client.getConfiguration(); | ||
| const sessionCookieExpiryTime: number = SessionManager.resolveSessionCookieExpiry( | ||
| config.sessionCookie?.expiryTime, | ||
| ); | ||
|
|
||
| const sessionToken: string = await SessionManager.createSessionToken( | ||
| response.assertion, | ||
| userIdFromToken, | ||
| sessionId, | ||
| scopes, | ||
| expiresIn, | ||
| '', | ||
| organizationId, | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect getDecodedIdToken + createSessionToken for signature/claims verification.
fd -t f -e ts | xargs rg -nP '\b(getDecodedIdToken|createSessionToken|verify(Session|Jwt|Token)?)\b' -l | sort -u
echo '--- getDecodedIdToken impl ---'
ast-grep run --pattern 'getDecodedIdToken($$$) { $$$ }' --lang typescript . 2>/dev/null | head -80
rg -nP -C4 'getDecodedIdToken\s*[=(]' --type=ts
echo '--- createSessionToken impl ---'
rg -nP -C6 'createSessionToken\s*[=(]' --type=tsRepository: thunder-id/javascript-sdks
Length of output: 2107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
ast-grep outline packages/nextjs/src/utils/SessionManager.ts --view expanded
echo '---'
ast-grep outline packages/javascript/src/ThunderIDJavaScriptClient.ts --view expanded
echo '---'
ast-grep outline packages/javascript/src/DefaultCrypto.ts --view expanded
echo '---'
ast-grep outline packages/javascript/src/IsomorphicCrypto.ts --view expanded
echo '---'
ast-grep outline packages/node/src/NodeCryptoUtils.ts --view expanded
echo '---'
ast-grep outline packages/nextjs/src/server/actions/signInAction.ts --view expanded
echo '--- SessionManager.ts excerpt ---'
wc -l packages/nextjs/src/utils/SessionManager.ts
sed -n '1,260p' packages/nextjs/src/utils/SessionManager.ts
echo '--- ThunderIDJavaScriptClient.ts excerpt ---'
wc -l packages/javascript/src/ThunderIDJavaScriptClient.ts
sed -n '1,260p' packages/javascript/src/ThunderIDJavaScriptClient.ts
echo '--- DefaultCrypto.ts excerpt ---'
wc -l packages/javascript/src/DefaultCrypto.ts
sed -n '1,280p' packages/javascript/src/DefaultCrypto.ts
echo '--- IsomorphicCrypto.ts excerpt ---'
wc -l packages/javascript/src/IsomorphicCrypto.ts
sed -n '1,260p' packages/javascript/src/IsomorphicCrypto.ts
echo '--- NodeCryptoUtils.ts excerpt ---'
wc -l packages/node/src/NodeCryptoUtils.ts
sed -n '1,280p' packages/node/src/NodeCryptoUtils.ts
echo '--- signInAction.ts excerpt ---'
sed -n '1,220p' packages/nextjs/src/server/actions/signInAction.tsRepository: thunder-id/javascript-sdks
Length of output: 30323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- getDecodedIdToken in ThunderIDJavaScriptClient.ts ---'
sed -n '650,710p' packages/javascript/src/ThunderIDJavaScriptClient.ts
echo '--- signInAction.ts relevant section ---'
sed -n '100,160p' packages/nextjs/src/server/actions/signInAction.ts
echo '--- any callers of createSessionToken in nextjs ---'
rg -n 'createSessionToken\(' packages/nextjs -g '*.ts'
echo '--- verifySessionToken usage in nextjs ---'
rg -n 'verifySessionToken|verifySessionTokenForRefresh|verifyTempSession' packages/nextjs -g '*.ts'Repository: thunder-id/javascript-sdks
Length of output: 9761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- signIn implementation(s) ---'
rg -n -C6 'signIn\(_options|\bsignIn\(' packages/javascript/src packages/nextjs/src packages/browser/src packages/node/src packages/nuxt/src -g '*.ts'
echo '--- response.assertion usage ---'
rg -n -C4 'response\.assertion|assertion' packages/javascript/src packages/nextjs/src packages/browser/src packages/node/src packages/nuxt/src -g '*.ts'
echo '--- token validation helpers around sign-in ---'
rg -n -C6 'tokenValidation|isValidIdToken|decodeJwtToken|getDecodedIdToken' packages/javascript/src packages/nextjs/src -g '*.ts'Repository: thunder-id/javascript-sdks
Length of output: 50382
Verify the V2 assertion before using its claims
getDecodedIdToken(...) only base64-decodes the payload, so sub, org, and scopes are accepted without checking signature, issuer, audience, or expiry. createSessionToken(...) only signs the session cookie; it does not validate the assertion.
🧰 Tools
🪛 ESLint
[error] 113-113: Unsafe member access .assertion on an any value.
(@typescript-eslint/no-unsafe-member-access)
[error] 117-117: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 117-117: Unsafe argument of type any assigned to a parameter of type string | undefined.
(@typescript-eslint/no-unsafe-argument)
[error] 117-117: Unsafe member access .assertion on an any value.
(@typescript-eslint/no-unsafe-member-access)
[error] 118-118: Unsafe member access .sub on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 119-119: ["user_org"] is better written in dot notation.
(@typescript-eslint/dot-notation)
[error] 119-119: Unsafe member access ['user_org'] on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 119-119: Prefer using nullish coalescing operator (??) instead of a logical or (||), as it is a safer operator.
(@typescript-eslint/prefer-nullish-coalescing)
[error] 119-119: ["organization_id"] is better written in dot notation.
(@typescript-eslint/dot-notation)
[error] 119-119: Unsafe member access ['organization_id'] on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 122-122: ["scope"] is better written in dot notation.
(@typescript-eslint/dot-notation)
[error] 122-122: Unsafe member access ['scope'] on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 123-123: ["iat"] is better written in dot notation.
(@typescript-eslint/dot-notation)
[error] 123-123: Unsafe member access ['iat'] on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 124-124: ["exp"] is better written in dot notation.
(@typescript-eslint/dot-notation)
[error] 124-124: Unsafe member access ['exp'] on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 127-127: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 127-127: Unsafe call of a type that could not be resolved.
(@typescript-eslint/no-unsafe-call)
[error] 129-129: Unsafe argument of type error typed assigned to a parameter of type number | undefined.
(@typescript-eslint/no-unsafe-argument)
[error] 129-129: Unsafe member access .sessionCookie on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
[error] 133-133: Unsafe argument of type any assigned to a parameter of type string.
(@typescript-eslint/no-unsafe-argument)
[error] 133-133: Unsafe member access .assertion on an any value.
(@typescript-eslint/no-unsafe-member-access)
🤖 Prompt for 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.
In `@packages/nextjs/src/server/actions/signInAction.ts` around lines 113 - 140,
The V2 assertion claims are being trusted after only decoding, so validation
must happen before any use of sub, organization, scope, iat, or exp in
signInAction. Update the flow around client.getDecodedIdToken and
SessionManager.createSessionToken to verify the assertion’s signature and
registered claims (issuer, audience, expiry, and any required nonce/session
binding) before deriving userIdFromToken, organizationId, and scopes. If
verification fails, stop the sign-in flow with an error instead of continuing
with the decoded payload.
| if (isEmbedded) { | ||
| const res: {data: any; success: boolean} = await $fetch<{data: any; success: boolean}>('/api/auth/signup', { | ||
| body: {payload}, | ||
| method: 'POST', | ||
| }); | ||
|
|
||
| // Flow complete — the server route replies with `{ afterSignUpUrl }` | ||
| // (no `flowStatus`). Synthesize one so `BaseSignUp`'s completion check | ||
| // (`response.flowStatus === Complete`) fires and `<ThunderIDSignUp>`'s | ||
| // `handleComplete` drives the post-registration redirect. | ||
| if (res.data?.afterSignUpUrl) { | ||
| return {afterSignUpUrl: res.data.afterSignUpUrl, flowStatus: EmbeddedSignUpFlowStatus.Complete}; | ||
| } | ||
|
|
||
| return res.data; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the Nuxt SignUp container to see how onComplete/afterSignUpUrl is consumed,
# and whether navigation forces a full reload (making the missing refresh a non-issue).
fd -t f 'SignUp.ts' packages/nuxt/src/runtime/components/auth
cat -n packages/nuxt/src/runtime/components/auth/SignUp.ts 2>/dev/null | sed -n '1,160p'Repository: thunder-id/javascript-sdks
Length of output: 7102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the composable and any auth-state updates.
cat -n packages/nuxt/src/runtime/composables/useThunderID.ts | sed -n '1,260p'
printf '\n--- SEARCH ---\n'
rg -n "useState\\('thunderid:auth'|api/auth/session|isSignedIn|session" packages/nuxt/src/runtime -g '!**/*.map'Repository: thunder-id/javascript-sdks
Length of output: 43785
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the embedded signup server route to see whether it sets the session cookie
# and what completion payload it returns.
fd -t f 'signup.post.ts' packages/nuxt/src/runtime/server/routes/auth/session
cat -n packages/nuxt/src/runtime/server/routes/auth/session/signup.post.ts 2>/dev/null | sed -n '1,240p'Repository: thunder-id/javascript-sdks
Length of output: 3785
Refresh auth state for no-redirect sign-up flows
navigateTo(afterSignUpUrl, { external: true }) reloads the page by default, but when shouldRedirectAfterSignUp is disabled this completion path leaves useState('thunderid:auth') stale until a manual refresh. Mirror the sign-in branch’s /api/auth/session refetch here.
🧰 Tools
🪛 ESLint
[error] This rule requires the strictNullChecks compiler option to be turned on to function correctly.
(@typescript-eslint/prefer-nullish-coalescing)
[error] 135-135: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 135-135: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 144-144: Unsafe member access .afterSignUpUrl on an any value.
(@typescript-eslint/no-unsafe-member-access)
[error] 145-145: Unsafe assignment of an any value.
(@typescript-eslint/no-unsafe-assignment)
[error] 145-145: Unsafe member access .afterSignUpUrl on an any value.
(@typescript-eslint/no-unsafe-member-access)
[error] 145-145: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
[error] 145-145: Unsafe member access .Complete on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
🤖 Prompt for 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.
In `@packages/nuxt/src/runtime/composables/useThunderID.ts` around lines 134 -
149, Refresh the auth state after embedded sign-up completes without redirect so
useState('thunderid:auth') does not stay stale. In the isEmbedded branch of
useThunderID, after the /api/auth/signup fetch and before returning the
synthesized complete response, mirror the sign-in flow by refetching
/api/auth/session and updating the shared auth state before completion is
handled. Keep the fix localized to the sign-up completion path that returns
afterSignUpUrl and EmbeddedSignUpFlowStatus.Complete.
| function escapeHtml(str) { | ||
| if (str == null) return '' | ||
| return String(str) | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>') | ||
| .replace(/"/g, '"') | ||
| .replace(/'/g, ''') | ||
| } | ||
|
|
||
| function getAvatarUrl(user) { | ||
| return user?.profile || user?.profileUrl || user?.picture || user?.URL || null | ||
| } | ||
|
|
||
| function getInitials(user) { | ||
| const given = user?.given_name || '' | ||
| const family = user?.family_name || '' | ||
| if (given && family) return (given[0] + family[0]).toUpperCase() | ||
| const name = user?.name || user?.username || user?.email || '?' | ||
| const parts = name.trim().split(/\s+/) | ||
| if (parts.length >= 2) return (parts[0][0] + parts[parts.length - 1][0]).toUpperCase() | ||
| return name.slice(0, 2).toUpperCase() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate escapeHtml/getInitials logic — consolidate into a shared module.
escapeHtml (lines 5-13) and getInitials (lines 19-27) are byte-for-byte duplicates of the functions already defined in samples/browser/quickstart/src/pages/home.js (lines 43-61). getAvatarUrl (lines 15-17) is likewise duplicated at home.js lines 63-65. Any future fix to the escaping/avatar logic (e.g., the CWE-79 hand-rolled-escaping concern flagged by static analysis) now has to be applied in two places, risking drift.
♻️ Suggested consolidation
-function escapeHtml(str) {
- if (str == null) return ''
- return String(str)
- .replace(/&/g, '&')
- .replace(/</g, '<')
- .replace(/>/g, '>')
- .replace(/"/g, '"')
- .replace(/'/g, '&`#39`;')
-}
-
-function getAvatarUrl(user) {
- return user?.profile || user?.profileUrl || user?.picture || user?.URL || null
-}
-
-function getInitials(user) {
- ...
-}
+import { escapeHtml, getAvatarUrl, getInitials } from '../utils/userDisplay.js'Create src/utils/userDisplay.js exporting these three helpers, and update home.js to import from the same module.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 6-11: Avoid hand-rolled HTML escaping (replacing characters with HTML entities); use a vetted encoder/sanitizer such as DOMPurify or sanitize-html.
Context: String(str)
.replace(/&/g, '&')
.replace(/</g, '<')
.replace(/>/g, '>')
.replace(/"/g, '"')
.replace(/'/g, ''')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(manual-sanitization)
[warning] 6-10: Avoid hand-rolled HTML escaping (replacing characters with HTML entities); use a vetted encoder/sanitizer such as DOMPurify or sanitize-html.
Context: String(str)
.replace(/&/g, '&')
.replace(/</g, '<')
.replace(/>/g, '>')
.replace(/"/g, '"')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(manual-sanitization)
[warning] 6-9: Avoid hand-rolled HTML escaping (replacing characters with HTML entities); use a vetted encoder/sanitizer such as DOMPurify or sanitize-html.
Context: String(str)
.replace(/&/g, '&')
.replace(/</g, '<')
.replace(/>/g, '>')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(manual-sanitization)
[warning] 6-8: Avoid hand-rolled HTML escaping (replacing characters with HTML entities); use a vetted encoder/sanitizer such as DOMPurify or sanitize-html.
Context: String(str)
.replace(/&/g, '&')
.replace(/</g, '<')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(manual-sanitization)
[warning] 6-7: Avoid hand-rolled HTML escaping (replacing characters with HTML entities); use a vetted encoder/sanitizer such as DOMPurify or sanitize-html.
Context: String(str)
.replace(/&/g, '&')
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(manual-sanitization)
🤖 Prompt for 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.
In `@samples/browser/quickstart/src/components/profileDialog.js` around lines 5 -
27, Consolidate the duplicated user-display helpers by moving escapeHtml,
getAvatarUrl, and getInitials into a shared module such as
src/utils/userDisplay.js, then update profileDialog.js and home.js to import and
use those exported functions instead of keeping local copies. Keep the helper
names stable so both ProfileDialog and the home page continue to reference the
same implementations and any future escaping or avatar logic changes happen in
one place.
| async function getSession(req) { | ||
| const client = req.thunderIDAuth; | ||
| const sessionId = req.cookies?.[SESSION_COOKIE_NAME]; | ||
| if (!client || !sessionId) return {signedIn: false, accessToken: null, user: null}; | ||
| const signedIn = (await client.isSignedIn(sessionId)) ?? false; | ||
| if (!signedIn) return {signedIn: false, accessToken: null, user: null}; | ||
| const [accessToken, user] = await Promise.all([client.getAccessToken(sessionId), client.getUser(sessionId)]); | ||
| return {signedIn, accessToken, user}; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled promise rejections in async route handlers (Express 4).
getSession() and the / and /token handlers call SDK methods (isSignedIn, getAccessToken, getUser) without try/catch. Express 4 does not forward rejected promises from async handlers to error middleware — a rejection here will hang the request or crash the process via unhandledRejection.
🔧 Proposed fix: wrap async handlers and add an error boundary
+function asyncHandler(fn) {
+ return (req, res, next) => Promise.resolve(fn(req, res, next)).catch(next);
+}
+
async function getSession(req) {
const client = req.thunderIDAuth;
const sessionId = req.cookies?.[SESSION_COOKIE_NAME];
if (!client || !sessionId) return {signedIn: false, accessToken: null, user: null};
- const signedIn = (await client.isSignedIn(sessionId)) ?? false;
- if (!signedIn) return {signedIn: false, accessToken: null, user: null};
- const [accessToken, user] = await Promise.all([client.getAccessToken(sessionId), client.getUser(sessionId)]);
- return {signedIn, accessToken, user};
+ try {
+ const signedIn = (await client.isSignedIn(sessionId)) ?? false;
+ if (!signedIn) return {signedIn: false, accessToken: null, user: null};
+ const [accessToken, user] = await Promise.all([client.getAccessToken(sessionId), client.getUser(sessionId)]);
+ return {signedIn, accessToken, user};
+ } catch {
+ return {signedIn: false, accessToken: null, user: null};
+ }
}
-app.get('/', async (req, res) => {
+app.get('/', asyncHandler(async (req, res) => {
...
-});
+}));
-app.get('/token', protect((res) => res.redirect('/login')), async (req, res) => {
+app.get('/token', protect((res) => res.redirect('/login')), asyncHandler(async (req, res) => {
...
-});
+}));
+
+// after all routes:
+app.use((err, req, res, next) => {
+ console.error(err);
+ res.status(500).json({error: 'internal_error'});
+});Also applies to: 132-133, 226-227
🤖 Prompt for 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.
In `@samples/express/quickstart/index.mjs` around lines 52 - 60, getSession and
the async Express handlers that use it can reject from SDK calls like
isSignedIn, getAccessToken, and getUser without being caught. Add a try/catch
around getSession and the / and /token route logic, and route failures through a
shared error boundary or next(err) so Express 4 does not leave rejected promises
unhandled; use the existing getSession, /, and /token handlers as the places to
update.
| let response; | ||
| try { | ||
| response = await fetch(`${baseUrl}/oauth2/userinfo`, { | ||
| headers: {Authorization: `Bearer ${token}`}, | ||
| }); | ||
| } catch { | ||
| return res.status(502).json({ | ||
| error: 'bad_gateway', | ||
| message: 'Could not reach ThunderID to validate the access token.', | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the userinfo fetch call.
The bearer-token verification calls ThunderID's /oauth2/userinfo with no timeout. A slow or unresponsive ThunderID instance will hang every protected request indefinitely instead of failing fast.
⏱️ Proposed fix: bound the fetch with an AbortController timeout
let response;
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 5000);
try {
response = await fetch(`${baseUrl}/oauth2/userinfo`, {
headers: {Authorization: `Bearer ${token}`},
+ signal: controller.signal,
});
} catch {
return res.status(502).json({
error: 'bad_gateway',
message: 'Could not reach ThunderID to validate the access token.',
});
+ } finally {
+ clearTimeout(timeout);
}📝 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.
| let response; | |
| try { | |
| response = await fetch(`${baseUrl}/oauth2/userinfo`, { | |
| headers: {Authorization: `Bearer ${token}`}, | |
| }); | |
| } catch { | |
| return res.status(502).json({ | |
| error: 'bad_gateway', | |
| message: 'Could not reach ThunderID to validate the access token.', | |
| }); | |
| } | |
| let response; | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 5000); | |
| try { | |
| response = await fetch(`${baseUrl}/oauth2/userinfo`, { | |
| headers: {Authorization: `Bearer ${token}`}, | |
| signal: controller.signal, | |
| }); | |
| } catch { | |
| return res.status(502).json({ | |
| error: 'bad_gateway', | |
| message: 'Could not reach ThunderID to validate the access token.', | |
| }); | |
| } finally { | |
| clearTimeout(timeout); | |
| } |
🧰 Tools
🪛 ESLint
[error] 26-26: 'fetch' is not defined.
(no-undef)
🤖 Prompt for 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.
In `@samples/express/quickstart/lib/auth.mjs` around lines 24 - 34, The userinfo
request in auth.mjs has no timeout, so protected requests can hang indefinitely
if ThunderID is slow or unresponsive. Update the fetch call in the bearer-token
verification flow to use an AbortController-based timeout, and make sure the
timeout is applied around the existing response fetch in the userinfo handler.
If the request is aborted, return the same bad_gateway path with a clear
timeout-related failure, and keep the change localized to the auth verification
logic.
| "type": "module", | ||
| "scripts": { | ||
| "start": "node index.js", | ||
| "dev": "node --watch index.js" | ||
| "start": "node --env-file=.env index.mjs", | ||
| "dev": "node --env-file=.env --watch index.mjs" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js --env-file flag stable version
💡 Result:
The --env-file flag in Node.js is considered stable as of Node.js version 24.10.0 and 22.21.0 [1][2][3]. It was introduced as an experimental feature in version 20.6.0 [4][5][6]. The flag allows users to load environment variables from a file, making them available on process.env [1][7][2].
Citations:
- 1: https://nodejs.org/api/cli.html
- 2: https://nodejs.org/dist/latest/docs/api/cli.html
- 3: http://nodejs.org/api/cli.html
- 4: https://nodejs.org/en/blog/release/v20.6.0
- 5: https://nodejs.org/docs/latest-v23.x/api/cli.html
- 6: https://nodejs.org/docs/latest-v24.x/api/cli.html
- 7: https://github.com/nodejs/node/blob/main/doc/api/cli.md
🏁 Script executed:
set -e
printf '--- package.json ---\n'
cat -n samples/express/quickstart/package.json
printf '\n--- Node version mentions in sample/docs ---\n'
rg -n --hidden -S '"engines"|node\s+v?([0-9]+\.){1,2}[0-9]+|Node.js|Node 20|Node 22|Node 24|--env-file' samples/express/quickstart . 2>/dev/null | sed -n '1,200p'Repository: thunder-id/javascript-sdks
Length of output: 255
🏁 Script executed:
set -e
echo '--- package.json ---'
cat -n samples/express/quickstart/package.json
echo
echo '--- Node version mentions in sample/docs ---'
rg -n -S '"engines"|node\s+v?([0-9]+\.){1,2}[0-9]+|Node\.js|Node 20|Node 22|Node 24|--env-file' samples/express/quickstart . 2>/dev/null | sed -n '1,200p'Repository: thunder-id/javascript-sdks
Length of output: 3370
🏁 Script executed:
sed -n '1,40p' samples/express/quickstart/README.md | cat -nRepository: thunder-id/javascript-sdks
Length of output: 2424
Update the quickstart Node requirement or drop --env-file. samples/express/quickstart/README.md says Node.js 18+, but node --env-file=.env needs a newer Node release, so the sample breaks on the documented runtime.
🤖 Prompt for 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.
In `@samples/express/quickstart/package.json` around lines 5 - 8, The quickstart
scripts use node --env-file in the package.json scripts, but the documented
runtime in the quickstart README is Node.js 18+, which is incompatible. Update
the quickstart to either raise the documented Node requirement to a version that
supports --env-file, or remove --env-file from the start/dev scripts in the
package.json so the sample works on the stated Node version.
| const REQUIRED_ENV_VARS = [ | ||
| 'NEXT_PUBLIC_THUNDERID_BASE_URL', | ||
| 'NEXT_PUBLIC_THUNDERID_CLIENT_ID', | ||
| 'THUNDERID_CLIENT_SECRET', | ||
| 'THUNDERID_SECRET', | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat samples/nextjs/quickstart/.env.example 2>/dev/null
rg -n 'NEXT_PUBLIC_THUNDERID_APPLICATION_ID' -g '!**/node_modules/**'Repository: thunder-id/javascript-sdks
Length of output: 666
Add NEXT_PUBLIC_THUNDERID_APPLICATION_ID to REQUIRED_ENV_VARS.
app/layout.tsx gates startup on this list, but the quickstart also depends on NEXT_PUBLIC_THUNDERID_APPLICATION_ID for the embedded sign-in flow. Leaving it out lets the app proceed past ConfigNotice and fail later with a less actionable error.
Suggested fix
const REQUIRED_ENV_VARS = [
'NEXT_PUBLIC_THUNDERID_BASE_URL',
'NEXT_PUBLIC_THUNDERID_CLIENT_ID',
+ 'NEXT_PUBLIC_THUNDERID_APPLICATION_ID',
'THUNDERID_CLIENT_SECRET',
'THUNDERID_SECRET',
]📝 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 REQUIRED_ENV_VARS = [ | |
| 'NEXT_PUBLIC_THUNDERID_BASE_URL', | |
| 'NEXT_PUBLIC_THUNDERID_CLIENT_ID', | |
| 'THUNDERID_CLIENT_SECRET', | |
| 'THUNDERID_SECRET', | |
| ] | |
| const REQUIRED_ENV_VARS = [ | |
| 'NEXT_PUBLIC_THUNDERID_BASE_URL', | |
| 'NEXT_PUBLIC_THUNDERID_CLIENT_ID', | |
| 'NEXT_PUBLIC_THUNDERID_APPLICATION_ID', | |
| 'THUNDERID_CLIENT_SECRET', | |
| 'THUNDERID_SECRET', | |
| ] |
🤖 Prompt for 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.
In `@samples/nextjs/quickstart/app/layout.tsx` around lines 13 - 18, The
REQUIRED_ENV_VARS list in app/layout.tsx is missing
NEXT_PUBLIC_THUNDERID_APPLICATION_ID, so the startup check can pass even though
the embedded sign-in flow still needs it. Update the REQUIRED_ENV_VARS constant
to include NEXT_PUBLIC_THUNDERID_APPLICATION_ID alongside the existing Thunder
ID env vars so ConfigNotice blocks startup until all required values are
present.
| "version": "0.0.0", | ||
| "scripts": { | ||
| "dev": "nuxt dev", | ||
| "dev": "TMPDIR=/tmp/nuxt-tmp/ nuxt dev", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the dev script cross-platform TMPDIR=/tmp/nuxt-tmp/ nuxt dev uses POSIX-only env assignment, so npm run dev will fail in cmd.exe unless this sample is Unix-only or you switch to cross-env.
🤖 Prompt for 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.
In `@samples/nuxt/quickstart/package.json` at line 6, The dev script in the
package.json sample uses a POSIX-only inline environment assignment, so update
the dev command to be cross-platform. Either switch the TMPDIR setup in the dev
script to a cross-env-based form or otherwise make the script work in cmd.exe
and other non-Unix shells, while keeping the nuxt dev invocation in place.
| 2. Fill in your ThunderID credentials in `.env`, using the values you set in `thunderid-config/thunderid.env`: | ||
| ``` | ||
| VITE_THUNDERID_CLIENT_ID=your-client-id | ||
| VITE_THUNDERID_CLIENT_ID=VUE_QUICKSTART | ||
| VITE_THUNDERID_BASE_URL=https://your-thunderid-instance | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced code block.
Line 34's code block lacks a language identifier (MD040).
📝 Suggested fix
- ```
+ ```env
VITE_THUNDERID_CLIENT_ID=VUE_QUICKSTART📝 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.
| 2. Fill in your ThunderID credentials in `.env`, using the values you set in `thunderid-config/thunderid.env`: | |
| ``` | |
| VITE_THUNDERID_CLIENT_ID=your-client-id | |
| VITE_THUNDERID_CLIENT_ID=VUE_QUICKSTART | |
| VITE_THUNDERID_BASE_URL=https://your-thunderid-instance | |
| ``` | |
| 2. Fill in your ThunderID credentials in `.env`, using the values you set in `thunderid-config/thunderid.env`: |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for 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.
In `@samples/vue/quickstart/README.md` around lines 33 - 37, The fenced `.env`
example in the Vue quickstart README is missing a language identifier,
triggering the MD040 lint warning. Update the markdown code block in the
ThunderID credentials section to specify the appropriate language on the fence
so the example is properly annotated.
Source: Linters/SAST tools
brionmario
force-pushed
the
migrate
branch
2 times, most recently
from
67d7690 to
bcd4502
Compare
| @@ -0,0 +1,112 @@ | |||
| import { updateMeProfile } from '@thunderid/browser' | |||
There was a problem hiding this comment.
Shall we add license header if not skipping for js files?
There was a problem hiding this comment.
since this is a sample that we expect users to build on top of, IMO better not to have the license header.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pnpm-workspace.yaml (1)
2-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider broader workspace globs if the layout may grow.
packages/*andsamples/*/*fit the current tree, but they will silently skip any package or sample added at a different depth.🤖 Prompt for 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. In `@pnpm-workspace.yaml` around lines 2 - 3, The workspace globs in pnpm-workspace.yaml are too narrow and may miss future packages or samples added at different nesting levels. Update the workspace pattern list to use broader globbing that covers the package and sample directories more flexibly, so pnpm still discovers new workspaces as the repository layout grows.
🤖 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.
Nitpick comments:
In `@pnpm-workspace.yaml`:
- Around line 2-3: The workspace globs in pnpm-workspace.yaml are too narrow and
may miss future packages or samples added at different nesting levels. Update
the workspace pattern list to use broader globbing that covers the package and
sample directories more flexibly, so pnpm still discovers new workspaces as the
repository layout grows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cdd7c27-673b-4cfd-9ca7-4cf9da7200fb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
packages/nextjs/src/client/components/presentation/UserAvatar/UserAvatar.tsxpackages/nextjs/src/client/contexts/ThunderID/ThunderIDProvider.tsxpackages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsxpnpm-workspace.yamlsamples/node/quickstart/.env.examplesamples/node/quickstart/README.mdsamples/node/quickstart/index.jssamples/node/quickstart/package.json
💤 Files with no reviewable changes (4)
- samples/node/quickstart/package.json
- samples/node/quickstart/README.md
- samples/node/quickstart/.env.example
- samples/node/quickstart/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nextjs/src/client/components/presentation/UserAvatar/UserAvatar.tsx
2 participants
Purpose
Approach
N/A
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
openidandprofile(internal login removed).