Repository navigation
fix(pds-core): redact request_uri from CSS middleware log - #80
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes add Coveralls coverage thresholds and replace a logged request URI value with a boolean presence indicator in client CSS injection error handling. ChangesCoverage configuration
Error logging metadata
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🚅 Deployed to the pr-915b6a-80 environment in ePDS
|
Coverage Report for CI Build 31029709896Coverage remained the same at 58.051%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
5991f75 to
0dfb6cd
Compare
|
There was a problem hiding this comment.
Pull request overview
This PR tightens log hygiene in pds-core by ensuring the CSS injection middleware no longer logs opaque PAR request_uri values, while also adding a Coveralls configuration to enforce coverage-related CI thresholds.
Changes:
- Redact raw PAR
request_urifrom the CSS middleware warning log (log only presence viahasRequestUri). - Add
.coveralls.ymlto configure Coveralls coverage thresholds for CI reporting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/pds-core/src/lib/client-css-injection.ts | Replaces logging of raw requestUri with a boolean presence flag in the error path. |
| .coveralls.yml | Introduces Coveralls thresholds for coverage decrease tolerance and failure gating. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.coveralls.yml:
- Around line 1-2: Remove the coverage_decreased_threshold and
coverage_threshold_for_failure entries from .coveralls.yml, leaving only
supported Coveralls configuration there. Configure both coverage thresholds
through the Coveralls repository settings or API so the intended coverage gate
is enforced.
🪄 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: 59479769-9c8a-4e7b-95eb-16d97dd157fd
📒 Files selected for processing (2)
.coveralls.ymlpackages/pds-core/src/lib/client-css-injection.ts
This comment has been minimized.
This comment has been minimized.
0dfb6cd to
06a30ac
Compare
|
(comment generated by Claude Opus 5 via Claude Code) Split as suggested. This PR is now only the security fix — one commit, one file, - { err, requestUri },
+ { err, hasRequestUri: true },The All four gates pass locally: |
The PR #80 preview environment was serving a demo build predating epds_handle_login_url in client-metadata.json, so the sign-in page rendered no .btn-atproto and 71 of 83 e2e scenarios failed on a change that only alters one log field in pds-core. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
(comment generated by Claude Opus 5 via Claude Code) The e2e failure is a stale preview environment, not this change. 71 of 83 scenarios failed on a diff that alters one log field in Traced it rather than just re-running:
The
I pushed an empty commit to force a redeploy. Railway reported the demo deploy as successful, but the served metadata is unchanged, so it appears to be reusing a cached build for this PR's environment rather than rebuilding from the rebased branch. This needs someone with Railway access to delete or force-rebuild the The change itself is sound: |
1ff0d30 to
404ec0e
Compare
request_uri is an opaque PAR handle that should not be logged verbatim. Log only its presence for debugging. Addresses CodeRabbit review on PR #48. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
404ec0e to
c4fbbb0
Compare
|
Closing and immediately reopening to retrigger the Railway preview deploy — no code change intended. The #41 had the same signature (its e2e aborted because a preview service never came up) and went green after a close/reopen provisioned a fresh Railway environment. This branch is older still, so its preview has most likely drifted the same way. Also worth noting: this branch is 3 commits behind One overlap to flag — #241 now redacts this same log line as part of splitting #148. It uses Reopening momentarily. |
|
request_uri is a short-lived bearer reference to the PAR entry, so a log line carrying its value is replayable by anyone who can read the logs. Log whether it was present instead. Raised independently by CodeRabbit and Copilot on #241. The value was already being logged before this series; hoisting the resolution out of the request_uri guard promoted the line from warn to error, which made it more visible without making it safe. #80 fixes the same leak on the pre-refactor code. typeof rather than a hardcoded true: the log now sits outside the `if (requestUri)` guard, so presence is no longer implied by reaching it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
request_uri is a short-lived bearer reference to the PAR entry, so a log line carrying its value is replayable by anyone who can read the logs. Log whether it was present instead. Raised independently by CodeRabbit and Copilot on #241. The value was already being logged before this series; hoisting the resolution out of the request_uri guard promoted the line from warn to error, which made it more visible without making it safe. #80 fixes the same leak on the pre-refactor code. typeof rather than a hardcoded true: the log now sits outside the `if (requestUri)` guard, so presence is no longer implied by reaching it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
request_urivalues in the CSS injection middleware's warn log. Log only presence (hasRequestUri: true) since the URI is an opaque short-lived handle.Addresses CodeRabbit review comment on PR #48: #48 (comment)
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores