Skip to content

fix: redact user content in the 7 passthrough endpoints - #2428

Open
Aias00 wants to merge 6 commits into
envoyproxy:mainfrom
Aias00:fix/redaction-passthrough-endpoints-2387
Open

fix: redact user content in the 7 passthrough endpoints#2428
Aias00 wants to merge 6 commits into
envoyproxy:mainfrom
Aias00:fix/redaction-passthrough-endpoints-2387

Conversation

@Aias00

@Aias00 Aias00 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Description

RedactSensitiveInfoFromRequest was only implemented for 4 of the 11 endpoint specs (ChatCompletions, Speech, Transcription, Translation); the other 7 (Completions, Embeddings, ImageGeneration, Responses, Messages, Rerank, Tokenize) were passthrough stubs returning the request unredacted. With debugLogEnabled && enableRedaction, those 7 endpoints logged raw user content (prompts, embedding inputs, images, conversation items, documents, PII) to the debug log despite redaction being requested — so enableRedaction was silently half-honored, which is arguably worse than not having it.

This implements real redaction for the 7 endpoints:

  • Concrete string fields (Prompt, Query, Documents, Instructions, User) use RedactString.
  • Chat messages (Embeddings OfChat, Tokenize OfChat) reuse the existing redactMessage helpers.
  • Untyped-union fields (PromptUnion.Value, EmbeddingRequestInput.Value) and typed-union fields whose unmarshalers discriminate by a "type" key (MessageContent, SystemPrompt, ResponseNewParamsInputUnion, ResponsePromptParam) use a new redaction.RedactJSONTree that recursively redacts every string leaf in a JSON tree, preserving "type" discriminator values so the redacted tree still round-trips into the typed union.

Over-redaction of structural strings in debug logs is acceptable and safe; the goal is to never miss a sensitive nested string.

Related Issues/PRs (if applicable)

Fixes #2387

Special notes for reviewers (if applicable)

  • The 4 already-implemented endpoints (ChatCompletions, Speech, Transcription, Translation) are left untouched; their careful typed redaction preserves tool names / schemas.
  • redactUnionField marshals/unmarshals via pointers (&in / &out) so pointer-receiver MarshalJSON/UnmarshalJSON on the union types is actually invoked — passing a value T to json.Marshal would skip pointer-receiver methods and corrupt the union.
  • RedactJSONTree preserves the value of a "type" map key (JSON-union discriminator) rather than redacting it; redacting "type" would make the redacted value fail to round-trip back into the typed union during redaction, and discriminator values carry no sensitive content.
  • No call-site change: internal/extproc/processor_impl.go already dispatches to RedactSensitiveInfoFromRequest for every spec when redaction is on; the 7 stubs simply start doing real work.
  • Tests assert that a known marker string does NOT appear in the redacted JSON, that a [REDACTED placeholder appears, and that the original request is not mutated.

RedactSensitiveInfoFromRequest was only implemented for 4 of the 11 endpoint
specs (ChatCompletions, Speech, Transcription, Translation); the other 7
(Completions, Embeddings, ImageGeneration, Responses, Messages, Rerank,
Tokenize) were passthrough stubs that returned the request unredacted. With
debugLogEnabled && enableRedaction, those 7 endpoints logged raw user content
(prompts, embedding inputs, images, conversation items, documents, PII) to the
debug log despite redaction being requested — so enableRedaction was silently
half-honored, which is arguably worse than not having it.

Implement real redaction for the 7 endpoints. For concrete string fields
(Prompt, Query, Documents, Instructions, User) use RedactString; for chat
messages (Embeddings OfChat, Tokenize OfChat) reuse the existing redactMessage
helpers. For the untyped-union fields (PromptUnion.Value,
EmbeddingRequestInput.Value) and the typed-union fields whose unmarshalers
discriminate by a "type" key (MessageContent, SystemPrompt,
ResponseNewParamsInputUnion, ResponsePromptParam), add a generic
redaction.RedactJSONTree that recursively redacts every string leaf in a
JSON tree, preserving "type" discriminator values so the redacted tree still
round-trips into the typed union. Over-redaction of structural strings in
debug logs is acceptable and safe; the point is to never miss a sensitive
nested string.

No change to the call site (internal/extproc/processor_impl.go already
dispatches to RedactSensitiveInfoFromRequest for every spec when redaction
is on); the 7 stubs simply start doing real work.

Fixes envoyproxy#2387

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: liuhy <liuhongyu@apache.org>
@Aias00
Aias00 requested a review from a team as a code owner July 27, 2026 03:51
Copilot AI review requested due to automatic review settings July 27, 2026 03:51
@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 27, 2026

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Aias00 Aias00 changed the title endpointspec: redact user content in the 7 passthrough endpoints fix: redact user content in the 7 passthrough endpoints Jul 27, 2026
@codecov-commenter

codecov-commenter commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.97%. Comparing base (f9a03db) to head (b312200).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2428      +/-   ##
==========================================
+ Coverage   84.86%   84.97%   +0.10%     
==========================================
  Files         154      154              
  Lines       22419    22504      +85     
==========================================
+ Hits        19026    19122      +96     
+ Misses       2237     2225      -12     
- Partials     1156     1157       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Aias00 and others added 3 commits July 26, 2026 21:24
Signed-off-by: liuhy <liuhongyu@apache.org>
Cover the reachable marshal-error paths in redactInterfaceValue and
redactUnionField (a non-marshalable value, and a value whose MarshalJSON
errors) to assert they fail safe — a placeholder string / zero T — rather
than panicking or leaking content.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: liuhy <liuhongyu@apache.org>
redactInterfaceValue and redactUnionField guarded json.Unmarshal-into-any
and json.Marshal(redactJSONTree(...)) with fail-safe error returns that are
provably unreachable: b is valid JSON straight from json.Marshal into *any,
and RedactJSONTree yields only JSON-native types (string, []any,
map[string]any, numbers, bools, nil). The reachable marshal-error paths
stay and stay tested; the dead branches were flagged by Codecov patch
coverage.

No behavior change — both helpers now reach 100% line coverage.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: liuhy <liuhongyu@apache.org>

Copilot AI 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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

internal/redaction/redaction.go:88

  • RedactJSONTree currently preserves the value under the "type" key verbatim. If a request includes a non-string value for "type" (e.g. an object/array), this bypasses redaction for any nested string leaves under that value and also violates the "returns a new tree" claim by reusing the original reference. To keep union discriminator strings while still ensuring all string leaves are redacted, only preserve "type" when it is a string; otherwise recurse and redact/copy as usual.
			// Keys are structural (field/type names), not user data — keep them.
			// A "type" key holds a JSON-union discriminator value, not user data — keep it.
			if k == "type" {
				out[k] = e
				continue
			}
			out[k] = RedactJSONTree(e)
		}

