🤖 fix: enforce canonical template and workspace leaf names - #111
Conversation
…regated API Coder resolves template and workspace names case-insensitively, so a request for `acme.Starter-Template` or `acme.alice.Dev-Workspace` fetched the canonical object and the aggregated API returned, updated, or deleted it under a different metadata.name than requested. Verified live against Coder 2.37.2: direct lookups with alternate casing return the canonical object with the same ID, and the aggregated API on main returned it under the canonical name. Compare the fetched object's own name with the requested final segment and reject a difference with the same BadRequest-naming-the-canonical-form used for organization and owner aliases. For templates the check runs in Get and Delete right after the fetch, before source-file download, delete preconditions, admission, or mutation; Update and create-on-update inherit it through Get. For workspaces the existing requireFetchedWorkspaceIdentity gains the comparison after the membership and organization/owner checks, so every workspace Get, Update, and Delete inherits it and cross-organization workspaces stay opaque. Genuinely mixed-case canonical names remain valid when requested exactly; there is no lowercase blacklist and Create is unchanged. Tests model the verified case-insensitive lookup in the mock backend and cover GET, UPDATE, create-on-update, DELETE (including matching delete preconditions), mixed-case round trips, repeated canonical apply, cross-organization opacity, and zero backend mutation on rejection. Fixes #107 _Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh`_ Change-Id: I0470bf38623167d0af0a3f2bea44b6eb055d5bc4 Signed-off-by: Thomas Kosiewski <tk@coder.com>
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 |
🛡️ 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. |
![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: 9c64f9de72
ℹ️ 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".
…the no-mutation note When the organization or owner segment and the leaf are aliases at the same time, the workspace rejection advertised the requested leaf (`acme.alice.Dev-Workspace` for `default.me.Dev-Workspace`), a name that is itself rejected on retry. Build the hint from the fetched workspace's own name after membership is settled, so every alias segment is corrected at once; keep the empty-name assertion before constructing the hint and after the opacity check. The template organization check runs before the template lookup, so its message now corrects only the organization segment, gives the corrected name as an example, and states that the template segment is not checked yet; the workspace create path says the same about its unchecked leaf. The how-to said alias requests never mutate the backend; that holds for lookups of existing objects (get, update, patch, delete, create-on-update) but not for a direct create of a template whose name differs only in casing from an existing one: with spec.files set, the archive is uploaded and a template version is created before Coder reports the collision. Scope the guarantee to lookups and describe the create case explicitly. No create preflight is added. Tests drive both resources through combined-alias GET/UPDATE/DELETE, mixed-case fetched hints, cross-organization opacity, the qualified template and workspace create messages, a successful retry with the advertised name, and zero additional mutations. Part of #107 _Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh`_ Change-Id: Ie13f139e57e8dcd6078644dfa8ec18f0a5560224 Signed-off-by: Thomas Kosiewski <tk@coder.com>
`preflighted` is not in the docs spell-check dictionary (the docs-cspell gate fails on it). Say that create requests are not checked against existing names first; the meaning is unchanged. Part of #107 _Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh`_ Change-Id: Id04cb81fc2708d191506582eee7f73acfd5cfc3e Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@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. |
|
Delivered through the required merge queue as
The maintainer desk owns follow-ups #109 (workspace resource versions, next) and #105 (creation/apply convergence). PR #89 remains separate. This fix rejects aliases before backend mutation; the necessary lookup still occurs. Direct-create side effects remain documented, not claimed fixed. Generated with |
1 participant
Summary
Reject existing-object lookups when Coder returns a template or workspace whose own name differs from the requested final segment. For example,
coder.E2E-Templatecannot return or deletecoder.e2e-template. The server returns400 BadRequest; genuinely mixed-case canonical names remain valid.Fixes #107. This follows #106 and #110. Resource-version representation (#109), import readiness (#105), Create normalization and LIST behavior remain out of scope.
Review focus
spec.filescan upload a file and create a version before Coder reports a case-insensitive name collision. The guide now states this limitation; no new preflight or backend lookup was added.The complete change is 394 added/deleted lines across six files, including tests and docs. There is no lowercase blacklist.
Validation
Final candidate:
7921a1797b47031ba8209d1b4b4c703738335d84.make verify-vendor,make test,make build,make lintandmake docs-checkafter integrating the exact same commit.resourceNamesonlyresourceNamesonlySuccessful DELETE cases used disposable fixtures. The original objects retained their backend IDs. Alias DELETE with matching preconditions returned 400; canonical wrong-UID DELETE returned 409. A direct alternate-case template Create returned AlreadyExists; the existing template's ID, name, active version and template count were unchanged. The live test did not enumerate orphan files/versions; a mock reproduction establishes the upload → version → create call order.
Owned cluster, image tag, kubeconfig, port forwards and recording session were removed. The shared kubeconfig hash was unchanged.
Compatibility and limits
Manifests and
resourceNamesgrants must use the exact name reported by Coder. This intentionally tightens alias behavior. The guide explains the migration, partial template-organization hints and direct-Create limitation. Live semantics were verified on Coder 2.37.2, not every backend version. These checks do not establish atomic compare-and-delete or a new resourceVersion scheme.Preserved validation failures and corrections
9c64f9deruntime had one failed final harness assertion (116/117 passed), followed by a successful targeted tail. Its original failed receipts remain intact. The final candidate above was tested in a fresh complete run after correcting the harness's deleted-fixture check and JSON response classification.444c0d91; a one-sentence docs correction fixed it. That commit omitted the full pre-commit test/build/vendor commands. The final commit's pre-commit checks and all 12 final-SHA checks passed. The extra pre-commit runner received a retrospective, not advance, path audit.Recorded dogfood — final candidate
The screenshot shows the complete final run's verdict. The video is a post-hoc, idle-trimmed replay: 61.4 seconds from the live 504.8-second terminal recording, not a second execution or real-time playback. Both show the 48-case classification and final native exit 0.
e2e-accelerated-timing.webm
Historical recording from the initial PR head (superseded)
The original screenshot shows the targeted tail, not the final candidate. The original video is a 49.2-second idle-trimmed replay of a 572-second session containing the first failed producer and its corrective tail. The screenshot was recovered from the recording host after a CLI timeout.
e2e-accelerated-timing.webm
📋 Implementation Plan (accepted before execution; results above)
Plan — issue #107: enforce canonical template and workspace leaf names (aggregated API)
Delivery boundary: one small correctness PR. Verify live baseline lookup semantics before production edits, then implement test-first and validate the exact final candidate.
Baseline and evidence
main=f9a78c162780629431a1e3b86ac7ea062d6c11ea(treeffeff639be67e359415a01477befb105dad57778),the squash landing of 🤖 fix: honor aggregated API delete preconditions #110 (delete preconditions). Verified read-only with
git fetch origin main+git show;the tree equals the 🤖 fix: honor aggregated API delete preconditions #108 finalization head tree, so every file reference below is valid on
main.delivery/issue107-planning.06FAqd/issue.json(🤖 fix: enforce canonical template and workspace leaf names #107, OPEN).delivery/issue105/case-triage/FINDINGS.md— storage-entrypoint scratch run against acase-folding mock proxy only; live Coder lookup casing for templates and workspaces is unverified.
main):internal/aggregated/storage/identity.go:26requireCanonicalTemplateName(org segment, pre-fetch) and:42requireCanonicalWorkspaceName/:68requireFetchedWorkspaceIdentity(org + owner, post-fetch,opaque cross-org NotFound). Neither compares the fetched object's own leaf name with the requested segment.
template.go:165(Get, also used by Update throughs.Get),:793(Delete).codersdk.TemplateByNameis documented case-insensitive (vendor/.../codersdk/organizations.go:637).workspace.go:131(Get),:489(Update current object, create-on-updatebranch at
:492),:683(Delete),:720(post-fetch conversion in the delete path).template.go:397,workspace.go:368) send the requested name to Coder; they are not a lookupand stay unchanged.
apierrors.NewBadRequestnaming the canonical form(
coder.BuildTemplateName,coder.BuildWorkspaceNameininternal/aggregated/coder/names.go).identity_test.go:42newAliasResolvingCoderServer(mock with alias resolution),storage_test.gofixtures,delete_preconditions_test.go(fixture/matrix pattern, mutation counter).Scope of the single PR
Production delta (bounded, both resources, no new subsystem):
identity.go: add a small template post-fetch check. For workspaces, extend the existingrequireFetchedWorkspaceIdentity, which already receives the requested leaf and fetched workspace: establishorganization membership, return any organization/owner canonicality error, then compare the leaf. Do not add
a second check at each workspace call site. Compare the fetched object's own
Nameexactly with the requestedfinal segment; mismatch →
BadRequestwith the canonical name. A genuinely mixed-case canonical name passes.preconditions, or mutation. Existing workspace Get/Update/Delete callers inherit the extended identity check.
Update/patch and repeated apply retain their existing storage paths. A leaf mismatch is
BadRequest, notNotFound, so create-on-update (forceAllowCreate && IsNotFound) cannot fall through to Create.preserved) → leaf → 🤖 fix: honor aggregated API delete preconditions #108 preconditions → admission → mutation. Cross-organization workspaces remain opaque
NotFound; the leaf check runs only after membership is established.
docs/how-to/deploy-aggregated-apiserver.md(canonical names section) stating that thefinal segment must match the object's own name exactly and that alternate casing is rejected with the canonical
name; note the RBAC consequence (grants use the requested URL name).
docs/reference/apiunchanged (no APIstruct change; run
make docs-referenceanyway and expect a clean diff).Out of scope (explicitly): #109 resourceVersion scheme, #105 import readiness, quickstart/installer, Create-path
name normalisation, LIST filtering, any adjacent refactor. #106 (org/owner canonicality) and #108 (delete
preconditions) are prerequisites already on
mainand are only exercised as regression coverage.Live baseline semantics gate (before production edits)
Build an immutable baseline clone/image from
f9a78c162780629431a1e3b86ac7ea062d6c11eain a separateowned Kind environment. Record source → binary → image → running pod identity. For each resource, create one
canonical fixture; query its canonical and alternate-cased names directly through Coder and through the
aggregated API. Record HTTP status, returned name, and backend ID. Probe acceptance and exact round-trip of a
genuinely mixed-case canonical name. Keep this narrow; the full RBAC matrix runs only on the final candidate.
Capture native receipts, screenshot and labelled video, then clean up owned baseline resources. Freeze this
baseline packet before starting implementation.
If a live lookup does not resolve the alternate name, record the ordinary NotFound result. An artificial
differently named response may test the storage invariant, but is not evidence of a reproduced backend defect.
Reassess that resource's runtime expectations before production edits; do not force an expected 400 where the
backend correctly returns 404.
Test-first (red before green)
New file
internal/aggregated/storage/leaf_names_test.go, mock extended to resolve template and workspace leaflookups case-insensitively only after the baseline live probe confirms that behavior. Otherwise, label any
explicitly differently named response as an invariant test, not a backend reproduction, and record the
reassessed runtime expectations before proceeding.
Red command (must fail on
main, pass on the candidate):GOFLAGS=-mod=vendor go test ./internal/aggregated/storage -run 'TestLeafName' -count=1 -vCases (both fixtures unless stated), asserting response, zero backend mutations, unchanged backend object identity
(ID) and state (template present / workspace latest build transition):
acme.Mixed-Case, backend object namedMixed-Case) → 200; itslower-cased alias → BadRequest (no blacklist).
UpdateTemplateMeta/builds calls; UPDATE canonical → unchanged.forceAllowCreate) with alias-cased leaf → BadRequest, zero Create calls.behaviour; DELETE alias with matching 🤖 fix: honor aggregated API delete preconditions #108 preconditions → still BadRequest (leaf before preconditions).
identity_test.go,delete_preconditions_test.go,storage_test.go.Repository checks (all native exits recorded, process-only isolated env)
Same 12 gates as the #108 packets, on the exact candidate HEAD, into a new root: verify-vendor, refdocs (
make docs-referenceclean diff), build,make test, focused (TestLeafName|TestTemplateStorage|TestWorkspaceStorage| TestDeletePreconditions),make lint, actionlint, govulncheck, Trivy fs HIGH/CRITICAL,make docs-check,markdownlint, cspell. Commit: one commit,
type: message, footer without a standalone---.Hands-on Kind dogfood (new exclusive root, e.g.
delivery/issue107/implementation/)Isolation: use a readonly, uniquely named issue-specific run root, never a mutable generic
D. Validate everyoutput destination before execution and after loading any helper: clone/build, docs site, caches, logs, media
exports, kubeconfig and session paths. Use a fresh real clone of the exact candidate SHA and unique cluster/tag/
kubeconfig/session names. Never source generic desk helpers or earlier MkDocs wrappers. Baseline and candidate
packets remain distinct; no output path may point into a frozen packet.
kindest/node:v1.32.0→ CNPG →CoderControlPlane); recordvcs.revision, image id, node digest, running pod imageID.port-forward, token in env only): after creating
coder.e2e-templateandcoder.<user>.e2e-workspace,GET /api/v2/organizations/{org}/templates/E2E-Templateand
GET /api/v2/users/{user}/workspace/E2E-Workspace; record status/body for both. Also create a mixed-casecanonical control (
coder.E2E-Mixedtemplate;coder.<user>.E2E-Mixedworkspace) if Coder accepts the names,otherwise preserve the rejection and use the expectations established by the baseline gate.
kubectl proxyand kubectl: GET/UPDATE(apply)/DELETE for canonical, alias-cased, andmixed-case canonical names of both resources; assert 200 vs 400 (
BadRequestnaming the canonical name), zerobackend change (Coder GET by ID, counts, latest build transition), repeated canonical apply unchanged,
create-on-update alias → 400 with no new backend object.
exercise canonical-only, alternate-case-only, no-grant, and namespace-wide ServiceAccount/Role grants against
both canonical and alternate names. Use raw verb-specific requests through the real kube-apiserver so kubectl
discovery/ancillary GETs do not hide authorization of the tested verb. Record authenticated identity, requested
URL/name/verb, HTTP status/error body, rejecting layer, and backend ID/state before/after. For a verified
case-insensitive lookup: canonical-only → canonical success, alias 403; alternate-only → canonical 403, alias
storage 400; no grant → 403/403; namespace-wide → canonical success, alias storage 400. Use separate disposable
fixtures for successful DELETEs; omit preconditions or supply matching values so 409 cannot mask leaf checks.
Retain repeated canonical apply, actual mixed-case canonical controls, alias create-on-update rejection and
membership-opacity coverage. Do not equate an unrelated authentication/validation failure with the leaf check.
codertemplateGET downloads the template zip and an Update does 4; Coder limits thatendpoint to 12/min → 61 s cooldowns before zip-heavy phases, one GET per uid/rv read, no blind retries; failures
stay recorded with native exits.
receipts), native HTTP status/body receipts per step, per-step exit receipts reconciled against the transcript,
operator log error count.
port-forward/proxy, tty session; shared kubeconfig hash unchanged. Then REPORT (≤9k), handoff.json, manifest.
Quality gates (each must pass before the next phase)
main(receipts) → 3. minimal fix, green focused/full suite → 4. all 12 static gates native 0 on the exact candidate →
cleanup receipts → 8. handoff. Preserve a fixed 150-minute implementation timebox across restarts; report the
baseline result before production edits. Keep the whole PR reviewable, targeting under 500 changed lines with
table-driven tests; do not remove required coverage merely to meet the size target.
Unknowns to resolve during implementation (not assumed here)
applyon an alias-cased name surfaces the storageBadRequestcleanly or a generic namemismatch first (both are acceptable rejections; record which).
Generated with
xum• Model:coder:openai/gpt-6-astra• Thinking:xhigh• Cost:$677.17