fix(logs): redact RPC payload from PII-leaking attestor logs#76
Conversation
|
|
📝 WalkthroughWalkthroughBoth files have logging adjustments: one refactors RPC request logging to use structured fields instead of full payloads, the other reduces HTTP transcript logging to report only byte lengths instead of full base64 content. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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 docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/external-rpc/handle-incoming-msg.ts (1)
64-76: ⚡ Quick winThe missing-ID warning is unreachable with the current guard order.
if(!reqId || !reqType) returnprevents Line 74 from ever executing, so the warning never fires. Split the checks so missing IDs are still logged.Proposed diff
- if(!reqId || !reqType) { + if(!reqType) { return } @@ if(!reqId) { logger.warn({ type: req.type }, 'Window RPC request missing ID') return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/external-rpc/handle-incoming-msg.ts` around lines 64 - 76, The early return "if(!reqId || !reqType) return" makes the logger.warn for missing IDs unreachable; change the guard order so you first check reqType (return if missing), then call RPC_MSG_BRIDGE.dispatch(req), then ignore response messages (if req.isResponse return), and only after that check if(!reqId) and call logger.warn({ type: req.type }, 'Window RPC request missing ID') before returning — update the checks around reqId/reqType and the dispatch/isResponse logic in handle-incoming-msg.ts accordingly.src/providers/http/index.ts (1)
439-445: ⚡ Quick winAvoid base64-encoding full transcripts just to log sizes.
This still materializes full transcript content in memory (
encodeBase64(...)) before logging only length. Compute byte counts directly from chunks to reduce sensitive-data handling and memory overhead.Proposed diff
- const clientTranscript = encodeBase64(concatenateUint8Arrays(clientMsgs)) - const serverTranscript = encodeBase64(concatenateUint8Arrays(serverMsgs)) + const requestBytes = clientMsgs.reduce((n, chunk) => n + chunk.length, 0) + const responseBytes = serverMsgs.reduce((n, chunk) => n + chunk.length, 0) logger.debug({ - requestBytes: clientTranscript.length, - responseBytes: serverTranscript.length, + requestBytes, + responseBytes, }, 'http transcript captured')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/http/index.ts` around lines 439 - 445, The code builds full base64 transcripts via encodeBase64(concatenateUint8Arrays(clientMsgs/serverMsgs)) only to log lengths, which materializes sensitive data and wastes memory; instead compute total byte counts by summing the byteLength (or .length for raw Uint8Array chunks) of each element in clientMsgs and serverMsgs and pass those totals to logger.debug as requestBytes/responseBytes, removing the encodeBase64 and concatenateUint8Arrays calls in this logging path (change references around clientTranscript/serverTranscript to use the computed totals and keep logger.debug usage the same).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/external-rpc/handle-incoming-msg.ts`:
- Around line 64-76: The early return "if(!reqId || !reqType) return" makes the
logger.warn for missing IDs unreachable; change the guard order so you first
check reqType (return if missing), then call RPC_MSG_BRIDGE.dispatch(req), then
ignore response messages (if req.isResponse return), and only after that check
if(!reqId) and call logger.warn({ type: req.type }, 'Window RPC request missing
ID') before returning — update the checks around reqId/reqType and the
dispatch/isResponse logic in handle-incoming-msg.ts accordingly.
In `@src/providers/http/index.ts`:
- Around line 439-445: The code builds full base64 transcripts via
encodeBase64(concatenateUint8Arrays(clientMsgs/serverMsgs)) only to log lengths,
which materializes sensitive data and wastes memory; instead compute total byte
counts by summing the byteLength (or .length for raw Uint8Array chunks) of each
element in clientMsgs and serverMsgs and pass those totals to logger.debug as
requestBytes/responseBytes, removing the encodeBase64 and concatenateUint8Arrays
calls in this logging path (change references around
clientTranscript/serverTranscript to use the computed totals and keep
logger.debug usage the same).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0866e7b9-0f9f-4a8d-aaab-a9df9005c533
📒 Files selected for processing (2)
src/external-rpc/handle-incoming-msg.tssrc/providers/http/index.ts
|
Promptless prepared a documentation update related to this change. Triggered by PR #76 Added a changelog entry documenting the security enhancement that sanitizes attestor logs to prevent PII leakage by logging only request metadata (ID, type, module, channel) and transcript byte lengths instead of full RPC payloads and HTTP transcript contents. Review: Add changelog entry for attestor-core PII log redaction |
3 participants
Description
Testing (ignore for documentation update)
Type of change
Checklist:
Additional Notes:
Summary by CodeRabbit