Skip to content

fix(targeting): sanitize context-agent validation error responses - #474

Closed
sujanchalla0510 wants to merge 2 commits into
adcontextprotocol:mainfrom
sujanchalla0510:fix/context-agent-sanitize-validation-errors
Closed

fix(targeting): sanitize context-agent validation error responses#474
sujanchalla0510 wants to merge 2 commits into
adcontextprotocol:mainfrom
sujanchalla0510:fix/context-agent-sanitize-validation-errors

Conversation

@sujanchalla0510

Copy link
Copy Markdown
Collaborator

Problem

targeting/contextagent/handler.go's ServeHTTP echoes the raw
err.Error() from tmproto.ValidateContextRequest directly into the HTTP
error response body:

if err := tmproto.ValidateContextRequest(&req); err != nil {
    writeError(w, tmproto.SafeRequestIDForEcho(req.RequestID), tmproto.ErrorCodeInvalidRequest, err.Error(), http.StatusBadRequest)
    return
}

A caller sending a malformed context_match_request gets back
validator-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.logger is otherwise used elsewhere in this file, just
not here). This is a direct violation of AGENTS.md's error-message
invariant:

Error messages must be generic. Internal errors return "internal error" to callers. Details go to structured logs (slog) inside the
service boundary. Never echo err.Error() in HTTP responses.

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, and identityagent's ServeHTTP to log
the validation failure server-side via a logValidationFailure helper
and return a generic "invalid request" to the caller. That PR's diff
does not touch targeting/contextagent/handler.go — the handler
actually wired into the cmd/context-agent binary — so it silently
kept the pre-#210 behavior. This PR closes that gap.

Verification

Fix

Add a logValidationFailure helper to
targeting/contextagent/handler.go, mirroring
identityagent.identityHandler.logValidationFailure field-for-field
(method, path, error, and either request_id or
request_id_valid=false depending on tmproto.SafeRequestIDForEcho),
logged via h.logger.Warn. The HTTP response now always gets the
generic "invalid request" message on a validation failure, matching
identityagent's and router.go's behavior for the same check.

Test proof

Added targeting/contextagent/handler_test.go with two regression
tests:

  • TestContextHandlerValidationErrorIsGenericAndLogged — sends a
    request with an invalid property_id, asserts the HTTP response
    message is exactly "invalid request" and never contains
    "property_id", and asserts the structured log does contain the
    detailed validator message ("property_id contains invalid characters") plus method/path/request_id.
  • TestContextHandlerInvalidRequestIDIsNotEchoed — sends a request
    whose request_id itself fails validateSafeID ("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.go
source change (git stash on that file only, keeping the new test
file), re-ran go test ./targeting/contextagent/... -run TestContextHandler -v and both tests failed against the pre-fix
code 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.

go build ./...                                   # root workspace, ok
go vet ./...                                      # root workspace, ok
cd targeting && go build ./... && go vet ./... && go test ./...   # ok, all packages
cd cmd/router && go test ./...                     # ok
cd reference/context-agent && go test ./...         # ok
cd e2e && go test ./... -skip TestPerformance_EndToEnd   # ok (perf/throughput
                                                     # subtest fails locally on
                                                     # unmodified main too, due
                                                     # to macOS ephemeral-port
                                                     # exhaustion under load —
                                                     # unrelated to this change)

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.

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
adcontextprotocol#190/adcontextprotocol#201 (landed in PR adcontextprotocol#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-adcontextprotocol#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 adcontextprotocol#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.
@github-actions

Copy link
Copy Markdown
Contributor

IPR Policy Agreement Required

@sujanchalla0510 — thanks for the contribution. Before this PR can be merged, the AgenticAdvertising.Org IPR Policy requires your agreement.

To agree, post a new comment on this PR with the exact phrase:

I have read the IPR Policy

Your signature is recorded once and covers all contributions to AAO repositories. See signatures/README.md for what gets recorded and why.

@sujanchalla0510

Copy link
Copy Markdown
Collaborator Author

I have read the IPR Policy

@bokelley

Copy link
Copy Markdown
Contributor

Thanks for the acknowledgement, @sujanchalla0510.


Generated by Claude Code

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean security fix, no blocking or medium findings.

Checked:

  • targeting/contextagent/handler.go now returns a generic "invalid request" instead of echoing tmproto.ValidateContextRequest's err.Error() into the 400 body, closing the last context-agent path missed by the #190/#201/#210 validator-error sweep. logValidationFailure mirrors the identity-agent helper field-for-field.
  • Two verified regression tests added in handler_test.go.
  • No schema/generated-type changes, no TMP signing/verification changes, no wire-shape or HTTP-status contract change (400 preserved; only body text sanitized).

high_risk is true only because both files match targeting/** — one is a benign modification with no findings, the other is a newly added test file. No deletion, no medium-or-higher concern on a modified sensitive file, so the flag alone does not warrant escalation. gated_paths is false. No no-auto-approve team match. Falls through to row 9.

@bokelley bokelley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer review: generic caller-facing validation error, structured internal logging, and regression coverage are the correct production-hardening shape. Approved pending CI.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve.

This PR hardens targeting/contextagent/handler.go by no longer echoing ValidateContextRequest's err.Error() into the HTTP response (which leaked request-shaped detail such as "property_id contains invalid characters"), instead returning a generic "invalid request" while logging detail server-side via a new logValidationFailure that mirrors identityagent's pattern. A new handler_test.go pins both the generic-message invariant and unsafe-request-id elision.

Checks:

  • No wire-shape/contract break; no HTTP-status change on a router endpoint.
  • No adcp/schemas/** or adcp/types_gen.go surface touched (schema↔generated-type coherence intact).
  • No TMP signing/verification, TEE boundary, or protocol-managed-skill surface touched.
  • high_risk is true only because targeting/** is a high-risk glob: one file (modified) with no medium-or-higher findings, one file (added) — normal scaffolding. Not escalation-worthy.
  • gated_paths: false; review_decision: APPROVED; no no-auto-approve team match.

Decision table: no critical/high (row 1 no), gated_paths false (row 2 no), no deletions (row 3 no), no medium findings (rows 4/5/8 no), prior decision was approve so row 6 does not apply, no team gate (row 7 no) → row 9 approve.

bokelley commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Superseded by #484 solely to satisfy the repository CodeQL ruleset, which GitHub default setup cannot evaluate on fork PRs. The original commit and contributor authorship are preserved in #484.

@bokelley bokelley closed this Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants