node: fail closed when a required-labels spec names no label

[HosniBelfeki]

Summary

X-Sam-Required-Labels — and the identical required_labels parameter on the call_remote_tool MCP tool — is a fail-closed control. Its own schema says so: "Fails closed: the call is rejected unless the peer attests any one of them. Empty means no requirement." An empty requirement set switches the control off completely: VerifyPeerLabels returns before it opens a stream, and rankProviders skips the label filter entirely.

parseRequiredLabels skipped empty comma-separated entries and then returned whatever it had accumulated, with no check that anything had been accumulated at all. A specification that carried content but named no label therefore produced an empty set and no error:

X-Sam-Required-Labels: ,,   ->   nil, nil   ->   no requirement at all

The caller asked to be constrained, was told nothing was wrong, and was served by an arbitrary provider. Every separator-only shape behaved this way: ",", ",,", ",,,", " , ", "\t,\t", " ,, ".

This is a fail-open outcome on a fail-closed control, reachable from in-band, caller-supplied input on both the sidecar's inference surface and the MCP tool surface.

Confirmed end to end at the facade boundary, not just at the parser: with a ,, header and one eligible provider, handleCompletions forwarded the request and answered 200. The new TestFacade_Completions_ContentfulRequiredLabelsNamingNoLabelIsRejected reproduces exactly that — it sees 200 before the change and 400 with no provider reached after it.

The change

Only a blank specification now means "no requirement"; a non-blank one that yields no label is rejected:

• strings.TrimSpace(h) == "" → nil, nil, unchanged and still the documented "no requirement". Trimming also covers the whitespace-only spec (" "), which net/http already collapses on the header path but which reaches the parser verbatim through the MCP tool parameter.
• a non-blank spec that produces zero labels → an error naming the offending input.

Empty entries alongside real ones stay tolerated, so a trailing comma remains harmless — "region=eu," and " region=eu , , team=platform ,," parse exactly as before. This is deliberate and pinned by its own test, because a list written with a trailing separator is normal and tightening it would be an unrelated behaviour change.

Both call sites already surface the error and needed no changes: 400 invalid_request on the facade (openai_facade.go), and a failed tool call wrapped as invalid required_labels over MCP (mcp_handlers.go).

The fix follows the precedent already set by the sibling policy parser in the same subsystem: NewEgressPolicy in internal/sambox/route.go rejects an empty allowlist entry outright rather than skipping it, for the same reason — "an allowlist entry that silently means something other than what it looks like is how allowlists leak."

Deliberately out of scope

parseLabelsFlag in cmd/sam-node/main.go shares the skip-empty shape but is left alone: it parses a node's own declared labels, where parsing to nothing grants less rather than more, and it is re-validated by api.ValidateLabels at startup. Noting it here so the asymmetry is a recorded decision rather than an oversight.

No new module, and no new dependency: stdlib strings and fmt only, both already imported. Net diff is +98/-1 across one source file and its test file.

Tests

Four tests are added next to the change in internal/node/openai_facade_test.go. All four were run against the unmodified parser first to confirm they actually pin the bug — the two marked below fail without the fix:

• TestParseRequiredLabels_ContentfulSpecNamingNoLabelFailsClosed — pins all six separator-only shapes as errors returning a nil map. ✗ without the fix: all six returned nil error.
• TestFacade_Completions_ContentfulRequiredLabelsNamingNoLabelIsRejected — the boundary behaviour the fix exists for: asserts 400 and that f.forward is never reached. ✗ without the fix: 200, request forwarded to the provider.
• TestParseRequiredLabels_BlankSpecMeansNoRequirement — keeps "", " ", "\t", " \t " unconstrained.
• TestParseRequiredLabels_EmptyEntriesBesideRealOnesStayTolerated — guards against over-correcting the trailing-comma case.

The pre-existing TestParseRequiredLabels is untouched and still passes, including its " region=eu , team=platform ,," case.

$ go test ./internal/node/ -run 'TestParseRequiredLabels|TestFacade|TestRankProviders' -v -count=1
--- PASS: TestParseRequiredLabels (0.00s)
--- PASS: TestParseRequiredLabels_BlankSpecMeansNoRequirement (0.00s)
--- PASS: TestParseRequiredLabels_ContentfulSpecNamingNoLabelFailsClosed (0.00s)
--- PASS: TestParseRequiredLabels_EmptyEntriesBesideRealOnesStayTolerated (0.00s)
--- PASS: TestFacade_Completions_ContentfulRequiredLabelsNamingNoLabelIsRejected (0.00s)
--- PASS: TestFacade_Completions_LabelRequirement (0.00s)
--- PASS: TestFacade_Completions_LabelAttestation (0.00s)
--- PASS: TestRankProviders (0.00s)
--- PASS: TestRankProviders_LabelMatchIsExactAndCaseSensitive (0.00s)
... (all facade/scorer tests pass)
PASS
ok      github.com/google/sam/internal/node  4.495s

$ go test ./api/... ./internal/node/discovery/... -count=1
ok      github.com/google/sam/api                        0.405s
ok      github.com/google/sam/internal/node/discovery    3.056s

$ go build ./...
$ go vet ./internal/node/
$ gofmt -l internal/node/openai_scorer.go internal/node/openai_facade_test.go
(all clean, no output)

One note on local verification

Six tests in internal/node fail on my Windows workstation both with and without this change — TestListenLocalSocket, TestSidecarSocketAuthorizesWithoutToken, TestStaticServiceRegistration*, TestBaseService_InitCommandBackend_BuildsBridge, TestIdentityEvidenceTrailingSlashReturnsNotFound. They are Unix-socket and subprocess-backend tests; I verified the identical set fails on a clean checkout of this branch's merge base, so they are platform artifacts of my environment and not related to this change. make lint and make e2e-test need a Linux toolchain I do not have locally, so CI on google/sam is the authority for the full gated matrix.

