stop serving another node's factset

Also strip a leading BOM and stop reporting a certname mismatch as a parse error.

internal/pdbmux/pdbmux.go

@@ -83,7 +83,7 @@ func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, er
 
 	records, err := decodeFactsets(body, certname)
 	if err != nil {
-		return nil, fmt.Errorf("parsing factset for %s: %w", certname, err)
+		return nil, err
 	}
 	if len(records) == 0 {
 		return nil, fmt.Errorf("%w: %s", ErrNodeNotFound, certname)
@@ -93,34 +93,60 @@ func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, er
 
 // decodeFactsets reads the two shapes /factsets/<certname> comes back as: real
 // PuppetDB returns an array of factsets, pdbmux currently returns a single
-// bare object.
+// bare object. A factset belonging to another node is never substituted for
+// the one that was asked for.
 func decodeFactsets(body []byte, certname string) ([]factRecord, error) {
+	body = bytes.TrimPrefix(body, []byte("\xef\xbb\xbf"))
+
 	if bytes.HasPrefix(bytes.TrimLeft(body, " \t\r\n"), []byte("[")) {
 		var sets []factset
 		if err := json.Unmarshal(body, &sets); err != nil {
-			return nil, err
-		}
-		for _, fs := range sets {
-			if fs.Certname == certname {
-				return fs.Facts.Data, nil
-			}
-		}
-		if len(sets) == 1 {
-			return sets[0].Facts.Data, nil
+			return nil, fmt.Errorf("parsing factset for %s: %w", certname, err)
 		}
 		if len(sets) == 0 {
 			return nil, nil
 		}
-		return nil, fmt.Errorf("%d factsets returned, none for the requested certname", len(sets))
+		for _, fs := range sets {
+			if sameCertname(fs.Certname, certname) {
+				return fs.Facts.Data, nil
+			}
+		}
+		if len(sets) == 1 && sets[0].Certname == "" {
+			return sets[0].Facts.Data, nil
+		}
+		return nil, fmt.Errorf("no factset for %s: pdbmux returned %s",
+			certname, strings.Join(certnames(sets), ", "))
 	}
 
 	var fs factset
 	if err := json.Unmarshal(body, &fs); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("parsing factset for %s: %w", certname, err)
+	}
+	if fs.Certname != "" && !sameCertname(fs.Certname, certname) {
+		return nil, fmt.Errorf("no factset for %s: pdbmux returned %s", certname, fs.Certname)
 	}
 	return fs.Facts.Data, nil
 }
 
+// sameCertname compares certnames case-insensitively: they are DNS names, and
+// PuppetDB matches the per-node route exactly, so folding can only absorb a
+// backend that normalised case, never mask a different node.
+func sameCertname(got, want string) bool {
+	return got != "" && strings.EqualFold(got, want)
+}
+
+func certnames(sets []factset) []string {
+	names := make([]string, 0, len(sets))
+	for _, fs := range sets {
+		if fs.Certname == "" {
+			names = append(names, "(unnamed)")
+			continue
+		}
+		names = append(names, fs.Certname)
+	}
+	return names
+}
+
 func (c *Client) get(ctx context.Context, path string) ([]byte, int, error) {
 	req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.BaseURL+path, nil)
 	if err != nil {

internal/pdbmux/pdbmux_test.go

@@ -225,8 +225,107 @@ func TestFactsRejectsAFactsetArrayWithNoMatch(t *testing.T) {
 	if err == nil {
 		t.Fatal("expected an error when no factset matches the certname")
 	}
-	if !strings.Contains(err.Error(), "none for the requested certname") {
-		t.Errorf("unhelpful error: %v", err)
+	assertNamesBoth(t, err, "host.example.net", "a.example.net", "b.example.net")
+}
+
+// assertNamesBoth checks the mismatch error names the node asked for and the
+// ones that came back, and that it is not dressed up as a parse failure.
+func assertNamesBoth(t *testing.T, err error, want string, got ...string) {
+	t.Helper()
+	if !strings.Contains(err.Error(), want) {
+		t.Errorf("error does not name the requested certname %s: %v", want, err)
+	}
+	for _, g := range got {
+		if !strings.Contains(err.Error(), g) {
+			t.Errorf("error does not name the returned certname %s: %v", g, err)
+		}
+	}
+	if strings.Contains(err.Error(), "parsing factset") {
+		t.Errorf("a certname mismatch is not a parse failure: %v", err)
+	}
+}
+
+func TestFactsRejectsASoleFactsetForAnotherNode(t *testing.T) {
+	srv := factsetServer(t, `[{
+	  "certname":"totally-different-host.example.net",
+	  "facts":{"data":[
+	    {"name":"id","value":"nobody"},
+	    {"name":"os","value":{"family":"Debian"}}
+	  ]}
+	}]`)
+
+	facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net")
+	if err == nil {
+		t.Fatalf("another node's facts were served as ours: %#v", facts)
+	}
+	assertNamesBoth(t, err, "host.example.net", "totally-different-host.example.net")
+}
+
+func TestFactsRejectsABareFactsetForAnotherNode(t *testing.T) {
+	srv := factsetServer(t, `{
+	  "certname":"totally-different-host.example.net",
+	  "facts":{"data":[{"name":"id","value":"nobody"}]}
+	}`)
+
+	facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net")
+	if err == nil {
+		t.Fatalf("another node's facts were served as ours: %#v", facts)
+	}
+	assertNamesBoth(t, err, "host.example.net", "totally-different-host.example.net")
+}
+
+// The per-node route already names the node, so a factset that asserts no
+// certname of its own cannot be contradicting it.
+func TestFactsAcceptsAFactsetWithNoCertname(t *testing.T) {
+	body := `{"facts":{"data":[
+	  {"name":"id","value":"root"},
+	  {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}},
+	  {"name":"processorcount","value":4}
+	]}}`
+
+	for name, resp := range map[string]string{"object": body, "array": "[" + body + "]"} {
+		t.Run(name, func(t *testing.T) {
+			srv := factsetServer(t, resp)
+			facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net")
+			if err != nil {
+				t.Fatalf("Facts: %v", err)
+			}
+			assertFacts(t, facts)
+		})
+	}
+}
+
+func TestFactsMatchesCertnameCaseInsensitively(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if strings.HasPrefix(r.URL.Path, "/pdb/query/v4/factsets/") {
+			_, _ = w.Write([]byte(factsetBody))
+			return
+		}
+		w.WriteHeader(http.StatusNotFound)
+		_, _ = w.Write([]byte(`{"error":"unknown node"}`))
+	}))
+	defer srv.Close()
+
+	facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "HOST.example.net")
+	if err != nil {
+		t.Fatalf("Facts: %v", err)
+	}
+	assertFacts(t, facts)
+}
+
+func TestFactsToleratesALeadingBOM(t *testing.T) {
+	for name, resp := range map[string]string{
+		"array":  "\ufeff" + factsetBody,
+		"object": "\ufeff" + bareFactsetBody,
+	} {
+		t.Run(name, func(t *testing.T) {
+			srv := factsetServer(t, resp)
+			facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net")
+			if err != nil {
+				t.Fatalf("Facts: %v", err)
+			}
+			assertFacts(t, facts)
+		})
 	}
 }