🤖 fix: enforce canonical organization and owner names - #106
Conversation
…objects Coder resolves aliases such as the "default" organization, the "me" user and raw IDs on the server side, so a CoderTemplate or CoderWorkspace requested under an alias name came back with a different metadata.name. kubectl apply then failed on repeated applies (name precondition) and turned a NotFound GET into an AlreadyExists POST (#105). The storage layer now keeps name parsing syntactic and validates identity through the namespace-bound Coder client before any mutation: - templates: the organization segment must equal the resolved organization name on Get/Create/Update/Delete (create-on-update included); otherwise a BadRequest names the canonical form and no upload, template version or template is created. - workspaces: existing-object paths reuse the fetched workspace's organization and owner; a workspace from another organization stays an opaque NotFound (membership is established by organization ID before any alias guidance), while alias segments are rejected with the canonical name. Create resolves the owner before the workspace is created. - literal organizations or users really named "default"/"me" remain valid; lookup and authorization failures keep their normal mapped errors. The E2E fixture and CI reference use the bootstrap organization's canonical name ("coder"), and the aggregated API how-to documents the naming rules, pre-creation discovery and migration. Template-import readiness (workspace creation while a version is importing) is out of scope and stays tracked in #105. Part of #105 Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh`_ Change-Id: I95bd8cda4b8e0a8274ef03a44095baec4f4663ca
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 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: ad1c090fea
ℹ️ 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. |
Review finding on identity.go: when a workspace fetched by owner belongs to another organization and the requested organization's lookup is denied (403), the aggregated API returned Forbidden, while a missing workspace under the same denied organization returned NotFound before any lookup. A caller without access to the organization could therefore tell whether a cross-organization workspace exists, contrary to the documented opaque NotFound. Map a denied membership-verification lookup to the same opaque NotFound as a genuine cross-organization mismatch, on the existing-workspace path (Get/Update/Delete) and on the create-on-update branch, where the fallback Create would otherwise surface the denial as Forbidden for a missing workspace only. Direct Create keeps its ordinary mapped errors, and 401/404/5xx lookup failures keep their normal mapping. The regression test drives Get/Update/Delete/create-on-update for an existing and a missing workspace under a denied organization through the real storage entrypoints (all opaque NotFound, nothing disclosed, zero backend mutations), keeps direct Create at Forbidden and an organization lookup outage at its normal mapping. Part of #105 Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh`_ Change-Id: I43bae79e936b55e7af2261c34d053d6c86c9ef17
|
@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: ef29d3a598
ℹ️ 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".
ThomasK33
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
Maintainer readiness decisionReady for the required merge queue at
After landing I will verify the admitted run, squash tree/parent/attribution, main checks, and published image revision/digest. Generated with |
Delivery verified
Unfixed follow-ups remain owned: #108 (DELETE preconditions) is next, then #107 (canonical leaf-name checks and live RBAC evidence). Issue #105 stays open for import readiness. Quickstart restoration is not included in this delivery. Generated with |
1 participant
Summary
Require canonical organization and owner names for aggregated
CoderTemplateandCoderWorkspaceobjects. Requests whose organization or owner segment resolves to another name now fail with an actionable400 BadRequestbefore backend mutation, rather than returning a differentmetadata.nameand breaking repeatedkubectl apply.Part of #105. Import readiness is unchanged; issue #105 remains open. Alias-based manifests must migrate to canonical names. This does not restore quickstart-bundle support.
Implementation and review focus
defaultorme: both remain valid when they are the resolved canonical names.NotFoundfor cross-organization workspaces. A denied organization-membership lookup also returns opaqueNotFoundon existing-workspace paths and create-on-update, so it does not distinguish an existing workspace from a missing one. Direct Create and ordinary authentication/outage errors keep their normal mapping.coderin the E2E fixture and CI lookup.Validation
Current candidate:
ef29d3a598e60e55330ece26c48d641f0c978309.Forbidden; a 503 lookup outage retains its ordinary mapping.make verify-vendor,make test(including envtest),make build,make lint, andmake docs-check, including an independent desk rerun. The docs target uses an isolated MkDocs executable because Nix is unavailable on the validation host. Actionlint, govulncheck, CI-configured Trivy filesystem scanning, Markdown lint, and spelling checks also passed.Denied-organization behavior is verified with mock backend 403/503 responses. Live testing covers the owner-role operator principal used by the namespace-bound provider, not lower-privileged credentials. External link checking and full admitted merge-group E2E remain CI gates.
Current recorded evidence
Screenshot: terminal end state of the
ef29d3a5execution. Video: replay rendered from that run's raw terminal recording with idle gaps compressed, not another test run. The replay was rendered separately after the evidence-path incident described below.ef29-stepwise-replay.mp4
Evidence provenance and historical recording
During the corrected run, a sourced harness helper reassigned its output-directory variable. This overwrote 94 files in the earlier
ad1c090fE2E packet after that packet had been verified. Of the 376 original manifest entries, 282 remain byte-for-byte intact; the 94 original files are no longer retained. The original manifest was not rewritten and no original receipts were reconstructed.The original complete terminal recording, screenshot/video, report, and handoff survive unchanged. The publicly uploaded original screenshot/video still match their verified hashes. They are historical
ad1c090fevidence, not validation of the current head. That original run passed 37/38 steps; the final command had invalid multi-resourcekubectl getsyntax, and a corrected final-step retry passed in the same session.For the current
ef29d3a5run, output files were relocated into its own packet. All 165 finalized artifact hashes were verified. Their source/image identity and all 35 native step exits were reconciled against the independent terminal transcript and raw recording, which were written outside the overwritten paths. The collision affected output destinations; source, image, cluster, and kubeconfig bindings remained those of the current run, and scenario inputs were regenerated before use. The actual as-run harness is retained; a separate corrected harness has not been represented as executed.Historical
ad1c090freplay, with idle gaps compressed:e2e-replay-compressed-gaps.mp4
Risks and follow-up
Existing alias manifests receive an explicit error and need the documented migration. Workspace creation now includes identity preflight reads. No broader credential-compatibility claim is made beyond the documented coverage.
This increment enforces organization and owner segments only, not the final template/workspace-name segment. Review identified two unchanged baseline limitations:
cfdd8974andef29d3a5; the normal-review finding is deferred, not fixed. Workspace lookup and real KubernetesresourceNamesbehavior need the live integration coverage specified in 🤖 fix: enforce canonical template and workspace leaf names #107. A canonical-name-only grant does not grant alternate casing; no new cross-tenant/backend-object access was demonstrated.The maintainer owns both immediate API-correctness follow-ups after this increment's disposition, before quickstart support is restored. The separate import-readiness contract remains in #105. These are not claims of complete canonical-name enforcement or atomic conditional deletion.
📋 Implementation Plan
Canonical identity for aggregated API objects
Goal and boundary
Require CoderTemplate and CoderWorkspace request names to match the backend's canonical organization and owner names. Reject aliases with an actionable BadRequest instead of returning a differently named object. Canonical names must still support repeated apply.
This increment does not fix template-import readiness or make alias-based quickstart manifests converge. Keep issue #105 open for the remaining readiness contract. Exclude API/schema changes, alias persistence, SSA changes, quickstart restoration, dependency changes, and unrelated cleanup. Final template/workspace-name casing and DELETE preconditions retain baseline behavior; follow-ups #107 and #108 cover those separate contracts.
Contract
defaultorme.default/meremains valid when resolution returns that exact name.Steps and acceptance gates
Dogfood evidence
Capture commands, exact source SHA/image binding, native exits, screenshots, and video of the actual CLI verification. Inspect the screenshot and recording for the claimed steps. Any playback copy must be labeled as replayed evidence. Harness waits document the existing import requirement and are not evidence of an import-readiness fix. Preserve a hash manifest, final handoff, and cleanup receipts outside the source tree.
Generated with
xum• Model:coder:openai/gpt-6-astra• Thinking:xhigh