Repository navigation
fix(id): validate friend_remove request IDs - #4368
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 5 minutes. View limit details
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. |
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d58d2326e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if body | ||
| .requests | ||
| .iter() | ||
| .any(|request| !is_friend_request_id(request)) | ||
| { |
There was a problem hiding this comment.
If a remote peer sends a correctly signed friend request with an ID such as r1, Directory::receive still accepts it: Envelope::verify checks the signature but not the ID format, and acceptance preserves that ID in friend_requests. Subsequently, the normal RemoveFriend action calls remove_friend, which includes the stored ID in its artifact; this new check rejects it with Forbidden, leaving the friendship intact. Validate IDs at envelope admission as well, and handle already-persisted noncanonical IDs so these friendships remain removable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disposition (task 5756): valid finding, not fixed in PR #4368 by scope. Issue #3146 is Phase 1 of design #3231 and is scoped to friend_remove requests validation in parsed(); validating incoming friend-request IDs at envelope admission (Directory::receive / Envelope::verify) plus migration handling for already-persisted noncanonical IDs is a separate receipt surface in the same design family. It is recorded for the Phase 3 field-by-field receipt-validation audit, issue #3232, with a comment carrying this finding and the r1 example, so the audit dispositions it explicitly rather than it being expanded into this Phase 1 PR. PR #4368 remains merge-blocked only on its pending checks, not on this thread.
Summary
Verification
rustfmt --check --edition 2024 pkgs/id/src/artifact.rs pkgs/id/src/directory.rs— pass.git diff --check— pass.cargo test --manifest-path pkgs/id/Cargo.toml --no-default-features --lib friend_removal— laptop run in progress; GitHub Actions is the required CI verification.Fixes #3146
Part of #3231.
Need help on this PR? Tag
@codesmith-botwith what you need.