Repository navigation
🤖 fix: require https for Coder's revocation URL in the CoderMCPServer consent check, and test the consent disable marker - #363
Merged
Conversation
…draws its opt-in When spec.mcpServers is removed, no longer lists the organization, or no longer allows force_on for a force_on server, the controller now disables a config that the object created, with one PATCH that holds only enabled: false. It reads no credential. The disable is a kill switch, so a restored opt-in needs a spec edit to enable the config. The read and the PATCH are shared with the consent disable. Fixes #336 Refs #155 Change-Id: Iebea23e388d9c43540daebf8380f256c59f282cf Signed-off-by: Thomas Kosiewski <tk@coder.com>
…lookup and resync MCPServersNotAllowed The organization lookup treated any 404 as a missing organization. A proxy page or "Route not found." for a listed name then skipped that name, so the scope check stalled with MCPServersNotAllowed, which had no timed requeue. The lookup now uses isCoderResourceNotFound, so another 404 waits with CoderUnavailable, and MCPServersNotAllowed resyncs every 10 minutes. The fake answers an unknown organization like Coder. Fixes #358 Refs #155 Change-Id: I90fa60c56e9f867d4c7c53bce35c1be49be366a8 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…it the opt-in disable to opt-in stalls, and document it - A read that finds the config already disabled now records the generation, so a lost disable answer and a restored opt-in do not apply a pending generation that enables the config. - Only MCPServersNotAllowed and ForceOnNotAllowed run the opt-in disable. Another stall of the scope check, such as a mismatched Coder answer, keeps the config. - The spec.mcpServers field docs describe the fail-closed disable. Refs #155 Change-Id: If9ea006bcd30e178dc579172f03ddce31b0b3923 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Ib67e349731c2986ff930b25276e6ba3ed6671b2c Signed-off-by: Thomas Kosiewski <tk@coder.com>
…isable through the pinned organization - The opt-in and consent disables write appliedGeneration to status before the disable PATCH, and a failed status write sends no PATCH. A lost disable answer followed by a restored opt-in then gives DisabledExternally instead of applying a pending generation. - When the organization name no longer resolves (OrganizationNotFound) or resolves to another ID (OrganizationChanged), a withdrawn opt-in still disables the config through the pinned organization ID. Refs #155 Change-Id: Ie54facd7656489a5436d639734912e3c5c26050b Signed-off-by: Thomas Kosiewski <tk@coder.com>
… and document appliedGeneration - A fail-closed disable that finds the config gone (Coder's own 404) now records the generation, so a restored opt-in or consent gives DeletedExternally instead of creating an enabled config before a spec edit. - The status.appliedGeneration doc now says that a kill switch also sets it, before the disable is sent. The dormant CRD is regenerated. Refs #155 Change-Id: I781f44dfea54d7834faf712e75d4fe764f65323f Signed-off-by: Thomas Kosiewski <tk@coder.com>
… consent check, and test the consent disable marker - The consent check now requires https for the OAuth2 revocation URL that Coder holds, as the spec does. Coder accepts an http loopback revocation URL, so a Secret that lists that origin still does not prove consent for it. - Tests for the consent path of the disable marker: a lost disable answer followed by restored consent, a failed marker write, and a config found disabled with a pending generation. Fixes #311 Fixes #334 Refs #155 Change-Id: I15caebdca4e137372ad3171e0f9007b207703413 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Member
Author
|
@codex security review |
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. |
🛡️ 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. |
Change-Id: I557c4ff360028d952dec024ab0e4338e1fc32d20 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Member
Author
|
@codex review |
Member
Author
|
@codex security 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 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The dormant
CoderMCPServerconsent 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 as69c18dd5. I mergedmaininto this branch with a merge commit, and the diff againstmainis the reviewed M6f change only.Fixes #311
Fixes #334
Background
appliedGenerationto 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.httprevocation URL for loopback hosts, while the CRD and the controller requirehttpsforspec.auth.oauth2.revocationURL. The consent check of the disable path checked Coder's revocation URL only against the Secret's origins, so a dashboard-sethttploopback revocation URL that the Secret listed passed the check.Implementation
mcpServerConsentHoldsreturns false when Coder's revocation URL is nothttps(4 lines), so the config is disabled.internal/controller/codermcpserver_marker_test.go.Validation
TestCoderMCPServerLostDisableAnswerHolds(🤖 CoderMCPServer: disable the config in Coder when credential consent is withdrawn #311): with a pending generation, the Secret is deleted, and the disable PATCH commits but answers 502. Status already holds the currentappliedGeneration. The Secret comes back before any other reconcile, and the sync givesDisabledExternallywith no write and the pending description not applied. A spec edit after the disable enables the config.TestCoderMCPServerDisableNeedsTheMarker(🤖 CoderMCPServer: disable the config in Coder when credential consent is withdrawn #311): when the status write before the disable PATCH fails, the reconcile returns the error and sends no PATCH.TestCoderMCPServerFoundDisabledHolds(🤖 CoderMCPServer: disable the config in Coder when credential consent is withdrawn #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 givesDisabledExternallywith no write.TestCoderMCPServerConsentCheckNeedsHTTPSRevocation(🤖 CoderMCPServer: require https for Coder's revocation URL in the consent check #334): Coder holdshttp://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.make 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
158 lines added, all hand-written: 4 in
codermcpserver_disable.goand 154 in the new tests.Refs #155
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$171.92