internal/redaction/redaction_test.go:74

  • Add a regression test to ensure that preserving the "type" key cannot be used to bypass redaction when its value is not a discriminator string (e.g., an object/array). This guards against accidental or malicious payloads leaking nested string content via the special-case.
	t.Run("type discriminator preserved", func(t *testing.T) {
		// "type" values are JSON-union discriminators, not user data — they must be
		// preserved so the redacted tree still round-trips into typed unions.
		in := map[string]any{
			"type":    "message",

nacx added a commit that referenced this pull request Jul 31, 2026
**Description**

The "request headers processed" debug log only ran header-mutation
redaction when Server.enableRedaction was on. With debug logging on but
redaction off (the aigw run default), the upstream filter's injected
Authorization (the backend API key) was logged in cleartext, even though
the incoming authorization header was already redacted by
filterSensitiveHeadersForLogging.

This was inconsistent with the RequestBody phase, whose
redactRequestBodyResponse always redacts header mutations and only gates
body-content redaction on the flag. This change aligns the
RequestHeaders phase to the same pattern: redact header mutations
whenever debug logging is on, independent of enableRedaction, and gate
only body content on the flag via a new redactBody parameter on
redactProcessingResponseRequestHeaders.

A nil-response guard is added matching redactRequestBodyResponse.

**Special notes for reviewers (if applicable)**

- No behavior change to the response sent to Envoy; redaction only
touches the logContent copy used for the debug log. Process-level tests
assert on the returned resp, not log content, so they are unaffected.
- Verified live: with AIGW_DEBUG=true and enableRedaction off, the
upstream Authorization mutation now logs as [REDACTED] while the actual
response sent upstream still carries the real key.
- Out of scope: Envoy's own [http] and [ext_proc] debug logs still emit
the raw Authorization header / proto and are outside extproc's control;
those require Envoy log-level tuning, not addressed here.

**Related Issues/PRs (if applicable)**

Adjacent to but distinct from #2428 (which covers request body content
redaction for the 7 passthrough endpoints). This PR covers a different
leak surface: the backend credential injected into the request-headers
header mutation by the extproc upstream filter.

Co-Authored-By: Claude <noreply@anthropic.com>


Partially addresses #2436 (extproc Go side; Envoy-side leak points
remain).

Signed-off-by: liuhy <liuhongyu@apache.org>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Ignasi Barrera <ignasi@tetrate.io>
sivanantha321 pushed a commit to sivanantha321/ai-gateway that referenced this pull request Jul 31, 2026
…yproxy#2435)

**Description**

The "request headers processed" debug log only ran header-mutation
redaction when Server.enableRedaction was on. With debug logging on but
redaction off (the aigw run default), the upstream filter's injected
Authorization (the backend API key) was logged in cleartext, even though
the incoming authorization header was already redacted by
filterSensitiveHeadersForLogging.

This was inconsistent with the RequestBody phase, whose
redactRequestBodyResponse always redacts header mutations and only gates
body-content redaction on the flag. This change aligns the
RequestHeaders phase to the same pattern: redact header mutations
whenever debug logging is on, independent of enableRedaction, and gate
only body content on the flag via a new redactBody parameter on
redactProcessingResponseRequestHeaders.

A nil-response guard is added matching redactRequestBodyResponse.

**Special notes for reviewers (if applicable)**

- No behavior change to the response sent to Envoy; redaction only
touches the logContent copy used for the debug log. Process-level tests
assert on the returned resp, not log content, so they are unaffected.
- Verified live: with AIGW_DEBUG=true and enableRedaction off, the
upstream Authorization mutation now logs as [REDACTED] while the actual
response sent upstream still carries the real key.
- Out of scope: Envoy's own [http] and [ext_proc] debug logs still emit
the raw Authorization header / proto and are outside extproc's control;
those require Envoy log-level tuning, not addressed here.

**Related Issues/PRs (if applicable)**

Adjacent to but distinct from envoyproxy#2428 (which covers request body content
redaction for the 7 passthrough endpoints). This PR covers a different
leak surface: the backend credential injected into the request-headers
header mutation by the extproc upstream filter.

Co-Authored-By: Claude <noreply@anthropic.com>


Partially addresses envoyproxy#2436 (extproc Go side; Envoy-side leak points
remain).

Signed-off-by: liuhy <liuhongyu@apache.org>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Ignasi Barrera <ignasi@tetrate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Request redaction is only implemented for 4 of 13 endpoints — enableRedaction is silently half-honored

4 participants