From e9f060e0bec3c2bda0365e40b8a90a56ce11aead Mon Sep 17 00:00:00 2001
From: jochen <jschoubben@novox.be>
Date: Mon, 12 Oct 2026 16:33:23 +0200
Subject: [PATCH] Read a plan by its id as a parameter, never as SQL (issue
 561)

PlanByID pasted its id into the query, and the id comes from a caller of
the plans verb: an id holding a quote changed the query (x' or '1'='1
answered another plan). The id is now $1, and RecentPlans's limit too, so
no plan query is built from a value.
---
 internal/inventory/plan_id_test.go | 45 ++++++++++++++++++++++++++++++
 internal/inventory/plans.go        |  7 +++--
 testdata/store-tests               |  1 +
 3 files changed, 50 insertions(+), 3 deletions(-)
 create mode 100644 internal/inventory/plan_id_test.go

diff --git a/internal/inventory/plan_id_test.go b/internal/inventory/plan_id_test.go
new file mode 100644
index 00000000..76323226
--- /dev/null
+++ b/internal/inventory/plan_id_test.go
@@ -0,0 +1,45 @@
+package inventory
+
+import (
+	"strings"
+	"testing"
+	"time"
+)
+
+// **A plan's id is a value, never SQL** (novox/hq issue 561): the id reaches PlanByID from a caller of the
+// controller's plans verb, so an id holding a quote is read as a plain id — no error from the store, and no
+// other plan answered for it.
+func TestAPlanIdHoldingAQuoteIsReadAsAPlainId(t *testing.T) {
+	inv := ForTest(t)
+	ctx := t.Context()
+	p := Plan{ID: "plan-561", Repository: "novox/mesh-controller", Commit: "abc", Created: time.Now().UTC(),
+		State: PlanBuilding, Tiers: [][]string{{"a"}}, Modules: map[string]*PlanModule{"a": {}}}
+	if err := inv.SavePlan(ctx, &p); err != nil {
+		t.Fatal(err)
+	}
+	for _, id := range []string{
+		"x' or '1'='1",            // every plan, when pasted into the query
+		"plan-561' or id like '%", // the plan above, by a pattern
+		"it's",                    // a quote alone: a syntax error, when pasted
+	} {
+		got, err := inv.PlanByID(ctx, id)
+		if err == nil {
+			t.Fatalf("PlanByID(%q) answered %s, a plan of another id", id, got.ID)
+		}
+		if want := "no plan " + id; err.Error() != want {
+			t.Fatalf("PlanByID(%q) failed with %q, not as a plain id no plan has (%q)", id, err, want)
+		}
+	}
+	// A plan whose id holds a quote is found by it, and by nothing else.
+	quoted := p
+	quoted.ID, quoted.Revision = "plan-'561'", 0
+	if err := inv.SavePlan(ctx, &quoted); err != nil {
+		t.Fatal(err)
+	}
+	if got, err := inv.PlanByID(ctx, quoted.ID); err != nil || got.ID != quoted.ID {
+		t.Fatalf("the plan with a quote in its id was not read back: %v %+v", err, got.ID)
+	}
+	if recent, err := inv.RecentPlans(ctx, 1); err != nil || len(recent) != 1 || !strings.HasPrefix(recent[0].ID, "plan-") {
+		t.Fatalf("RecentPlans with a parameter: %v %+v", err, recent)
+	}
+}
diff --git a/internal/inventory/plans.go b/internal/inventory/plans.go
index b6530854..d6933d04 100644
--- a/internal/inventory/plans.go
+++ b/internal/inventory/plans.go
@@ -623,7 +623,7 @@ func (i *Inventory) PlansSince(ctx context.Context, since time.Time) ([]Plan, er
 
 // RecentPlans is the last few plans, newest first, open or not — what the overview shows.
 func (i *Inventory) RecentPlans(ctx context.Context, limit int) ([]Plan, error) {
-	return i.plans(ctx, fmt.Sprintf(`order by created desc limit %d`, limit))
+	return i.plans(ctx, `order by created desc limit $1`, limit)
 }
 
 // NewestMergeOf is the plan, in any state, of the newest merge into a repository's branch that the mesh
@@ -638,9 +638,10 @@ func (i *Inventory) NewestMergeOf(ctx context.Context, repository, branch string
 	return plans[0], true, nil
 }
 
-// PlanByID is one plan.
+// PlanByID is one plan. The id is a parameter, never part of the query (novox/hq issue 561): it comes from a
+// caller of the plans verb.
 func (i *Inventory) PlanByID(ctx context.Context, id string) (Plan, error) {
-	plans, err := i.plans(ctx, `where id = '`+id+`'`)
+	plans, err := i.plans(ctx, `where id = $1`, id)
 	if err != nil {
 		return Plan{}, err
 	}
diff --git a/testdata/store-tests b/testdata/store-tests
index 591ba44c..cdb56e70 100644
--- a/testdata/store-tests
+++ b/testdata/store-tests
@@ -13,6 +13,7 @@
 ./internal/inventory TestAnIssuedValueIsSealedToEveryHolderOnceAndNotAgain
 ./internal/inventory TestAnIssuedValueIsSealedAgainAfterAKeyChangeOrAnAcceptance
 ./internal/inventory TestAConditionsLooksAreKeptBesideTheBucket
+./internal/inventory TestAPlanIdHoldingAQuoteIsReadAsAPlainId
 ./cmd/mesh-controller TestTheConditionsVerbReadsTheLooksTheBucketNoLongerHolds
 ./internal/inventory TestAWalksStartIsKeptAndARefusedSaveLeavesItUnsaid
 ./cmd/mesh-controller TestAWalkLetGoAndStoppedByVerbsIsSaidStartedAndEnded