Repository navigation
Conversation
OAUTH_STAFF_ONLY=true limits OAuth to Identity#staff?, whichever client asks. The pilot and acceptance switches choose clients; this one chooses people. - Consent: anyone else gets a page saying OAuth is staff only, and no code. Declining still works. - Code redemption and refresh refuse a grant whose owner isn't staff, even one issued while they were. - An OAuth token, delegated ones included, stops authenticating once its owner isn't staff, so the delegation exchange refuses it too. - Personal access tokens aren't OAuth, so they're untouched, and Connected Apps and revocation stay open so anyone can shed a grant. Production, staging and the betas set it, so OAuth lands staff-gated everywhere until we open it up.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)
ℹ️ 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: a8bdf4869b
ℹ️ 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".
There was a problem hiding this comment.
🔵 Needs a closer look
OAuth push delivery remains outside the staff gate, and rollout depends on a held draft stack requiring human confirmation.
1 open issue
What changed in this PR
Adds a staff-only OAuth pilot restriction alongside existing client availability controls, while preserving personal access tokens and grant management.
Changes:
- Gates consent, code redemption, refresh, and OAuth token use by staff eligibility.
- Enables the restriction in production, staging, and beta.
- Adds coverage for staff access, loss of staff status, delegation, and personal-token exemptions.
[!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 |
|---|---|
| test/test_helpers/oauth_availability_test_helper.rb | Adds staff-only configuration overrides and restoration. |
| test/models/oauth/availability_test.rb | Tests identity admission and strict boolean handling. |
| test/integration/oauth_token_exchange_test.rb | Tests delegation after staff status is removed. |
| test/integration/oauth_staff_only_test.rb | Covers staff-only flows and personal-token exemptions. |
| saas/config/deploy.staging.yml | Enables staff-only OAuth in staging. |
| saas/config/deploy.production.yml | Enables staff-only OAuth in production. |
| saas/config/deploy.beta.yml | Enables staff-only OAuth in beta. |
| config/environments/production.rb | Reads the staff-only environment switch. |
| app/views/oauth/authorizations/staff_only.html.erb | Adds an explanatory access-denied page. |
| app/models/oauth/availability.rb | Defines and documents identity admission. |
| app/models/identity/access_token.rb | Checks staff eligibility when honoring OAuth tokens. |
| app/controllers/oauth/tokens_controller.rb | Gates code redemption and refresh by identity. |
| app/controllers/oauth/authorizations_controller.rb | Gates consent while allowing explicit declines. |
Review effort: Balanced
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: a8bdf4869b
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
…s staff only Push delivery weighs grants as a set rather than through a token's honored?, so it needs the person check too. Browser subscriptions, which no grant registered, keep delivering.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 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". |
There was a problem hiding this comment.
🔵 Needs a closer look
The stacked production authentication changes need human integration review, particularly where #3205 changes how personal access tokens are identified and exempted.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
🛡️ 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. |
…anch OAUTH_STAFF_ONLY=true limits OAuth to staff, and the production, staging and beta deploy configs set it. Resolved against #3205: personal access tokens now come from a first-party client, so the gate exempts them by personal? rather than by a missing client. A test covers a token issued that way.
|
Converged on a533581: CI is green, Codex and Codex Security report no issues, Copilot has no open issues, and all three threads (push delivery for former staff) are fixed and resolved. Copilot asked for care where #3205 changes how personal access tokens are identified. The integration merge (d506c83) handles that. With #3205, personal access tokens come from a first-party client, so the gate exempts them by |
* tp/oauth-token-exchange: Name v0.2.0 in the pin's comment Pin fizzy-mcp-server v0.2.0 Refuse a join code that was reset while it was being redeemed Filter presigned storage signatures from logs Keep the reviewer's memberships in reach, and print the member audit escaped Import an export that lacks a column this database has only just gained List the reviewer accounts' members when signing the reviewer out Bump thruster from 0.1.26 to 0.1.27 (#3199) Give the asset build a placeholder direct-upload credential Anchor the stubbed storage host pattern Sign direct uploads with a write-only storage credential Lock the reviewer's email address, so a code holder can't move its membership out of revocation's reach Use Ruby's GC defaults and log job memory (#3238)
…n in order * tp/oauth-consent-design: (34 commits) Say what a self-registered app claims to be, and that we haven't checked it Document OAUTH_DCR_ALLOWED_HTTPS_HOSTS for self-hosted installs Say which accounts a read grant's people come from, and read hosts in lowercase Pin that DCR refuses callbacks on hosts 37signals runs Leave a client_id list to the shared single-valued check Tell people about an unknown client on the error page, and switch only when the request can be resumed Hold metadata documents to per-person budgets and the strict address policy Offer "Not you?" on the consent screen, and call last activity what it is Require a host on app-scheme redirects, and http for loopback Accept self-registered redirects only on loopback, cursor:// and claude://, and allowlisted https hosts Name the metadata document hosts in the SaaS deploy env Name every remote host a connected app can send its access to Identify clients by their metadata document URL, from allowlisted hosts only Design the consent screen and Connected apps: who's asking, what it may do, where it goes Hand the issuer's tokens back before removing it, and run down only once no code knows it Remove the personal access issuer before handing its tokens back Pin both personal token forms to the train's dark delivery and replica heal Refuse to destroy the personal access issuer by its stored client_id Keep a read-only personal access token's write a 401, whichever form it takes Find the personal access issuer a concurrent first use provisioned ... # Conflicts: # app/models/oauth/availability.rb
* tp/oauth-consent-design: Bring the train's tests up to the consent design, and carry an audienced request through Not you? Report a personal access token as pat whether or not it has the personal access client
Since personal access tokens are issued by the first-party personal access client, they have an oauth_client_id, so "no client" no longer identifies them. The staff gate now asks personal? at authentication and at the token endpoint, and a test pins a staff-gated non-staff identity's personal access token working.
* tp/oauth-consent-design: Keep the metadata-document client's line on the redesigned consent screen Audit a personal access token from the personal access client as a pat Bring main's tests up to this branch: Ruby 4 and the access token finder
* tp/oauth-consent-design: Pin the load balancers to kamal-proxy v0.10.2, and say why configure-lb-mcp.sh still checks Give the load balancer the real client IP
* tp/oauth-consent-design: Report a refresh only once its rotation commits Set nothing up for an app disconnected while its request is in flight
* tp/oauth-consent-design: Answer a disconnected app's token from ApplicationController, clear of Active Storage's handlers
* tp/oauth-consent-design: Bring main's SQL redaction tests up to bearer lookup by find_honored Redact OAuth client statements from logged SQL, now that a client has a secret
* tp/oauth-consent-design: Redact retired refresh tokens from logged SQL
|
@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. |
|
Codex Review: Didn't find any major issues. Nice work! 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. |
* origin/oauth-consent-design: Answer a denial with access_denied whatever the stored row's scopes, and refund the fetch budget when admission is lost
|
🤖 @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8ba6733c8
ℹ️ 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".
🛡️ 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. |
…ad no identity for push while OAuth isn't staff only
* origin/oauth-consent-design: Answer a consent error as JSON when JSON is asked for, and group https loopback apps as on this device
|
🤖 @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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a62f789b13
ℹ️ 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".
…f consent as JSON when JSON is asked for
|
🤖 @codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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. |
Stacked on #3214.
OAUTH_STAFF_ONLY=truelimits OAuth to staff (Identity#staff?), whichever client asks.Oauth::Availability's acceptance, issuance and pilot switches choose clients; this one chooses people, and composes with them: a request has to pass both.This works like Basecamp's, where only staff can authorize an agent for now, and anyone else is told so, not shown a 404.
What's gated
invalid_grant, even for a code issued while they were staffinvalid_grant. The grant stays, for its owner to revoke.invalid_token, viaIdentity::AccessToken#honored?honored?Gating
honored?means someone who stops being staff loses what they connected on their next request. That's the only part that runs per request, and it reads the identity, which is already loaded to authenticate.Turning it on
OAUTH_STAFF_ONLY: "true"is set in the clear env of the production, staging and beta deploy configs, so OAuth lands staff-gated everywhere it might be lit. Opening it up means removing those lines. Anything but exactly"true"is off, and OSS installs never set it.Tests
test/integration/oauth_staff_only_test.rbcovers:The exchange test covers a de-staffed subject and its delegated token.
Oauth::AvailabilityTestcoversadmits?and exact-trueparsing.I removed each of the four gates in turn, and each removal fails at least one of these tests.
Stacking note
The personal-access-token exemption reads
oauth_client_id.nil?here, as C does. #3205 moves PATs onto a first-party client and addspersonal?. When the two meet, this check becomespersonal?, or every PAT would be gated.