Repository navigation
Lock the reviewer's email address so revocation can't be bypassed - #3239
Conversation
…mbership out of revocation's reach An email change moves a user to another identity. A holder of the reviewer code could move the reviewer's membership to an address they control, where rotating the code and saas:reviewer_sign_in:sign_out can't reach it. The reviewer's users now refuse an email change, both the request and a confirmation link minted earlier. Sign-out also deletes pending session transfers and resets the join codes of the reviewer's accounts, the other ways a code holder could keep a way back in.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsFinding details are still loading. Check the individual review comments. ℹ️ 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. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
A code holder who redeemed a join code during a review has a user of their own in the demo account, and rotation doesn't remove it. Jeremy chose to check the members after each review rather than restrict the reviewer's role or deactivate users automatically, so saas:reviewer_sign_in:sign_out now prints every active member of the reviewer's accounts for that check.
|
@codex review |
![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: 65bb7f0a4f
ℹ️ 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".
…escaped Deactivating a user clears its identity, so a reviewer deactivated from the app would drop its account out of sign_out's join-code reset and member list. The app now refuses to deactivate the reviewer's users, whoever asks; operators use a console. The audit prints names and emails with inspect, so a member's display name can't inject newlines or terminal control sequences into the check.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
jeremy
changed the title
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive revocation retains an accepted dependency on manual membership auditing that warrants final human validation before enablement.
No open issues 🎉
What changed in this PR
This PR hardens the SaaS-only reviewer sign-in introduced in #3227 before enablement, keeping the reviewer’s membership reachable by the revocation task.
Changes:
- Blocks reviewer email changes and deactivation through the app.
- Extends sign-out to delete pending session transfers, reset account join codes, and print an escaped member list for manual review.
- Adds model and controller tests for the restrictions and cleanup behavior.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
saas/test/models/user/reviewer_email_address_lock_test.rb |
Tests email locking and continued revocation reach. |
saas/test/models/identity/reviewer_sign_in_test.rb |
Tests transfer cleanup, join-code rotation, and member listing. |
saas/test/controllers/users/reviewer_membership_lock_test.rb |
Tests reviewer deactivation restrictions. |
saas/test/controllers/users/reviewer_email_address_lock_test.rb |
Tests email-change request and confirmation rejection. |
saas/lib/tasks/reviewer_sign_in.rake |
Prints escaped member details for the post-review audit. |
saas/lib/fizzy/saas/engine.rb |
Installs the SaaS reviewer guards. |
saas/app/models/user/reviewer_email_address_lock.rb |
Prevents reviewer email confirmation delivery and identity moves. |
saas/app/models/identity/reviewer_sign_in.rb |
Expands revocation cleanup and exposes member auditing. |
saas/app/controllers/concerns/users/reviewer_membership_lock.rb |
Rejects reviewer deactivation through the controller. |
saas/app/controllers/concerns/users/reviewer_email_address_lock.rb |
Rejects reviewer email-change requests in HTML and JSON. |
Review effort: Balanced
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This fixes a revocation bypass in the reviewer sign-in from #3227, which is merged but not lit. Land it before the light-up.
The bypass
The finding was raised by Codex on #3192: thread. With a normal session from the reviewer code, a code holder can request an email change to an address they control and confirm it. An email change doesn't rename an identity.
User#change_email_addressmoves the user to another identity, found or created for the new address. The demo account's membership then belongs to the code holder's own identity, which signs in by emailed code like anyone else.saas:reviewer_sign_in:sign_outcleans the configured identity, which no longer holds anything, so the code holder keeps the account after rotation.The fix: lock the address, don't chase it
User::ReviewerEmailAddressLock, prepended ontoUserin the SaaS engine. For the reviewer's users,send_email_address_change_confirmationsends nothing andchange_email_addressreturnsfalse. That second guard also refuses a confirmation link minted earlier, andUsers::EmailAddresses::ConfirmationsControlleralready answersfalsewith its invalid-token response.Users::ReviewerEmailAddressLock, included intoUsers::EmailAddressesController. It refusesnewandcreateup front: a redirect with "This email address can't be changed", or403as JSON. The API path is refused too.Identity#email_address; the only one is this move. So recording the identity's id at generate time would add a config value and a production lookup, and protect against nothing that's still possible.The same class, closed in the same pass
I asked what else could leave a code holder a way back in after rotation, beyond the email instance:
Identity::Transfertokens live for 4 hours and mint a session. Sign-out now deletes them.Account::JoinCodesController#showhas no admin check), and redeeming it adds a user under an identity of the code holder's own. Sign-out now resets the join codes of the reviewer's accounts, so a code read during a review stops working at rotation.Decided by Jeremy (2026-10-24): (a). A code holder who redeemed the join code during a review has their own user in the demo account, and rotation doesn't remove it. So the members are checked after each review:
saas:reviewer_sign_in:sign_outnow also prints every active member of the reviewer's accounts (Identity::ReviewerSignIn.members_of_reviewer_accounts, 65bb7f0). Deactivate anyone there who isn't the reviewer or the account's own staff.Rotation after each review: the operational steps live in the internal reviewer runbook, not here. Sign-out's member list is the step that catches anyone who joined during the review.
Tests
saas/test/models/user/reviewer_email_address_lock_test.rbholds the test the finding asked for:Other coverage:
saas/test/controllers/users/reviewer_email_address_lock_test.rb:422, and the user doesn't move.saas/test/models/identity/reviewer_sign_in_test.rb:Mutation checks:
The existing email-change tests pass unchanged.
Review round on 65bb7f0, and where this control stops
Codex raised three findings on 65bb7f0:
Account::JoinCode#redeem_iffix would belong on main.These are the fourth and fifth ways back in that we've found, after the email change and the join code. Each new one fixes another path by which a code holder keeps a membership in the demo account. The protected thing is a seeded demo account with no customer data. The code reaches nothing else, because the identity belongs only to that account. The controls here are bounded to the paths that move or add a membership:
Accepted residual: a determined code holder may keep access to the demo account until someone notices them in the member list. A further finding of the same kind should be weighed against that residual, not patched by reflex.