[ProtoApiScrubber] Add response scrubber to the filter. by sumitkmr2 · Pull Request #42145 · envoyproxy/envoy

sumitkmr2 · 2025-11-20T07:33:28Z

Commit Message: Add response scrubber to the filter.
Additional Description:

Changes in filter_config.cc and filter.cc and respective tests for the response flow.
Also added a couple of tests to increase coverage for request flow as well.
Now, this filter has 100% line coverage.
Risk Level: None.
Testing: UTs are added. Integration tests will be added in a separate PR once IT setup PR ([ProtoApiScrubber] Add Integration Tests #42121) gets submitted.
Docs Changes: None.
Release Notes: None.
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

repokitteh-read-only · 2025-11-20T07:33:33Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #42145 was opened by sumitkmr2.

see: more, trace.

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

adisuissa · 2025-11-21T16:56:06Z

  // vice-versa.
  GrpcFieldExtraction::MessageConverterPtr request_msg_converter_{nullptr};

+  // Response message converter which converts Envoy Buffer data to StreamMessage (for scrubbing)


High-level comment:
looking at the request vs response related fields, it seems that there are the same group of objects needed for each of these. Using a generic struct with the relevant fields that is initialized differently for each of them may reduce the copy-paste between the two.
(not AI for this PR, just high-level observation to take into account).

Yeah, that makes sense. I'll add that in the backlog for now, will pick it up once the current lined-up items are complete for the filter.

adisuissa · 2025-11-21T17:04:20Z

+    if (stream_message->message() == nullptr) {
+      // Expect end_stream=true when the MessageConverter signals an stream end.
+      ASSERT(end_stream);
+


style-nit: please remove empty lines that don't split logical code segments

adisuissa · 2025-11-21T17:08:40Z

+
+    auto buf_convert_status =
+        response_msg_converter_->convertBackToBuffer(std::move(stream_message));
+    RELEASE_ASSERT(buf_convert_status.ok(), "failed to convert message back to envoy buffer");


I'm pretty sure I had reservations about this RELEASE_ASSERT in the request path.
I'm worried this will create an attack vector that may crash the proxy.
If this is needed, can you add a comment before these lines describing why the release-assert is necessary here (say compared to sending local reply)?

Yeah, you are right. Although, the message converter is designed to be very robust, it is still possible that it can fail due to a bug introduced later. I've replaced release_assert() with rejectResponse which sends a local reply with appropriate error details. I'll make the change for request path in a separate PR.

adisuissa · 2025-11-21T17:14:34Z

+
+using ProtoApiScrubberResponsePassThroughTest = ProtoApiScrubberFilterTest;
+
+TEST_F(ProtoApiScrubberResponsePassThroughTest, UnaryResponseSingleBuffer) {


Please add one-liner comments describing these tests.

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

sumitkmr2

Addressed comments, thanks!

sumitkmr2 · 2025-11-24T08:18:51Z

  // vice-versa.
  GrpcFieldExtraction::MessageConverterPtr request_msg_converter_{nullptr};

+  // Response message converter which converts Envoy Buffer data to StreamMessage (for scrubbing)


Yeah, that makes sense. I'll add that in the backlog for now, will pick it up once the current lined-up items are complete for the filter.

sumitkmr2 · 2025-11-24T08:24:55Z

+    if (stream_message->message() == nullptr) {
+      // Expect end_stream=true when the MessageConverter signals an stream end.
+      ASSERT(end_stream);
+


sumitkmr2 · 2025-11-24T08:29:09Z

+
+using ProtoApiScrubberResponsePassThroughTest = ProtoApiScrubberFilterTest;
+
+TEST_F(ProtoApiScrubberResponsePassThroughTest, UnaryResponseSingleBuffer) {


sumitkmr2 · 2025-11-24T08:45:58Z

+
+    auto buf_convert_status =
+        response_msg_converter_->convertBackToBuffer(std::move(stream_message));
+    RELEASE_ASSERT(buf_convert_status.ok(), "failed to convert message back to envoy buffer");


Yeah, you are right. Although, the message converter is designed to be very robust, it is still possible that it can fail due to a bug introduced later. I've replaced release_assert() with rejectResponse which sends a local reply with appropriate error details. I'll make the change for request path in a separate PR.

adisuissa

LGTM, thanks

…2145) Signed-off-by: Sumit Kumar <sumitkmr@google.com> Signed-off-by: Gustavo <grnmeira@gmail.com>

Add response scrubbing flow

7607ac0

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

sumitkmr2 added 6 commits November 20, 2025 10:29

Fixing tests

b6e6720

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Improve coverage

eb4becd

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Reordering tests

d7f7c71

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Fix test

f504314

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Add tests

357567f

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

Add tests

159af04

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

sumitkmr2 marked this pull request as ready for review November 20, 2025 13:54

sumitkmr2 requested a review from adisuissa as a code owner November 20, 2025 13:54

adisuissa reviewed Nov 21, 2025

View reviewed changes

wbpcode assigned adisuissa Nov 23, 2025

sumitkmr2 added 2 commits November 24, 2025 09:29

Address comments

a36aa83

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

build fix

dbfc00a

Signed-off-by: Sumit Kumar <sumitkmr@google.com>

sumitkmr2 commented Nov 24, 2025

View reviewed changes

adisuissa approved these changes Nov 24, 2025

View reviewed changes

adisuissa merged commit 716a496 into envoyproxy:main Nov 24, 2025
24 checks passed

grnmeira pushed a commit to grnmeira/envoy that referenced this pull request Mar 20, 2026

[ProtoApiScrubber] Add response scrubber to the filter. (envoyproxy#4…

9d24e88

…2145) Signed-off-by: Sumit Kumar <sumitkmr@google.com> Signed-off-by: Gustavo <grnmeira@gmail.com>


		using ProtoApiScrubberResponsePassThroughTest = ProtoApiScrubberFilterTest;

		TEST_F(ProtoApiScrubberResponsePassThroughTest, UnaryResponseSingleBuffer) {

Conversation

sumitkmr2 commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Nov 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumitkmr2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sumitkmr2 commented Nov 20, 2025 •

edited

Loading