Add pino logger with redaction for Stellar secret keys (issue #442)#482
Conversation
|
@KCEE0901 is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@KCEE0901 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Warning Review limit reached
More reviews will be available in 55 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR implements structured logging with automatic redaction of Stellar secret keys. It adds pino as a logger with both field-path masking and regex pattern detection, integrates it into request logging, and documents the redaction behavior in the security policy. ChangesLogging and Secret Redaction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/logger.test.ts`:
- Around line 3-18: Tests only exercise redactObject directly but don't assert
the logger actually emits redacted lines nor check the seed field; update the
"logger redaction" test to call the real logging path (use the module's logger
or logging function that wraps redactObject) and capture emitted log output
(e.g., spy or capture console/process logs) then assert the logged message
string contains "[REDACTED]" for each secret occurrence; also add an explicit
assertion that a "seed" field (e.g., obj.seed or similar key) is redacted in
both the object result from redactObject and in the emitted log output. Ensure
you reference redactObject and the exported logger/logging function used by the
code under test when adding the integration assertion.
In `@backend/src/middleware/requestLogger.ts`:
- Line 35: The current logger.info call in requestLogger.ts embeds
req.originalUrl into the free-form message which can leak sensitive query data;
change the call that references logger.info(logEntry, `${req.method}
${req.originalUrl} ${res.statusCode} - ${duration}ms | id=${requestId}`) so the
message text is static (e.g., "Request completed") and move dynamic values into
structured fields instead, using req.path (not req.originalUrl) for the URL, and
include method, res.statusCode, duration and requestId as separate properties on
the log entry so sensitive query params are not written into the free-form
message.
🪄 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: 1fc1dee2-a070-4c9e-9f88-9ffc02368a1b
📒 Files selected for processing (5)
SECURITY.mdbackend/package.jsonbackend/src/logger.test.tsbackend/src/logger.tsbackend/src/middleware/requestLogger.ts
| describe("logger redaction", () => { | ||
| test("redacts stellar secret keys in objects and strings", () => { | ||
| const secret = "S" + "A".repeat(55); | ||
| const obj = { | ||
| nested: { secretKey: secret, other: "ok" }, | ||
| token: secret, | ||
| arr: [secret, { privateKey: secret }], | ||
| } as const; | ||
|
|
||
| const redacted = redactObject(obj as any); | ||
|
|
||
| expect(redacted.nested.secretKey).toBe("[REDACTED]"); | ||
| expect(redacted.token).toBe("[REDACTED]"); | ||
| expect(redacted.arr[0]).toBe("[REDACTED]"); | ||
| expect(redacted.arr[1].privateKey).toBe("[REDACTED]"); | ||
| }); |
There was a problem hiding this comment.
Add an integration-style assertion against actual logger output.
Current coverage only tests redactObject; it does not verify emitted log lines are redacted, which is part of the acceptance criteria. Also add an explicit seed field assertion to cover all required paths.
Suggested test expansion
import { redactObject } from "./logger";
+import { logger } from "./logger";
describe("logger redaction", () => {
test("redacts stellar secret keys in objects and strings", () => {
@@
- const obj = {
+ const obj = {
nested: { secretKey: secret, other: "ok" },
+ wallet: { seed: secret },
token: secret,
arr: [secret, { privateKey: secret }],
} as const;
@@
expect(redacted.nested.secretKey).toBe("[REDACTED]");
+ expect(redacted.wallet.seed).toBe("[REDACTED]");
expect(redacted.token).toBe("[REDACTED]");
expect(redacted.arr[0]).toBe("[REDACTED]");
expect(redacted.arr[1].privateKey).toBe("[REDACTED]");
});
+
+ test("does not emit raw stellar secrets in logger output", () => {
+ const secret = "S" + "A".repeat(55);
+ const spy = vi.spyOn(process.stdout, "write").mockImplementation(() => true);
+
+ logger.info({ secretKey: secret, token: secret }, "test log");
+
+ const written = spy.mock.calls.map(([chunk]) => String(chunk)).join("");
+ expect(written).not.toContain(secret);
+ expect(written).toContain("[REDACTED]");
+ spy.mockRestore();
+ });
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/logger.test.ts` around lines 3 - 18, Tests only exercise
redactObject directly but don't assert the logger actually emits redacted lines
nor check the seed field; update the "logger redaction" test to call the real
logging path (use the module's logger or logging function that wraps
redactObject) and capture emitted log output (e.g., spy or capture
console/process logs) then assert the logged message string contains
"[REDACTED]" for each secret occurrence; also add an explicit assertion that a
"seed" field (e.g., obj.seed or similar key) is redacted in both the object
result from redactObject and in the emitted log output. Ensure you reference
redactObject and the exported logger/logging function used by the code under
test when adding the integration assertion.
| ); | ||
| } | ||
| // ✅ STEP 4: Use structured logger with redaction | ||
| logger.info(logEntry, `${req.method} ${req.originalUrl} ${res.statusCode} - ${duration}ms | id=${requestId}`); |
There was a problem hiding this comment.
Avoid embedding raw URL data in the log message string.
Line 35 duplicates req.originalUrl into free-form text, which can leak sensitive query values. Keep message static and rely on structured fields (and preferably log req.path instead of full originalUrl when possible).
Safer logging pattern
- logger.info(logEntry, `${req.method} ${req.originalUrl} ${res.statusCode} - ${duration}ms | id=${requestId}`);
+ logger.info(logEntry, "request_completed");📝 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.
| logger.info(logEntry, `${req.method} ${req.originalUrl} ${res.statusCode} - ${duration}ms | id=${requestId}`); | |
| logger.info(logEntry, "request_completed"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/middleware/requestLogger.ts` at line 35, The current logger.info
call in requestLogger.ts embeds req.originalUrl into the free-form message which
can leak sensitive query data; change the call that references
logger.info(logEntry, `${req.method} ${req.originalUrl} ${res.statusCode} -
${duration}ms | id=${requestId}`) so the message text is static (e.g., "Request
completed") and move dynamic values into structured fields instead, using
req.path (not req.originalUrl) for the URL, and include method, res.statusCode,
duration and requestId as separate properties on the log entry so sensitive
query params are not written into the free-form message.
2 participants
Closes #442
Checklist
Summary by CodeRabbit
Documentation
New Features
Tests