VIBE-311 - Create audit log view - #316
Conversation
- Created specification document detailing three main screens (dashboard, list view, detail view) with filtering and access control requirements - Created implementation plan with phased approach covering database schema, audit logging infrastructure, and UI components - Created detailed task list with clear dependencies and testing requirements - Downloaded JIRA attachment for reference Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
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 a new Audit Log feature: database migration and Prisma model, a reusable audit-log library and system-admin audit modules (repository, service, logger, middleware), UI list/detail pages with i18n and tests, schema discovery update, middleware registration, and request-level audit metadata wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Express as "Express App"
participant Middleware as "Audit Middleware"
participant Logger as "Logger"
participant Prisma as "Prisma Client"
participant DB as "PostgreSQL"
participant Service as "Audit Service"
User->>Express: POST/PUT/PATCH/DELETE request
Express->>Middleware: auditLogMiddleware()
alt authenticated & SYSTEM_ADMIN and not /audit-log routes
Middleware->>Middleware: generateActionName()/generateDetails()
Middleware->>Logger: logAction(userId, userEmail, userRole, provenance, action, details)
Logger->>Prisma: prisma.auditLog.create(...)
Prisma->>DB: INSERT INTO audit_log
DB-->>Prisma: insert result
Prisma-->>Logger: created entry
Logger-->>Middleware: success (or error logged)
else skip logging
Middleware-->>Express: next()
end
Middleware-->>Express: next()
Express->>User: response
User->>Express: GET /audit-log-list
Express->>Service: getAuditLogs(filters, page, pageSize)
Service->>Prisma: prisma.auditLog.findMany(...) / count
Prisma->>DB: SELECT ...
DB-->>Prisma: rows
Prisma-->>Service: records
Service->>Express: formatted PaginatedAuditLogs
Express->>User: rendered audit log list page
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (13)
libs/audit-log/src/config.ts (1)
1-7: Consider adding other standardized exports if applicable.Per coding guidelines,
config.tsshould export standardized interfaces:pageRoutes,apiRoutes,prismaSchemas,assets. Currently onlyprismaSchemasis exported, which is acceptable if this library doesn't require routes or assets. However, verify whether the module should expose additional standardised interfaces for consistency.libs/system-admin-pages/src/pages/audit-log-detail/index.njk (1)
41-41: Consider using a translated variable for the fallback text.The hardcoded
"N/A"string breaks i18n consistency. Consider using a translated variable (e.g.,notAvailableText) passed from the route handler for bilingual support.Suggested fix
- { text: log.details if log.details else "N/A" } + { text: log.details if log.details else notAvailableText }Then ensure
notAvailableTextis provided in your en.ts/cy.ts translation files.libs/system-admin-pages/src/audit-log/repository.ts (1)
33-69: Extract shared filter-building logic to reduce duplication.The
whereclause construction infindAllandcountByFiltersis identical. Consider extracting a helper function.♻️ Proposed refactor
+function buildWhereClause(filters: AuditLogFilters): Record<string, unknown> { + const where: Record<string, unknown> = {}; + + if (filters.email) { + where.userEmail = { contains: filters.email, mode: "insensitive" }; + } + + if (filters.userId) { + where.userId = filters.userId; + } + + if (filters.date) { + const startOfDay = new Date(filters.date); + startOfDay.setHours(0, 0, 0, 0); + + const endOfDay = new Date(filters.date); + endOfDay.setHours(23, 59, 59, 999); + + where.timestamp = { + gte: startOfDay, + lte: endOfDay + }; + } + + if (filters.actions && filters.actions.length > 0) { + where.action = { in: filters.actions }; + } + + return where; +} + export async function findAll(filters: AuditLogFilters = {}, page = 1, pageSize = 50): Promise<AuditLog[]> { - const where: Record<string, unknown> = {}; - - if (filters.email) { - where.userEmail = { contains: filters.email, mode: "insensitive" }; - } - - if (filters.userId) { - where.userId = filters.userId; - } - - if (filters.date) { - const startOfDay = new Date(filters.date); - startOfDay.setHours(0, 0, 0, 0); - - const endOfDay = new Date(filters.date); - endOfDay.setHours(23, 59, 59, 999); - - where.timestamp = { - gte: startOfDay, - lte: endOfDay - }; - } - - if (filters.actions && filters.actions.length > 0) { - where.action = { in: filters.actions }; - } - + const where = buildWhereClause(filters); const skip = (page - 1) * pageSize; // ... rest unchanged } export async function countByFilters(filters: AuditLogFilters = {}): Promise<number> { - const where: Record<string, unknown> = {}; - // ... duplicated logic + const where = buildWhereClause(filters); return await prisma.auditLog.count({ where }); }Also applies to: 77-106
libs/system-admin-pages/src/audit-log/service.ts (1)
24-33: Consider explicit timezone handling for timestamp formatting.
formatTimestampuses the server's local timezone. If logs are displayed across different timezones or the server timezone differs from user expectation, timestamps may be misleading. Consider using UTC or including timezone information.libs/system-admin-pages/src/audit-log/service.test.ts (1)
14-14: Consider typing the repository mock.Using
anyhere bypasses type safety. Consider usingvi.Mocked<typeof import('./repository.js')>for better type inference and autocomplete on mock methods.libs/system-admin-pages/src/audit-log/middleware.ts (2)
1-1: Clarify the purpose of the empty type import.
import type {} from "@hmcts/auth";appears to be a side-effect import for type augmentation. Consider adding a comment explaining this is needed forreq.usertyping, or useimport "@hmcts/auth";if runtime side effects are intended.
113-113: Consider typing the session parameter.The
session?: anyparameter loses type safety. If session typing is available from express-session, consider usingSession & Partial<SessionData>or a custom interface.libs/system-admin-pages/src/audit-log/middleware.test.ts (3)
457-468: Remove unused variable.
originalRenderis declared but never used in this test.Proposed fix
it("should handle render with callback", async () => { - const originalRender = mockResponse.render; - const middleware = auditLogMiddleware(); await middleware(mockRequest as Request, mockResponse as Response, mockNext); const callback = vi.fn(); (mockResponse.render as any)("test-view", {}, callback); // No audit log should be created for renders without errors expect(logAction).not.toHaveBeenCalled(); });
470-480: Remove unused variable.
originalRenderis declared but never used.Proposed fix
it("should handle render with options only", async () => { - const originalRender = mockResponse.render; - const middleware = auditLogMiddleware(); await middleware(mockRequest as Request, mockResponse as Response, mockNext); (mockResponse.render as any)("test-view", { data: "test" }); // No audit log should be created for renders without errors expect(logAction).not.toHaveBeenCalled(); });
482-492: Remove unused variable.
originalRenderis declared but never used.Proposed fix
it("should handle render with view only", async () => { - const originalRender = mockResponse.render; - const middleware = auditLogMiddleware(); await middleware(mockRequest as Request, mockResponse as Response, mockNext); (mockResponse.render as any)("test-view"); // No audit log should be created for renders without errors expect(logAction).not.toHaveBeenCalled(); });libs/system-admin-pages/src/pages/audit-log-list/en.ts (1)
1-49: Duplicate translations withaudit-log-detail/en.ts.This file contains identical content to
libs/system-admin-pages/src/pages/audit-log-detail/en.ts. Consider extracting shared translations to a common location (e.g.,audit-log/translations/en.ts) and importing where needed, or splitting so each file only contains its own view's strings.Currently, both list and detail views maintain the same full translation object, which violates DRY and increases maintenance burden.
libs/system-admin-pages/src/pages/audit-log-detail/en.ts (1)
1-49: Duplicate ofaudit-log-list/en.ts.This file is identical to
libs/system-admin-pages/src/pages/audit-log-list/en.ts. Extract shared translations to a single source of truth, then import into both page controllers. This prevents drift and simplifies maintenance.♻️ Suggested approach
Create a shared translations file:
// libs/system-admin-pages/src/audit-log/translations/en.ts export const en = { // All shared translations here };Then import in each page:
// audit-log-list/en.ts and audit-log-detail/en.ts export { en } from "../../audit-log/translations/en.js";e2e-tests/tests/audit-log-viewer.spec.ts (1)
13-18: Use Playwright's recommended selectors for test resilience.Lines 13 and 16 use CSS locators (
locator("h1")andclick('a:has-text(...)')) rather than Playwright's recommended methods. Migrate togetByRole(),getByLabel(), orgetByText()in priority order for better stability and accessibility alignment.
| 1. **Database Schema** | ||
| - `libs/postgres/prisma/schema.prisma` - Add AuditLog model | ||
| - `libs/postgres/prisma/migrations/` - Migration for audit_log table | ||
|
|
There was a problem hiding this comment.
Update Prisma paths to match the repo structure.
The plan references libs/postgres/prisma/..., but the PR changes indicate apps/postgres/prisma/... for schema and migrations. Please correct the paths to avoid confusion.
| **New Table: audit_log** | ||
| - `id` (PK) - Unique identifier |
There was a problem hiding this comment.
Use a proper heading instead of bold text.
Line 17 reads like a heading but is formatted as emphasis, which will trip markdownlint (MD036). Consider a proper sub‑heading.
✍️ Suggested tweak
-**New Table: audit_log**
+#### New Table: audit_log📝 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.
| **New Table: audit_log** | |
| - `id` (PK) - Unique identifier | |
| #### New Table: audit_log | |
| - `id` (PK) - Unique identifier |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
17-17: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
| - [ ] Update `libs/system-admin/src/pages/dashboard.ts` to add "Audit Log Viewer" tab | ||
| - [ ] Update `libs/system-admin/src/pages/dashboard.njk` template |
There was a problem hiding this comment.
Dashboard path outdated.
References libs/system-admin/src/pages/dashboard.ts, but actual path is libs/system-admin-pages/src/pages/system-admin-dashboard/. Update for accuracy.
| test.describe("Audit Log Viewer @nightly", () => { | ||
| test("system admin can view audit log list and details", async ({ page }) => { | ||
| // Navigate to system admin dashboard and login | ||
| await page.goto("/system-admin-dashboard"); | ||
| await loginWithSSO(page, process.env.SSO_TEST_SYSTEM_ADMIN_EMAIL!, process.env.SSO_TEST_SYSTEM_ADMIN_PASSWORD!); | ||
| await page.waitForURL("/system-admin-dashboard"); | ||
|
|
||
| // Verify dashboard loaded | ||
| await expect(page.locator("h1")).toHaveText("System Admin Dashboard"); | ||
|
|
||
| // Click on Audit Log Viewer tile | ||
| await page.click('a:has-text("Audit Log Viewer")'); | ||
| await page.waitForURL("**/audit-log-list"); | ||
|
|
||
| // Verify audit log list page loaded | ||
| const heading = page.locator("h1"); | ||
| await expect(heading).toBeVisible(); | ||
| await expect(heading).toHaveText("Audit Log"); | ||
|
|
||
| // Check accessibility on list page | ||
| const listAccessibilityResults = await new AxeBuilder({ page }) | ||
| .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"]) | ||
| .analyze(); | ||
| expect(listAccessibilityResults.violations).toEqual([]); | ||
|
|
||
| // Verify table is present | ||
| const table = page.locator("table.govuk-table"); | ||
| await expect(table).toBeVisible(); | ||
|
|
||
| // Verify table headers | ||
| await expect(page.locator("th:has-text('Timestamp')")).toBeVisible(); | ||
| await expect(page.locator("th:has-text('Email')")).toBeVisible(); | ||
| await expect(page.locator("th:has-text('Action')")).toBeVisible(); | ||
| await expect(page.locator("th:has-text('View')")).toBeVisible(); | ||
|
|
||
| // Verify filter panel is present | ||
| const filterHeading = page.locator("h2:has-text('Filter')"); | ||
| await expect(filterHeading).toBeVisible(); | ||
|
|
||
| // Test Welsh translation | ||
| await page.click('a:has-text("Cymraeg")'); | ||
| await page.waitForURL("**/audit-log-list?lng=cy"); | ||
| await expect(page.locator("h1")).toHaveText("Cofnod Archwilio"); | ||
|
|
||
| // Switch back to English | ||
| await page.click('a:has-text("English")'); | ||
| await page.waitForURL("**/audit-log-list"); | ||
|
|
||
| // Check accessibility on Welsh page | ||
| await page.goto("/audit-log-list?lng=cy"); | ||
| const welshAccessibilityResults = await new AxeBuilder({ page }) | ||
| .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"]) | ||
| .analyze(); | ||
| expect(welshAccessibilityResults.violations).toEqual([]); | ||
| await page.goto("/audit-log-list"); | ||
|
|
||
| // Test clicking "View" link for first entry (if any exist) | ||
| const viewLinks = page.locator('a:has-text("View")'); | ||
| const viewLinkCount = await viewLinks.count(); | ||
|
|
||
| if (viewLinkCount > 0) { | ||
| // Click first "View" link | ||
| await viewLinks.first().click(); | ||
| await page.waitForURL(/.*audit-log-detail.*/); | ||
|
|
||
| // Verify detail page loaded | ||
| const detailHeading = page.locator("h1"); | ||
| await expect(detailHeading).toBeVisible(); | ||
| await expect(detailHeading).toHaveText("Audit Log Entry"); | ||
|
|
||
| // Check accessibility on detail page | ||
| const detailAccessibilityResults = await new AxeBuilder({ page }) | ||
| .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"]) | ||
| .analyze(); | ||
| expect(detailAccessibilityResults.violations).toEqual([]); | ||
|
|
||
| // Verify detail fields are present | ||
| await expect(page.locator("text=User ID")).toBeVisible(); | ||
| await expect(page.locator("text=Email")).toBeVisible(); | ||
| await expect(page.locator("text=Role")).toBeVisible(); | ||
| await expect(page.locator("text=Action")).toBeVisible(); | ||
|
|
||
| // Test Welsh translation on detail page | ||
| await page.click('a:has-text("Cymraeg")'); | ||
| await page.waitForURL(/.*lng=cy.*/); | ||
| await expect(page.locator("h1")).toHaveText("Cofnod Archwilio"); | ||
|
|
||
| // Switch back to English | ||
| await page.click('a:has-text("English")'); | ||
|
|
||
| // Test "Back to audit log list" link | ||
| const backLink = page.locator('a:has-text("Back to audit log list")'); | ||
| await expect(backLink).toBeVisible(); | ||
| await backLink.click(); | ||
| await page.waitForURL("**/audit-log-list"); | ||
|
|
||
| // Verify we're back on the list page | ||
| await expect(page.locator("h1")).toHaveText("Audit Log"); | ||
| } | ||
|
|
||
| // Test keyboard navigation | ||
| await page.goto("/audit-log-list"); | ||
| await page.keyboard.press("Tab"); | ||
|
|
||
| // Verify "Back to top" link functionality (if present) | ||
| const backToTopLink = page.locator('a:has-text("Back to top")'); | ||
| const backToTopCount = await backToTopLink.count(); | ||
| if (backToTopCount > 0) { | ||
| await backToTopLink.click(); | ||
| // Verify focus moved to top of page | ||
| const skipLink = page.locator("a.govuk-skip-link"); | ||
| await expect(skipLink).toBeFocused(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Add validation checks to this main journey.
The E2E guideline requires validation checks alongside Welsh translation, accessibility, and keyboard navigation in a single journey; this test covers the latter three only. Please add invalid input assertions here (e.g., invalid email/date). As per coding guidelines, validation, Welsh, accessibility, and keyboard checks must live in one journey.
| // Verify "Back to top" link functionality (if present) | ||
| const backToTopLink = page.locator('a:has-text("Back to top")'); | ||
| const backToTopCount = await backToTopLink.count(); | ||
| if (backToTopCount > 0) { | ||
| await backToTopLink.click(); | ||
| // Verify focus moved to top of page | ||
| const skipLink = page.locator("a.govuk-skip-link"); | ||
| await expect(skipLink).toBeFocused(); |
There was a problem hiding this comment.
Back-to-top focus assertion targets the wrong element.
The link navigates to #top, so focus typically stays on the clicked link or moves to the #top heading—not the skip link. Assert the hash or focus on the target heading instead.
💡 Suggested adjustment
- const skipLink = page.locator("a.govuk-skip-link");
- await expect(skipLink).toBeFocused();
+ await expect(page).toHaveURL(/#top$/);
+ await expect(page.locator("#top")).toBeVisible();📝 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.
| // Verify "Back to top" link functionality (if present) | |
| const backToTopLink = page.locator('a:has-text("Back to top")'); | |
| const backToTopCount = await backToTopLink.count(); | |
| if (backToTopCount > 0) { | |
| await backToTopLink.click(); | |
| // Verify focus moved to top of page | |
| const skipLink = page.locator("a.govuk-skip-link"); | |
| await expect(skipLink).toBeFocused(); | |
| // Verify "Back to top" link functionality (if present) | |
| const backToTopLink = page.locator('a:has-text("Back to top")'); | |
| const backToTopCount = await backToTopLink.count(); | |
| if (backToTopCount > 0) { | |
| await backToTopLink.click(); | |
| // Verify focus moved to top of page | |
| await expect(page).toHaveURL(/#top$/); | |
| await expect(page.locator("#top")).toBeVisible(); |
| const id = req.query.id as string | undefined; | ||
|
|
||
| if (!id) { | ||
| return res.status(404).render("errors/404", { | ||
| en, | ||
| cy, | ||
| message: content.entryNotFound | ||
| }); | ||
| } | ||
|
|
||
| const log = await auditLogService.getAuditLogById(id); |
There was a problem hiding this comment.
Validate the audit log id before querying.
Line 11 casts req.query.id directly to string; if it is missing, empty, or non‑string, it still flows to the service. Guarding the type/emptiness avoids unexpected lookups and aligns with endpoint input validation.
✅ Suggested guard
- const id = req.query.id as string | undefined;
+ const idParam = req.query.id;
+ const id = typeof idParam === "string" ? idParam.trim() : undefined;
- if (!id) {
+ if (!id) {
return res.status(404).render("errors/404", {
en,
cy,
message: content.entryNotFound
});
}As per coding guidelines, please validate endpoint inputs.
| @@ -0,0 +1,226 @@ | |||
| {% extends "layouts/base-template.njk" %} | |||
There was a problem hiding this comment.
Extend the correct base template.
Guidelines require layouts/base-templates.njk, but this template extends layouts/base-template.njk. Please update the extends path. As per coding guidelines, Nunjucks pages must extend the base-templates layout.
🔧 Proposed fix
-{% extends "layouts/base-template.njk" %}
+{% extends "layouts/base-templates.njk" %}📝 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.
| {% extends "layouts/base-template.njk" %} | |
| {% extends "layouts/base-templates.njk" %} |
| Email: {{ filters.email }} | ||
| <a href="/audit-log-list?userId={{ filters.userId }}&day={{ filters.day }}&month={{ filters.month }}&year={{ filters.year }}{% for action in filters.actions %}&actions={{ action }}{% endfor %}" class="filter-tag-remove" aria-label="Remove email filter">×</a> | ||
| </span> | ||
| {% endif %} | ||
| {% if filters.userId %} | ||
| <span class="filter-tag"> | ||
| User ID: {{ filters.userId }} | ||
| <a href="/audit-log-list?email={{ filters.email }}&day={{ filters.day }}&month={{ filters.month }}&year={{ filters.year }}{% for action in filters.actions %}&actions={{ action }}{% endfor %}" class="filter-tag-remove" aria-label="Remove user ID filter">×</a> | ||
| </span> | ||
| {% endif %} | ||
| {% if filters.day %} | ||
| <span class="filter-tag"> | ||
| Date: {{ filters.day }}/{{ filters.month }}/{{ filters.year }} | ||
| <a href="/audit-log-list?email={{ filters.email }}&userId={{ filters.userId }}{% for action in filters.actions %}&actions={{ action }}{% endfor %}" class="filter-tag-remove" aria-label="Remove date filter">×</a> | ||
| </span> | ||
| {% endif %} | ||
| {% for action in filters.actions %} | ||
| <span class="filter-tag"> | ||
| {{ action | replace("_", " ") | title }} | ||
| <a href="/audit-log-list?email={{ filters.email }}&userId={{ filters.userId }}&day={{ filters.day }}&month={{ filters.month }}&year={{ filters.year }}{% for a in filters.actions %}{% if a != action %}&actions={{ a }}{% endif %}{% endfor %}" class="filter-tag-remove" aria-label="Remove {{ action | replace('_', ' ') | title }} filter">×</a> | ||
| </span> |
There was a problem hiding this comment.
URL-encode filter values in generated links.
Raw values are interpolated into query strings; emails or action names with special characters can break the URL or cause parameter pollution. Apply | urlencode (or equivalent) for each value.
💡 Example fix (apply to all query params)
- <a href="/audit-log-list?userId={{ filters.userId }}&day={{ filters.day }}&month={{ filters.month }}&year={{ filters.year }}{% for action in filters.actions %}&actions={{ action }}{% endfor %}" class="filter-tag-remove" aria-label="Remove email filter">×</a>
+ <a href="/audit-log-list?userId={{ filters.userId | urlencode }}&day={{ filters.day | urlencode }}&month={{ filters.month | urlencode }}&year={{ filters.year | urlencode }}{% for action in filters.actions %}&actions={{ action | urlencode }}{% endfor %}" class="filter-tag-remove" aria-label="Remove email filter">×</a>Also applies to: 200-212
| // Parse query parameters | ||
| const email = req.query.email as string | undefined; | ||
| const userId = req.query.userId as string | undefined; | ||
| const day = req.query.day as string | undefined; | ||
| const month = req.query.month as string | undefined; | ||
| const year = req.query.year as string | undefined; | ||
| const actions = req.query.actions ? (Array.isArray(req.query.actions) ? (req.query.actions as string[]) : [req.query.actions as string]) : undefined; | ||
| const page = Number.parseInt(req.query.page as string, 10) || 1; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the full audit-log-list file
fd -t f "audit-log-list" --exec wc -l {} \;Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Read the full file to see validation logic after line 29
cat -n libs/system-admin-pages/src/pages/audit-log-list/index.tsRepository: hmcts/cath-service
Length of output: 4751
🌐 Web query:
Express req.query type handling multiple values query string parameters
💡 Result:
Short answer — req.query can be string | string[] | ParsedQs | ParsedQs[] (TypeScript shows ParsedQs). What you get depends on how the query is written and which parser Express uses:
- Repeated keys (e.g. ?a=1&a=2) → Express groups values into an array (string[]). [1][2]
- Bracket/nested syntax (e.g. ?a[x]=1&a[y]=2) or using the qs "extended" parser → values can be objects (ParsedQs) or arrays of objects. The ParsedQs union covers string, string[], ParsedQs, ParsedQs[]. [2][3]
- Plain single key (?a=1) → string.
If you need a predictable type, normalize/validate at the boundary. Example (TypeScript + Express):
const q = req.query.items; // type: string | string[] | ParsedQs | undefined
const items = Array.isArray(q) ? q : (q ? [String(q)] : []); // now string[]
Convert to numbers/booleans explicitly (parseInt/Number, / q === 'true' etc.) and consider setting app.set('query parser', ...) or using a custom parser if you want different behavior. [2][3]
Sources:
- Explanation of req.query parsing and arrays/objects in Express. [1]
- Type/behavior details (ParsedQs / qs / when arrays/objects appear). [2]
- Express docs on query parser modes (simple vs extended). [3]
References:
[1] Mastering JS — Query Parameters in Express. (masteringjs.io)
[2] StackOverflow / TypeScript ParsedQs discussion. (stackoverflow.com)
[3] Express API (query parser / req.query notes). (expressjs.com)
Citations:
- 1: https://masteringjs.io/tutorials/express/query-parameters?utm_source=openai
- 2: https://stackoverflow.com/questions/72350924/express-request-query-type-definition-is-parsedqs-why-is-it-recursive?utm_source=openai
- 3: https://expressjs.com/en/4x/api.html?utm_source=openai
Normalise query parameters to single strings before validation, and constrain page bounds.
Lines 22–27 cast query parameters with as string | undefined without ensuring they're actually strings. When Express receives repeated query keys (e.g. ?email=a@b.com&email=c@d.com), req.query returns an array, not a string. The unsafe cast bypasses type safety and allows arrays to be passed to validators. Coerce each parameter to a single string (or undefined) first, then validate.
Additionally, line 28 parses the page parameter without bounds checking, allowing negative or zero values. Constrain the page number to a minimum of 1.
Suggested normalisation
- const email = req.query.email as string | undefined;
- const userId = req.query.userId as string | undefined;
- const day = req.query.day as string | undefined;
- const month = req.query.month as string | undefined;
- const year = req.query.year as string | undefined;
- const actions = req.query.actions ? (Array.isArray(req.query.actions) ? (req.query.actions as string[]) : [req.query.actions as string]) : undefined;
- const page = Number.parseInt(req.query.page as string, 10) || 1;
+ const getSingle = (value: unknown): string | undefined =>
+ typeof value === "string" ? value : undefined;
+
+ const email = getSingle(req.query.email);
+ const userId = getSingle(req.query.userId);
+ const day = getSingle(req.query.day);
+ const month = getSingle(req.query.month);
+ const year = getSingle(req.query.year);
+ const actions = typeof req.query.actions === "string"
+ ? [req.query.actions]
+ : Array.isArray(req.query.actions)
+ ? req.query.actions.filter((v): v is string => typeof v === "string")
+ : undefined;
+ const page = Math.max(1, Number.parseInt(req.query.page as string, 10) || 1);📝 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.
| // Parse query parameters | |
| const email = req.query.email as string | undefined; | |
| const userId = req.query.userId as string | undefined; | |
| const day = req.query.day as string | undefined; | |
| const month = req.query.month as string | undefined; | |
| const year = req.query.year as string | undefined; | |
| const actions = req.query.actions ? (Array.isArray(req.query.actions) ? (req.query.actions as string[]) : [req.query.actions as string]) : undefined; | |
| const page = Number.parseInt(req.query.page as string, 10) || 1; | |
| // Parse query parameters | |
| const getSingle = (value: unknown): string | undefined => | |
| typeof value === "string" ? value : undefined; | |
| const email = getSingle(req.query.email); | |
| const userId = getSingle(req.query.userId); | |
| const day = getSingle(req.query.day); | |
| const month = getSingle(req.query.month); | |
| const year = getSingle(req.query.year); | |
| const actions = typeof req.query.actions === "string" | |
| ? [req.query.actions] | |
| : Array.isArray(req.query.actions) | |
| ? req.query.actions.filter((v): v is string => typeof v === "string") | |
| : undefined; | |
| const page = Math.max(1, Number.parseInt(req.query.page as string, 10) || 1); |
| const actions = req.query.actions ? (Array.isArray(req.query.actions) ? (req.query.actions as string[]) : [req.query.actions as string]) : undefined; | ||
| const page = Number.parseInt(req.query.page as string, 10) || 1; | ||
|
|
There was a problem hiding this comment.
Clamp page to a positive integer.
parseInt(...) || 1 allows negative values; this can lead to invalid paging and unexpected offsets. Consider an explicit positive‑integer check.
✅ Suggested clamp
- const page = Number.parseInt(req.query.page as string, 10) || 1;
+ const rawPage = typeof req.query.page === "string" ? Number.parseInt(req.query.page, 10) : NaN;
+ const page = Number.isInteger(rawPage) && rawPage > 0 ? rawPage : 1;As per coding guidelines, please validate endpoint inputs.
📝 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 actions = req.query.actions ? (Array.isArray(req.query.actions) ? (req.query.actions as string[]) : [req.query.actions as string]) : undefined; | |
| const page = Number.parseInt(req.query.page as string, 10) || 1; | |
| const actions = req.query.actions ? (Array.isArray(req.query.actions) ? (req.query.actions as string[]) : [req.query.actions as string]) : undefined; | |
| const rawPage = typeof req.query.page === "string" ? Number.parseInt(req.query.page, 10) : NaN; | |
| const page = Number.isInteger(rawPage) && rawPage > 0 ? rawPage : 1; | |
🎭 Playwright E2E Test Results257 tests 257 ✅ 23m 12s ⏱️ Results for commit d76d8e4. ♻️ This comment has been updated with latest results. |
|
|
||
| // Only log redirects to final action pages (success/complete/confirmation) | ||
| // This prevents logging intermediate steps in multi-step flows | ||
| const isFinalAction = redirectUrl && (redirectUrl.includes("-success") || redirectUrl.includes("-complete") || redirectUrl.includes("-confirmation")); |
There was a problem hiding this comment.
I like the way the middleware has been attached into the overall flow, however I think the use of the URL to log here could cause issues where we want to log something that doesn't match this.
Is there a way of attaching a flag to specific pages instead, maybe within the controller or similar
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/admin-pages/src/pages/remove-list-confirmation/index.ts (1)
147-148: Hardcoded English error message.This error message bypasses the localisation system. Welsh users would see English text here. Use a key from the
langobject instead.Proposed fix
Add to
en.js:errorGeneric: "An error occurred while removing content. Please try again later."Add to
cy.js:errorGeneric: "Digwyddodd gwall wrth dynnu cynnwys. Rhowch gynnig arall yn nes ymlaen."Then update the code:
- return renderConfirmationPage(res, sessionData, lang, locale, [ - { - text: "An error occurred while removing content. Please try again later.", - href: "#" - } - ]); + return renderConfirmationPage(res, sessionData, lang, locale, [ + { + text: lang.errorGeneric, + href: "#" + } + ]);As per coding guidelines: "Provide both
enandcylanguage objects in page controllers for English and Welsh support".
🧹 Nitpick comments (12)
libs/admin-pages/src/pages/remove-list-confirmation/index.ts (2)
130-138: Audit metadata setup looks reasonable.The location lookup and metadata assignment are correctly placed after the successful deletion and session save. One minor inconsistency: line 131 uses
Number()whilst line 33 usesNumber.parseInt(). Consider standardising.Consistency suggestion
- const location = await getLocationById(Number(sessionData.locationId)); + const location = await getLocationById(Number.parseInt(sessionData.locationId, 10));
143-143: Verify error object doesn't leak sensitive data.The
errorobject is logged directly. If it contains request context or user data, this could violate the guideline against sensitive data in logs. Consider logging onlyerror.messageor a sanitised representation.Safer logging
- console.error("Error deleting artefacts:", error); + console.error("Error deleting artefacts:", error instanceof Error ? error.message : "Unknown error");Based on learnings: "Do not include sensitive data in logs".
libs/system-admin-pages/src/audit-log/service.ts (2)
5-22: Module ordering differs from guidelines.Interfaces are placed at the top, but guidelines specify interfaces and types should be at the bottom of the module. Consider relocating these after the exported functions.
As per coding guidelines: "Module ordering: constants at the top, exported functions next, other functions in order of use, interfaces and types at the bottom"
24-33: Consider timezone handling for audit timestamps.The formatting uses the server's local timezone. If audit logs are viewed across different regions or need consistent UTC timestamps, consider using
toISOString()or explicit timezone formatting.libs/admin-pages/src/pages/manual-upload-summary/index.ts (1)
15-23: Centralise the Request auditMetadata type augmentation.The
express-serve-static-coremodule augmentation forauditMetadatais duplicated across at least four files (remove-list-confirmation, non-strategic-upload-summary, manual-upload-summary, and user-profile), creating unnecessary maintenance overhead. Move this to a shared typings file to ensure a single source of truth.libs/admin-pages/src/pages/non-strategic-upload-summary/index.ts (2)
15-24: Duplicate module augmentation.This
Requestinterface extension is duplicated inlibs/system-admin-pages/src/audit-log/middleware.ts. Consider centralising this declaration in a shared types file (e.g.,@hmcts/typesor a dedicatedaudit-types.ts) and importing it where needed to avoid drift between definitions.
148-157: Redundant list type lookup.
listTypeat line 150 duplicates the lookup already performed at line 124 (selectedListType). Reuse the existing variable to avoid the redundant iteration.♻️ Suggested fix
// Get location and list type for audit log const location = await getLocationById(Number(uploadData.locationId)); - const listType = mockListTypes.find((lt) => lt.id === listTypeId); // Set audit log flag req.auditMetadata = { shouldLog: true, action: "NON_STRATEGIC_UPLOAD", - entityInfo: `Court: ${location?.name || uploadData.locationId}, List Type: ${listType?.englishFriendlyName || listTypeId}, File: ${uploadData.fileName}` + entityInfo: `Court: ${location?.name || uploadData.locationId}, List Type: ${selectedListType?.englishFriendlyName || listTypeId}, File: ${uploadData.fileName}` };libs/system-admin-pages/src/audit-log/middleware.ts (3)
117-117: Consider typing thesessionparameter.The
sessionparameter is typed asany. For better type safety, consider usingExpress.Sessionor a more specific session type from the Express types.♻️ Suggested fix
-function determineRedirectOutcome(redirectUrl: string, requestPath: string, session?: any): "success" | "validation_error" | "cancelled" | "other" { +function determineRedirectOutcome(redirectUrl: string, requestPath: string, session?: Record<string, unknown>): "success" | "validation_error" | "cancelled" | "other" {
25-29: UseUSER_ROLESconstant for role comparison.For consistency with other files and to avoid potential drift, import and use
USER_ROLES.SYSTEM_ADMINfrom@hmcts/authinstead of the string literal.♻️ Suggested fix
-import type {} from "@hmcts/auth"; +import { USER_ROLES } from "@hmcts/auth"; import type { NextFunction, Request, Response } from "express"; import { logAction } from "./logger.js";// Only log if user is authenticated and has system admin role const user = req.user; - if (!user || user.role !== "SYSTEM_ADMIN") { + if (!user || user.role !== USER_ROLES.SYSTEM_ADMIN) { return next(); }
225-229: Avoidanytype in error mapping.The
anycast at line 227 bypasses type checking. Consider defining an error type or using a type guard.♻️ Suggested fix
if (errorField) { const sessionErrors = req.session[errorField]; - const errorMessages = sessionErrors.map((e: any) => (typeof e === "object" && e.text ? e.text : String(e))).join("; "); + const errorMessages = sessionErrors.map((e: unknown) => (typeof e === "object" && e !== null && "text" in e ? (e as { text: string }).text : String(e))).join("; "); details.push(`Errors: ${errorMessages}`); }libs/system-admin-pages/src/audit-log/middleware.test.ts (2)
14-14: Use typed mock forlogAction.Consider using Vitest's
MockedFunctiontype for better type safety instead ofany.♻️ Suggested fix
+import type { MockedFunction } from "vitest"; +import type { logAction as LogActionType } from "./logger.js"; ... - let logAction: any; + let logAction: MockedFunction<typeof LogActionType>;
557-592: Remove unusedoriginalRendervariables.The
originalRendervariable is declared but never used in these three test cases.♻️ Suggested fix
it("should handle render with callback", async () => { - const originalRender = mockResponse.render; - const middleware = auditLogMiddleware(); await middleware(mockRequest as Request, mockResponse as Response, mockNext); const callback = vi.fn(); (mockResponse.render as any)("test-view", {}, callback); // No audit log should be created for renders without errors expect(logAction).not.toHaveBeenCalled(); }); it("should handle render with options only", async () => { - const originalRender = mockResponse.render; - const middleware = auditLogMiddleware(); await middleware(mockRequest as Request, mockResponse as Response, mockNext); (mockResponse.render as any)("test-view", { data: "test" }); // No audit log should be created for renders without errors expect(logAction).not.toHaveBeenCalled(); }); it("should handle render with view only", async () => { - const originalRender = mockResponse.render; - const middleware = auditLogMiddleware(); await middleware(mockRequest as Request, mockResponse as Response, mockNext); (mockResponse.render as any)("test-view"); // No audit log should be created for renders without errors expect(logAction).not.toHaveBeenCalled(); });
| declare module "express-serve-static-core" { | ||
| interface Request { | ||
| auditMetadata?: { | ||
| shouldLog?: boolean; | ||
| action?: string; | ||
| entityInfo?: string; | ||
| [key: string]: string | number | boolean | undefined; | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -n "auditMetadata" --type tsRepository: hmcts/cath-service
Length of output: 3393
🏁 Script executed:
cat -n libs/system-admin-pages/src/audit-log/middleware.ts | head -30Repository: hmcts/cath-service
Length of output: 1149
🏁 Script executed:
head -25 libs/admin-pages/src/pages/remove-list-confirmation/index.tsRepository: hmcts/cath-service
Length of output: 887
🏁 Script executed:
head -25 libs/admin-pages/src/pages/non-strategic-upload-summary/index.tsRepository: hmcts/cath-service
Length of output: 1076
🏁 Script executed:
head -25 libs/admin-pages/src/pages/manual-upload-summary/index.tsRepository: hmcts/cath-service
Length of output: 1052
🏁 Script executed:
# Check if `@hmcts/web-core` has types or a type definition location
fd -t f "\.d\.ts$|types\." libs/web-core/src/ | head -20Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Check if there's a shared types location or index file in web-core
find libs/web-core -name "index.ts" -o -name "types.ts" -o -name "types" -type d | head -20Repository: hmcts/cath-service
Length of output: 369
🏁 Script executed:
ls -la libs/web-core/src/types/Repository: hmcts/cath-service
Length of output: 229
🏁 Script executed:
cat -n libs/web-core/src/types/express.tsRepository: hmcts/cath-service
Length of output: 280
🏁 Script executed:
# Check if the web-core types are exported from the main index
cat -n libs/web-core/src/index.ts | head -50Repository: hmcts/cath-service
Length of output: 2363
Move auditMetadata interface augmentation to a shared location.
The module augmentation is duplicated across three page files in libs/admin-pages:
remove-list-confirmation/index.tsnon-strategic-upload-summary/index.tsmanual-upload-summary/index.ts
Add the auditMetadata declaration to libs/web-core/src/types/express.ts alongside the existing fileUploadError augmentation. This location already handles Express Request extensions and is imported globally by web-core's index, making it available to all packages that depend on @hmcts/web-core (including admin-pages).
| export async function getAuditLogs(filters: AuditLogFilters = {}, page = 1, pageSize = 20): Promise<PaginatedAuditLogs> { | ||
| const logs = await repository.findAll(filters, page, pageSize); | ||
| const totalCount = await repository.countByFilters(filters); | ||
| const totalPages = Math.ceil(totalCount / pageSize); | ||
|
|
||
| return { | ||
| logs: logs.map(formatAuditLog), | ||
| totalCount, | ||
| currentPage: page, | ||
| pageSize, | ||
| totalPages | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add validation for pagination parameters.
page and pageSize are not validated. If pageSize is 0, Math.ceil(totalCount / pageSize) causes division by zero returning Infinity. Negative values could also cause unexpected behaviour in the repository layer.
🛠️ Suggested fix
export async function getAuditLogs(filters: AuditLogFilters = {}, page = 1, pageSize = 20): Promise<PaginatedAuditLogs> {
+ const validPage = Math.max(1, page);
+ const validPageSize = Math.max(1, Math.min(pageSize, 100));
+
- const logs = await repository.findAll(filters, page, pageSize);
+ const logs = await repository.findAll(filters, validPage, validPageSize);
const totalCount = await repository.countByFilters(filters);
- const totalPages = Math.ceil(totalCount / pageSize);
+ const totalPages = Math.ceil(totalCount / validPageSize);
return {
logs: logs.map(formatAuditLog),
totalCount,
- currentPage: page,
- pageSize,
+ currentPage: validPage,
+ pageSize: validPageSize,
totalPages
};
}As per coding guidelines: "Input validation must be performed on all endpoints"
📝 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 async function getAuditLogs(filters: AuditLogFilters = {}, page = 1, pageSize = 20): Promise<PaginatedAuditLogs> { | |
| const logs = await repository.findAll(filters, page, pageSize); | |
| const totalCount = await repository.countByFilters(filters); | |
| const totalPages = Math.ceil(totalCount / pageSize); | |
| return { | |
| logs: logs.map(formatAuditLog), | |
| totalCount, | |
| currentPage: page, | |
| pageSize, | |
| totalPages | |
| }; | |
| } | |
| export async function getAuditLogs(filters: AuditLogFilters = {}, page = 1, pageSize = 20): Promise<PaginatedAuditLogs> { | |
| const validPage = Math.max(1, page); | |
| const validPageSize = Math.max(1, Math.min(pageSize, 100)); | |
| const logs = await repository.findAll(filters, validPage, validPageSize); | |
| const totalCount = await repository.countByFilters(filters); | |
| const totalPages = Math.ceil(totalCount / validPageSize); | |
| return { | |
| logs: logs.map(formatAuditLog), | |
| totalCount, | |
| currentPage: validPage, | |
| pageSize: validPageSize, | |
| totalPages | |
| }; | |
| } |
| export function validateUserId(userId: string): boolean { | ||
| // Early length check for security | ||
| if (userId.length > 50) { | ||
| return false; | ||
| } | ||
|
|
||
| // Alphanumeric validation with bounded quantifier | ||
| const alphanumericRegex = /^[a-zA-Z0-9]{1,50}$/; | ||
| return alphanumericRegex.test(userId); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for userId patterns to understand actual format
rg "userId\s*[:=]|new UUID|uuid" --type ts --type tsx -B2 -A2 | head -100Repository: hmcts/cath-service
Length of output: 89
🏁 Script executed:
# Look for test files that might show userId examples
fd "\.test\.|\.spec\." --type f | xargs rg "userId" -l | head -10Repository: hmcts/cath-service
Length of output: 656
🏁 Script executed:
# Check for any user-related services or models
fd "user|auth" --type f -name "*.ts" | head -20Repository: hmcts/cath-service
Length of output: 292
🏁 Script executed:
# Read the service test file
cat libs/system-admin-pages/src/audit-log/service.test.tsRepository: hmcts/cath-service
Length of output: 11303
🏁 Script executed:
# Search for userId patterns more broadly
rg "userId" libs/system-admin-pages/src/audit-log/ -A2 -B2Repository: hmcts/cath-service
Length of output: 15080
🏁 Script executed:
# Look for user/auth related files
fd "user|auth" --type f libs/ -name "*.ts" | head -20Repository: hmcts/cath-service
Length of output: 292
🏁 Script executed:
# Search for UUID patterns in the codebase
rg "uuid|UUID|[a-f0-9]{8}-[a-f0-9]{4}" --type ts -l | head -20Repository: hmcts/cath-service
Length of output: 1069
🏁 Script executed:
# Check middleware.ts to see where userId comes from
cat libs/system-admin-pages/src/audit-log/middleware.ts | head -100Repository: hmcts/cath-service
Length of output: 3624
🏁 Script executed:
# Search for user object assignments across the codebase
rg "user\.id|user\s*=" --type ts -B3 -A1 | head -150Repository: hmcts/cath-service
Length of output: 10871
🏁 Script executed:
# Look for authentication/session handling
fd "session|auth|user" --type f libs/ -name "*.ts" | grep -E "(session|auth)" | head -15Repository: hmcts/cath-service
Length of output: 292
🏁 Script executed:
# Check the GraphUser interface definition
rg "interface GraphUser" --type ts -A10Repository: hmcts/cath-service
Length of output: 626
🏁 Script executed:
# Look at how user.id is used in actual auth flow
cat libs/auth/src/graph-api/client.ts | head -80Repository: hmcts/cath-service
Length of output: 1669
🏁 Script executed:
# Check if there's any user ID formatting or transformation
rg "user\.id|userId.*=" --type ts -B2 -A2 | grep -E "(uuid|format|transform)" -iRepository: hmcts/cath-service
Length of output: 132
🏁 Script executed:
# Check if user.id is actually a UUID format (which contains hyphens)
rg "randomUUID\|uuid\|[a-f0-9]{8}-" --type ts | head -20Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Look for any Microsoft documentation or comments about the id field
rg "Azure|Graph|id.*uuid" --type ts -i -B2 -A2 | head -50Repository: hmcts/cath-service
Length of output: 3750
The alphanumeric-only constraint is too restrictive and will reject valid Azure AD user IDs.
User IDs from Azure AD Graph API are UUIDs containing hyphens (e.g. xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx). The current validation rejects hyphens, but test mock data consistently uses hyphenated userIds like "user-123". Update the regex to accept hyphens: /^[a-zA-Z0-9-]{1,50}$/ to align with actual userId format from the authentication system.
|
3 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-311
Change description
Creating audit log view functionality
Checklist
Summary by CodeRabbit
New Features
Audit Capture
Database
Tests
Documentation