Repository navigation
fix(api): refuse create-tenant for a tenant that already exists - #452
Conversation
create-tenant makes a new tenant; it never joins one. When the tenant directory already exists, the workflow skips writing the tenant and carries on provisioning, which is the retry path for a half-finished run. A request for a taken name should say so instead of starting that path. The API now answers 409 before submitting anything when the name is taken, and 503 when the gitops checkout cannot tell (never synced, older than 10 minutes, or unreadable): "could not check" is not "absent". Admins are not checked, so they can still re-run create-tenant to finish or redo a tenant's provisioning. Claude-Session: https://claude.ai/code/session_01D1kGUJMWeA8hyhQuxKuHd2
|
Claude finished @mashkovd's task in 2m 55s —— View job Review complete for PR #452 (commit
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if errors.Is(err, fs.ErrNotExist) { | ||
| return false, nil | ||
| } | ||
| return false, fmt.Errorf("checking tenant %q: %w", name, err) |
There was a problem hiding this comment.
P3: the wrapped stat error (e.g. an os.PathError containing the absolute checkout path) propagates verbatim into the HTTP 503 body via checkTenantIsNew in handlers_write.go, which does err.Error() into the client-facing message. This matches the existing convention elsewhere in this handler (e.g. the Submit-failure path), so not a regression, but worth a note: it leaks server filesystem layout to API callers on this new path. Not blocking.
There was a problem hiding this comment.
Fixed in f3a18a8. When the check fails, the 503 body is now a fixed message ("cannot check whether workspace "" exists right now; try again later"), and the underlying error goes to slog.Error instead. TestCreateTenant_UncheckableRefuses feeds in an error containing an absolute path and asserts that the body contains neither the path nor lstat. Putting err.Error() back in the body fails that test.
| // provisioning. A checkout that cannot answer refuses too: "could not | ||
| // check" is not "does not exist". | ||
| if opName == "create-tenant" && !user.IsAdmin() { | ||
| if status, msg := h.checkTenantIsNew(tenantParam); status != 0 { |
There was a problem hiding this comment.
P3: check-then-submit race — checkTenantIsNew only consults the periodically-synced gitops checkout (refreshed every minute, tolerated stale up to TenantExistsMaxAge=10m), not in-flight submissions. Two concurrent non-admin requests for the same brand-new tenant name can both pass this check and both get submitted before either lands in the checkout. This is a pre-existing class of race (not introduced here) and the PR's own description scopes the fix to the already-exists case, so not blocking, but worth flagging since de-duplicating concurrent creates was part of the motivation.
There was a problem hiding this comment.
Agreed that this is check-then-submit, and it is deliberately not the only guard. Since mctlhq/mctl-gitops#1650 (merged), wft-create-tenant refuses an existing tenant in two places, without reprovision=true:
- in validate, on a fresh clone;
- in the commit step, for a tenant that appeared between validate and its own clone.
So a duplicate that slips past this API check within the sync window still fails loudly in the workflow, instead of provisioning. This check is the early, cheap answer. Leaving it as is.
There was a problem hiding this comment.
No P1/P2 findings (2 P3). Reviewed internal/gitops/reader.go (TenantExists: name validated as a single path segment, lock usage matches GetTenant/ListTenants, correctly distinguishes ErrNotExist from other stat errors, staleness bound respected), internal/api/handlers_write.go (check runs after validation/before submit, tenantParam sourced from the already-validated tenant_name, admin bypass correct, audit logged on denial), and the new tests (good coverage per the PR's own mutation-testing claims). P3 notes left inline: (1) wrapped stat errors may leak server filesystem paths to API clients via the 503 body — consistent with existing convention elsewhere in this handler, not a regression; (2) a check-then-submit race lets two concurrent non-admin requests for the same brand-new tenant name both pass and both submit — pre-existing race class, out of scope for this fix. Good to merge.
TenantExists now accepts only create-tenant's tenant_name pattern and checks that the cleaned path stays under platform-gitops/tenants/ before the lstat, so the path read is provably one entry of that directory. The 503 for a failed check no longer echoes the checkout error, which can carry absolute paths; it is logged instead. Claude-Session: https://claude.ai/code/session_01D1kGUJMWeA8hyhQuxKuHd2
|
Claude finished @mashkovd's task in 1m 55s —— View job Review complete for PR #452 (commit
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
There was a problem hiding this comment.
No P1/P2 findings (0 P3). This commit cleanly addresses both the CodeQL path-taint alert and the prior P3 (error-message leak) from the review of bb33ca3: TenantExists now validates the name against the same tenant_name pattern used elsewhere (single DNS-safe path segment), adds a filepath.IsLocal + root-prefix containment check before Lstat, and the handler now logs the raw stat error via slog.Error instead of embedding it in the 503 body. Verified by static review (go build/test blocked by sandbox approval in this run).
What
POST /api/v1/operations/create-tenant/execute(also used by the MCPmctl_create_tenanttool) now checks, for non-admin callers, whetherplatform-gitops/tenants/<tenant_name>already exists in the API's gitops checkout before submitting anything:409 workspace "<name>" already exists;503. That covers a checkout that never synced, a last successful sync older than 10 minutes (gitops.TenantExistsMaxAge), or a stat error other than "not found";Every refusal is audited as
denied. Admins are not checked.Why
create-tenant makes a new tenant. When the tenant directory already exists,
wft-create-tenantskips writing the tenant files (SKIP_GIT, added in 935f0bbf as the retry path for a half-finished run) and carries on with Vault and the ApplicationSet. A request for a name that is already taken should get a clear answer, not a workflow that "succeeds" without creating anything.Admins are not checked here; for them, the workflow decides. Today it re-runs provisioning. With mctlhq/mctl-gitops#1650 it fails in validate unless submitted with
reprovision=true, which the API does not declare, so a re-run is a deliberate Argo submission. Non-admins could never use that path for their own tenant anyway. Once the first run commits the tenant, its creator is a member, and the one-workspace rule refuses them.Changes
internal/gitops/reader.go: addsTenantExists(name) (bool, error). It rejects names that are not a single path segment. OnlyErrNotExistcounts as absent; any other error is reported as an error.internal/api/interfaces.go:GitReadergainsTenantExists.internal/api/handlers_write.go: the check runs after input validation and before submit.internal/openapi/openapi.yaml: documents the 409 and 503 responses on execute.Tests
internal/api/create_tenant_existing_test.go:internal/gitops/reader_test.go:values.yaml, and a plain file all count as existing;go test ./...passes.golangci-lint runreports only the gosec G710 atinternal/api/router.go:274that is already on main.Type of change
Checklist
go test ./...)🤖 Generated with Claude Code
https://claude.ai/code/session_01D1kGUJMWeA8hyhQuxKuHd2