Skip to content

feat(appsec): HTTP response monitoring SDK#3576

Merged
RomainMuller merged 5 commits into
mainfrom
romain.marcadier/APPSEC-57834/http-response-body-sdk
Jun 3, 2025
Merged

feat(appsec): HTTP response monitoring SDK#3576
RomainMuller merged 5 commits into
mainfrom
romain.marcadier/APPSEC-57834/http-response-body-sdk

Conversation

@RomainMuller

@RomainMuller RomainMuller commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

What does this PR do?

Introduce a new appsec.MonitorHTTPResponseBody to submit the response body to the WAF via an SDK, so that it can be classified and its schema extracted.

Motivation

This is a P0 feature for AAP, and a required building block for baking in automated collection in various integrations and via Orchestrion.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • New code is free of linting errors. You can check this by running golangci-lint run locally.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@RomainMuller
RomainMuller requested review from a team as code owners June 2, 2025 12:18
@github-actions github-actions Bot added the apm:ecosystem contrib/* related feature requests or bugs label Jun 2, 2025
@pr-commenter

pr-commenter Bot commented Jun 2, 2025 •
edited
Loading

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2025-06-03 13:54:43

Comparing candidate commit 450a46e in PR branch romain.marcadier/APPSEC-57834/http-response-body-sdk with baseline commit 52eeea9 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 21 metrics, 0 unstable metrics.

genesor
genesor reviewed Jun 2, 2025
View reviewed changes
Comment thread appsec/appsec.go
Comment on lines +54 to +55
// response body (in object form, not encoded as the literal HTTP response body
// payload bytes), and returns an error if the HTTP response is configured to be

@genesor genesor Jun 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we rely on the object and not the payload it will require users to modify every handler for which they want to enable this feature, as done the gin test. Is this something that we can expect from customers ?

We could do it with a middleware intercepting the response to reduce the cost of adoption.

EDIT: it seems to be the first of multiple PRs for this feature. Don't mind my comment if it won't be used as is later on.

@RomainMuller RomainMuller Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the only we we can cover all possible use-cases... separate PRs will integrate this in the contribs where possible (or via orchestrion, again, where possible).

See #3578 and #3579 for example.

@RomainMuller
RomainMuller force-pushed the romain.marcadier/APPSEC-57834/http-response-body-sdk branch from 9950270 to 9179334 Compare June 2, 2025 14:39
@RomainMuller Graphite App

RomainMuller commented Jun 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

This was referenced Jun 2, 2025
Merged
Merged
eliottness
eliottness approved these changes Jun 2, 2025
View reviewed changes

@eliottness eliottness 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.

LGTM

Comment thread contrib/gin-gonic/gin/appsec_test.go

@eliottness eliottness Jun 2, 2025

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.

100% Optional: Move this test in internal/appsec/waf_test.go and create a ruleset in testdata to not make this test fail next time something changes the postprocessors in the default ruleset

@RomainMuller RomainMuller Jun 3, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I feel like this test belongs here, but indeed it could use a dedicated fixed ruleset to be more future-proof.

@RomainMuller
RomainMuller enabled auto-merge (squash) June 3, 2025 07:55
kakkoyun
kakkoyun reviewed Jun 3, 2025
View reviewed changes
Comment thread appsec/appsec.go
@RomainMuller
RomainMuller merged commit 1eedb8b into main Jun 3, 2025
@RomainMuller
RomainMuller deleted the romain.marcadier/APPSEC-57834/http-response-body-sdk branch June 3, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kakkoyun kakkoyun kakkoyun left review comments
@genesor genesor genesor left review comments
@eliottness eliottness eliottness approved these changes

Assignees

No one assigned

Labels

apm:ecosystem contrib/* related feature requests or bugs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@RomainMuller @kakkoyun @genesor @eliottness