fix(targeting): sanitize context-agent validation error responses - #484
Merged
Conversation
The context-agent HTTP handler (targeting/contextagent/handler.go)
echoed tmproto.ValidateContextRequest's raw err.Error() text straight
into the HTTP error response body, e.g. "property_id contains invalid
characters" or "seller_agent_url exceeds maximum length of 2048". This
violates AGENTS.md's generic-error-message invariant ("Never echo
err.Error() in HTTP responses ... details go to structured logs
(slog) inside the service boundary") and, unlike every other place
that runs the same ValidateContextRequest/ValidateIdentityRequest
check, never logged the failure server-side at all — so an operator
had zero record of why a request was rejected.
This exact pattern was swept and fixed elsewhere in this repo by
#190/#201 (landed in PR #210): router.HandleContextMatch,
router.HandleIdentityMatch, and identityagent's ServeHTTP were all
changed to log the validation error server-side via a
logValidationFailure helper and return a generic "invalid request"
message to the caller. That sweep did not touch
targeting/contextagent/handler.go, which is the handler actually
wired into the cmd/context-agent binary, so it kept the pre-#210
behavior.
Fix: add a logValidationFailure helper to the context-agent handler,
mirroring identityagent's implementation exactly (method, path, error,
and either request_id or request_id_valid=false, logged via
slog.Warn), and replace the err.Error()-in-response call with the
generic "invalid request" message.
Verification: confirmed via git blame that this line predates and was
untouched by the #210 sweep, and confirmed no open issue or PR already
covers this file. Regression tests added in handler_test.go assert
the HTTP response never contains the validator's field-specific text
while the structured log does; both tests were checked to fail against
the pre-fix source (reverting the handler.go change while keeping the
tests) and pass after restoring it.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean, well-scoped security fix.
This PR replaces an err.Error() echo on the context-agent validation path with a generic "invalid request" response, adding a server-side logValidationFailure helper that mirrors identityagent field-for-field (method/path/error, request_id only when SafeRequestIDForEcho passes, else request_id_valid=false). Two verified regression tests confirm the response never leaks validator detail while the log retains it.
Checked:
- No critical/high/medium findings from the reviewer.
high_riskis true only because files matchtargeting/**— one file(modified)with no medium-or-higher concern, one file(added)test. Per the high-risk flag rules, a modification with no medium finding is presumed safe and a new file is normal scaffolding; neither warrants escalation.gated_pathsis false, so the gated-paths hard gate does not apply despitereview_decision: REVIEW_REQUIRED.- No no-auto-approve team match.
- No prior decision.
Rows 1–8 do not fire (no findings at all). Falls through to row 9 → approve. One non-blocking follow-up was noted on the adjacent unsupported-adcp_major_version branch, which is pre-existing and outside this diff.
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
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.
Policy-compliant relay of external contribution #474 so repository-default CodeQL can run. The original contributor remains the commit author; maintainer and Ladon reviews plus CI passed on #474.
Problem
targeting/contextagent/handler.go'sServeHTTPechoes the rawerr.Error()fromtmproto.ValidateContextRequestdirectly into the HTTPerror response body:
A caller sending a malformed
context_match_requestgets backvalidator-internal text such as
"property_id contains invalid characters"or"seller_agent_url exceeds maximum length of 2048"instead of a generic message, and the failure is never logged
server-side (
h.loggeris otherwise used elsewhere in this file, justnot here). This is a direct violation of AGENTS.md's error-message
invariant:
Just as importantly, this exact pattern was already identified and
swept out of the rest of the codebase: #190 ("Sweep TMP validator
errors for user-controlled value echoes") and #201 ("Audit TMP
validation rejection logging after error sanitization") were fixed by
PR #210, which changed
router.HandleContextMatch,router.HandleIdentityMatch, andidentityagent'sServeHTTPto logthe validation failure server-side via a
logValidationFailurehelperand return a generic
"invalid request"to the caller. That PR's diffdoes not touch
targeting/contextagent/handler.go— the handleractually wired into the
cmd/context-agentbinary — so it silentlykept the pre-#210 behavior. This PR closes that gap.
Verification
git blameon the affected line shows it predates PR fix(tmproto): sanitize validation error responses #210 and wasuntouched by it.
targeting/contextagent/handler.go; found none (Sweep TMP validator errors for user-controlled value echoes #190/Audit TMP validation rejection logging after error sanitization #201/fix(tmproto): sanitize validation error responses #210 coverrouter.go,targeting/identityagent/handler.go, andtmproto/validate.goonly).tmproto/validate.go/validate_ladder.gomessages don't interpolate raw user-supplied values (they use
static field names, lengths, and counts), so this isn't a literal
secret leak today — but it still directly contradicts the written
invariant, is inconsistent with the identical scenario already fixed
in the sibling identity-agent handler, and drops all server-side
diagnostics for rejected requests on this path.
Fix
Add a
logValidationFailurehelper totargeting/contextagent/handler.go, mirroringidentityagent.identityHandler.logValidationFailurefield-for-field(method, path, error, and either
request_idorrequest_id_valid=falsedepending ontmproto.SafeRequestIDForEcho),logged via
h.logger.Warn. The HTTP response now always gets thegeneric
"invalid request"message on a validation failure, matchingidentityagent's androuter.go's behavior for the same check.Test proof
Added
targeting/contextagent/handler_test.gowith two regressiontests:
TestContextHandlerValidationErrorIsGenericAndLogged— sends arequest with an invalid
property_id, asserts the HTTP responsemessage is exactly
"invalid request"and never contains"property_id", and asserts the structured log does contain thedetailed validator message (
"property_id contains invalid characters") plus method/path/request_id.TestContextHandlerInvalidRequestIDIsNotEchoed— sends a requestwhose
request_iditself failsvalidateSafeID("bad/id"),asserts it's neither echoed in the response nor written verbatim to
the log (elided in favor of
request_id_valid=false).Confirmed both are real regression tests: reverted the
handler.gosource change (
git stashon that file only, keeping the new testfile), re-ran
go test ./targeting/contextagent/... -run TestContextHandler -vand both tests failed against the pre-fixcode with exactly the expected diffs (response message
"property_id contains invalid characters"/"request_id contains invalid characters"instead of"invalid request", and empty logs).Restored the fix and re-ran — both pass.
No schema changes, no new dependencies, no behavior change on the
happy path — only the shape of the 400 response and server-side
logging on the validation-failure path for
POST /context.