[gemini-code-assist]

Code Review

This pull request updates the parseRequiredLabels function in internal/node/openai_scorer.go to handle whitespace-only inputs and reject contentful specifications that do not resolve to any valid labels (e.g., ",,"). This ensures that a caller's intent to constrain a request is not silently ignored, preventing a potential fail-open security issue. Accompanying unit tests have been added in internal/node/openai_facade_test.go to verify these new behaviors. There are no review comments, so I have no feedback to provide.

[HosniBelfeki]

@aojea — the test job on this PR is red, but the failure is unrelated to the change. Details below in case they're useful, and I'd be grateful for a re-run when you have a moment (as a fork contributor I don't have the rights to trigger one myself).

The failure

test job:

--- FAIL: TestVerifyBiscuit_Concurrent (3.96s)
    biscuit_test.go:221: Concurrent verification failed: no valid key found for verification: datalog: world runtime limit: timeout
FAIL	github.com/google/sam/internal/identity	4.906s

Worth noting because it's easy to misread from the end of the log: tests/integration passed (ok github.com/google/sam/tests/integration 259.452s), and the bare FAIL after it is the aggregate summary from go test ./.... The DHT and OIDC lines in the log are DEBUG output from tests that passed. The only failing package is internal/identity.

This PR touches two files, both in internal/node (+98/−1), and nothing in internal/identity.

Reproduced on a clean tree

I reproduced it on a clean checkout of main at de7c5dd with no changes of mine, under CPU contention — same error string, same line.

The mechanism looks like a wall-clock deadline losing to scheduler starvation rather than anything about the code under test: TestVerifyBiscuit_Concurrent runs 50 workers × 100 iterations = 5,000 concurrent VerifyBiscuit calls, each passing a 500 ms budget that reaches datalog as WithMaxDuration — a wall-clock bound. make test runs -race over ./... on a 2-core runner with packages in parallel, and per the note above DefaultAuthorizerTimeout, one authorization already costs ~1.1 ms under -race. Under that load, goroutines can spend longer than 500 ms simply waiting to be scheduled.

On the remedy — measurements, since the obvious fix doesn't hold

I tried the tidy answer (use DefaultAuthorizerTimeout) and it didn't survive testing, so rather than guess, here is what I measured on clean main under identical contention, 6 runs each:

Budget / workers	Failures
500ms, 50 workers (current)	5 of 6
1s (DefaultAuthorizerTimeout), 50 workers	3 of 6
5s, 50 workers	2 of 6
5s, 8 workers	1 of 6
30s, 50 workers	0 of 6

Raising the constant improves the odds but none of these is immune — under enough starvation a wall-clock deadline will always lose. My load is deliberately pathological (~18× oversubscription), harsher than a CI runner, so this exaggerates the effect rather than reproducing CI exactly.

One possibly relevant detail: biscuit_test.go is already inconsistent here — 500ms at lines 80, 126, 219 and 263, but 5s at nine newer call sites (363–614). Line 219 is the only one of the four inside a concurrent loop, so it's the only one exposed.

Since the right fix looks like a judgement call about what this test should assert rather than a constant to bump, I didn't want to fold a guess into this PR. Happy to open a separate issue with the above, or a PR in whichever direction you prefer — just let me know.

Sorry for the long comment; I wanted to save you the log-digging rather than just asking for a re-run.

[aojea]

One possibly relevant detail: biscuit_test.go is already inconsistent here — 500ms at lines 80, 126, 219 and 263, but 5s at nine newer call sites (363–614). Line 219 is the only one of the four inside a concurrent loop, so it's the only one exposed.

yeah, this is something we need to enforce more, the unit test has an option to pass a larger biscuit timeout to deal with resource constraints as you correctly explain

[aojea]

good catch

[aojea]

Thanks @HosniBelfeki — the analysis is exactly right, and the fix is correct. Reviewed against current main and confirmed the bug is still live there (",," → nil requirement → gate off).

Two updates pushed (your commit preserved, authorship intact):

• Rebased onto main: 226740b had moved parseRequiredLabels to labels_gate.go, so your parser change now lands there; tests unchanged.
• ecec9d2: since your PR was written, feat(node): add A2A support #347 added a third call site (the a2a egress gate), which shared the same fail-open shape. Your parser fix covers it automatically; I extended the a2a boundary test to pin ",," → 400 there too.

Re-ran CI; the earlier TestVerifyBiscuit_Concurrent failure is the unrelated flake you diagnosed — please do open that issue, the measurements are worth keeping.

All green locally: full internal/node, make lint.

[aojea]

Also pushed 8e6c047 folding in the flake fix, since your measurements already did the hard part: no finite budget survives arbitrary starvation, and none of these tests asserts timing — so the budget just needs to never bind. All thirteen call sites (the four 500ms and nine 5s you spotted) now share one deliberately generous testTimeout = time.Minute constant. Verified under GOMAXPROCS=2 -race repeats locally.

[aojea]

Thanks

[HosniBelfeki]

Thanks @aojea — and for going beyond the review itself: the rebase onto labels_gate.go, spotting that #347 had introduced a third call site with the same shape, and folding in the flake fix.

On the separate issue you asked for: I'll skip it, since 8e6c047 resolved the bug and the rationale now lives in the testTimeout comment — where anyone changing a deadline in that file will actually read it, which an issue wouldn't guarantee. The measurements stay in this thread if they're ever needed again.

Happy to file one anyway if you'd still prefer it tracked separately.

[aojea]

Sounds fair, thanks for your contributions