Repository navigation
Conversation
Fail early when an HTTPS mbus username contains a decoded colon, since Basic Auth treats it as the username/password separator. Return a credential-free error before creating an unusable password verifier. Add a regression test covering a percent-encoded colon in the username. ai-assisted=yes TNZ-129825
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description check The description explains the change, motivation, context, affected users, and validation performed. It does not explicitly provide release notes, breaking-change status, team tags, or AI review feedba… Title check The title clearly and concisely describes the main change: rejecting colons in HTTP agent usernames before hashing. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the change, motivation, context, affected users, and validation performed. It does not explicitly provide release notes, breaking-change status, team tags, or AI review feedback status, but the core information is complete.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
There was a problem hiding this comment.
🟢 Approval recommended
The narrow validation matches Basic Auth constraints and has targeted regression coverage, with no unresolved findings.
No open issues 🎉
What changed in this PR
Adds defensive validation to HashURL so HTTPS usernames incompatible with Basic Auth are rejected before password hashing.
Changes:
- Rejects colons in decoded usernames with a credential-free error.
- Adds regression coverage for a percent-encoded colon and an empty result on failure.
| File | Description |
|---|---|
| agentpassword/agentpassword.go | Validates decoded usernames before hashing. |
| agentpassword/agentpassword_test.go | Tests rejection and the credential-free error. |
Review effort: Balanced
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-project-automation
Bot
moved this from Inbox
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
aramprice
moved this from Pending Merge | Prioritized
to Pending Review | Discussion
in Foundational Infrastructure Working Group
A percent-encoded colon in an HTTPS mbus username currently passes
HashURL, but the agent splits Basic Auth credentials at the first colon and cannot authenticate that username. Reject the decoded username before generating a verifier, with an error that contains no credentials.Adds a regression test for
user%3Aextra. This is defensive validation for custom manifests; normalvcapusernames are unaffected. This follow-up to #487 is independent of cloudfoundry/bosh-cli#742 and is not a prerequisite for merging that PR.Validation:
go test ./agentpassword/... ./mbus/... ./app/...golangci-lint run ./agentpassword/...(0 issues)git diff --checkTNZ-129825