diff --git a/internal/handlers/deploy_private.go b/internal/handlers/deploy_private.go index 832ddff..2bffd73 100644 --- a/internal/handlers/deploy_private.go +++ b/internal/handlers/deploy_private.go @@ -8,12 +8,14 @@ package handlers // deploy.go calls parsePrivateDeployFields once before persisting the row. import ( + "errors" "fmt" "net" "strings" "github.com/gofiber/fiber/v2" "instant.dev/internal/middleware" + "instant.dev/internal/models" "log/slog" @@ -60,6 +62,33 @@ func parsePrivateDeployFields(c *fiber.Ctx, form *multipart.Form, planTier strin return false, nil, nil } + entries := splitAllowedIPsField(rawAllowedIPs) + return validatePrivateDeployFields(c, planTier, true, entries) +} + +// validatePrivateDeployFields is the shared validation routine used by both +// the POST /deploy/new multipart flow (parsePrivateDeployFields) and the +// PATCH /api/v1/deployments/:id JSON flow (DeployHandler.Patch). Centralising +// the rule-set guarantees the two surfaces can't drift on a contract that the +// U3 reviewer audits as a single rule-set. +// +// Inputs: +// - planTier: team.PlanTier (e.g. "hobby", "pro"). Used for the tier gate. +// - private: the parsed private boolean. +// - allowedIPs: already-split, already-trimmed entries (nil/empty allowed +// only when private=false). +// +// On failure, writes the 400/402 response inline and returns a non-nil error +// (same pattern as the multipart helper). On success returns +// (private, allowedIPs, nil) — the slice is returned verbatim so the +// caller doesn't have to keep its own copy. +func validatePrivateDeployFields(c *fiber.Ctx, planTier string, private bool, allowedIPs []string) (bool, []string, error) { + if !private { + // Public — the caller is responsible for ignoring allowedIPs on this + // path. No tier gate (every tier can run a public deploy). + return false, nil, nil + } + // Tier gate FIRST — hides downstream validation rules from tiers that // don't have access to the feature at all. if !privateDeployAllowedTiers[planTier] { @@ -72,8 +101,7 @@ func parsePrivateDeployFields(c *fiber.Ctx, form *multipart.Form, planTier strin } // Required-field gate. - entries := splitAllowedIPsField(rawAllowedIPs) - if len(entries) == 0 { + if len(allowedIPs) == 0 { return false, nil, respondErrorWithAgentAction(c, fiber.StatusBadRequest, "private_deploy_requires_allowed_ips", @@ -86,17 +114,17 @@ func parsePrivateDeployFields(c *fiber.Ctx, form *multipart.Form, planTier strin // list would otherwise burn CPU through 200 net.ParseCIDR calls before // being rejected anyway. 32 is the max we'll ever stuff into an nginx // annotation responsibly; bigger lists belong in CF Access. - if len(entries) > maxAllowedIPs { + if len(allowedIPs) > maxAllowedIPs { return false, nil, respondError(c, fiber.StatusBadRequest, "too_many_allowed_ips", fmt.Sprintf("allowed_ips has %d entries; max is %d. For larger allowlists use a real VPN or Cloudflare Access — see https://instanode.dev/docs/private-deploys.", - len(entries), maxAllowedIPs)) + len(allowedIPs), maxAllowedIPs)) } // Per-entry validation. Surface the bad literal verbatim — the LLM agent // gets to feed the typo back to the human. - for _, entry := range entries { + for _, entry := range allowedIPs { if !isValidIPOrCIDR(entry) { return false, nil, respondError(c, fiber.StatusBadRequest, @@ -105,7 +133,158 @@ func parsePrivateDeployFields(c *fiber.Ctx, form *multipart.Form, planTier strin } } - return true, entries, nil + return true, allowedIPs, nil +} + +// patchAccessControlBody is the JSON body for PATCH /api/v1/deployments/:id. +// +// Both fields are optional pointers so the handler can distinguish "field +// omitted" (keep current state) from "field set to zero" (private=false / +// allowed_ips=[]). REST PATCH semantics: send only what you want to change. +// +// Semantics decision (REPLACE, not APPEND): when allowed_ips is supplied, the +// new slice REPLACES the current list rather than merging into it. This +// matches REST conventions for collection fields and is what the dashboard +// PrivacyPanel expects — the editor renders the current list, the user +// edits it, and submits the new authoritative list. Append semantics would +// silently grow the allow-list over multiple PATCHes (a known footgun for +// "I removed an IP but it's still there" bug reports). +type patchAccessControlBody struct { + Private *bool `json:"private,omitempty"` + AllowedIPs *[]string `json:"allowed_ips,omitempty"` +} + +// Patch handles PATCH /api/v1/deployments/:id for in-place access-control +// edits — flipping a deploy public ↔ private or replacing the allowed_ips +// list. Does NOT rebuild the image; the apply-annotation helper that backs +// POST /deploy/new is reused so the two paths can't diverge. +// +// Behaviour matrix: +// +// - {private:true, allowed_ips:[...]} → set private, set list +// - {allowed_ips:[...]} only → keep current private; update list +// (rejected if currently public — can't have allow-list on public deploy) +// - {private:false} → clear allow-list, set public +// - {private:true} only, no allow_ips → 400 (need allowed_ips) +// - {} empty body → 400 (nothing to change) +// +// All validation routes through validatePrivateDeployFields so the rule-set +// (tier gate → required IPs → cap → per-entry parse) is byte-identical to +// POST /deploy/new. The compute.Provider.UpdateAccessControl call patches +// the live Ingress; the models.UpdateDeploymentAccessControl call persists +// the row. Compute runs first because if it fails we don't want the DB to +// claim a state the Ingress can't enforce — but we also have to handle the +// reverse: if the Ingress doesn't exist yet (deploy is still building), the +// k8s provider returns nil so the DB is still updated and the next runDeploy +// picks up the fields. +func (h *DeployHandler) Patch(c *fiber.Ctx) error { + team, err := h.requireTeam(c) + if err != nil { + return err + } + + appID := c.Params("id") + d, err := models.GetDeploymentByAppID(c.Context(), h.db, appID) + if err != nil { + var notFound *models.ErrDeploymentNotFound + if errors.As(err, ¬Found) { + return respondError(c, fiber.StatusNotFound, "not_found", "Deployment not found") + } + return respondError(c, fiber.StatusServiceUnavailable, "fetch_failed", "Failed to fetch deployment") + } + + if d.TeamID != team.ID { + return respondError(c, fiber.StatusForbidden, "forbidden", "You do not own this deployment") + } + + var body patchAccessControlBody + if err := c.BodyParser(&body); err != nil { + return respondError(c, fiber.StatusBadRequest, "invalid_body", + "Request body must be valid JSON: {\"private\": bool, \"allowed_ips\": [\"ip\",\"cidr\"]}") + } + + if body.Private == nil && body.AllowedIPs == nil { + return respondError(c, fiber.StatusBadRequest, "missing_fields", + "At least one of 'private' or 'allowed_ips' must be supplied") + } + + // Resolve the post-PATCH (private, allowed_ips) pair from the current + // state + the supplied deltas. Sending only allowed_ips keeps the + // current private flag (so a Pro user can edit their list without + // having to also resend private=true). Sending private=false clears + // the allow-list to empty regardless of what allowed_ips contains — + // the public-deploy invariant is "no whitelist annotation". + newPrivate := d.Private + if body.Private != nil { + newPrivate = *body.Private + } + + var newAllowedIPs []string + switch { + case body.Private != nil && !*body.Private: + // Explicit public — drop the list entirely regardless of allowed_ips + // in the same body. Prevents the surface "I set private=false but + // the allow-list is still there" bug. + newAllowedIPs = nil + case body.AllowedIPs != nil: + // Caller supplied a new authoritative list (REPLACE semantics). + newAllowedIPs = *body.AllowedIPs + default: + // allowed_ips omitted, private flipped (or unchanged) but stays + // private — preserve the existing list verbatim. + newAllowedIPs = d.AllowedIPs + } + + // Run through the shared validation rule-set. Tier gate fires first so + // hobby callers can't drill past it via "PATCH the public deploy I + // already have to private". The team's CURRENT plan tier is what's + // checked (matches POST semantics) — not the snapshot on the deployment + // row. + validatedPrivate, validatedAllowedIPs, vErr := validatePrivateDeployFields(c, team.PlanTier, newPrivate, newAllowedIPs) + if vErr != nil { + return vErr + } + + // Compute-side first. The Ingress lives in k8s and a successful k8s + // update is the truth that matters to inbound traffic. If this fails, + // we surface 503 and skip the DB write so the row keeps reflecting + // reality. + if err := h.compute.UpdateAccessControl(c.Context(), d.AppID, validatedPrivate, validatedAllowedIPs); err != nil { + slog.Error("deploy.patch.compute_update_failed", + "app_id", appID, "error", err, + "request_id", middleware.GetRequestID(c)) + return respondError(c, fiber.StatusServiceUnavailable, "compute_update_failed", + "Failed to update ingress access control") + } + + if err := models.UpdateDeploymentAccessControl(c.Context(), h.db, d.ID, validatedPrivate, validatedAllowedIPs); err != nil { + slog.Error("deploy.patch.db_update_failed", + "app_id", appID, "error", err, + "request_id", middleware.GetRequestID(c)) + return respondError(c, fiber.StatusServiceUnavailable, "update_failed", + "Failed to update deployment access control") + } + + // Re-fetch so the response reflects the persisted row (status, updated_at). + updated, err := models.GetDeploymentByAppID(c.Context(), h.db, appID) + if err != nil { + // Update succeeded but read-back failed — return the in-memory + // representation we just wrote so the dashboard isn't blocked. + d.Private = validatedPrivate + d.AllowedIPs = validatedAllowedIPs + updated = d + } + + slog.Info("deploy.patch.access_control_updated", + "app_id", appID, "team_id", team.ID, + "private", validatedPrivate, + "allowed_ip_count", len(validatedAllowedIPs), + "request_id", middleware.GetRequestID(c)) + + return c.JSON(fiber.Map{ + "ok": true, + "item": deploymentToMap(updated), + }) } // firstFormValue returns the first value for a multipart field, or "" when diff --git a/internal/handlers/deploy_private_patch_test.go b/internal/handlers/deploy_private_patch_test.go new file mode 100644 index 0000000..12323e8 --- /dev/null +++ b/internal/handlers/deploy_private_patch_test.go @@ -0,0 +1,441 @@ +package handlers_test + +// deploy_private_patch_test.go — PATCH /api/v1/deployments/:id for in-place +// access-control edits (private + allowed_ips). +// +// Seven cases, mirroring the brief's spec: +// +// 1. PATCH {private:true, allowed_ips:[...]} on existing Pro deploy → 200 +// 2. PATCH replacing allowed_ips on existing private deploy → 200 (REPLACE) +// 3. PATCH {private:false} clears allow-list → 200 +// 4. PATCH on hobby tier flipping private → 402 with agent_action +// 5. PATCH with invalid IP → 400 with the bad literal surfaced +// 6. PATCH on missing deploy → 404 +// 7. PATCH cross-team → 403 +// +// All tests run against the noop compute provider — same as the POST suite. +// The handler-level contract (status codes, error keys, agent_action, JSON +// shape) is what's under test; the live Ingress patch is exercised by the +// k8s provider tests. + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "mime/multipart" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "instant.dev/internal/testhelpers" +) + +// createPrivateDeploy is a small test-only helper that posts a private +// deployment as a Pro team and returns the app_id. We piggy-back on the +// already-tested POST surface so the PATCH tests don't have to keep their +// own DB-insertion logic in sync with CreateDeploymentParams. +func createPrivateDeploy(t *testing.T, app httpTester, sessionJWT, initialIPs string) string { + t.Helper() + body := &bytes.Buffer{} + w := multipart.NewWriter(body) + fw, err := w.CreateFormFile("tarball", "app.tar.gz") + require.NoError(t, err) + _, err = fw.Write([]byte("fake-tarball-bytes")) + require.NoError(t, err) + require.NoError(t, w.WriteField("private", "true")) + require.NoError(t, w.WriteField("allowed_ips", initialIPs)) + require.NoError(t, w.Close()) + + req := httptest.NewRequest(http.MethodPost, "/deploy/new", body) + req.Header.Set("Content-Type", w.FormDataContentType()) + req.Header.Set("Authorization", "Bearer "+sessionJWT) + req.Header.Set("X-Forwarded-For", "10.30.0.1") + resp, err := app.Test(req, 10000) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusAccepted, resp.StatusCode, "precondition: POST /deploy/new must succeed before PATCH tests can run") + + var created struct { + Item struct { + AppID string `json:"app_id"` + } `json:"item"` + } + require.NoError(t, json.NewDecoder(resp.Body).Decode(&created)) + require.NotEmpty(t, created.Item.AppID) + return created.Item.AppID +} + +// createPublicDeploy is the same as createPrivateDeploy but with no privacy +// fields — produces a baseline deploy we can flip private via PATCH. +func createPublicDeploy(t *testing.T, app httpTester, sessionJWT string) string { + t.Helper() + body := &bytes.Buffer{} + w := multipart.NewWriter(body) + fw, err := w.CreateFormFile("tarball", "app.tar.gz") + require.NoError(t, err) + _, err = fw.Write([]byte("fake-tarball-bytes")) + require.NoError(t, err) + require.NoError(t, w.Close()) + + req := httptest.NewRequest(http.MethodPost, "/deploy/new", body) + req.Header.Set("Content-Type", w.FormDataContentType()) + req.Header.Set("Authorization", "Bearer "+sessionJWT) + req.Header.Set("X-Forwarded-For", "10.30.0.2") + resp, err := app.Test(req, 10000) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusAccepted, resp.StatusCode) + + var created struct { + Item struct { + AppID string `json:"app_id"` + } `json:"item"` + } + require.NoError(t, json.NewDecoder(resp.Body).Decode(&created)) + return created.Item.AppID +} + +// httpTester is the minimal subset of *fiber.App we use here. Defined to keep +// the helper signatures readable without importing fiber. +type httpTester interface { + Test(*http.Request, ...int) (*http.Response, error) +} + +// jsonPatch builds an http.Request for PATCH /api/v1/deployments/:id with a +// JSON body. Centralised so each test case is two-line readable. +func jsonPatch(t *testing.T, appID, sessionJWT string, body any) *http.Request { + t.Helper() + buf, err := json.Marshal(body) + require.NoError(t, err) + req := httptest.NewRequest(http.MethodPatch, "/api/v1/deployments/"+appID, bytes.NewReader(buf)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+sessionJWT) + req.Header.Set("X-Forwarded-For", "10.30.0.99") + return req +} + +// TestDeployPatch_Pro_SetsPrivate is case 1: PATCH a public Pro deploy → +// private with a real IP list. The handler must flip the row and emit the +// new private + allowed_ips in the response, no rebuild required. +func TestDeployPatch_Pro_SetsPrivate(t *testing.T) { + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + teamID := testhelpers.MustCreateTeamDB(t, db, "pro") + sessionJWT := testhelpers.MustSignSessionJWT(t, "a0000000-0000-0000-0000-000000000001", teamID, "agent-patch-pro@example.com") + + app, cleanApp := testhelpers.NewTestAppWithServices(t, db, rdb, "postgres,redis,mongodb,queue,webhook,storage,deploy") + defer cleanApp() + + appID := createPublicDeploy(t, app, sessionJWT) + + req := jsonPatch(t, appID, sessionJWT, map[string]any{ + "private": true, + "allowed_ips": []string{"1.2.3.4", "10.0.0.0/8"}, + }) + resp, err := app.Test(req, 10000) + require.NoError(t, err) + defer resp.Body.Close() + bodyBytes, _ := io.ReadAll(resp.Body) + + require.Equal(t, http.StatusOK, resp.StatusCode, + "PATCH flipping public→private with valid IPs must be 200; got %d, body: %s", resp.StatusCode, string(bodyBytes)) + + var got struct { + Item struct { + Private bool `json:"private"` + AllowedIPs []string `json:"allowed_ips"` + } `json:"item"` + } + require.NoError(t, json.Unmarshal(bodyBytes, &got)) + assert.True(t, got.Item.Private, "private must round-trip true on the response") + assert.Equal(t, []string{"1.2.3.4", "10.0.0.0/8"}, got.Item.AllowedIPs, + "allowed_ips must be the new list verbatim") + + // Confirm via GET that the row was actually persisted (not just echoed). + getReq := httptest.NewRequest(http.MethodGet, "/deploy/"+appID, nil) + getReq.Header.Set("Authorization", "Bearer "+sessionJWT) + getResp, err := app.Test(getReq, 5000) + require.NoError(t, err) + defer getResp.Body.Close() + var fetched struct { + Item struct { + Private bool `json:"private"` + AllowedIPs []string `json:"allowed_ips"` + } `json:"item"` + } + require.NoError(t, json.NewDecoder(getResp.Body).Decode(&fetched)) + assert.True(t, fetched.Item.Private) + assert.Equal(t, []string{"1.2.3.4", "10.0.0.0/8"}, fetched.Item.AllowedIPs) +} + +// TestDeployPatch_ReplacesAllowedIPs is case 2: PATCH with only allowed_ips +// REPLACES the existing list (not appends). The brief explicitly picks +// REPLACE semantics — this test is the contract test that documents it. +func TestDeployPatch_ReplacesAllowedIPs(t *testing.T) { + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + teamID := testhelpers.MustCreateTeamDB(t, db, "pro") + sessionJWT := testhelpers.MustSignSessionJWT(t, "a0000000-0000-0000-0000-000000000002", teamID, "agent-patch-replace@example.com") + + app, cleanApp := testhelpers.NewTestAppWithServices(t, db, rdb, "postgres,redis,mongodb,queue,webhook,storage,deploy") + defer cleanApp() + + // Existing private deploy with ["1.1.1.1","2.2.2.2"]. + appID := createPrivateDeploy(t, app, sessionJWT, "1.1.1.1,2.2.2.2") + + // PATCH with ONLY allowed_ips (no `private` field). private must stay + // true; the list must REPLACE (not append). + req := jsonPatch(t, appID, sessionJWT, map[string]any{ + "allowed_ips": []string{"9.9.9.9"}, + }) + resp, err := app.Test(req, 10000) + require.NoError(t, err) + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode) + + var got struct { + Item struct { + Private bool `json:"private"` + AllowedIPs []string `json:"allowed_ips"` + } `json:"item"` + } + require.NoError(t, json.NewDecoder(resp.Body).Decode(&got)) + assert.True(t, got.Item.Private, "omitting `private` must preserve the existing private=true") + assert.Equal(t, []string{"9.9.9.9"}, got.Item.AllowedIPs, + "allowed_ips PATCH must REPLACE the existing list — append semantics would leave 1.1.1.1 / 2.2.2.2 in there. The brief explicitly chose REPLACE.") + assert.NotContains(t, got.Item.AllowedIPs, "1.1.1.1", + "old IPs must be gone — append-style merging would be a silent allow-list growth bug.") +} + +// TestDeployPatch_PrivateFalseClearsList is case 3: PATCH {private:false} +// drops the allow-list to empty regardless of allowed_ips in the same body. +// The invariant "public deploy has no whitelist annotation" is what's under +// test — a public deploy with a residual allow-list would be a UX trap. +func TestDeployPatch_PrivateFalseClearsList(t *testing.T) { + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + teamID := testhelpers.MustCreateTeamDB(t, db, "pro") + sessionJWT := testhelpers.MustSignSessionJWT(t, "a0000000-0000-0000-0000-000000000003", teamID, "agent-patch-public@example.com") + + app, cleanApp := testhelpers.NewTestAppWithServices(t, db, rdb, "postgres,redis,mongodb,queue,webhook,storage,deploy") + defer cleanApp() + + appID := createPrivateDeploy(t, app, sessionJWT, "1.1.1.1,2.2.2.2,3.3.3.3") + + req := jsonPatch(t, appID, sessionJWT, map[string]any{ + "private": false, + }) + resp, err := app.Test(req, 10000) + require.NoError(t, err) + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode) + + var got struct { + Item struct { + Private bool `json:"private"` + AllowedIPs []string `json:"allowed_ips"` + } `json:"item"` + } + require.NoError(t, json.NewDecoder(resp.Body).Decode(&got)) + assert.False(t, got.Item.Private, "private=false must persist") + assert.Equal(t, []string{}, got.Item.AllowedIPs, + "public deploy must have an empty allow-list — keeping the prior list would create a 'public but with residual rules' UX trap.") +} + +// TestDeployPatch_Hobby_Returns402 is case 4: a hobby team trying to flip a +// deploy private hits the 402 wall with the same agent_action POST emits. +// Reuses AgentActionPrivateDeployRequiresPro — no separate constant for the +// PATCH path. +func TestDeployPatch_Hobby_Returns402(t *testing.T) { + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + teamID := testhelpers.MustCreateTeamDB(t, db, "hobby") + sessionJWT := testhelpers.MustSignSessionJWT(t, "a0000000-0000-0000-0000-000000000004", teamID, "agent-patch-hobby@example.com") + + app, cleanApp := testhelpers.NewTestAppWithServices(t, db, rdb, "postgres,redis,mongodb,queue,webhook,storage,deploy") + defer cleanApp() + + // Hobby can create a public deploy fine. + appID := createPublicDeploy(t, app, sessionJWT) + + req := jsonPatch(t, appID, sessionJWT, map[string]any{ + "private": true, + "allowed_ips": []string{"1.2.3.4"}, + }) + resp, err := app.Test(req, 10000) + require.NoError(t, err) + defer resp.Body.Close() + + require.Equal(t, http.StatusPaymentRequired, resp.StatusCode, + "hobby tier flipping private must be 402 — the contract is identical to POST /deploy/new") + + var errBody struct { + Error string `json:"error"` + AgentAction string `json:"agent_action"` + UpgradeURL string `json:"upgrade_url"` + } + require.NoError(t, json.NewDecoder(resp.Body).Decode(&errBody)) + assert.Equal(t, "private_deploy_requires_pro", errBody.Error, + "error key must match POST so dashboards branch on a single code") + assert.True(t, strings.HasPrefix(errBody.AgentAction, "Tell the user"), + "agent_action must satisfy the U3 contract") + assert.Contains(t, errBody.AgentAction, "https://instanode.dev/pricing", + "agent_action must contain the full upgrade URL verbatim") + assert.Equal(t, "https://instanode.dev/pricing", errBody.UpgradeURL, + "upgrade_url must be set so the dashboard can render the CTA without parsing the sentence") +} + +// TestDeployPatch_InvalidIP_Returns400 is case 5: an invalid CIDR/IP literal +// must surface verbatim in the 400 message — same behaviour as POST so the +// agent can feed the typo back to the human. +func TestDeployPatch_InvalidIP_Returns400(t *testing.T) { + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + teamID := testhelpers.MustCreateTeamDB(t, db, "pro") + sessionJWT := testhelpers.MustSignSessionJWT(t, "a0000000-0000-0000-0000-000000000005", teamID, "agent-patch-bad-ip@example.com") + + app, cleanApp := testhelpers.NewTestAppWithServices(t, db, rdb, "postgres,redis,mongodb,queue,webhook,storage,deploy") + defer cleanApp() + + appID := createPublicDeploy(t, app, sessionJWT) + + const badEntry = "999.bad.literal/16" + req := jsonPatch(t, appID, sessionJWT, map[string]any{ + "private": true, + "allowed_ips": []string{"1.2.3.4", badEntry}, + }) + resp, err := app.Test(req, 10000) + require.NoError(t, err) + defer resp.Body.Close() + + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + + var errBody struct { + Error string `json:"error"` + Message string `json:"message"` + } + require.NoError(t, json.NewDecoder(resp.Body).Decode(&errBody)) + assert.Equal(t, "invalid_allowed_ip", errBody.Error, + "error key must be invalid_allowed_ip (matches POST) so agents branch identically across surfaces") + assert.Contains(t, errBody.Message, badEntry, + "message must include the bad literal verbatim — agent has to fix the exact thing the user typed; got %q", errBody.Message) +} + +// TestDeployPatch_NotFound is case 6: PATCH on a missing deploy → 404. +func TestDeployPatch_NotFound(t *testing.T) { + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + teamID := testhelpers.MustCreateTeamDB(t, db, "pro") + sessionJWT := testhelpers.MustSignSessionJWT(t, "a0000000-0000-0000-0000-000000000006", teamID, "agent-patch-404@example.com") + + app, cleanApp := testhelpers.NewTestAppWithServices(t, db, rdb, "postgres,redis,mongodb,queue,webhook,storage,deploy") + defer cleanApp() + + req := jsonPatch(t, "does-not-exist", sessionJWT, map[string]any{ + "private": true, + "allowed_ips": []string{"1.2.3.4"}, + }) + resp, err := app.Test(req, 10000) + require.NoError(t, err) + defer resp.Body.Close() + + require.Equal(t, http.StatusNotFound, resp.StatusCode, + "PATCH on a missing deploy must be 404 — must NOT leak 'forbidden' (would tell anonymous probers the id-space exists).") +} + +// TestDeployPatch_CrossTeam_Returns403 is case 7: PATCHing a deploy owned by +// another team is 403, not 404. The ID is known to exist (the auth'd caller +// owns a different deploy in the same id-space), so 403 is the honest answer +// and the dashboard can show a "this isn't yours" message instead of a +// generic 404. +func TestDeployPatch_CrossTeam_Returns403(t *testing.T) { + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + // Team A owns the deploy. + teamA := testhelpers.MustCreateTeamDB(t, db, "pro") + sessionA := testhelpers.MustSignSessionJWT(t, "a0000000-0000-0000-0000-00000000000a", teamA, "agent-patch-owner@example.com") + + // Team B tries to PATCH it. + teamB := testhelpers.MustCreateTeamDB(t, db, "pro") + sessionB := testhelpers.MustSignSessionJWT(t, "a0000000-0000-0000-0000-00000000000b", teamB, "agent-patch-attacker@example.com") + + app, cleanApp := testhelpers.NewTestAppWithServices(t, db, rdb, "postgres,redis,mongodb,queue,webhook,storage,deploy") + defer cleanApp() + + appID := createPublicDeploy(t, app, sessionA) + + req := jsonPatch(t, appID, sessionB, map[string]any{ + "private": true, + "allowed_ips": []string{"1.2.3.4"}, + }) + resp, err := app.Test(req, 10000) + require.NoError(t, err) + defer resp.Body.Close() + + require.Equal(t, http.StatusForbidden, resp.StatusCode, + "cross-team PATCH must be 403 — matches GET / DELETE behaviour on the same id-space.") +} + +// TestDeployPatch_EmptyBody_Returns400 covers a paranoid edge: an empty {} +// body must return 400 with a clear key. Avoids silent no-ops that hide +// dashboard bugs (PrivacyPanel sending the wrong shape). +func TestDeployPatch_EmptyBody_Returns400(t *testing.T) { + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + teamID := testhelpers.MustCreateTeamDB(t, db, "pro") + sessionJWT := testhelpers.MustSignSessionJWT(t, "a0000000-0000-0000-0000-00000000000e", teamID, "agent-patch-empty@example.com") + + app, cleanApp := testhelpers.NewTestAppWithServices(t, db, rdb, "postgres,redis,mongodb,queue,webhook,storage,deploy") + defer cleanApp() + + appID := createPublicDeploy(t, app, sessionJWT) + + req := jsonPatch(t, appID, sessionJWT, map[string]any{}) + resp, err := app.Test(req, 10000) + require.NoError(t, err) + defer resp.Body.Close() + + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + + var errBody struct { + Error string `json:"error"` + } + require.NoError(t, json.NewDecoder(resp.Body).Decode(&errBody)) + assert.Equal(t, "missing_fields", errBody.Error, + "empty body must surface a distinct 'missing_fields' key (not 'invalid_body') so the dashboard can branch the message.") +} + +// shut up unused-import lint when the fmt helper isn't otherwise needed — +// declared here so future cases referring to fmt.Sprintf don't break. +var _ = fmt.Sprintf diff --git a/internal/handlers/openapi.go b/internal/handlers/openapi.go index 6307a21..c0ce57b 100644 --- a/internal/handlers/openapi.go +++ b/internal/handlers/openapi.go @@ -1208,6 +1208,35 @@ const openAPISpec = `{ "404": { "description": "Not found" } } }, + "patch": { + "summary": "Update access-control fields (private + allowed_ips) in place", + "description": "Edits the private flag and allowed_ips list on an existing deployment without rebuilding the image. The dashboard PrivacyPanel writes here. Body fields are optional: sending only 'allowed_ips' keeps the current private state; sending 'private': false clears the allow-list regardless of allowed_ips. allowed_ips uses REPLACE semantics (the supplied list is the new authoritative list, not merged into the existing one) — matches REST conventions and avoids silent allow-list growth across multiple PATCHes. Validation reuses the POST /deploy/new rule-set: Pro+ tier required (returns 402 with private_deploy_requires_pro), private=true with empty allowed_ips returns 400, invalid IPs/CIDRs surface verbatim, >32 entries returns too_many_allowed_ips. Compute layer patches the live Ingress annotations via the same helper POST uses (no image rebuild, no pod restart).", + "security": [{ "bearerAuth": [] }], + "parameters": [{ "name": "id", "in": "path", "required": true, "schema": { "type": "string" } }], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "private": { "type": "boolean", "description": "Flip the deploy public ↔ private. When false, the allow-list is cleared regardless of allowed_ips in the same body." }, + "allowed_ips": { "type": "array", "items": { "type": "string" }, "description": "REPLACE the allow-list with this exact set of IPs/CIDRs. Max 32 entries; each must be a valid IP literal or CIDR." } + } + } + } + } + }, + "responses": { + "200": { "description": "Access control updated", "content": { "application/json": { "schema": { "$ref": "#/components/schemas/DeployResponse" } } } }, + "400": { "description": "Bad request — missing_fields (empty body), private_deploy_requires_allowed_ips, invalid_allowed_ip, too_many_allowed_ips, or invalid_body" }, + "401": { "description": "Unauthorized" }, + "402": { "description": "private_deploy_requires_pro — hobby/anonymous/free trying to flip a deploy private. agent_action points to https://instanode.dev/pricing." }, + "403": { "description": "Not your deployment" }, + "404": { "description": "Not found" }, + "503": { "description": "compute_update_failed (ingress patch failed) or update_failed (DB write failed)" } + } + }, "delete": { "summary": "Tear down + delete a deployment (alias of DELETE /deploy/{id})", "security": [{ "bearerAuth": [] }], diff --git a/internal/models/deployment.go b/internal/models/deployment.go index f63c2d5..cfc59c0 100644 --- a/internal/models/deployment.go +++ b/internal/models/deployment.go @@ -320,6 +320,31 @@ func UpdateDeploymentEnvVars(ctx context.Context, db *sql.DB, id uuid.UUID, envV return nil } +// UpdateDeploymentAccessControl replaces the private flag and allowed_ips list +// on an existing deployment row. Single-row UPDATE — no aggregation, +// no caching concerns. Backs PATCH /api/v1/deployments/:id. +// +// allowedIPs uses REPLACE semantics (matches the storage column shape — the +// row holds the canonical comma-joined list). Empty allowedIPs slice persists +// as an empty string in the column, which is what splitAllowedIPs reads back +// as nil — symmetric round-trip with CreateDeployment's behaviour. +// +// Caller is responsible for having validated the slice (each entry a valid +// IP or CIDR, len ≤ maxAllowedIPs, non-empty when private=true). This +// function trusts its inputs. +func UpdateDeploymentAccessControl(ctx context.Context, db *sql.DB, id uuid.UUID, private bool, allowedIPs []string) error { + allowed := JoinAllowedIPs(allowedIPs) + _, err := db.ExecContext(ctx, ` + UPDATE deployments + SET private = $1, allowed_ips = $2, updated_at = now() + WHERE id = $3 + `, private, allowed, id) + if err != nil { + return fmt.Errorf("models.UpdateDeploymentAccessControl: %w", err) + } + return nil +} + // DeleteDeployment hard-deletes a deployment row. // Compute resources are real money — no soft-delete; callers must deprovision // the k8s Deployment before calling this. diff --git a/internal/providers/compute/k8s/client.go b/internal/providers/compute/k8s/client.go index 6546d6e..3806994 100644 --- a/internal/providers/compute/k8s/client.go +++ b/internal/providers/compute/k8s/client.go @@ -1107,6 +1107,31 @@ func (p *K8sProvider) applyServiceInNS(ctx context.Context, ns, name, deployName return nodePort, nil } +// ingressWhitelistAnnotation is the nginx ingress controller annotation that +// gates inbound traffic to a whitelist of IPs/CIDRs. Centralised here so the +// create path (applyIngressForDeploy) and the update path +// (UpdateAccessControl) refer to the same key — a typo in one used to silently +// produce a public ingress. +const ingressWhitelistAnnotation = "nginx.ingress.kubernetes.io/whitelist-source-range" + +// buildIngressAccessAnnotations is the single source of truth for the access- +// control annotations applied to a deploy's Ingress. Both the create path +// (applyIngressForDeploy → POST /deploy/new) and the update path +// (UpdateAccessControl → PATCH /api/v1/deployments/:id) call this so the +// "private=true with N IPs" → annotation mapping cannot drift between the two. +// +// Returns a fresh map (callers may merge it into a larger annotations map). +// Empty allowedIPs on private=true is treated as "skip the annotation" — the +// handler validates non-empty up front; this is belt-and-suspenders against +// an accidental "allow nobody" ingress. +func buildIngressAccessAnnotations(private bool, allowedIPs []string) map[string]string { + out := map[string]string{} + if private && len(allowedIPs) > 0 { + out[ingressWhitelistAnnotation] = strings.Join(allowedIPs, ",") + } + return out +} + // applyIngressForDeploy creates an Ingress for a single-service /deploy/new app. // // Mirrors the pattern used by K8sStackProvider.createIngress: when DEPLOY_DOMAIN @@ -1152,12 +1177,11 @@ func (p *K8sProvider) applyIngressForDeploy(ctx context.Context, ns, svcName, ap }} scheme = "https" } - // Private deploy → nginx whitelist-source-range. Empty allowedIPs here - // would silently lock everyone out, so the handler enforces non-empty - // before this is reached. Belt-and-suspenders: skip the annotation when - // the slice is empty to avoid an accidental "allow nobody" Ingress. - if private && len(allowedIPs) > 0 { - annotations["nginx.ingress.kubernetes.io/whitelist-source-range"] = strings.Join(allowedIPs, ",") + // Private deploy → nginx whitelist-source-range. Centralised via + // buildIngressAccessAnnotations so the create path and the PATCH-update + // path (UpdateAccessControl) can never diverge on the annotation key. + for k, v := range buildIngressAccessAnnotations(private, allowedIPs) { + annotations[k] = v } publicURL := scheme + "://" + host @@ -1212,6 +1236,73 @@ func (p *K8sProvider) applyIngressForDeploy(ctx context.Context, ns, svcName, ap return publicURL, nil } +// UpdateAccessControl patches the access-control annotations on an existing +// deploy's Ingress without rebuilding the image. Backs PATCH +// /api/v1/deployments/:id so a Pro+ user can flip a deploy public ↔ private or +// edit the allowed_ips list in-place. +// +// Semantics: +// +// - private=false → strip the whitelist-source-range annotation entirely +// (the Ingress becomes public). allowedIPs is ignored. +// - private=true with non-empty allowedIPs → set the annotation to the +// comma-joined list (REPLACE semantics — the new list is the new truth, +// no append). +// - private=true with empty allowedIPs is a no-op at the k8s layer +// (handler validates non-empty up front; this is belt-and-suspenders). +// +// When DEPLOY_DOMAIN is unset (local dev) the deploy has no Ingress and this +// is a no-op — same warn breadcrumb the create path emits. Returns +// IsNotFound-style errors for callers that want to surface 404 separately +// from generic 503; today the handler treats either as a 503 because the +// DB row already reflects the intent and a redeploy heals divergence. +func (p *K8sProvider) UpdateAccessControl(ctx context.Context, appID string, private bool, allowedIPs []string) error { + domain := os.Getenv("DEPLOY_DOMAIN") + if domain == "" { + slog.Warn("k8s.UpdateAccessControl: DEPLOY_DOMAIN unset; no Ingress to patch — DB-only update", + "app_id", appID, + ) + return nil + } + ns := deployNamespace(appID) + name := "app-" + appID + + ing, err := p.clientset.NetworkingV1().Ingresses(ns).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + // The deploy row exists but the Ingress hasn't been created yet + // (e.g. PATCH lands during the building window). Skip — the next + // runDeploy will pick up the new private/allowed_ips from the DB + // row via opts.Private / opts.AllowedIPs. + slog.Info("k8s.UpdateAccessControl: ingress not yet created — DB-only update", + "app_id", appID, "namespace", ns) + return nil + } + return fmt.Errorf("get ingress %q in %q: %w", name, ns, err) + } + + if ing.Annotations == nil { + ing.Annotations = map[string]string{} + } + // Strip any prior whitelist annotation first so private=false reliably + // produces a public Ingress regardless of what was there before. + delete(ing.Annotations, ingressWhitelistAnnotation) + for k, v := range buildIngressAccessAnnotations(private, allowedIPs) { + ing.Annotations[k] = v + } + + if _, err := p.clientset.NetworkingV1().Ingresses(ns).Update(ctx, ing, metav1.UpdateOptions{}); err != nil { + return fmt.Errorf("update ingress %q in %q: %w", name, ns, err) + } + slog.Info("k8s.UpdateAccessControl: ingress annotations patched", + "app_id", appID, + "namespace", ns, + "private", private, + "allowed_ip_count", len(allowedIPs), + ) + return nil +} + // deployIngressURL returns the public Ingress URL for an appID if DEPLOY_DOMAIN // is configured. Caller uses this to compute the AppURL during Status/Redeploy // without re-querying the k8s API (the value is deterministic from env + appID). diff --git a/internal/providers/compute/noop/noop.go b/internal/providers/compute/noop/noop.go index a014b22..86f3afc 100644 --- a/internal/providers/compute/noop/noop.go +++ b/internal/providers/compute/noop/noop.go @@ -76,3 +76,14 @@ func (n *NoopProvider) Redeploy(_ context.Context, providerID string, _ []byte, UpdatedAt: time.Now(), }, nil } + +// UpdateAccessControl logs a warning and returns nil. Tests use this — the +// DB-only update is the user-visible change. +func (n *NoopProvider) UpdateAccessControl(_ context.Context, appID string, private bool, allowedIPs []string) error { + slog.Warn("compute.noop: UpdateAccessControl called but compute is disabled", + "app_id", appID, + "private", private, + "allowed_ip_count", len(allowedIPs), + ) + return nil +} diff --git a/internal/providers/compute/provider.go b/internal/providers/compute/provider.go index 5d5951d..e3d342a 100644 --- a/internal/providers/compute/provider.go +++ b/internal/providers/compute/provider.go @@ -55,6 +55,16 @@ type Provider interface { // Redeploy rebuilds the image from a new tarball and rolls out. Redeploy(ctx context.Context, providerID string, tarball []byte, envVars map[string]string) (*AppDeployment, error) + + // UpdateAccessControl patches the access-control annotations on an + // existing deploy's Ingress in place — no image rebuild, no pod restart. + // Backs PATCH /api/v1/deployments/:id for the private + allowed_ips + // edit flow. private=false strips the whitelist annotation; private=true + // with non-empty allowedIPs sets it (REPLACE semantics — the supplied + // list is the new truth). Implementations on backends without a real + // Ingress concept (noop, local-dev without DEPLOY_DOMAIN) should return + // nil after a slog.Warn — the DB-only update is the user-visible change. + UpdateAccessControl(ctx context.Context, appID string, private bool, allowedIPs []string) error } // TierResources returns k8s resource requests/limits for a tier. diff --git a/internal/router/router.go b/internal/router/router.go index 8cb5ac1..56fdafa 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -366,6 +366,11 @@ func New(cfg *config.Config, db *sql.DB, rdb *redis.Client, geoDbs *middleware.G api.Get("/deployments", deployH.List) api.Get("/deployments/:id", deployH.Get) api.Delete("/deployments/:id", deployH.Delete) + // PATCH edits access-control fields (private + allowed_ips) without a + // rebuild. Pro+ tier gate enforced inside the handler; shares + // validatePrivateDeployFields with POST /deploy/new so the rule-set is + // audited in one place. + api.Patch("/deployments/:id", deployH.Patch) // Stack management endpoints — Phase 6 (under /api/v1) api.Get("/stacks", stackH.List) diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go index b3e72df..691491c 100644 --- a/internal/testhelpers/testhelpers.go +++ b/internal/testhelpers/testhelpers.go @@ -402,6 +402,7 @@ func NewTestAppWithServices(t *testing.T, db *sql.DB, rdb *redis.Client, service api.Get("/deployments", deployH.List) api.Get("/deployments/:id", deployH.Get) api.Delete("/deployments/:id", deployH.Delete) + api.Patch("/deployments/:id", deployH.Patch) // A/B-experiment conversion sink — wired into the test app so // handler tests can exercise the full route stack (router +