Repository navigation
🤖 feat: disable the CoderMCPServer config when a credential Secret withdraws consent - #330
Conversation
The reconciler now reads the config on every reconcile and sets back any field that differs with one sparse PATCH. A PATCH that sets a URL or the auth type carries every credential of the auth type. Credentials of other auth types are cleared and checked on every apply (#314). The ACL sync also runs every reconcile, so a user grant added in the dashboard goes. A config deleted before its generation is applied is created again. Refs #155 Change-Id: Iecb93e07d2fe2d13630dd026827c948afa0c6a73 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…tion After a full apply, a dashboard disable, credential clear or auth type change now stops every write until the spec changes. An observed user_oidc auth type is always set back with a PATCH that holds only auth fields, so a disable stays. An in-memory SHA-256 of each sent credential skips the resend after a metadata-only Secret change. Refs #155 Change-Id: I0ded9edf8413c7dff9c7cb3aaf1186ccf2681188 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…thdraws consent When a credential read fails for a config that this object created, the controller checks whether every credential that Coder holds still comes from a recorded Secret that allows the URLs Coder holds. If not, it disables the config in Coder with one PATCH that holds only enabled: false. A spec edit is needed to enable it again. Fixes #311 Refs #155 Change-Id: I37c5933cb4d3abd50351308e454f8d6c09499fc9 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex security 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. |
A PATCH that clears a credential left its hash in memory. After an auth type round trip, the hash claimed that Coder held the Secret's value while Coder held a key set in the dashboard, so the key was not sent. Refs #155 Change-Id: Id76a171929671334c958fc990bce5eec696ce132 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I9730576dccc25dc8c027b77ab3e5105bb5030c74 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…dc when the credential read fails Without a readable credential the controller cannot set user_oidc back, and user_oidc sends each chat user's login token to the server URL. The consent check now disables such a config. Refs #155 Change-Id: I66b394e3d13ece60d23b604333803f9537d5a4a6 Signed-off-by: Thomas Kosiewski <tk@coder.com>
![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: f693edeb85
ℹ️ 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 ReviewSecurity 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. |
… check A Secret with the same UID whose value became invalid UTF-8 or gained a CR, LF or NUL failed the credential read, but the consent check accepted it. The check now applies the same value rules as the read. Refs #155 Change-Id: I04342bc648f04d7cdcb96d0a8d673ccd0fa82353 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 203f73b02e
ℹ️ 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.
🛡️ Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 203f73b02e
ℹ️ 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.
…neration after the disable - The consent check requires the spec to name the recorded Secrets, so a credential write whose answer was lost cannot pass on the old Secret. - All reads of one Secret in the check must see one resourceVersion. - The disable answer must be for the same organization and slug. - The disable sets appliedGeneration, so a generation that was not applied yet does not enable the config after consent comes back. Refs #155 Change-Id: I4b0ee409911f9edb18cf43e1552f761ed413b2bf Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity 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: 8f0a048d14
ℹ️ 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".
Change-Id: Ie499fafffdadc474df5ed3218454097afde41133 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13780902db
ℹ️ 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 ReviewSecurity 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. |
… consent check, and test the consent disable marker (#363) ## Summary The dormant `CoderMCPServer` consent check now requires https for the OAuth2 revocation URL that Coder holds, as the spec does. This PR also adds the consent-path tests for the disable marker that #360 introduced. The kind is not registered, so users see no change. **Base branch:** `main`. #360 landed as `69c18dd5`. I merged `main` into this branch with a merge commit, and the diff against `main` is the reviewed M6f change only. Fixes #311 Fixes #334 ## Background - #311 (plan gate G8): #330 disables the config when a credential Secret withdraws consent. #311 stayed open for one case: with a pending generation, a disable whose answer or status write is lost, followed by restored consent, applied the pending generation and enabled the config without a spec edit. #360 fixes this for both fail-closed paths: the controller writes `appliedGeneration` to status before the disable PATCH, and a read that finds the config disabled or gone also records the generation. This PR adds the tests for the consent path. - #334: Coder accepts an `http` revocation URL for loopback hosts, while the CRD and the controller require `https` for `spec.auth.oauth2.revocationURL`. The consent check of the disable path checked Coder's revocation URL only against the Secret's origins, so a dashboard-set `http` loopback revocation URL that the Secret listed passed the check. ## Implementation - `mcpServerConsentHolds` returns false when Coder's revocation URL is not `https` (4 lines), so the config is disabled. - New tests in `internal/controller/codermcpserver_marker_test.go`. ## Validation - `TestCoderMCPServerLostDisableAnswerHolds` (#311): with a pending generation, the Secret is deleted, and the disable PATCH commits but answers 502. Status already holds the current `appliedGeneration`. The Secret comes back before any other reconcile, and the sync gives `DisabledExternally` with no write and the pending description not applied. A spec edit after the disable enables the config. - `TestCoderMCPServerDisableNeedsTheMarker` (#311): when the status write before the disable PATCH fails, the reconcile returns the error and sends no PATCH. - `TestCoderMCPServerFoundDisabledHolds` (#311): with a pending generation and a deleted Secret, Coder already holds the disable (a lost answer). The disable path records the generation, and the restored Secret gives `DisabledExternally` with no write. - `TestCoderMCPServerConsentCheckNeedsHTTPSRevocation` (#334): Coder holds `http://127.0.0.1:8080/revoke`, and the Secret lists that origin. A spec URL edit that fails the credential read gives exactly one `{"enabled":false}` PATCH. - Mutation check: without the https check, the #334 test fails. The marker mutants are in #360. - Local gates on this head: `make verify-vendor`, `make build`, `make lint`, `GOFLAGS=-mod=vendor go tool govulncheck ./...`, `make test`, `make test-integration`, `go test -race ./internal/controller/`, and `make docs-check` pass. `make codegen`, `make manifests`, `make build-installer` and `make docs-reference` leave no diff. ## Size 158 lines added, all hand-written: 4 in `codermcpserver_disable.go` and 154 in the new tests. Refs #155 --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$171.92`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=171.92 --> --------- Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 participant
Summary
When a credential Secret of an applied
CoderMCPServerwithdraws consent, the dormant reconciler now disables the config in Coder with one PATCH that holds onlyenabled: false. A spec edit is needed to enable it again. This is plan gate G8 and the same rule asCoderAIProvider(#276, #290). The kind is not registered, so users see no change.Base branch:
mcp-kill-switches(#329). Review only the last commit. After #329 lands, I retarget this PR tomain.Refs #311 (it stays open for one remaining item, see the review record)
Background
Before this PR, a credential read failure stalled the object before any Coder call (fail static). Coder kept the config enabled with the stored credential and kept sending it to the MCP server. #311 asks for the fail-closed rule of
CoderAIProvider.How it works
The reconciler runs the disable path when
readMCPServerCredentialsreturns a stalled credential failure andstatus.configIDis set. ACredentialSecretChangedwait does not trigger it.CoderAnswerMismatch.status.credentialsentry with the same field, Secret name and key. A PATCH whose answer was lost after the commit can only have sent a Secret that the spec names. Then, for eachhas_*flag that is true, every matchingstatus.credentialsentry must name a Secret that still passes the credential contract, with the same UID, for each URL of Coder's config that receives it:urlfor the API key and headers, plus the OAuth2 authorize, token and revocation URLs for the client secret. A held credential with no recorded entry, a URL that does not parse, another UID, or a malformed value fails the check. All reads of one Secret must see one resourceVersion; otherwise the reconcile waits 30 seconds and checks again.urlto an origin that the Secret does not list, while Coder still holds the old, allowed URL (plan S12).PATCH {"enabled": false}. A failed PATCH waits 30 seconds with a message that Coder can still send the credentials. An answer for another ID, organization or slug, or one that is still enabled, retries. Success setsappliedGenerationand stalls with the credential reason.The disable counts as an external disable of the current generation, also when that generation was not applied yet. After the Secret is fixed, the next sync therefore finds
DisabledExternally(#329) and writes nothing until a spec edit after the disable.Like
CoderAIProvider, it fails closed on every credential failure, also a missing or malformed value: the Secret then no longer proves consent for the value that Coder uses. The consent check applies the same value rules as the read (UTF-8, and HTTP header value rules for the API key and headers).When Coder shows the auth type
user_oidc, the controller skips the consent check and disables the config.user_oidcsends each chat user's login token tourl, and without a readable credential the controller cannot set it back (plan 4.7 item 4).The disable runs after the scope gate. When the control plane no longer allows the organization, the reconciler stalls with
MCPServersNotAllowedand sends no MCP request, including this disable.Review record
Codex round 1 (normal review of
f693ede): three findings.4e84a445:user_oidcwith no held credential passed the consent check.203f73b0: a malformed value with the same UID passed the consent check.CoderAIProvider.The security review of
f693edefound no issues.Codex round 2 (normal and security reviews of
203f73b): four findings, all fixed in8f0a048d:Codex round 3 (normal and security reviews of
8f0a048): the security review found no issues. The normal review found two items. This PR reached its two fix rounds, and an independent advisor recommended "ready with tracked follow-ups" and "Refs #311":One more limit, for the M9 decision: when the control plane withdraws its opt-in, the scope gate stops the reconcile before the credential read, so the controller does not disable the config.
Validation
New tests in
internal/controller/codermcpserver_disable_test.go:TestCoderMCPServerDisablesWithoutConsent(G8, S12): for a deleted Secret, a removed opt-in label, a removed origin, an API key that gains a line feed (same UID), and an OAuth2 Secret that drops the token origin:{"enabled":false}, and stalls with the credential reason. Coder holds the same state with onlyenabledchanged.DisabledExternallywith no writeTestCoderMCPServerConsentCheckUsesCoderState(S12): a spec URL with an unlisted origin fails the read withCredentialDestinationNotAllowed. Then:user_oidc: one disable PATCH.TestCoderMCPServerDisableHoldsPendingGeneration: a spec edit whose PATCH fails, then a deleted Secret, gives the disable. The restored Secret givesDisabledExternallywith no write, and the pending description is not applied. A spec edit after the disable applies and enables.TestCoderMCPServerDisableChecksTheAnswer: a disable answer with another slug gives aCoderAnswerMismatchwait.TestCoderMCPServerConsentCheckNeedsOneSecretState: an interceptor changes the resourceVersion of the OAuth2 Secret between two consent reads. The reconcile waits 30 seconds and writes nothing.TestCoderMCPServerWatchesnow expectsDisabledExternallyafter a Secret loses and regains its label, and after another control plane refuses the Secret. A spec edit then givesApplied.user_oidcruleappliedGenerationon the disablemake verify-vendor,make build,make lint,GOFLAGS=-mod=vendor go tool govulncheck ./...,make test,make test-integration,go test -race ./internal/controller/, andmake docs-checkpass.make codegen,make manifests,make build-installerandmake docs-referenceleave no diff.Size
481 lines added and 11 removed, all hand-written: 175 in
codermcpserver_disable.go, 28 changed lines in the reconciler (the hook and the shared value check), 274 in the new tests, and 15 changed lines inTestCoderMCPServerWatches. The two review rounds added 189 of these lines.Refs #155
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$127.81