fix(api): respondError safety + vault validation order + Go 1.26 test compat#13
Merged
Conversation
… compat
This PR surfaced from a "test everything" sweep across api/worker/
provisioner/common. Three real production bugs and three test-infra
gaps came out together; they share enough code that splitting them
would force ordering dependencies between PRs.
## 1. respondError used to silently bypass multi-return validators
respondError returned c.Status(status).JSON()'s result — nil on every
successful body write. Helper functions like resolveEnv and
requireTeamMatch composed it as:
return zeroValue, respondError(c, 400, "invalid_x", "...")
Their callers checked `if err != nil`, which was false on the happy
path of respondError (response written successfully). The handler
then continued PAST the validation gate with zeroValue, producing:
- silent acceptance of invalid env names (response said
env="production" while the URL said env=Prod)
- 500 instead of 403 when an actor's JWT team didn't match the path
:team_id (handler proceeded with uuid.Nil → DB error)
Fix:
- respondError now returns ErrResponseWritten (non-nil sentinel)
regardless of the JSON write result.
- The custom ErrorHandlers in router.go, testhelpers.go, and the
three per-test fiber.App builders (stack_test, billing_test,
vault_test, teams_test) detect this sentinel and return nil so
the original 400/403/etc. body is preserved.
The visible effect in prod (verified live on v2.2.0):
$ curl -X POST 'https://api.instanode.dev/db/new?env=Prod' -d '{}'
Before: 201 with env="production" (invalid input silently coerced)
After: 400 {"error":"invalid_env", ...}
## 2. Vault tier-check order was misleading
Hobby-tier PUT to /api/v1/vault/staging/X when already at the 20-entry
quota used to return 402 vault_quota_exceeded. The agent reading that
might add seats or upgrade for capacity — but the real block was the
env allowlist (staging isn't a hobby env). Now env check fires first
(403 vault_env_not_allowed) so the agent learns what would actually
help.
## 3. Test-infra fixes uncovered by the sweep
- vault_test space-encoding: Go 1.26 httptest.NewRequest panics
on unescaped spaces in URL paths. Pre-encode to %20.
- stack_test domain assertion: still checked "instant.dev/start" —
updated to "instanode.dev/start".
- deploy_env_vars_test body double-read: readBody(t, resp) was
being evaluated unconditionally in a require.NotEqual message
arg, consuming the body before the success-path Decode could
read it. Read once into a buffer, reuse.
- dashboardsvc/server_test resourceSelectColumns added the env
column (migration 009) — sqlmock was emitting 18 columns into a
model that scans 19, surfacing as "sql: expected 18 destination
arguments in Scan, not 19".
- testhelpers wires /api/v1/whoami (the route added in PR #6 was
in production but missing from the integration test app).
## Sweep results
Before this PR: 12 test failures across api/internal/handlers,
internal/dashboardsvc, internal/middleware.
After: zero failures. Full sweep across api, worker, provisioner,
common all green.
Verified live: v2.2.0-validation-bugs deployed; sample env=Prod
returns 400 invalid_env as expected; valid env=production still
succeeds with 201.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Test-everything sweep across api/worker/provisioner/common surfaced three real production bugs and three test-infra gaps. Shipping together because they share enough code that splitting would force ordering dependencies.
Real bugs
Test-infra fixes (revealed by the sweep)
Live verification on v2.2.0-validation-bugs in prod
```
$ curl -X POST 'https://api.instanode.dev/db/new?env=Prod' -d '{}'
→ 400 {"error":"invalid_env", "message":"env must match ^[a-z0-9-]{1,32}$ ..."}
$ curl -X POST 'https://api.instanode.dev/db/new?env=production' -d '{}'
→ 201 {"ok":true,"tier":"anonymous","env":"production",...} (regression-clean)
$ curl -X POST 'https://api.instanode.dev/db/new?env=my_env' -d '{}'
→ 400 {"error":"invalid_env"}
```
Sweep results
Full sweep across api, worker, provisioner, common all green.
Test plan
🤖 Generated with Claude Code