✨ feat: Audit log UI for SystemGrants changes#52
Conversation
dustinhealy
force-pushed
the
feat/audit-log-ui
branch
from
ddaa9a8 to
8eb02c6
Compare
Wire the audit-log tab into the grants page, switch the server function from a stub to a real /api/admin/audit-log call with filter query params, generate the CSV client-side from already-fetched entries, and add unit coverage for the audit log utilities.
…er validation Defang CSV formula injection (CWE-1236) with leading-quote escape for cells beginning with =/+/-/@/tab/CR, switch to CRLF line endings, prepend UTF-8 BOM, and emit localized headers via a new auditLogToCsv(entries, localize) signature. Migrate the audit log tab UI to click-ui: ButtonGroup for the action filter, DatePicker for date inputs, Button for export, Badge with state="success"/"danger"/"neutral" for action and principal-type pills (fixes the failing 4.5:1 contrast on the prior badge-success class). Fix the focus-loss bug where the search input unmounted on every keystroke: drop the isLoading early-return, debounce search at 300ms, render LoadingState inline within the table body, and handle the isError case explicitly. Wire useAnnouncement + ScreenReaderAnnouncer so filter changes announce the result count to assistive tech; give SearchInput a proper aria-label; rename the entry-count plural keys to the i18next v25 _zero/_one/_other suffix convention; harden the CSV blob download for Safari/Firefox via appendChild plus a deferred URL.revokeObjectURL. Server-side: add a requireAnyCapability defense-in-depth guard, tighten the Zod schema with ISO date validation and a 200-char cap on search, parse the response body via Zod, bump staleTime to 60s, and add placeholderData: keepPreviousData so filter changes don't flash empty.
…wer, CSP, click-ui Server: paginated getAuditLogPageFn with cursor/limit + multi-action + facet params (actorId, targetPrincipalType, targetPrincipalId, capability), Zod-parsed response schema, auditLogInfiniteQueryOptions factory for useInfiniteQuery, exportAuditLogServerFn that proxies the backend CSV endpoint, all behind the same triple-capability defense-in-depth guard. UI: new AuditLogDetailDrawer (click-ui Flyout) renders the full entry with copyable IDs and before/after diff highlighted via Badge state. Local AuditLogEntryWithDiff type carries optional before/after arrays until the data-schemas package upstreams the fields. Parser: parseAuditSearch handles actor: / target: / capability: / created:>YYYY-MM-DD qualifiers with quoted multi-word values, falling back to free text for unknown keys. diffGrantState reports added/removed/unchanged sets. Click-ui migration: GrantTableRow and EditCapabilitiesDialog now use Badge state for status pills and the principal-type chip; deleted unused badge-success and badge-danger CSS classes from styles.css. GrantManagementTab keeps its raw table for now since click-ui Table does not support per-row tabIndex/role/onKeyDown/ref (documented inline). Security: Content-Security-Policy plus X-Content-Type-Options, Referrer-Policy, X-Frame-Options on every HTML response, with HSTS gated on production. Inline filter action wrapped in an array to match the new multi-action server schema (batch B will replace this filter UI entirely).
…arch, permalinks Replace useQuery with useInfiniteQuery against auditLogInfiniteQueryOptions so audit log pages on demand via cursor pagination — both a manual Load more button and an IntersectionObserver sentinel auto-load when the bottom row scrolls into view. The legacy single-shot getAuditLogFn and auditLogQueryOptions are gone. Multi-select action facet via click-ui CheckboxMultiSelect plus four faceted text/select filters (actor ID, target ID, target principal type, capability) collapsed behind a "More filters" disclosure with debounced inputs. Structured search runs the live input through parseAuditSearch on every debounce tick, extracts actor: / target: / capability: / created:>YYYY-MM-DD qualifiers, and renders each one as a dismissible Badge chip; clicking a chip regex-strips the corresponding token from the input. Qualifiers override the manual facet inputs when both are present. Row click and Enter/Space activation set ?entryId= on the route via TanStack Router; the matching entry opens in the AuditLogDetailDrawer with copy-permalink and Esc-to-close semantics. validateSearch on /_app/grants is extended so the param survives tab switches. Dual-mode CSV export: client-side auditLogToCsv for ≤500 loaded entries, server-side exportAuditLogServerFn for larger result sets or when more pages remain. Filter changes announce the result count via ScreenReaderAnnouncer, and Load More announces page-loaded count for assistive tech.
…r, dead-code purge Replace the cursor-based useInfiniteQuery with offset-based useQuery + placeholderData: keepPreviousData and the shared numbered Pagination component, matching the GroupsTab pattern; debounced filter setters reset the page in the same callback so search and pagination stay in sync. Drop the qualifier-parser and the disclosure-collapsed More-filters block; the four facet fields (Actor, Target, Target type, Capability) sit always-visible and partial-match against denormalized name fields on the backend. Top search box is plain regex-substring across actor, target, and capability. Replace click-ui Flyout with @radix-ui/react-dialog directly for the side panel so enter and exit animations actually play, driven by data-state keyframes added to styles.css. Every ID-like field in the drawer gets a CopyableMono button with per-button copied feedback. Each DatePicker renders a single tab stop and the shared danger-styled Clear button resets both date inputs together. Delete the unused AuditLogRow.tsx, the parseAuditSearch parser plus its types and tests, the dead ACTION_FILTER_LABELS and AUDIT_ACTION_FILTERS exports, the diffGrantState helper, and the locale keys left over from the load-more / qualifier-chip iteration. Net 383 lines deleted.
TanStack Start's SSR injects an inline `<script type="module">import("...")</script>`
into the root HTML to boot the client. The previous enforced policy of
`script-src 'self'` (no nonce, no `'unsafe-inline'`) would cause browsers to refuse
that inline script in production, breaking hydration before any UI rendered. Local
`bun run dev` never exercises `server.ts`, so the regression hid in plain sight.
Threading a per-request nonce through TanStack Start's manifest is non-trivial.
As an interim, the policy now ships as `Content-Security-Policy-Report-Only` so
violations still surface in browser devtools and reporting endpoints without
blocking hydration. Set `ADMIN_PANEL_CSP_ENFORCE=true` to flip back to enforcement
once the nonce wiring lands.
`useLocalize` returned a fresh closure on every render, so any effect that listed it in its deps array re-fired every render. In `AuditLogTab` that was the screen-reader announce effect, causing assistive tech to be spammed every time React reconciled the component. Wrapping the closure in `useCallback` keyed on `translate` keeps the function identity stable across renders while still picking up language changes.
The previous prefix regex `^[=+\-@\t\r]` missed payloads that lead with whitespace before the formula trigger (e.g. ` =SUM(...)`), payloads that start with `\n` or `|` (the latter is Excel's DDE invocation marker), and Unicode decoy characters such as NBSP and BOM that spreadsheets render as zero-width but JavaScript's `\s` does not always cover symmetrically. The defang now treats a value as dangerous if either its first character is a trigger or if the first character after stripping space/NBSP/BOM is a trigger; stripping the entire `\s` class would falsely accept payloads led by `\r` / `\n` / `\t`, which are themselves triggers. Local-day date helpers (`isoDateToDate`, `dateToIsoDate`, `localDayBoundaryIso`) also moved here so the timezone fix in `AuditLogTab` can be unit tested in isolation; new cases cover round-trips, rolled-over input rejection, and both start/end boundaries.
Each `getAuditLogPageFn` / `exportAuditLogServerFn` invocation previously did
two round-trips: `requireAnyCapability` would call `getEffectiveCapabilitiesFn`,
then the handler would call the audit-log endpoint. Pagination doubled the
backend traffic of the whole tab.
Handlers now fetch capabilities once via a new `guardAuditLogAccess` helper and
run `checkAnyCapability` against the in-memory list. `checkAnyCapability` is
exposed so future server functions can adopt the same pattern; `requireAnyCapability`
is implemented in terms of it to keep behaviour identical for unchanged callers.
Adds `getAuditLogEntryFn` and `auditLogEntryQueryOptions` so the UI can deep-link
to entries that aren't on the current page. The endpoint returns `{ entry: null }`
for 404 so callers can render an explicit "not found" state without crashing.
Four near-identical debounced-text-filter handlers in `AuditLogTab` collapsed into a single hook that owns the controlled value, the debounced commit value, and timer cleanup. The optional `onCommit` callback fires once per quiescent settle so callers can reset pagination or log analytics without re-rolling their own ref/`setTimeout` plumbing.
Drawer permalinks no longer silently fail for entries off the current page. When `?entryId=` points at a row that isn't in `pageEntries`, the tab falls back to `getAuditLogEntryFn` via React Query and renders the drawer from either the on-page row or the fetched record. A new not-found state in `AuditLogDetailDrawer` surfaces the case where the id is gone instead of leaving the drawer empty. CSV export now always hits the backend. The previous client/server split truncated CSVs whenever a result set had between 51 and 500 matching rows: the client path serialized at most one page (`AUDIT_LOG_PAGE_SIZE = 50`) but the threshold for switching to the server endpoint was 500. Pulling the client path keeps `auditLogToCsv` (and its tests) as the contract the server is expected to honor, and removes the now-unused `com_audit_export_client` translation key. Clipboard writes for the permalink button and the inline copyable cells now await the promise and only flip to the "Copied!" affordance on success. Permission-denied, HTTP-origin, and `navigator.clipboard === undefined` paths all surface via the existing `ScreenReaderAnnouncer` with a new `com_a11y_copy_failed` key. The permalink itself is now built from `window.location.origin` + the canonical `/grants?tab=audit-log&entryId=…` shape so copied links don't carry the current filter state. Filter pages now use `useDebouncedFilter` instead of four ad-hoc handlers. `DatePickerCell`'s `useEffect` no longer re-runs every render; the comment captures *why* the workaround exists so future readers don't strip it. Date filters now anchor at local-day boundaries (`localDayBoundaryIso`) instead of mixing UTC midnight with local-time picker values, fixing off-by-one filter results for any non-UTC user. `pageEntries` is memoized to avoid being a fresh array each render. Dead `com_audit_filter_*` translation keys from the qualifier-parser cleanup are removed.
The LibreChat backend already enforces ACCESS_ADMIN on every /api/admin/audit-log route, and any future tightening (e.g. a dedicated READ_AUDIT_LOG capability) belongs there. The BFF-layer guard was running an extra /effective round-trip on every page request without buying real protection, since the backend would reject the same callers we did. It was also inconsistent with GrantManagementTab, which sits on the same page and already calls getAllGrantsFn with no BFF guard. Removes guardAuditLogAccess, AUDIT_LOG_REQUIRED_CAPS, and the three call sites in getAuditLogPageFn / getAuditLogEntryFn / exportAuditLogServerFn. The checkAnyCapability helper extracted in eef26ce stays — it's still used by requireAnyCapability and is useful on its own.
The audit-log tab was visible to anyone who could reach the Grants page (i.e.
anyone with `ACCESS_ADMIN`). With the LibreChat backend now requiring
`READ_AUDIT_LOG` on `/api/admin/audit-log`, users without that grant will hit
a 403 if they click the tab — surfacing the right backend policy but a bad UX.
The tab trigger, panel slot, and body render are all gated on
`hasCapability('read:audit_log')` via the existing `useCapabilities` hook,
which reads from the cached effective-capabilities lookup the sidebar already
uses. A stale `?tab=audit-log` URL on a session that lost the cap silently
falls back to management rather than rendering an empty page.
`READ_AUDIT_LOG_CAPABILITY` lives in `@/constants` as a forward-compat string
constant until the `@librechat/data-schemas` dependency bumps to a version
that exports it from `SystemCapabilities`.
LC PR #13087 adds READ_AUDIT_LOG to @librechat/data-schemas, so the local forward-compat constant is just a string-literal detour. Removed READ_AUDIT_LOG_CAPABILITY and updated GrantsPage to read the cap from SystemCapabilities.READ_AUDIT_LOG. This commit will not typecheck against the currently-published data-schemas 0.0.48 (the cap does not exist there). That is the intended state until LC merges, data-schemas re-publishes, and this PR's package.json pin is bumped as a final commit. Until then, local verification via `bun link` against the LC checkout exercises the full path. Also adds the picker labels: com_cap_read_audit_log and com_cap_desc_read_audit_log so the System category renders the toggle and tooltip correctly once data-schemas publishes with the cap in CAPABILITY_CATEGORIES.
This PR depends on the READ_AUDIT_LOG capability added in danny-avila/LibreChat#13087. Pinning ahead of publication: the install will fail until the LC PR merges and data-schemas is republished, at which point bun install populates the lockfile and CI goes green.
Post-rebase fixup so house-style import ordering applies to a file main edited but the rebase didn't re-run sort-imports against.
dustinhealy
force-pushed
the
feat/audit-log-ui
branch
from
8eb02c6 to
5bfc287
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The npm-published @librechat/data-schemas@0.0.52 was built against a
librechat-data-provider that already exports RetentionMode, so the
previous ^0.8.502 pin (which resolves to 0.8.502, before RetentionMode
landed) breaks the dev server boot with:
[MISSING_EXPORT] "RetentionMode" is not exported by
node_modules/librechat-data-provider/dist/index.es.js
Bumping to ^0.8.503 (now on npm) restores the missing export and aligns
the admin panel with the version data-schemas@0.0.52 was built against.
CSV export now flows entirely through exportAuditLogServerFn against the
LibreChat backend, so the in-bundle helpers auditLogToCsv, escapeCsvCell,
hasFormulaPrefix, CSV_COLUMNS, and the _CsvColumnsExhaustive compile-time
check are dead code with no production callers. The corresponding
describe('auditLogToCsv', ...) block in auditLogUtils.test.ts and the eight
orphaned com_audit_csv_col_* locale keys go with them.
Formula-injection defang is unchanged in behavior because the equivalent
guarantee now lives on the backend CSV writer in packages/api with its
own regression coverage.
The post-filter live-region announcement was being passed pageEntries.length, which is capped at AUDIT_LOG_PAGE_SIZE (50). For a filter matching 200 rows, the announcement read "50 audit log entries match the current filters" while the visible bottom-of-page counter correctly read "200 entries" off the API total. Swap the announcement source to use total and update the effect's dependency array to track total instead of pageEntries.length.
The post-filter announcement effect kept isFetching in its dependency array, so every pagination click re-fired the same X-entries-match message to screen readers even though no filter had changed. Tracking a filterSignature in a ref now short-circuits the announcement unless the debounced filter inputs themselves have actually changed, leaving page navigation silent. handleExport had a try/finally with no catch, and the caller invokes it via () => void handleExport(), so a rejection from exportAuditLogServerFn went unhandled and the user saw the loading state vanish with no download and no error. A catch branch now announces a new com_a11y_audit_export_failed locale key, mirroring how copy paths use com_a11y_copy_failed when the clipboard write fails.
The wrapper's mount-only effect (empty deps) only ran once, so after the Clear button bumps dateResetNonce and the inner DatePicker remounts via its key prop, the freshly created input element retains its default tabIndex and the double-tab-stop the wrapper exists to prevent reappears. DatePickerCell now takes a resetKey prop that callers pass the same dateResetNonce they use to remount the inner DatePicker. Including it in the effect dep array re-runs the patch each time the inner input is replaced, keeping the keyboard tab order stable across clears.
The LibreChat audit-log backend (PR #13087) is now merged and published as @librechat/data-schemas@0.0.54, which reshapes the audit surface into a general-purpose event log. Update the UI to consume it. - Bump @librechat/data-schemas ^0.0.52 → ^0.0.54 and librechat-data-provider ^0.8.503 → ^0.8.506 (0.0.54 imports BASE_ONLY_CONFIG_SECTIONS from it). - BFF (server/capabilities.ts): rebuild the audit filter + entry + page zod schemas to the new shape — namespaced actions (grant.assigned/grant.removed), structured actor/target/integrity, metadata/context, category/outcome/severity /actorType/targetType facets, cursor, and nextCursor. Enums sourced from the data-schemas constant arrays so they can't drift. - AuditLogTab + AuditLogDetailDrawer: read the new nested fields (actor.name, target.name/type/id, metadata.capability via auditCapability helper), send targetType (was targetPrincipalType), and use the namespaced action values. - Collateral from the version bump (unrelated to audit, required to compile): role.ts adds the new SHARED_LINKS permission type; CapabilityPanel casts the capability key; config.ts normalizes parseImportedYaml's appConfig union for the changed AppService return type. tsc + eslint clean; auditLogUtils tests updated and green.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee45643ddf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| AUDIT_ACTIONS, | ||
| AUDIT_CATEGORIES, | ||
| AUDIT_OUTCOMES, | ||
| AUDIT_SEVERITIES, | ||
| AUDIT_ACTOR_TYPES, |
There was a problem hiding this comment.
/workspace/librechat-admin-panel/AGENTS.md warns that the main @librechat/data-schemas barrel pulls Node-only modules and must not be imported for runtime values in client-side code. This module is reached by browser components via @/server (AuditLogTab imports the audit query options), and these top-level audit constants are evaluated to build the validators, so the client bundle can pull in the Node-only barrel and break the Vite build. Keep these literals in a clean client-safe module or make the import server-only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f18f681 — moved the audit enum literals to a new client-safe src/constants/audit.ts and switched server/capabilities.ts to import them from @/constants. The only remaining @librechat/data-schemas reference there is an erased import type, so the Node-only barrel no longer reaches the client bundle (per AGENTS.md). The local mirrors carry satisfies readonly AuditAction[] (etc.) against type-only imports, so tsc fails if they drift from the published unions.
| Permissions.SHARE, | ||
| Permissions.SHARE_PUBLIC, | ||
| ], | ||
| [PermissionTypes.SHARED_LINKS]: [Permissions.CREATE, Permissions.SHARE, Permissions.SHARE_PUBLIC], |
There was a problem hiding this comment.
Adding SHARED_LINKS here updates defaultPermissions, but RolePermissionsPanel renders only the hard-coded PERMISSION_TYPE_ORDER in src/components/access/RolePermissionsPanel.tsx, and that list still omits PermissionTypes.SHARED_LINKS. In installs using the bumped provider, shared-link permissions are persisted/defaulted but never shown in the role editor, so admins cannot grant or revoke create/share/share_public for shared links; add this type to the order (and its labels) alongside the schema entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f18f681 — added PermissionTypes.SHARED_LINKS to PERMISSION_TYPE_ORDER in RolePermissionsPanel (after SKILLS) plus com_perm_type_SHARED_LINKS / com_perm_desc_SHARED_LINKS labels, so the role editor now renders the create/share/share_public toggles for shared links instead of silently defaulting them.
- R1 (P1): stop importing runtime values from the @librechat/data-schemas barrel in client-reachable code. server/capabilities.ts is reached by AuditLogTab via @/server, and the main barrel pulls Node-only modules (AGENTS.md). Move the audit enum literals to a client-safe @/constants/audit module (type-checked against the package via erased type-only imports + satisfies); capabilities.ts now sources them from @/constants. - R2 (P2): surface the new SHARED_LINKS permission type in the role editor. Adding it to PERMISSION_TYPE_SCHEMA defaulted/persisted it but RolePermissionsPanel's hard-coded PERMISSION_TYPE_ORDER omitted it, so admins couldn't grant/revoke it. Added it to the order plus com_perm_type/desc_SHARED_LINKS labels. tsc + eslint clean; audit utils tests green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f18f6818db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!response.ok) { | ||
| await extractApiError(response, 'Failed to export audit log'); | ||
| } | ||
| const csv = await response.text(); |
There was a problem hiding this comment.
When an export matches a large audit range, this server function reads the backend's streaming CSV response into one string and then serializes it back to the browser as JSON. That defeats the backend backpressure/cancellation work and can exhaust BFF memory or hit server-function payload limits before the download starts; proxy or expose a streamed download response instead of returning { csv }.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, but deferring as a focused follow-up rather than folding it into this migration. The buffering predates this PR's data-schemas migration and is bounded by the backend's MAX_AUDIT_EXPORT_ROWS cap (100k rows), so it can't grow unbounded. A true streamed download can't go through createServerFn (it serializes a JSON return value), so the fix is a new session-authenticated route in server.ts (alongside the existing /metrics handler) that validates READ_AUDIT_LOG and pipes the backend's /export.csv Response body straight to the browser — which is what honors the backend backpressure/cancellation end-to-end. That's a self-contained change to the export path; I'd rather land it as its own PR than mix a new authenticated route into the contract migration. Tracked for follow-up.
There was a problem hiding this comment.
Implemented in eed37d0 (no longer deferred). exportAuditLogServerFn now returns a Response that pipes the backend's export.csv body straight through instead of response.text() + JSON. A server fn that returns a Response is passed through verbatim, so the BFF no longer buffers the whole file or wraps it in a JSON payload — preserving the backend's streaming/backpressure and removing the server-fn payload-limit risk. (This TanStack Start version has no file-based server routes — createServerFileRoute doesn't exist — and dev/e2e run on vite, so a server.ts Bun route wouldn't be portable; the server-fn-returning-Response is the cross-env mechanism.) Auth + grant-scoping unchanged; the client turns the streamed Response into a Blob download. New src/server/auditLogExport.test.ts covers it.
| "input-otp": "^1.4.2", | ||
| "js-yaml": "^4.1.1", | ||
| "librechat-data-provider": "^0.8.502", | ||
| "librechat-data-provider": "^0.8.506", |
There was a problem hiding this comment.
This data-provider bump brings the MCP_SERVERS.CONFIGURE_OBO role permission, but PERMISSION_TYPE_SCHEMA still lists only USE/CREATE/SHARE/SHARE_PUBLIC for MCP_SERVERS. As a result, admins cannot grant this new permission from the panel, and using the MCP Servers “select all” toggle rewrites that section with only the four known keys, dropping CONFIGURE_OBO from roles that already had it seeded by config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa80c00 — added Permissions.CONFIGURE_OBO to the MCP_SERVERS entry in PERMISSION_TYPE_SCHEMA (matching the bumped data-provider's mcpServersPermissionsSchema) plus a com_perm_CONFIGURE_OBO label, so the role editor renders the toggle and 'select all' no longer drops a seeded CONFIGURE_OBO.
| const handleTabChange = (value: string) => { | ||
| if (isValidTab(value)) { | ||
| navigate({ search: { tab: value } }); | ||
| navigate({ search: (prev: Record<string, unknown>) => ({ ...prev, tab: value }) }); |
There was a problem hiding this comment.
When a user opens an audit entry and then switches to the Management tab, this preserves entryId in the URL while unmounting the drawer. Switching back to Audit Log immediately reopens that old entry, and copied/bookmarked Management URLs keep an irrelevant deep-link parameter; clear entryId when changing away from audit-log.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa80c00 — handleTabChange now drops entryId from the URL when switching to any non-audit-log tab, so it no longer reopens the old entry on return or lingers as a stale deep-link param in Management URLs.
| }); | ||
|
|
||
| export const getAuditLogEntryFn = createServerFn({ method: 'GET' }) | ||
| .inputValidator(z.object({ id: z.string().min(1).max(128) })) |
There was a problem hiding this comment.
A deep link can set entryId=export.csv (or another literal audit sub-route); because this validator accepts any non-empty string, the single-entry fetch proxies /api/admin/audit-log/export.csv instead of the backend /:id handler. Opening that URL can trigger a full CSV export through the detail query and then fail JSON parsing, so constrain id to the backend ObjectId shape before building the path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa80c00 — getAuditLogEntryFn's validator now requires the backend ObjectId shape (/^[a-f0-9]{24}$/i), so a crafted entryId=export.csv is rejected before any proxy instead of hitting the sibling /export.csv route. The client auditLogEntryQueryOptions also only enables the query for a well-formed id (defense in depth).
- R6 (P2, security): constrain the single-entry fetch id to the backend ObjectId shape. A crafted ?entryId=export.csv was proxied to a sibling audit-log sub-route; the BFF validator now rejects non-ObjectId ids and the client query only fires for a well-formed id. - R4 (P2): surface the MCP_SERVERS CONFIGURE_OBO permission (new in the bumped data-provider) in PERMISSION_TYPE_SCHEMA + a com_perm_CONFIGURE_OBO label, so admins can grant it and "select all" no longer drops it. - R5 (P3): clear the audit-entry deep link (entryId) when switching away from the audit-log tab, so it doesn't silently reopen or linger in Management URLs. tsc + eslint clean; audit utils tests green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa80c00a38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const withDefaults: AuditFilters = { limit: AUDIT_LOG_PAGE_SIZE, ...data }; | ||
| const response = await apiFetch(`/api/admin/audit-log${buildAuditLogQuery(withDefaults)}`); |
There was a problem hiding this comment.
The audit UI/parser only handles grant rows (adminAuditLogEntrySchema.action accepts only grant.assigned/grant.removed), but the default request here only adds limit and sends no category=grant when filters are empty. On any instance whose general /api/admin/audit-log contains auth/config/tool rows, the page fetch can parse a non-grant action and reject, leaving the Audit Log tab in the load-error state; the export path can also include unrelated audit rows. Add a grant category default before calling the general audit endpoint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5209a75 — both getAuditLogPageFn and exportAuditLogServerFn now force category=['grant'] before calling the general /api/admin/audit-log endpoint, so this grant-only UI never receives a non-grant row that the strict adminAuditLogEntrySchema.action enum would reject. Belt-and-suspenders with the existing action enum.
| "com_audit_filter_target_id": "Target", | ||
| "com_audit_filter_target_type": "Target type", | ||
| "com_audit_filter_capability": "Capability", | ||
| "com_audit_export_server": "Export all matching", |
There was a problem hiding this comment.
When the Grants e2e suite runs, the two audit-log tests still locate the CSV button with /export as csv/i (e2e/grants.spec.ts:150 and :183). This changed translation makes the rendered accessible name “Export all matching”, so those tests will time out even when the feature works; either preserve the old accessible name or update the tests in this commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5209a75 — updated both Grants e2e selectors (grants.spec.ts:151,183) to match the rendered button name 'Export all matching' instead of the stale 'export as csv'.
| void navigate({ | ||
| search: (prev: Record<string, unknown>) => { | ||
| const next = { ...prev }; | ||
| delete next.entryId; | ||
| return next; |
There was a problem hiding this comment.
For the supported direct permalink form /grants?entryId=... (where the route defaults a missing tab to audit-log), closing the drawer only removes entryId. With no tab left in the URL, GrantsRoute falls back to the Management tab, so users who close a shared audit-entry link are bounced out of the audit log instead of returning to the audit list; keep or set tab: 'audit-log' in this close path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5209a75 — the drawer close path now sets tab: 'audit-log' (in addition to clearing entryId), so closing a bare ?entryId= permalink returns to the audit list instead of falling back to Management.
- R7 (P1): scope audit queries to category=grant. The LibreChat audit endpoint
is now a general-purpose event log, but this UI/parser only handles grant rows
(adminAuditLogEntrySchema.action is grant.assigned/removed only). Force
category=['grant'] in both the page and export server fns so a non-grant event
can't reach the strict parser and error the whole tab.
- R9 (P3): preserve the audit-log tab when closing a bare ?entryId= permalink —
the close path now keeps tab=audit-log so users aren't bounced to Management.
- R8 (P2): align the Grants e2e selectors with the actual export button name
("Export all matching", was searching for "export as csv").
tsc + eslint clean; audit utils tests green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5209a757ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const csv = await response.text(); | ||
| return { csv }; |
There was a problem hiding this comment.
When an export matches a large audit history, this server function reads the backend's streamed CSV response fully into memory and then returns it inside a JSON server-function payload. That defeats the backend export endpoint's streaming/backpressure behavior and can time out or exhaust memory for large tenant logs before the browser receives a file; proxy the CSV through a streamed response/route instead of response.text().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same deliberate deferral as the earlier streaming-export thread: this buffering predates the migration and is bounded by the backend's MAX_AUDIT_EXPORT_ROWS cap. The proper fix (a session-authenticated streaming proxy route in server.ts, since createServerFn can't stream a JSON return) is tracked as a focused follow-up rather than mixed into this contract migration. Surfaced to the maintainer.
There was a problem hiding this comment.
Implemented in eed37d0 (no longer deferred). exportAuditLogServerFn now returns a Response that pipes the backend's export.csv body straight through instead of response.text() + JSON. A server fn that returns a Response is passed through verbatim, so the BFF no longer buffers the whole file or wraps it in a JSON payload — preserving the backend's streaming/backpressure and removing the server-fn payload-limit risk. (This TanStack Start version has no file-based server routes — createServerFileRoute doesn't exist — and dev/e2e run on vite, so a server.ts Bun route wouldn't be portable; the server-fn-returning-Response is the cross-env mechanism.) Auth + grant-scoping unchanged; the client turns the streamed Response into a Blob download. New src/server/auditLogExport.test.ts covers it.
| @@ -0,0 +1,553 @@ | |||
| import * as Dialog from '@radix-ui/react-dialog'; | |||
There was a problem hiding this comment.
This file imports @radix-ui/react-dialog directly, but the package is only present transitively via click-ui/overrides and is not declared in package.json. The repo documents pnpm as a supported package manager, and pnpm's default strict layout will not expose undeclared transitive dependencies at the app root, so builds that include this drawer can fail to resolve the module; add it as a direct dependency or use the existing click-ui dialog surface.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6993b7b — added @radix-ui/react-dialog to dependencies (it was previously only an overrides version pin, resolved transitively via click-ui). AuditLogDetailDrawer imports it at runtime, so declaring it keeps pnpm-strict / prod installs resolvable.
| const entryFetch = useQuery({ | ||
| ...auditLogEntryQueryOptions(entryId), | ||
| enabled: !!entryId && !entryOnPage, | ||
| }); |
There was a problem hiding this comment.
For a malformed deep link such as /grants?tab=audit-log&entryId=not-an-object-id, this override discards the enabled guard returned by auditLogEntryQueryOptions, which is the only client-side check that keeps invalid ids from reaching the server function. The request then runs and fails validation, showing a generic load-error drawer instead of treating the permalink as invalid/not found; combine this condition with the option's existing enabled value rather than replacing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6993b7b — the useQuery override now ANDs the id-validity guard (isAuditEntryId(entryId) && !entryOnPage) instead of replacing the option's enabled. Extracted a shared client-safe isAuditEntryId helper in @/constants/audit used by the query option, the component override, and (via the BFF) the entry validator, so a malformed ?entryId= can't reach the server fn.
- R11 (P2): declare @radix-ui/react-dialog as a direct dependency (it was only an overrides pin + transitive via click-ui). AuditLogDetailDrawer imports it at runtime, so pnpm strict / prod installs need it declared. - R12 (P3): keep the entry-id validity guard in AuditLogTab. The useQuery override was replacing the option's enabled, dropping the only client-side check that blocks a malformed ?entryId= from reaching the server fn. Extracted a shared isAuditEntryId helper in @/constants/audit (client-safe) used by both the query option and the component override (and the BFF reuses it too). tsc + eslint clean; audit utils tests green.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The export server function read the backend's streamed CSV fully into memory and returned it inside a JSON server-function payload, defeating the backend's streaming/backpressure and risking BFF memory / server-fn payload limits on large exports (capped at MAX_AUDIT_EXPORT_ROWS = 100k). This TanStack Start version has no file-based server routes (no createServerFileRoute) and dev/e2e run on vite, so a Bun route in server.ts wouldn't be portable. Instead, exportAuditLogServerFn now returns a `Response` that pipes the backend's `export.csv` body straight through (a server fn that returns a Response is passed through verbatim). The BFF no longer buffers the whole file or wraps it in JSON, preserving backend streaming/backpressure; auth + grant-scoping are unchanged (requireCapability + category=grant). The client turns the streamed Response into a Blob download. Tests: new src/server/auditLogExport.test.ts covers the authorized streaming path (text/csv attachment, grant-scoped backend call, body piped through) and the unauthorized rejection. tsc + eslint clean.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eed37d04d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const entryNotFound = | ||
| !!entryId && !entryOnPage && entryFetch.isSuccess && entryFetch.data?.entry === null; |
There was a problem hiding this comment.
When the URL changes to a malformed entryId after a valid drawer has been opened, the fetch is disabled by the isAuditEntryId guard, so entryNotFound, loading, and loadError all remain false. AuditLogDetailDrawer then keeps its latched latestEntry and can display the previous audit record under the new bad permalink; treat invalid IDs as not-found or otherwise clear/close the drawer state to avoid showing stale forensic details.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f645250 — a malformed entryId (fetch disabled by the validity guard) now sets entryNotFound, so the drawer's not-found branch clears the latched entry instead of showing the previously-opened record under the bad permalink.
| <Badge | ||
| size="sm" | ||
| state="neutral" | ||
| text={ |
There was a problem hiding this comment.
The existing Playwright test dialog shows principal type badge opens an Admin role row and asserts that the dialog contains .badge-role. This replacement no longer renders principalConfig.badgeClass anywhere in the dialog, so that test fails even though the dialog itself opens; either preserve a stable role/group/user hook here or update the test to assert the new accessible badge content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f645250 — restored principalConfig.badgeClass on the dialog's click-ui Badge via className (the refactor had dropped it, leaving the .badge-role/.badge-group/.badge-user CSS dead and breaking the e2e hook). Note: the rows assertion at grants.spec.ts:47 targets a per-row type badge the management table doesn't render (it shows Name/Capabilities/Status; type is the filter buttons) — that's a separate pre-existing stale assertion, flagging it for a follow-up rather than adding a row badge here.
- R13 (P2): treat a malformed ?entryId= as not-found. The isAuditEntryId guard disables the fetch, leaving entryNotFound/loading/loadError all false, so the drawer latched a previously-opened entry under the bad permalink. entryNotFound is now true for an invalid id, so the drawer clears instead of showing stale forensic details. - R14 (P2): restore the principal-type badge hook on EditCapabilitiesDialog. The badge refactor dropped principalConfig.badgeClass, leaving the .badge-role/ group/user CSS dead and breaking the e2e badge assertion; re-applied it via the click-ui Badge className. tsc + eslint clean.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Resolve conflicts after main advanced 9 commits: - package.json: keep data-schemas ^0.0.54 / data-provider ^0.8.506 (ship the audit-log backend) + @radix-ui/react-dialog; bun.lock regenerated via bun install. - config.ts: take main's parseImportedYaml return (drop AppService call, return result.data) — main's tsdown type-collision fix supersedes the incidental re-cast on this branch. - EditCapabilitiesDialog.tsx: take main's version (Badge→span + cn, notify toasts); preserves the ['auditLog'] invalidation and obsoletes the R14 badgeClass band-aid. - translation.json: union of both key sets; defer to main's wording for SHARED_LINKS/CONFIGURE_OBO; add com_config_field_pinned and com_config_field_promptCacheTtl for the 0.8.505→0.8.506 config fields. Validated: tsc --noEmit clean, eslint --max-warnings 0 clean, full vitest suite 727/727.
librechat-data-provider@0.8.506's ESM build (dist/index.mjs) imports `dayjs/plugin/utc` / `dayjs/plugin/timezone` without a file extension. dayjs@1.11 ships no `exports` map, so under strict Node ESM (vitest's externalized deps) that bare specifier fails to resolve (ERR_MODULE_NOT_FOUND), taking down 9 test suites that transitively import data-provider. 0.8.505 imports only the dayjs root, so it's unaffected — and it still exports every symbol @librechat/data-schemas @0.0.54 re-imports (BASE_ONLY_CONFIG_SECTIONS, skillSyncConfigSchema, RetentionMode, PrincipalModel, …), so the audit-log types are intact. Pinned exactly (not ^) because ^0.8.505 would re-resolve to the broken 0.8.506. Dropped com_config_field_pinned/_promptCacheTtl (0.8.506-only config fields). Matches main's data-provider version. Verified: tsc clean, eslint --max-warnings 0 clean, vitest 727/727 natively (no resolver workaround).
0.8.507 fixes the ESM dayjs-plugin resolution bug that forced the exact 0.8.505 pin (LibreChat #13851). Back to a caret range now that the broken 0.8.506 is below the floor. Re-adds com_config_field_pinned / _promptCacheTtl (config fields present in 0.8.50x but not 0.8.505). Verified: tsc clean, eslint --max-warnings 0 clean, vitest 727/727 natively (no dayjs resolver workaround needed).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbf1f7a6c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| announce(localize('com_a11y_copy_failed')); | ||
| return false; | ||
| } | ||
| const url = `${window.location.origin}/grants?tab=audit-log&entryId=${encodeURIComponent(id)}`; |
There was a problem hiding this comment.
When the admin panel is deployed under a configured VITE_BASE_PATH such as /adminpanel, this constructs copied audit-log permalinks as https://host/grants?..., dropping the base path that the router/server expect. Those links will land outside the mounted app (404 or the wrong route) for every subpath deployment; build the URL with the configured base path or the current routed pathname before appending the search params.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 20ddddd — handleCopyPermalink now builds the URL via a pure buildEntryPermalink(id, origin, basePath) helper that prefixes the normalized VITE_BASE_PATH (same pattern as the __root.tsx favicon href), so permalinks resolve under subpath deployments like /adminpanel. Added unit tests for the root, base-path, trailing-slash, and id-encoding cases.
handleCopyPermalink built `${origin}/grants?...`, dropping a configured
VITE_BASE_PATH (e.g. /adminpanel) so links 404 on subpath deployments.
Extract a pure buildEntryPermalink(id, origin, basePath) helper that
prefixes the normalized base path (matching the __root.tsx favicon
precedent) and unit-test it. Addresses Codex P2.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Sibling PR: danny-avila/LibreChat#13087
Adds an Audit Log tab on the Grants page (
/grants?tab=audit-log) that surfaces every SystemGrant assign and revoke event from the LibreChat admin backend. The tab provides faceted filters (action chips, date range, actor name, target name, target type, capability), debounced search across all denormalized name fields, offset-based pagination using the shared<Pagination>component (50 entries per page), and a side panel with copy-to-clipboard buttons for every ID-like field, an optional before/after capability diff, and a permalink that copies a canonical?tab=audit-log&entryId=...URL.CSV export goes through the backend's streaming
/export.csvendpoint so result sets of any size are handled by a single code path. The client-sideauditLogToCsvserializer is kept and tested as the contract the server is expected to satisfy.The audit log tab is gated on
SystemCapabilities.READ_AUDIT_LOG. The tab trigger, content slot, and body render are hidden when the user does not hold that cap. A stale?tab=audit-logURL on a session without the cap silently falls back to the management tab.This PR depends on
@librechat/data-schemas@0.0.52(added by the sibling LibreChat PR), which exports theREAD_AUDIT_LOGcapability. The version pin inpackage.jsonhas been bumped accordingly.bun installwill fail in CI until that version publishes to npm; once it does, this PR can merge.Change Type
Testing
Local Vitest suite:
bun run testruns 176 unit tests including CSV serializer coverage (formula-injection defanging with leading whitespace, NBSP, and BOM stripping plus\nand|triggers in addition to=+-@\t\r, BOM, CRLF, non-ASCII round-trip, empty entries, RFC 4180 quoting),localDayBoundaryIsoround-trip for the date filter, anduseDebouncedFilterbehavior.bun run sort-importsclean (181 files), ESLint clean, TypeScript clean against a locally-linked@librechat/data-schemasfrom the sibling LC branch.Manual:
/grants?tab=audit-logrenders the empty state when no entries existstaleTimecom_a11y_copy_failed${origin}/grants?tab=audit-log&entryId=...URL, not the current filter-laden href?entryId=<id>opens the side panel on cold load even when the entry is not on the current page (a single-entry fetch viagetAuditLogEntryFnpopulates the drawer). When the entry does not exist, the drawer renders a "not found" empty state instead of staying emptyACCESS_ADMINbut withoutREAD_AUDIT_LOGdoes not see the audit log tab; navigating directly to?tab=audit-logfalls back to the management tabTest Configuration
/api/admin/audit-log(see parallel PR)@librechat/data-schemas@0.0.52(published by the sibling PR) forSystemCapabilities.READ_AUDIT_LOGChecklist
Note
Medium Risk
Touches admin authorization (
READ_AUDIT_LOGBFF gate) and ships CSP on HTML responses (report-only unlessADMIN_PANEL_CSP_ENFORCE); depends on unpublished LibreChat audit-log APIs until the sibling backend PR lands.Overview
Ships a read audit log experience on Grants: the tab is back behind
read:audit_log, with BFF calls to/api/admin/audit-log(paginated list, single entry, CSV export) replacing stubs, and@librechat/data-schemas/librechat-data-providerbumped to 0.0.52 / 0.8.503.Audit log UI is rebuilt: multi-select action filters, click-ui date pickers with local day boundaries, debounced actor/target/capability filters, 50-row pagination, clickable rows, and an
AuditLogDetailDrawer(loading / not-found / error shells, before/after diff, permalink + copy). Deep links use?tab=audit-log&entryId=...; export always hits the backend so large result sets are not truncated.Supporting changes:
READ_AUDIT_LOGand category shim until upstream publishes the constant;useDebouncedFilter; grant tables swap custom badge classes for click-uiBadge; HTML responses get CSP (report-only by default), HSTS, frame denial, and related headers inserver.ts; drawer/overlay CSS and i18n for audit/a11y strings.Reviewed by Cursor Bugbot for commit 1b4442f. Bugbot is set up for automated code reviews on this repo. Configure here.