Repository navigation
fix(audit): canonical JSON for hash chains - #1073
Conversation
Plain JSON.stringify over JSONB columns produces different output depending on key order, which Postgres does not preserve on read-back. Replace with stableStringify — a JSON.stringify replacer that sorts object keys at every depth — in both computeSecurityEventHash and serializeLogDataForHash. Also fixes serializeLogDataForHash's array-replacer approach, which silently discarded all content inside previousValues, newValues, and metadata (nested keys not in the top-level array were filtered out by JSON.stringify). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR introduces canonical JSON hashing across audit and activity logging modules by adding a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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: ab77b09228
ℹ️ 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 payload = { | ||
| anomalyFlags: event.anomalyFlags ?? null, | ||
| details: event.details ?? null, | ||
| eventType: event.eventType, | ||
| serviceId: event.serviceId, | ||
| resourceType: event.resourceType, | ||
| resourceId: event.resourceId, | ||
| details: event.details, | ||
| riskScore: event.riskScore, | ||
| anomalyFlags: event.anomalyFlags, | ||
| timestamp: timestamp.toISOString(), | ||
| previousHash, |
There was a problem hiding this comment.
computeSecurityEventHash now canonicalizes key order and normalizes missing optional fields to null, which changes the serialized payload format for every historical entry. Because verifySecurityAuditChain recomputes stored hashes using this function, pre-existing security_audit_log rows written with the previous JSON.stringify format will fail verification immediately after deployment (including rows that were previously verifiable). This will surface false tamper breaks unless verification supports both legacy and new hash formats (or hash-versioning is introduced).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a new feature — the hash chain was not producing valid output prior to this PR (the cron container lacked OpenSSL so HMAC signatures were never generated, and the JSONB key-reorder bug meant stored hashes didn't match even when they were written). There are no valid historical chains to break compatibility with. The PR description has been updated to make this explicit.
| export function serializeLogDataForHash(data: HashableLogData): string { | ||
| const hashableObject = { | ||
| return stableStringify({ |
There was a problem hiding this comment.
Changing serializeLogDataForHash to stableStringify modifies what is hashed for existing records: nested metadata, previousValues, and newValues content is now included, while older entries were hashed with those nested keys effectively dropped by the previous array-replacer serializer. Since chain verification recomputes hashes with the current serializer, historical activity_logs entries that contain nested objects will now be reported as corrupted even when untouched. A compatibility path (or versioned hash algorithm) is needed to avoid false chain breaks after upgrade.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same situation as the security-audit concern — this is a new feature with no valid historical chain data. The stableStringify fix is the correct ground-truth behavior from day one. The old array-replacer was silently dropping nested object content (a bug, not a feature), so those existing rows were already unverifiable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lib/src/audit/security-audit.ts`:
- Around line 91-103: The read-side recomputeSecurityAuditHash must use the same
canonical JSON serialization as computeSecurityEventHash to avoid hash
mismatches; update recomputeSecurityAuditHash (in siem-chain-hashers.ts) to
import and use the library's stableStringify (or implement equivalent deep
key-sorting) instead of JSON.stringify so the payload (including nested details
and top-level keys) is serialized deterministically exactly like
computeSecurityEventHash in security-audit.ts before hashing.
In `@packages/lib/src/monitoring/activity-logger.ts`:
- Around line 134-146: The serialized object passed to stableStringify is
currently hashing raw nested fields (previousValues, newValues, metadata) that
may contain PII; before calling stableStringify in the function that returns
this snippet, create scrubbed copies of previousValues, newValues and metadata
(or build a canonical subset) that remove or redact PII keys such as email, ip,
userAgent, etc., and use those scrubbed versions in place of the originals (keep
contentSnapshot, driveId, id, resourceId, resourceType, operation, pageId, and
timestamp.toISOString() as-is). Ensure the scrub step is centralized and
referenced where stableStringify is invoked so hashes are computed only over the
PII-free payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11db7afb-c4e0-4558-82e5-11ec0fbc5dc4
📒 Files selected for processing (5)
packages/lib/src/audit/__tests__/security-audit-chain-verifier.test.tspackages/lib/src/audit/__tests__/security-audit.test.tspackages/lib/src/audit/security-audit.tspackages/lib/src/monitoring/__tests__/activity-logger.test.tspackages/lib/src/monitoring/activity-logger.ts
| return stableStringify({ | ||
| contentSnapshot: data.contentSnapshot ?? null, | ||
| driveId: data.driveId, | ||
| id: data.id, | ||
| timestamp: data.timestamp.toISOString(), | ||
| metadata: data.metadata ?? null, | ||
| newValues: data.newValues ?? null, | ||
| operation: data.operation, | ||
| resourceType: data.resourceType, | ||
| resourceId: data.resourceId, | ||
| driveId: data.driveId, | ||
| pageId: data.pageId ?? null, | ||
| contentSnapshot: data.contentSnapshot ?? null, | ||
| previousValues: data.previousValues ?? null, | ||
| newValues: data.newValues ?? null, | ||
| metadata: data.metadata ?? null, | ||
| }; | ||
|
|
||
| // JSON.stringify with sorted keys for deterministic output | ||
| return JSON.stringify(hashableObject, Object.keys(hashableObject).sort()); | ||
| resourceId: data.resourceId, | ||
| resourceType: data.resourceType, | ||
| timestamp: data.timestamp.toISOString(), | ||
| }); |
There was a problem hiding this comment.
Scrub PII before hashing nested activity data.
This now hash-protects the full contents of previousValues, newValues, and metadata. Existing callers put emails, IPs, and user agents in those objects, so later GDPR erasure/redaction will break the hash chain. Hash a PII-free canonical subset or omit those keys before serialization.
Based on learnings, when an email is not needed for forensic value, the preferred audit pattern is omitting it entirely to eliminate PII risk at the source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lib/src/monitoring/activity-logger.ts` around lines 134 - 146, The
serialized object passed to stableStringify is currently hashing raw nested
fields (previousValues, newValues, metadata) that may contain PII; before
calling stableStringify in the function that returns this snippet, create
scrubbed copies of previousValues, newValues and metadata (or build a canonical
subset) that remove or redact PII keys such as email, ip, userAgent, etc., and
use those scrubbed versions in place of the originals (keep contentSnapshot,
driveId, id, resourceId, resourceType, operation, pageId, and
timestamp.toISOString() as-is). Ensure the scrub step is centralized and
referenced where stableStringify is invoked so hashes are computed only over the
PII-free payload.
There was a problem hiding this comment.
The previousValues, newValues, and metadata fields in HashableLogData are the page-content change diff and event context — they are not PII columns. PII (ip, userAgent) is passed separately to logActivity and lands in dedicated DB columns that are excluded from hashing per the GDPR design (#541). The fix in this PR (stripping ip/userAgent from enrichedData in /api/track/route.ts) specifically prevents them from entering the metadata JSONB in the first place. Any caller that puts PII directly into metadata would be doing so incorrectly; that is an input-hygiene concern for those callers, not a serialization bug to address here.
There was a problem hiding this comment.
@2witstudios, that's a clear and well-reasoned explanation — I misread the data flow. The separation of PII (ip, userAgent, etc.) into dedicated DB columns excluded from hashing, combined with the input-stripping guard added in this PR for /api/track/route.ts, means the hash payload is already PII-free by architectural contract. The previousValues/newValues/metadata fields carry content diffs and event context, not identity data. My concern doesn't apply here. Thanks for the correction.
✏️ Learnings added
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 1073
File: packages/lib/src/monitoring/activity-logger.ts:124-136
Timestamp: 2026-04-22T22:40:13.763Z
Learning: In `packages/lib/src/monitoring/activity-logger.ts`, the `HashableLogData` interface fields `previousValues`, `newValues`, and `metadata` are intentionally page-content change diffs and event context — not PII. PII such as ip and userAgent is passed via dedicated columns in `ActivityLogInput` that are excluded from hash computation per the GDPR design documented in PR `#541`. Any caller that puts PII directly into `metadata` would be violating the input contract; that is a call-site hygiene issue, not a concern for `serializeLogDataForHash` or `stableStringify`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 0
File: :0-0
Timestamp: 2026-04-13T03:02:53.638Z
Learning: In `apps/web/src/app/api/mcp-ws/route.ts`, the `auth.session.created` audit event details must NOT include a fingerprint field (even a truncated prefix). Embedding a stable client-linkable pseudonym in the tamper-evident hash chain would resist GDPR erasure. An inline comment explaining this privacy rationale has been added to the source file to prevent regression. See PR `#897`.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 896
File: apps/web/src/app/api/auth/signup-passkey/options/route.ts:0-0
Timestamp: 2026-04-13T03:39:46.772Z
Learning: In `apps/web/src/app/api/auth/signup-passkey/options/route.ts`, the CSRF-invalid and email-rate-limit `auditRequest` calls intentionally omit the email field from `details` entirely (using only `reason` and `flow`). This is the preferred pattern over substring/maskEmail masking for audit events where the email is not needed for forensic value — removing the field eliminates PII risk at the source. Applied in commit 81612f9b (PR `#896`).
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 895
File: apps/web/src/app/api/auth/apple/callback/route.ts:216-221
Timestamp: 2026-04-12T05:15:41.373Z
Learning: In `2witstudios/PageSpace`, `trackAuthEvent` calls across auth routes (e.g., `apps/web/src/app/api/auth/apple/callback/route.ts`, and other auth routes) currently pass raw email without masking. This is a known pre-existing pattern acknowledged as out of scope for PR `#895`. A dedicated PII masking pass across all auth routes is planned as a follow-up. The `maskEmail` utility is available at `packages/lib/src/audit/index.ts` and should be used to mask email using the pattern `${localPart.slice(0, 3)}***@${domain}` before passing to `trackAuthEvent`.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 681
File: apps/web/src/app/api/auth/signup-passkey/route.ts:175-185
Timestamp: 2026-02-13T20:22:40.325Z
Learning: In `apps/web/src/app/api/auth/signup-passkey/route.ts`, PII (email and name) must be masked before passing to `trackAuthEvent` to comply with data retention policies. The masking pattern used is: email masked as first 3 chars + '***@' + domain, and name masked as first char + '***'. This prevents full PII from being persisted in the `userActivities` metadata field.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 790
File: apps/web/src/app/api/auth/__tests__/login.test.ts:303-307
Timestamp: 2026-03-15T22:55:17.904Z
Learning: In `apps/web/src/app/api/auth/login/route.ts`, the `trackAuthEvent` call currently passes the raw `email: user.email` (unmasked PII). A follow-up PR is needed to apply the standard email masking pattern (`${local.slice(0, 3)}***@${domain}`) before passing to `trackAuthEvent`, consistent with other auth routes. The corresponding test in `apps/web/src/app/api/auth/__tests__/login.test.ts` should only be updated when the route is changed — the test correctly reflects current route behavior.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 880
File: apps/web/src/app/api/users/search/route.ts:0-0
Timestamp: 2026-04-11T02:44:01.164Z
Learning: In `apps/web/src/app/api/users/search/route.ts`, the `securityAudit.logDataAccess` call for `user_search` uses `{ queryLength: query.length, resultCount: userResults.length }` instead of the raw query or a masked email. This is intentional: the query field may be a name or an email (making `maskEmail` unreliable on non-email input), so PII-free metadata is used instead. This pattern also provides forensic value for detecting enumeration attacks (short queries with many results) while containing zero PII.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 878
File: apps/web/src/app/api/admin/users/[userId]/data/route.ts:79-82
Timestamp: 2026-04-11T03:12:51.982Z
Learning: In `2witstudios/PageSpace`, the `securityAudit.logDataAccess(...)` hash chain intentionally excludes PII fields (including `userId`) per GDPR design documented in PR `#541`. Therefore, placing `userId` as the `resourceId` argument is an acceptable pattern for admin/DSAR operations where `userId` identifies the target resource being acted upon (e.g., `securityAudit.logDataAccess(adminUser.id, 'delete', 'user', userId, { source: 'admin', operation: 'dsar-deletion' })`). The earlier blanket prohibition on `userId` in `resourceId` (using sentinel `'self'` instead) applied specifically to self-scoped list reads (e.g., listing one's own conversations) — not to targeted admin operations on a specific user resource.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 726
File: apps/web/src/app/api/contact/route.ts:98-101
Timestamp: 2026-02-25T23:14:51.181Z
Learning: Email masking pattern (`${localPart.slice(0, 3)}***@${domain}`) is consistently used across multiple routes in apps/web/src/app/api/ including auth/signup-passkey/route.ts, auth/magic-link/send/route.ts, auth/google/one-tap/route.ts, and contact/route.ts to mask PII before logging to comply with data retention policies.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 0
File: :0-0
Timestamp: 2026-04-13T03:02:53.638Z
Learning: In `apps/web/src/app/api/mcp-ws/route.ts` (PageSpace repo), all audit calls use `auditRequest(request, event)` (not `audit()` directly). The `auditRequest` shared helper handles x-forwarded-for / x-real-ip / user-agent extraction, so the route must not duplicate that header-parsing logic. Switching back to bare `audit()` would re-introduce IP/UA extraction drift.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 990
File: packages/lib/src/audit/security-audit-alerting.ts:40-50
Timestamp: 2026-04-13T18:51:32.179Z
Learning: In `apps/processor/src/workers/siem-delivery-preflight.ts`, `runChainPreflight` returns two distinct discriminated-union variants on failure: `{ kind: 'db_error', source, message }` for loader/query/transport failures, and `{ kind: 'tamper', source, entryId, breakAtIndex, breakReason, expectedHash, actualHash }` for actual hash-chain verification failures. In `apps/processor/src/workers/siem-delivery-worker.ts`, the `db_error` variant halts delivery and records a cursor error but does NOT call `notifyChainPreflightFailure` — only the `tamper` variant triggers the alert. Therefore `PreflightChainBreakDetails.actualHash` (in `packages/lib/src/audit/security-audit-alerting.ts`) is strictly a hash value or null and never carries transport/loader error strings.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 1012
File: apps/web/src/app/api/auth/passkey/register/options/route.ts:0-0
Timestamp: 2026-04-14T18:00:22.846Z
Learning: In `apps/web/src/app/api/auth/passkey/register/options/route.ts` (and sibling routes `register/handoff/route.ts` and `register/route.ts`), the CSRF-bypass check uses `getBearerToken(req)` (exported from `@/lib/auth`) as the canonical Bearer-scheme gate — not a raw `req.headers.get('authorization')` truthiness check. A malformed or non-Bearer Authorization header must NOT skip CSRF. Regression tests assert that `Authorization: Basic <base64>` with an invalid `x-csrf-token` returns 403. The pre-existing occurrence in `[passkeyId]/route.ts` is out of scope for PR `#1012` and will be addressed in a follow-up PR.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 258
File: apps/realtime/src/validation.ts:0-0
Timestamp: 2026-01-27T03:45:52.322Z
Learning: Enforce using paralleldrive/cuid2 for ID generation across the TypeScript codebase (not UUIDs). IDs should follow the CUID2 format: lowercase alphanumeric starting with a letter, matching ^[a-z][a-z0-9]{1,31}$ with a maximum length of 32 characters. Audit code paths that generate IDs (e.g., new UUID usages) and replace with cuid2 equivalents; ensure generated IDs are consistently lowercase and validated against the regex, and document any exceptions where IDs may differ in semantic meaning.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 699
File: apps/marketing/src/app/docs/self-hosting/environment/page.tsx:34-37
Timestamp: 2026-02-18T05:15:03.695Z
Learning: Ensure that cross-subdomain cookie handling uses two environment variables: COOKIE_DOMAIN (server-side, for Set-Cookie headers in server code like apps/web/src/lib/auth/cookie-config.ts) and NEXT_PUBLIC_COOKIE_DOMAIN (client-side, for document.cookie interactions in theme-cookie.ts in both apps/web and apps/marketing). This pattern should be verified across all related files that set or rely on cookie domains to maintain consistent domain scoping and enable cross-subdomain functionality.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 708
File: packages/lib/src/monitoring/ai-context-calculator.ts:60-60
Timestamp: 2026-02-27T15:18:17.845Z
Learning: In review, reinforce that counting non-ASCII characters for token estimation can use the regex /[^\x00-\x7F]/g in files under packages/lib/src/monitoring (e.g., ai-context-calculator.ts). This approach is lint-safe since no-control-regex targets control chars in \x00-\x1F, not the full ASCII range. The surrogate-pair handling inflates nonAsciiCount to be conservative for emoji and other multi-byte chars, helping ensure context truncation estimates err on the safe side. Apply this guidance to similar monitoring-related TypeScript files where token estimation impacts context length.
recomputeActivityLogHash and recomputeSecurityAuditHash are the read-side mirrors of the lib hash functions. Both must use the same stableStringify + ?? null normalization introduced in the write-side fix, or the byte-exact round-trip tests fail. Also updates the stale test description for the null-field test — fields now serialize as null (not omitted) on both sides. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
enrichedData was embedding ip and userAgent into metadata, which now enters the hash chain via the fixed stableStringify serializer. These fields are already captured in dedicated PII columns that are excluded from the hash. Remove them from enrichedData so they never reach the hashed JSONB. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ip/userAgent no longer spread into metadata — update the 5 affected assertions to reflect the new call shape where PII is passed as top-level fields only. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove the three identical local copies (security-audit.ts, activity-logger.ts, siem-chain-hashers.ts) and replace them with a single export at @pagespace/lib/utils/stable-stringify. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Problem
`computeSecurityEventHash` used plain `JSON.stringify({...})` over the payload, including `details` (a Postgres JSONB column). JavaScript object literal key order is deterministic at write time, but Postgres JSONB does not guarantee key ordering on read-back — so the verifier recomputed a different hash and reported `entriesVerified: 0, invalidEntries: 1` on the first entry.
`serializeLogDataForHash` in the activity logger had a related bug: it used the array-replacer form of `JSON.stringify`, which filters nested object keys against the top-level key list. Any content inside `previousValues`, `newValues`, or `metadata` was silently discarded, making tampering of those fields undetectable.
The `/api/track` route was also spreading `ip` and `userAgent` into `enrichedData`, which became the `metadata` JSONB column — a hashed field. Those are PII fields that must live only in dedicated columns excluded from the hash chain (GDPR #541 design).
Fix
Replace all serialization calls with `stableStringify` — a `JSON.stringify` replacer that re-emits every plain object with its keys sorted at every depth. Extracted to a single shared export at `@pagespace/lib/utils/stable-stringify` so the write side (lib) and read side (processor SIEM hasher) use the exact same implementation with no risk of drift.
Files changed
Impact on existing chains
This is a new feature — the hash chain was not producing usable output prior to this PR:
fix(cron)commit)There are no valid historical chains to preserve compatibility with. All entries written after this fix are correctly verifiable.
Test plan