Conversation
- Created specification document detailing Azure B2C authentication with three provider options, session management, and timeout handling - Created implementation plan with phased approach covering B2C integration, OAuth/OIDC flows, Redis session storage, and client-side timeout tracking - Created detailed task list with clear dependencies on Azure B2C configuration and testing requirements - Downloaded JIRA attachment for reference Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…eature/VIBE-142-cath-sign-in-b2c
|
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:
📝 WalkthroughWalkthroughAdds Azure B2C sign-in (non‑prod) with login, callback, forgot‑password and logout handlers; server/client session timeout tracking and UI; B2C configuration utilities; route and helmet integrations; i18n for session-expired; tests, docs and env/helm updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant Frontend as PIP Frontend
participant B2C as Azure B2C
participant DB as Database
participant SessionStore as Session Store
User->>Browser: Visit /sign-in, choose CaTH
Browser->>Frontend: POST /sign-in (accountType=cath)
Frontend->>Browser: Redirect to /b2c-login?lng=en
Browser->>Frontend: GET /b2c-login
Frontend->>Frontend: generate state/nonce, store session b2cProvider/b2cLocale
Frontend->>Browser: Redirect to B2C authorize URL
Browser->>B2C: Authorize request (state, nonce, policy)
B2C->>Browser: Prompt credentials
Browser->>B2C: Submit credentials
B2C->>Browser: Redirect to /login/return?code=...&state=...
Browser->>Frontend: GET /login/return
Frontend->>Frontend: validate state, exchange code for tokens
Frontend->>B2C: POST token endpoint (code, client creds)
B2C->>Frontend: Return id_token, access_token
Frontend->>Frontend: extract profile, create/update user
Frontend->>DB: persist user
Frontend->>SessionStore: req.login() and set lastActivity
Frontend->>Browser: Redirect to /account-home?lng=en
Note over Browser,Frontend: Session Timeout Flow
Browser->>Frontend: (inactive 25min) requests show sessionTimeoutMs
Frontend->>Browser: include sessionTimeoutMs in responses
Browser->>Browser: initSessionTimeout() -> show modal countdown
User->>Browser: Click "Continue"
Browser->>Frontend: POST /api/extend-session
Frontend->>SessionStore: update lastActivity
Frontend->>Browser: success, modal dismissed
Browser->>Browser: (if countdown reaches logout) redirect to /session-expired
Browser->>Frontend: GET /session-expired
Frontend->>Browser: Render session-expired page
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
🎭 Playwright E2E Test Results260 tests 260 ✅ 25m 28s ⏱️ Results for commit 23f04d5. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
e2e-tests/tests/sign-out.spec.ts-56-65 (1)
56-65:⚠️ Potential issue | 🟡 MinorReplace
waitForTimeoutwithwaitForURL.Fixed delays are flaky. Use Playwright's built-in URL assertion which auto-retries.
Proposed fix
// Visit logout endpoint without being authenticated await page.goto("/logout"); // Should redirect to session-logged-out page - await page.waitForTimeout(1000); - await expect(page).toHaveURL(/session-logged-out/); + await expect(page).toHaveURL(/session-logged-out/);libs/auth/src/pages/b2c-forgot-password/index.ts-23-28 (1)
23-28:⚠️ Potential issue | 🟡 MinorPotential undefined values in URL parameters.
If
b2cConfig.clientIdis undefined (e.g., misconfiguration that passesisB2cConfigured()check), the URL will contain"undefined". Consider adding defensive validation or ensuringgetB2cConfig()returns validated values whenisB2cConfigured()is true.libs/auth/src/assets/js/session-timeout.ts-162-175 (1)
162-175:⚠️ Potential issue | 🟡 MinorhandleContinue doesn't verify response success.
The
.then()callback executes regardless of HTTP status. A 4xx/5xx response would still reset timers rather than triggering logout.🛠️ Proposed fix
fetch("/api/extend-session", { method: "POST", headers: { "Content-Type": "application/json" } }) - .then(() => { + .then((response) => { + if (!response.ok) { + throw new Error(`HTTP ${response.status}`); + } resetTimers(); })libs/auth/src/pages/b2c-callback/index.test.ts-13-16 (1)
13-16:⚠️ Potential issue | 🟡 MinorRemove
anytype annotations to maintain strict typing in tests.
mockSession: anyat line 15 andRecord<string, any>at line 390 bypass type safety and can hide shape regressions. The session mock should be typed using express-session types, and the JWT payload should useRecord<string, unknown>.Suggested changes
+import type { Session } from "express-session"; + - let mockSession: any; + type MockSession = Partial<Session> & { + id: string; + b2cProvider: string; + returnTo: string; + b2cLocale: string; + save: (callback: (err?: Error | null) => void) => void; + }; + let mockSession: MockSession; -function createMockIdToken(payload: Record<string, any>): string { +function createMockIdToken(payload: Record<string, unknown>): string {Note: Line 229 also uses
callback: anywhich should be typed as(err?: Error | null) => void.docs/tickets/229/ticket.md-16-35 (1)
16-35:⚠️ Potential issue | 🟡 MinorStandardise “sign in”/“sign‑in” wording
Several sentences use “sign into”, “sign in”, and “log in” interchangeably. Please standardise to “sign in to” (verb), “sign‑in” (noun/adjective), and “log in” (verb) for consistency in user‑facing text.
Also applies to: 39-74
docs/tickets/229/tasks.md-82-83 (1)
82-83:⚠️ Potential issue | 🟡 MinorMinor wording tweak for test checklist
Consider “requires an authenticated session” for clarity.
Proposed edit
- - [ ] Test session timeout warning modal (requires authenticated session - manual testing) - - [ ] Test session timeout auto-logout (requires authenticated session - manual testing) + - [ ] Test session timeout warning modal (requires an authenticated session - manual testing) + - [ ] Test session timeout auto-logout (requires an authenticated session - manual testing)libs/auth/src/config/b2c-config.test.ts-92-98 (1)
92-98:⚠️ Potential issue | 🟡 MinorMake authority‑URL tests deterministic when custom domain env vars are set
getB2cAuthorityUrlrelies ongetB2cBaseUrl, which prefersB2C_CUSTOM_DOMAIN/B2C_CUSTOM_DOMAIN_PATH. If those are set in the environment, these expectations will fail. Clear or set them explicitly in this describe block.Suggested fix
describe("getB2cAuthorityUrl", () => { beforeEach(() => { process.env.B2C_TENANT_NAME = "test-tenant"; process.env.B2C_POLICY_HMCTS = "B2C_1A_HMCTS"; process.env.B2C_POLICY_COMMON_PLATFORM = "B2C_1A_CP"; process.env.B2C_POLICY_CATH = "B2C_1A_CaTH"; + delete process.env.B2C_CUSTOM_DOMAIN; + delete process.env.B2C_CUSTOM_DOMAIN_PATH; });docs/tickets/229/plan.md-17-21 (1)
17-21:⚠️ Potential issue | 🟡 MinorEnsure environment placeholders are clearly non-secret.
Please confirm these are placeholders and not real values. Consider using obvious dummy tokens (e.g.,
<redacted>), so scanners don’t flag them as potential secrets.docs/tickets/VIBE-142/plan.md-204-208 (1)
204-208:⚠️ Potential issue | 🟡 MinorUse “sign in” as a verb.
Line 207 reads “Redirect to sign-in if not authenticated”; as a verb it should be “sign in”.
Suggested change
- - Redirect to sign-in if not authenticated + - Redirect to sign in if not authenticateddocs/tickets/VIBE-142/specification.md-5-11 (1)
5-11:⚠️ Potential issue | 🟡 MinorUse “sign in to” instead of “sign into”.
This is the standard phrasing for authentication.
Suggested change
-These users must sign into CaTH using their verified credentials before gaining access to their accounts. +These users must sign in to CaTH using their verified credentials before gaining access to their accounts. @@ -**I want to** sign into CaTH +**I want to** sign in to CaTHdocs/tickets/229/plan.md-88-92 (1)
88-92:⚠️ Potential issue | 🟡 MinorFix the export name typo.
The snippet shows
isBtoCConfigured, but the actual helper isisB2cConfigured.Suggested change
-export { isBtoCConfigured } from "./config/b2c-config.js"; +export { isB2cConfigured } from "./config/b2c-config.js";docs/tickets/VIBE-142/plan.md-71-85 (1)
71-85:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block.
This resolves the Markdown lint warning (MD040) and improves readability.
Suggested change
- ``` + ```env AZURE_B2C_TENANT_NAME=your-tenant AZURE_B2C_TENANT_ID=your-tenant-id ... - ``` + ```e2e-tests/tests/sign-in.spec.ts-112-116 (1)
112-116:⚠️ Potential issue | 🟡 MinorReplace fixed waits with
toHaveURL()to reduce test flakiness.Lines 113 and 130 use hard
waitForTimeout(1000)which is brittle. Playwright'stoHaveURL()auto-waits for navigation with a 30-second default timeout, eliminating timing brittleness.For line 113–115, replace:
await page.waitForTimeout(1000); const currentUrl = page.url(); expect(currentUrl).toMatch(/pip-frontend\.staging\.platform\.hmcts\.net|b2c-login/);with:
await expect(page).toHaveURL(/pip-frontend\.staging\.platform\.hmcts\.net|b2c-login/);For line 129–132, apply the same pattern while preserving the conditional Welsh locale check.
libs/auth/src/pages/b2c-callback/index.ts-1-4 (1)
1-4:⚠️ Potential issue | 🟡 MinorAdd
@hmcts/cloud-native-platformto the runtime dependencies in libs/auth/package.json.The import on line 2 will fail at runtime since this package is not declared as a dependency. Add
"@hmcts/cloud-native-platform": "workspace:*"to align with other workspace packages.
🧹 Nitpick comments (12)
libs/auth/src/assets/css/session-timeout.scss (1)
5-33: Consider using GOV.UK Design System colour variables.The modal styling is functional and accessible. However, the hardcoded colours (
#ffffff,#0b0c0c,rgba(0, 0, 0, 0.5)) could be replaced with GOV.UK Design System colour variables for consistency across the service.e2e-tests/tests/sign-out.spec.ts (3)
9-19: Consider using semantic selectors per coding guidelines.The test uses CSS class selectors (
.govuk-panel--confirmation). Per coding guidelines, prefergetByRole()first, thengetByText(). For a panel, considergetByRole('region')orgetByText()for the heading.Example using semantic selectors
- const panel = page.locator(".govuk-panel--confirmation"); - await expect(panel).toBeVisible(); - - // Check panel title - const panelTitle = panel.locator(".govuk-panel__title"); - await expect(panelTitle).toContainText(/you have been signed out/i); + // Check for signed out heading + const heading = page.getByRole("heading", { level: 1 }); + await expect(heading).toContainText(/you have been signed out/i);
21-29: Consider inlining accessibility checks with the journey test.As per coding guidelines, E2E tests should use AxeBuilder to test accessibility inline within journey tests, not as separate tests. This could be combined with the panel display test above.
67-83: This test largely duplicates the panel display test.Lines 71-77 repeat the same panel visibility checks from lines 12-18. Consider consolidating these assertions into a single comprehensive test per the coding guidelines for minimising test count.
libs/auth/src/assets/js/session-timeout.ts (2)
52-62: Static analysis false positive — innerHTML is safe here.The static analysis flagged this as XSS-vulnerable, but the content is a static string literal with no user input interpolation. This is safe.
However, the hardcoded English text ("You will soon be signed out, due to inactivity") does not support Welsh. Per coding guidelines, every page must support both English and Welsh.
Consider reading locale from
document.documentElement.langand providing translated strings, or injecting the modal HTML server-side via templates.
48-74: Modal lacks accessibility features.The warning modal should:
- Have
role="dialog"andaria-modal="true"- Include
aria-labelledbypointing to the heading- Trap keyboard focus while visible
- Return focus to the triggering element when dismissed
♿ Proposed accessibility improvements
modal.id = "session-timeout-modal"; modal.className = "session-timeout-modal"; + modal.setAttribute("role", "dialog"); + modal.setAttribute("aria-modal", "true"); + modal.setAttribute("aria-labelledby", "session-timeout-heading"); modal.style.display = "none"; modal.innerHTML = ` <div class="session-timeout-overlay"></div> <div class="session-timeout-content govuk-!-padding-4"> - <h2 class="govuk-heading-m">You will soon be signed out, due to inactivity</h2> + <h2 id="session-timeout-heading" class="govuk-heading-m">You will soon be signed out, due to inactivity</h2>libs/auth/src/middleware/session-timeout.ts (1)
8-12: Lift and rename the public route list to a module-level constant.This keeps constants top-level and in SCREAMING_SNAKE_CASE, and avoids re-allocating the array per request.
Proposed refactor
import type { NextFunction, Request, Response } from "express"; import { getTimeUntilExpiry, isSessionExpired, updateLastActivity } from "../session/timeout-tracker.js"; +const PUBLIC_ROUTES = ["/", "/sign-in", "/session-expired", "/logout", "/health"]; + /** * Middleware to track session inactivity and enforce timeout * Checks if session has expired and updates last activity timestamp */ export function sessionTimeoutMiddleware(req: Request, res: Response, next: NextFunction) { // Skip timeout tracking for public routes - const publicRoutes = ["/", "/sign-in", "/session-expired", "/logout", "/health"]; - if (publicRoutes.includes(req.path)) { + if (PUBLIC_ROUTES.includes(req.path)) { next(); return; }As per coding guidelines: Constants should use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT). Module ordering: constants outside function scope at top.
libs/auth/src/pages/session-expired/index.test.ts (1)
65-77: Content verification tests are reasonable for translation regression.These tests verify exact content strings, which serves as a safeguard against accidental translation changes. Consider whether these assertions duplicate the source of truth in
en.ts/cy.tsunnecessarily.e2e-tests/tests/sign-in.spec.ts (1)
118-136: Consider folding the Welsh redirect assertion into the main CaTH journey test.This adds another micro-test for the same journey. To keep the suite lean, merge the Welsh locale assertion into the CaTH flow test and keep accessibility checks inline there. Based on learnings: Applies to e2e-tests/**/*.spec.ts : E2E tests in Playwright should minimize test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests.
e2e-tests/tests/session-expired.spec.ts (1)
8-113: Consolidate the micro-tests into a single journey-style test with inline Axe checks.This file splits the same journey into multiple small tests (content, accessibility, Welsh, keyboard, screen reader). Please merge these into one or two full-journey tests with Axe inline to align with the Playwright e2e guidance. Based on learnings: Applies to e2e-tests/**/*.spec.ts : E2E tests in Playwright should minimize test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests.
libs/auth/src/pages/b2c-callback/index.ts (1)
24-33: Type the callback parameters and alias snake_case fields.Using
anyanderror_descriptionhere weakens type-safety and breaks camelCase conventions; a small params type and alias keeps strictness without much noise.As per coding guidelines: TypeScript variables should use camelCase (e.g., `userId`, `caseDetails`, `documentId`). Use TypeScript strict mode. Avoid `any` without justification.♻️ Suggested refactor
+type B2cCallbackParams = { + code?: string | string[]; + state?: string | string[]; + error?: string | string[]; + error_description?: string | string[]; +}; + -async function handleCallback(req: Request, res: Response, params: any) { - const { code, state, error, error_description } = params; +async function handleCallback(req: Request, res: Response, params: B2cCallbackParams) { + const { code, state, error, error_description: errorDescription } = params;- if (error_description?.includes("AADB2C90118")) { + if (errorDescription?.includes("AADB2C90118")) { const locale = req.session.b2cLocale || "en"; return res.redirect(`/b2c-forgot-password?lng=${locale}`); } - console.error(`B2C authentication error: ${error} - ${error_description}`); + console.error(`B2C authentication error: ${error} - ${errorDescription}`);libs/auth/src/config/b2c-config.ts (1)
3-18: MoveB2cConfigto the bottom to match module ordering.Keeping interfaces/types after functions matches the agreed module layout and makes exports easier to scan.
As per coding guidelines: Module ordering: constants outside function scope at top, exported functions next, other functions in usage order, interfaces and types at bottom.
| export function isB2cConfigured(): boolean { | ||
| // Check if B2C should be disabled for local development | ||
| if (process.env.NODE_ENV === "development" && !process.env.ENABLE_B2C) { | ||
| return false; | ||
| } | ||
|
|
||
| const b2cConfig = getB2cConfig(); | ||
| return !!(b2cConfig.clientId && b2cConfig.clientSecret); | ||
| } |
There was a problem hiding this comment.
Tighten the config completeness check.
isB2cConfigured only checks clientId/secret; missing tenantName/tenantId yields invalid base URLs and confusing failures. Treat empty values as not configured.
✅ Suggested fix
- return !!(b2cConfig.clientId && b2cConfig.clientSecret);
+ return !!(
+ b2cConfig.clientId &&
+ b2cConfig.clientSecret &&
+ b2cConfig.tenantName &&
+ b2cConfig.tenantId
+ );📝 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.
| export function isB2cConfigured(): boolean { | |
| // Check if B2C should be disabled for local development | |
| if (process.env.NODE_ENV === "development" && !process.env.ENABLE_B2C) { | |
| return false; | |
| } | |
| const b2cConfig = getB2cConfig(); | |
| return !!(b2cConfig.clientId && b2cConfig.clientSecret); | |
| } | |
| export function isB2cConfigured(): boolean { | |
| // Check if B2C should be disabled for local development | |
| if (process.env.NODE_ENV === "development" && !process.env.ENABLE_B2C) { | |
| return false; | |
| } | |
| const b2cConfig = getB2cConfig(); | |
| return !!( | |
| b2cConfig.clientId && | |
| b2cConfig.clientSecret && | |
| b2cConfig.tenantName && | |
| b2cConfig.tenantId | |
| ); | |
| } |
| trackException(error as Error, { | ||
| area: "B2C callback", | ||
| userEmail: userProfile.email, | ||
| userId: userProfile.id | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file in question
cat -n libs/auth/src/pages/b2c-callback/index.ts | head -100Repository: hmcts/cath-service
Length of output: 4225
🏁 Script executed:
# Find the trackException function definition
rg -n "trackException" --type ts -A 5 -B 2Repository: hmcts/cath-service
Length of output: 32671
🏁 Script executed:
# Search for correlation ID or tracing ID patterns
rg -n "correlationId|tracingId|requestId|traceId" --type tsRepository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Check telemetry/logger infrastructure
fd -e ts -e js | grep -E "(telemetry|logger|tracking)" | head -20Repository: hmcts/cath-service
Length of output: 44
Remove user identifiers from telemetry.
userEmail and userId are PII. Since these are not required for diagnosing the database error, remove them and rely on the implicit request context and session information already tracked by Application Insights.
Suggested change
- trackException(error as Error, {
- area: "B2C callback",
- userEmail: userProfile.email,
- userId: userProfile.id
- });
+ trackException(error as Error, {
+ area: "B2C callback"
+ });📝 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.
| trackException(error as Error, { | |
| area: "B2C callback", | |
| userEmail: userProfile.email, | |
| userId: userProfile.id | |
| }); | |
| trackException(error as Error, { | |
| area: "B2C callback" | |
| }); |
| // Get return URL and locale from session | ||
| const returnTo = req.session.returnTo || "/account-home"; | ||
| const locale = req.session.b2cLocale; | ||
|
|
||
| // Build redirect URL with locale if Welsh | ||
| const redirectUrl = locale === "cy" ? `${returnTo}?lng=cy` : returnTo; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find where returnTo is set in the session across the codebase
rg -n "returnTo\s*=" -g '*.ts' --context 3Repository: hmcts/cath-service
Length of output: 3337
🏁 Script executed:
# Check the full context of the file under review
wc -l libs/auth/src/pages/b2c-callback/index.tsRepository: hmcts/cath-service
Length of output: 107
🏁 Script executed:
# Read the file to understand the complete context
head -120 libs/auth/src/pages/b2c-callback/index.ts | tail -50Repository: hmcts/cath-service
Length of output: 1708
🏁 Script executed:
# Search for session middleware and returnTo initialization
rg -n "session\." -g '*.ts' -A 2 | grep -E "(returnTo|b2cLocale)" -B 2 -A 2Repository: hmcts/cath-service
Length of output: 6481
🏁 Script executed:
# Check the full redirect-helpers.ts file for validation logic
cat -n libs/auth/src/middleware/redirect-helpers.tsRepository: hmcts/cath-service
Length of output: 1294
🏁 Script executed:
# Check the b2c-callback test file for existing test coverage on returnTo
grep -n "returnTo" libs/auth/src/pages/b2c-callback/index.test.ts -A 3 -B 3Repository: hmcts/cath-service
Length of output: 822
Sanitise returnTo and fix query string handling.
The code has two issues:
returnTooriginates fromreq.originalUrl(user-controlled) with no validation. This enables open redirect attacks via protocol-relative URLs or path traversal.- The query string append breaks when
returnToalready contains?(e.g.,/admin-dashboard?foo=bar), creating a malformed URL.
Validate that returnTo is a relative path, and use conditional ? vs & separator:
Proposed fix
- const returnTo = req.session.returnTo || "/account-home";
- const locale = req.session.b2cLocale;
-
- const redirectUrl = locale === "cy" ? `${returnTo}?lng=cy` : returnTo;
+ const returnTo = req.session.returnTo || "/account-home";
+ const safeReturnTo =
+ returnTo.startsWith("/") && !returnTo.startsWith("//") ? returnTo : "/account-home";
+ const locale = req.session.b2cLocale;
+ const separator = safeReturnTo.includes("?") ? "&" : "?";
+ const redirectUrl = locale === "cy" ? `${safeReturnTo}${separator}lng=cy` : safeReturnTo;📝 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.
| // Get return URL and locale from session | |
| const returnTo = req.session.returnTo || "/account-home"; | |
| const locale = req.session.b2cLocale; | |
| // Build redirect URL with locale if Welsh | |
| const redirectUrl = locale === "cy" ? `${returnTo}?lng=cy` : returnTo; | |
| // Get return URL and locale from session | |
| const returnTo = req.session.returnTo || "/account-home"; | |
| const safeReturnTo = | |
| returnTo.startsWith("/") && !returnTo.startsWith("//") ? returnTo : "/account-home"; | |
| const locale = req.session.b2cLocale; | |
| const separator = safeReturnTo.includes("?") ? "&" : "?"; | |
| const redirectUrl = locale === "cy" ? `${safeReturnTo}${separator}lng=cy` : safeReturnTo; |
| const response = await fetch(tokenUrl, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/x-www-form-urlencoded" | ||
| }, | ||
| body: params.toString() | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n libs/auth/src/pages/b2c-callback/index.ts | head -200 | tail -50Repository: hmcts/cath-service
Length of output: 1779
🏁 Script executed:
cat -n libs/auth/src/pages/b2c-callback/index.ts | head -160 | tail -40Repository: hmcts/cath-service
Length of output: 1441
🏁 Script executed:
# Check for any global fetch timeout or request timeout handling
rg -t ts "AbortController|setTimeout.*fetch|timeout.*fetch" libs/auth/src/
# Check for any Express middleware that sets request timeout
rg -t ts "req\.setTimeout|requestTimeout|timeout:" libs/auth/src/ | head -20Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Check the parent POST handler signature and first lines of file for imports
cat -n libs/auth/src/pages/b2c-callback/index.ts | head -130Repository: hmcts/cath-service
Length of output: 5372
Add timeout protection to the B2C token exchange fetch call.
The fetch() call lacks a timeout, allowing hung B2C endpoints to indefinitely tie up request threads. Implement AbortController with a 10-second timeout and clear the timer in a finally block to ensure reliable timeout handling.
Proposed fix
- const response = await fetch(tokenUrl, {
- method: "POST",
- headers: {
- "Content-Type": "application/x-www-form-urlencoded"
- },
- body: params.toString()
- });
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 10000);
+ let response: Response;
+ try {
+ response = await fetch(tokenUrl, {
+ method: "POST",
+ headers: {
+ "Content-Type": "application/x-www-form-urlencoded"
+ },
+ body: params.toString(),
+ signal: controller.signal
+ });
+ } finally {
+ clearTimeout(timeoutId);
+ }| function extractUserProfile(idToken: string, _provider: string): UserProfile { | ||
| // Decode JWT (basic implementation - in production use a JWT library) | ||
| const parts = idToken.split("."); | ||
| if (parts.length !== 3) { | ||
| throw new Error("Invalid ID token format"); | ||
| } | ||
|
|
||
| const payload = JSON.parse(Buffer.from(parts[1], "base64").toString("utf-8")); | ||
|
|
||
| return { | ||
| id: payload.sub || payload.oid, | ||
| email: payload.email || payload.emails?.[0] || payload.signInNames?.emailAddress, | ||
| displayName: payload.name || payload.given_name || payload.family_name || "Unknown", | ||
| role: "VERIFIED", | ||
| provenance: "B2C" | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the complete file to understand the full context
wc -l libs/auth/src/pages/b2c-callback/index.tsRepository: hmcts/cath-service
Length of output: 107
🏁 Script executed:
# Read the entire file to see the complete implementation
cat -n libs/auth/src/pages/b2c-callback/index.tsRepository: hmcts/cath-service
Length of output: 8166
🏁 Script executed:
# Search for where extractUserProfile is called
rg "extractUserProfile" --type tsRepository: hmcts/cath-service
Length of output: 295
🏁 Script executed:
# Search for JWT verification in the codebase
rg "jwtVerify|verifyToken|jwt.verify|jose" --type tsRepository: hmcts/cath-service
Length of output: 123
🏁 Script executed:
# Check what JWT/auth libraries are available
find . -name "package.json" -type f | head -5 | xargs grep -l "jose\|jsonwebtoken\|jwt"Repository: hmcts/cath-service
Length of output: 46
🏁 Script executed:
# Check if there's middleware applied to this endpoint
rg "b2c-callback|oauth" --type ts libs/auth/src | grep -i "middleware\|route\|app\."Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Check package.json files for JWT libraries
cat libs/auth/package.json | grep -A 5 -B 5 "jose\|jsonwebtoken\|jwt" || echo "Checking root package.json..."Repository: hmcts/cath-service
Length of output: 91
🏁 Script executed:
# Check root package.json
cat package.json | grep -A 5 -B 5 "jose\|jsonwebtoken\|jwt" 2>/dev/null || echo "No match in root"Repository: hmcts/cath-service
Length of output: 716
🏁 Script executed:
# Search for any other JWT verification in the auth library
rg "jwt\.|verify\(" libs/auth/src --type ts -A 2Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Check the oauth-middleware to understand the context
head -50 libs/api/src/middleware/oauth-middleware.tsRepository: hmcts/cath-service
Length of output: 1569
Verify ID token and validate required claims.
The code decodes the JWT without verifying the signature, issuer, audience, or expiry. Although the token is obtained directly from Azure B2C via HTTPS, claims should still be validated before use. The function also silently allows missing required claims (sub, email) which would result in undefined user identifiers.
Use jsonwebtoken with jwks-rsa (already in dependencies) to verify the token against B2C's public key set and validate required claims exist before provisioning the user profile. Alternatively, add jose as a dependency and configure token verification with issuer, audience, and claims checks.
| // Build authorization URL with policy as query param | ||
| const b2cBaseUrl = getB2cBaseUrl(); | ||
| const authUrl = new URL(`${b2cBaseUrl}/oauth2/v2.0/authorize`); | ||
| authUrl.searchParams.set("p", b2cConfig.policyCath); | ||
| authUrl.searchParams.set("client_id", b2cConfig.clientId); | ||
| authUrl.searchParams.set("redirect_uri", b2cConfig.redirectUri); | ||
| authUrl.searchParams.set("response_type", b2cConfig.responseType); | ||
| authUrl.searchParams.set("response_mode", b2cConfig.responseMode); | ||
| authUrl.searchParams.set("scope", b2cConfig.scope.join(" ")); | ||
| authUrl.searchParams.set("ui_locales", uiLocale); | ||
| authUrl.searchParams.set("state", generateState(req)); | ||
| authUrl.searchParams.set("nonce", generateNonce()); |
There was a problem hiding this comment.
Persist and validate the OAuth state value.
The state value is generated and sent but not stored for callback validation. Please persist it in the session and verify/clear it in the callback handler to ensure CSRF protection.
Suggested change
- authUrl.searchParams.set("state", generateState(req));
+ const state = generateState(req);
+ req.session.b2cState = state;
+ authUrl.searchParams.set("state", state);📝 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.
| // Build authorization URL with policy as query param | |
| const b2cBaseUrl = getB2cBaseUrl(); | |
| const authUrl = new URL(`${b2cBaseUrl}/oauth2/v2.0/authorize`); | |
| authUrl.searchParams.set("p", b2cConfig.policyCath); | |
| authUrl.searchParams.set("client_id", b2cConfig.clientId); | |
| authUrl.searchParams.set("redirect_uri", b2cConfig.redirectUri); | |
| authUrl.searchParams.set("response_type", b2cConfig.responseType); | |
| authUrl.searchParams.set("response_mode", b2cConfig.responseMode); | |
| authUrl.searchParams.set("scope", b2cConfig.scope.join(" ")); | |
| authUrl.searchParams.set("ui_locales", uiLocale); | |
| authUrl.searchParams.set("state", generateState(req)); | |
| authUrl.searchParams.set("nonce", generateNonce()); | |
| // Build authorization URL with policy as query param | |
| const b2cBaseUrl = getB2cBaseUrl(); | |
| const authUrl = new URL(`${b2cBaseUrl}/oauth2/v2.0/authorize`); | |
| authUrl.searchParams.set("p", b2cConfig.policyCath); | |
| authUrl.searchParams.set("client_id", b2cConfig.clientId); | |
| authUrl.searchParams.set("redirect_uri", b2cConfig.redirectUri); | |
| authUrl.searchParams.set("response_type", b2cConfig.responseType); | |
| authUrl.searchParams.set("response_mode", b2cConfig.responseMode); | |
| authUrl.searchParams.set("scope", b2cConfig.scope.join(" ")); | |
| authUrl.searchParams.set("ui_locales", uiLocale); | |
| const state = generateState(req); | |
| req.session.b2cState = state; | |
| authUrl.searchParams.set("state", state); | |
| authUrl.searchParams.set("nonce", generateNonce()); |
| export function getTimeoutConfig() { | ||
| return { | ||
| warningMs: Number.parseInt(process.env.SESSION_TIMEOUT_WARNING_MS || String(DEFAULT_WARNING_MS), 10), | ||
| logoutMs: Number.parseInt(process.env.SESSION_TIMEOUT_LOGOUT_MS || String(DEFAULT_LOGOUT_MS), 10) | ||
| }; |
There was a problem hiding this comment.
Guard against NaN env values to avoid disabling timeouts.
If the env values are empty or non-numeric, parseInt returns NaN, and expiry checks can silently stop working. Please fallback to defaults when parsing fails.
Proposed fix
export function getTimeoutConfig() {
- return {
- warningMs: Number.parseInt(process.env.SESSION_TIMEOUT_WARNING_MS || String(DEFAULT_WARNING_MS), 10),
- logoutMs: Number.parseInt(process.env.SESSION_TIMEOUT_LOGOUT_MS || String(DEFAULT_LOGOUT_MS), 10)
- };
+ const warningEnv = Number(process.env.SESSION_TIMEOUT_WARNING_MS);
+ const logoutEnv = Number(process.env.SESSION_TIMEOUT_LOGOUT_MS);
+ return {
+ warningMs: Number.isFinite(warningEnv) ? warningEnv : DEFAULT_WARNING_MS,
+ logoutMs: Number.isFinite(logoutEnv) ? logoutEnv : DEFAULT_LOGOUT_MS
+ };
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/auth/src/pages/b2c-forgot-password/index.ts (1)
8-35: Page controller doesn’t render bilingual contentThis handler only redirects and never renders an en/cy content object. If it must live under
libs/*/src/pages, consider rendering a minimal bilingual view (or relocate this to a non-page handler) to keep page conventions consistent. As per coding guidelines, “libs//src/pages/**/.ts: Page controllers should export GET and/or POST functions with type annotations (req: Request, res: Response) and render views with content objects. Every page must support both English and Welsh by providing en and cy content objects to the renderer.”
| function generateState(req: Request): string { | ||
| const sessionId = req.session.id || "unknown"; | ||
| const timestamp = Date.now(); | ||
| const random = randomBytes(16).toString("hex"); | ||
|
|
||
| return Buffer.from(`${sessionId}:${timestamp}:${random}`).toString("base64"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -type f -name "index.ts" -path "*/b2c-forgot-password/*"Repository: hmcts/cath-service
Length of output: 112
🏁 Script executed:
# Get the full file content to understand context
cat -n libs/auth/src/pages/b2c-forgot-password/index.ts | head -100Repository: hmcts/cath-service
Length of output: 2444
🏁 Script executed:
# Search for state validation in the codebase
rg "b2cState|state.*validat|validat.*state" -A 3 -B 3 libs/auth/Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Check for callback handlers related to this OAuth flow
rg "b2c.*callback|oauth.*callback" -i libs/auth/ -A 5Repository: hmcts/cath-service
Length of output: 5906
🏁 Script executed:
# Examine the b2c-callback handler to see state validation
cat -n libs/auth/src/pages/b2c-callback/index.ts | head -80Repository: hmcts/cath-service
Length of output: 3473
🏁 Script executed:
# Look for the state validation logic in detail
rg "state.*==|state.*validat|validateState" libs/auth/src/pages/b2c-callback/ -A 5 -B 2Repository: hmcts/cath-service
Length of output: 640
🏁 Script executed:
# Check the test file to understand expected state format
cat -n libs/auth/src/pages/b2c-callback/index.test.ts | head -150Repository: hmcts/cath-service
Length of output: 5925
Store state token server-side instead of embedding session ID in OAuth state parameter
The session ID is exposed in the redirect URL and the state parameter lacks server-side validation storage. This leaks session identifiers and deviates from OAuth2 best practices. Replace with a random token persisted in the session for callback validation.
Suggested fix
function generateState(req: Request): string {
- const sessionId = req.session.id || "unknown";
- const timestamp = Date.now();
- const random = randomBytes(16).toString("hex");
-
- return Buffer.from(`${sessionId}:${timestamp}:${random}`).toString("base64");
+ const state = randomBytes(32).toString("base64url");
+ req.session.b2cState = state;
+ return state;
}📝 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.
| function generateState(req: Request): string { | |
| const sessionId = req.session.id || "unknown"; | |
| const timestamp = Date.now(); | |
| const random = randomBytes(16).toString("hex"); | |
| return Buffer.from(`${sessionId}:${timestamp}:${random}`).toString("base64"); | |
| } | |
| function generateState(req: Request): string { | |
| const state = randomBytes(32).toString("base64url"); | |
| req.session.b2cState = state; | |
| return state; | |
| } |
| @@ -0,0 +1,151 @@ | |||
| # #229: [VIBE-142] CaTH Sign In - B2C | |||
There was a problem hiding this comment.
remove VIBE-142 folder. Its a duplicate of folder 229
libs/auth/src/config/b2c-config.ts
Outdated
| const b2cConfig = getB2cConfig(); | ||
| let policyName: string; | ||
|
|
||
| switch (policy) { |
There was a problem hiding this comment.
this switch statement is bit strange. We do not use b2c config for policyHmcts and policyCommonPlatform. hmcts is CFT IDAM and already working.
| try { | ||
| await createOrUpdateUser({ | ||
| email: userProfile.email, | ||
| userProvenance: "B2C_IDAM", |
There was a problem hiding this comment.
There will be no B2C_IDAM, it will be PI_AAD
| // Get policy based on provider | ||
| let policy: string; | ||
| switch (provider) { | ||
| case "hmcts": |
There was a problem hiding this comment.
same as I mentioned above, this switch is not looking correct to me, we do not have b2c config for hmcts and common platform
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
libs/account/src/repository/model.ts (1)
1-8: Consider extracting duplicated union types into shared type aliases.The
roleunion ("VERIFIED" | "LOCAL_ADMIN" | …) is duplicated on lines 7 and 11. Similarly,userProvenancecould benefit from a named type. This is pre-existing, so just a nudge for a future tidy-up.♻️ Example extraction
+export type UserProvenance = "SSO" | "CFT_IDAM" | "CRIME_IDAM" | "B2C_IDAM" | "PI_AAD"; +export type UserRole = "VERIFIED" | "LOCAL_ADMIN" | "CTSC_ADMIN" | "SYSTEM_ADMIN"; + export interface User { email: string; firstName?: string; surname?: string; - userProvenance: "SSO" | "CFT_IDAM" | "CRIME_IDAM" | "B2C_IDAM" | "PI_AAD"; + userProvenance: UserProvenance; userProvenanceId: string; - role: "VERIFIED" | "LOCAL_ADMIN" | "CTSC_ADMIN" | "SYSTEM_ADMIN"; + role: UserRole; } export interface UpdateUserInput { - role?: "VERIFIED" | "LOCAL_ADMIN" | "CTSC_ADMIN" | "SYSTEM_ADMIN"; + role?: UserRole; lastSignedInDate?: Date; }libs/auth/src/config/b2c-config.ts (2)
22-33:getConfigValuetreats empty string env vars as unset.
if (envValue)is falsy for"", so an explicitly set empty env var (e.g.B2C_CUSTOM_DOMAIN="") falls through toconfig.get. This is probably fine for this use-case but worth being aware of — if deliberate clearing via env is ever needed, this won't work.
1-16: Interface placement and inline defaults.Per coding guidelines, interfaces and types should be declared at the bottom of the module. Also, the default policy strings (
"B2C_1_SignInUserFlow","B2C_1A_PASSWORD_RESET") could be extracted asSCREAMING_SNAKE_CASEconstants at the top for clarity, per guidelines.libs/auth/src/config/b2c-config.test.ts (1)
86-95: No test coverage forgetB2cBaseUrl.
getB2cBaseUrlhas two branches (custom domain vs. default tenant URL) but neither is tested. Consider adding tests for both paths.libs/auth/src/pages/b2c-callback/index.ts (2)
24-24:params: anybypasses strict typing.The
handleCallbackparameter is untyped. Consider usingRecord<string, unknown>or a narrower interface to satisfy TypeScript strict mode and the project'sanyavoidance guideline.Proposed fix
-async function handleCallback(req: Request, res: Response, params: any) { +async function handleCallback(req: Request, res: Response, params: Record<string, unknown>) {As per coding guidelines: "Avoid
anywithout justification."
143-177:exchangeCodeForTokensreturn type is unvalidated.
response.json()returnsany. The caller then accesses.id_tokenwithout confirming the property exists. A missing or malformed response from B2C would surface as an opaque error inextractUserProfilerather than a clear token-exchange failure.Consider adding a runtime check on the response shape.
| it("should return false when B2C is not configured", () => { | ||
| delete process.env.B2C_TENANT_NAME; | ||
| delete process.env.B2C_CLIENT_ID; | ||
| delete process.env.B2C_CLIENT_SECRET; | ||
|
|
||
| expect(isB2cConfigured()).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Test may pass for the wrong reason when NODE_ENV is "development".
If the test runner sets NODE_ENV=development (common with vitest), isB2cConfigured() returns false due to the dev-mode guard — not because of missing credentials. Set NODE_ENV explicitly to avoid a false positive.
Proposed fix
it("should return false when B2C is not configured", () => {
+ process.env.NODE_ENV = "production";
delete process.env.B2C_TENANT_NAME;
delete process.env.B2C_CLIENT_ID;
delete process.env.B2C_CLIENT_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.
| it("should return false when B2C is not configured", () => { | |
| delete process.env.B2C_TENANT_NAME; | |
| delete process.env.B2C_CLIENT_ID; | |
| delete process.env.B2C_CLIENT_SECRET; | |
| expect(isB2cConfigured()).toBe(false); | |
| }); | |
| it("should return false when B2C is not configured", () => { | |
| process.env.NODE_ENV = "production"; | |
| delete process.env.B2C_TENANT_NAME; | |
| delete process.env.B2C_CLIENT_ID; | |
| delete process.env.B2C_CLIENT_SECRET; | |
| expect(isB2cConfigured()).toBe(false); | |
| }); |
| export function getB2cBaseUrl(): string { | ||
| const b2cConfig = getB2cConfig(); | ||
|
|
||
| if (b2cConfig.customDomain && b2cConfig.customDomainPath) { | ||
| return `https://${b2cConfig.customDomain}/${b2cConfig.customDomainPath}`; | ||
| } | ||
|
|
||
| return `https://${b2cConfig.tenantName}.b2clogin.com/${b2cConfig.tenantName}.onmicrosoft.com`; | ||
| } |
There was a problem hiding this comment.
getB2cBaseUrl silently produces invalid URLs when tenantName is empty.
When neither custom domain nor tenantName is configured, this returns https://.b2clogin.com/.onmicrosoft.com. Consider guarding or throwing to surface the misconfiguration early.
Proposed fix
+ if (!b2cConfig.tenantName) {
+ throw new Error("B2C tenantName is required but not configured");
+ }
+
return `https://${b2cConfig.tenantName}.b2clogin.com/${b2cConfig.tenantName}.onmicrosoft.com`;📝 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.
| export function getB2cBaseUrl(): string { | |
| const b2cConfig = getB2cConfig(); | |
| if (b2cConfig.customDomain && b2cConfig.customDomainPath) { | |
| return `https://${b2cConfig.customDomain}/${b2cConfig.customDomainPath}`; | |
| } | |
| return `https://${b2cConfig.tenantName}.b2clogin.com/${b2cConfig.tenantName}.onmicrosoft.com`; | |
| } | |
| export function getB2cBaseUrl(): string { | |
| const b2cConfig = getB2cConfig(); | |
| if (b2cConfig.customDomain && b2cConfig.customDomainPath) { | |
| return `https://${b2cConfig.customDomain}/${b2cConfig.customDomainPath}`; | |
| } | |
| if (!b2cConfig.tenantName) { | |
| throw new Error("B2C tenantName is required but not configured"); | |
| } | |
| return `https://${b2cConfig.tenantName}.b2clogin.com/${b2cConfig.tenantName}.onmicrosoft.com`; | |
| } |
| try { | ||
| // Verify state matches session | ||
| const stateData = Buffer.from(state, "base64").toString("utf-8"); | ||
| const [sessionId] = stateData.split(":"); | ||
|
|
||
| if (sessionId !== req.session.id) { | ||
| console.error("B2C callback state mismatch"); | ||
| return res.status(403).send("Invalid request: state mismatch"); | ||
| } |
There was a problem hiding this comment.
State decoding lacks error handling for malformed input.
If state is not valid base64 or the decoded string doesn't contain :, Buffer.from may produce garbage and the split won't yield a meaningful sessionId. Wrap in try/catch to return a clear 403.
Proposed fix
try {
- // Verify state matches session
- const stateData = Buffer.from(state, "base64").toString("utf-8");
- const [sessionId] = stateData.split(":");
+ let sessionId: string;
+ try {
+ const stateData = Buffer.from(state, "base64url").toString("utf-8");
+ [sessionId] = stateData.split(":");
+ } catch {
+ console.error("B2C callback: failed to decode state parameter");
+ return res.status(403).send("Invalid request: malformed state");
+ }📝 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.
| try { | |
| // Verify state matches session | |
| const stateData = Buffer.from(state, "base64").toString("utf-8"); | |
| const [sessionId] = stateData.split(":"); | |
| if (sessionId !== req.session.id) { | |
| console.error("B2C callback state mismatch"); | |
| return res.status(403).send("Invalid request: state mismatch"); | |
| } | |
| try { | |
| let sessionId: string; | |
| try { | |
| const stateData = Buffer.from(state, "base64url").toString("utf-8"); | |
| [sessionId] = stateData.split(":"); | |
| } catch { | |
| console.error("B2C callback: failed to decode state parameter"); | |
| return res.status(403).send("Invalid request: malformed state"); | |
| } | |
| if (sessionId !== req.session.id) { | |
| console.error("B2C callback state mismatch"); | |
| return res.status(403).send("Invalid request: state mismatch"); | |
| } |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Resolved conflict in apps/web/src/app.ts by keeping both: - B2C auth imports from this branch (b2cCallbackHandler, sessionTimeoutMiddleware, etc.) - Administrative court module import from master 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
3 participants
Jira link
[VIBE-142] CaTH Sign In - B2C
#229
Change description
Implement CaTH B2C sign in
Checklist
Summary by CodeRabbit
New Features
Tests
Chores
Documentation