Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The narrowly scoped logging changes preserve existing behavior, with no blocking issues identified.
Review effort: Balanced
Findings: None
What changed in this PR
Reduces sensitive data exposure in Signing Server’s central logs while preserving HTTP responses and process behavior.
Changes:
- Replaces exception details with fixed failure messages.
- Removes URLs, referrers, and user agents from access logs.
- Adds canary tests for log redaction and unchanged responses.
| File | Description |
|---|---|
| tests/unit/errorLogging.test.ts | Tests redaction and response compatibility. |
| src/utilities/logger.ts | Uses fixed process exception messages. |
| src/middleware/validation.ts | Redacts validation exception logs. |
| src/middleware/errorHandling.ts | Redacts route exception logs and updates documentation. |
| src/index.ts | Limits access logs to basic request metrics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
antuz123
left a comment
There was a problem hiding this comment.
Logging review at 7339939: I inspected the source log sinks and confirmed the access format excludes request URLs, referrers, and user agents; route, process, notification, cron, delayed-work, and database-configuration messages use fixed categories. Exact-head GitHub CI compiled and passed 112/112 tests (run 37626412363), and current-source plus fetched-history scan jobs passed (run 37626412300). A fresh local current-source Gitleaks check also found no leaks. The existing HTTP error response contract is intentionally preserved here; route-specific response hardening and mobile compatibility checks remain separate. This branch also needs reviewed dependency PR #1 before final contributor handoff, followed by private hosted log/configuration review. These checks do not establish what a deployed service logged, and this comment is not independent maintainer approval or deployment authorization.
R4 backend acceptance handoff — 12 October 2026State: BLOCKED_ON_STAGING_OPERATOR_ACCEPTANCE Source/compatibility evidence: Public Relay runtime pin ee935359f22ff7932ea0a62e9084d4b49801f4aa passed Node 22 compile, 105/105 baseline/local-mode and 104/104 Mongo 7 replica-set backup tests during this continuation. SigningServer #1 3cc7728, #2 7339939 and #5 7c14b6e merged without conflicts into isolated unpushed verification SHA 8de93e58358d0f9066c4ccda27fbeac474c9279d; Node 22.23.3 compile and 127/127 tests in 6 suites passed. All three source PRs and Relay #3 have Utkarsh's 9 October approvals on the pinned heads; hosted source/test checks on those unchanged heads are green. These are source/local-fixture results, not a deployed-service claim. Operator/staging acceptance still required:
Native Mac testing is not required to review backend source itself. The final app/Desktop Local handoff must separately record iOS/Android/physical signer round trips against explicitly identified compatible services. |
Raw route, validation, process, cron, notification and delayed-signing errors could write exception contents or user/configuration identifiers into logs. This records fixed failure categories and removes inheritance request IDs and database access identifiers from operational messages. Access logs retain method, status, response size and elapsed time.
HTTP statuses and the mobile { err } response contract stay compatible. README records the remaining public-error-contract and private hosted-log review boundaries.
Validation: Node 22 compile; 112/112 tests including route/validation canaries, six failed scheduled jobs and notification provider failures; current-source and fetched-history Gitleaks scans. This is a source-only logging change with no production deployment or credential rotation. Dependency remediation remains in PR #1.
Combined local validation (9 Oct 2026): isolated merge of #1 (
3cc7728c) and #2 (73399398) passed Node 22npm ci --ignore-scripts, compile and 125/125 tests across 6 suites. Disposable SES loopback serialization, Google Secret Manager provider mock and access-log canary passed; no external provider call or real credential was used. Combined runtime audit: 0 critical/high/moderate and 1 low AWS SDK v2 advisory; current-source Gitleaks passed. Hosted provider/log acceptance still requires non-production SES IAM/verified identity/region, GCP staging project and Secret Manager access, and private running artifact/log/config visibility.Cloud continuation (12 October 2026): this source head is unchanged and was approved by Utkarsh on 9 October. Exact-head GitHub source/test checks remain successful. An isolated unpushed integration of #1/#2/#5 at
8de93e58358d0f9066c4ccda27fbeac474c9279dpassed Node 22.23.3 compile and 127/127 tests across six suites. This is compatibility evidence only, not a deployment or production route/log/provider attestation. Mac-native testing is not needed for backend source review; staging/operator and app/hardware integration acceptance remain open.