Skip to content

Commit ed198f7

Browse files
eliottnessdarccio
authored andcommitted
fix(appsec): keep first redirect hop for redirection derivatives (#4972)
### What does this PR do? Merges WAF attribute derivatives with **first-write-wins** in `AbsorbDerivatives` so a uniform-status redirect chain reports the **first** hop's `appsec.api.redirection.*` value instead of the last. It also enables the `APPSEC_RASP_NON_BLOCKING` scenario in the system-tests CI matrix (across all weblog variants, matching `APPSEC_RASP`) so this stays covered. ### Motivation #4938 moved per-hop downstream request/response WAF evaluation from the persistent request context to per-roundtrip **ephemeral subcontexts**. The persistent context deduplicates attribute generation within a request, so a redirection attribute rule matching the same status (e.g. 302) on every hop produced its derivative only once (the first hop). Ephemeral subcontexts don't share that dedup, so the rule re-fires on every hop and `AbsorbDerivatives`' last-write-wins kept the **final** hop. This regressed system-tests `Test_API10_redirect_status::test_api10_redirect` on `golang@2.10.0-dev` (bisected to `40280d189`): `appsec.api.redirection.move_target` became `/mirror/200` (last hop) instead of `/redirect?totalRedirects=2` (first hop). The fix restores the pre-#4938 "first discovery wins" invariant at the derivative merge boundary while preserving #4938's per-hop ephemeral evaluation (RASP SSRF still fires on every hop). The existing blocked-response-schema skip is kept ahead of the new guard. ### Testing - New `TestAppsecHTTP30XRedirectChainKeepsFirstHop` (uniform-302 chain) — verified it FAILS without the fix and PASSES with it. The existing `TestAppsecHTTP30X` used distinct 308/307 per hop, so it never collided on a single key. - New `AbsorbDerivatives` unit tests (first-write-wins + blocked-response-schema skip preserved). - Full `contrib/net/http` appsec suite and `internal/appsec/emitter/waf` package pass; `go vet` clean. ### Reviewer's Checklist - [x] Changed code has unit tests for its functionality at or near 100% coverage. - [x] [System-Tests](https://github.com/DataDog/system-tests/) covering this feature have been enabled (`APPSEC_RASP_NON_BLOCKING` added to the CI matrix). - [ ] 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. - [x] New code is free of linting errors. - [x] New code doesn't break existing tests. - [ ] Add an appropriate team label so this PR gets put in the right place for the release notes. - [x] All generated files are up to date. - [ ] Non-trivial go.mod changes reviewed by @DataDog/dd-trace-go-guild (no go.mod changes). Co-authored-by: eliott.bouhana <eliott.bouhana@datadoghq.com>
1 parent 4be11d4 commit ed198f7

4 files changed

Lines changed: 104 additions & 0 deletions

File tree

.github/workflows/system-tests.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@ jobs:
141141
- APPSEC_BLOCKING_FULL_DENYLIST
142142
- APPSEC_API_SECURITY
143143
- APPSEC_RASP
144+
- APPSEC_RASP_NON_BLOCKING
144145
- APPSEC_RUNTIME_ACTIVATION
145146
- APM_TRACING_E2E_SINGLE_SPAN
146147
- APM_TRACING_E2E_OTEL

contrib/net/http/appsec_test.go

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,3 +328,62 @@ func TestAppsecHTTP30X(t *testing.T) {
328328
// But we have not analyzed any of the redirect response bodies
329329
assert.NotContains(t, serviceSpan.Tags(), "_dd.appsec.trace.3xx_res_body")
330330
}
331+
332+
// TestAppsecHTTP30XRedirectChainKeepsFirstHop guards against the #4938 regression surfaced by
333+
// system-tests Test_API10_redirect_status: when every hop of a redirect chain returns the SAME
334+
// status (302 here), the same api-010 rule fires on every hop and writes the same redirection
335+
// attribute from each hop's Location header. The value reported must be the FIRST hop's, not the
336+
// last. Per-hop ephemeral WAF subcontexts (#4938) had regressed this to last-write-wins.
337+
func TestAppsecHTTP30XRedirectChainKeepsFirstHop(t *testing.T) {
338+
t.Setenv("DD_APPSEC_RULES", "../../../internal/appsec/testdata/api10.json")
339+
340+
// Uniform-302 chain: /redirect?totalRedirects=3 -> 2 -> 1 -> 0 -> /final, mirroring the
341+
// system-tests internal_server. api10.json's api-010-110 rule maps 302 -> redirect_target.
342+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
343+
switch {
344+
case strings.HasPrefix(r.URL.Path, "/redirect"):
345+
n, _ := strconv.Atoi(r.URL.Query().Get("totalRedirects"))
346+
loc := "/final"
347+
if n > 0 {
348+
loc = fmt.Sprintf("/redirect?totalRedirects=%d", n-1)
349+
}
350+
http.Redirect(w, r, loc, http.StatusFound)
351+
case r.URL.Path == "/final":
352+
w.WriteHeader(http.StatusOK)
353+
default:
354+
require.Failf(t, "unexpected request", "path: %s", r.URL.Path)
355+
}
356+
}))
357+
defer srv.Close()
358+
359+
httpClient := WrapClient(srv.Client())
360+
361+
mt := mocktracer.Start()
362+
defer mt.Stop()
363+
364+
testutils.StartAppSec(t)
365+
366+
w := httptest.NewRecorder()
367+
r, err := http.NewRequest("GET", srv.URL+"/redirect?totalRedirects=3", nil)
368+
require.NoError(t, err)
369+
370+
TraceAndServe(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
371+
resp, err := httpClient.Do(r)
372+
require.NoError(t, err)
373+
if resp != nil && resp.Body != nil {
374+
defer resp.Body.Close()
375+
}
376+
}), w, r, &ServeConfig{
377+
Service: "service",
378+
Resource: "resource",
379+
})
380+
381+
spans := mt.FinishedSpans()
382+
383+
// 1 handler span + 5 downstream requests (4x 302 + the final 200).
384+
require.Len(t, spans, 6)
385+
serviceSpan := spans[len(spans)-1]
386+
387+
assert.Equal(t, "/redirect?totalRedirects=2", serviceSpan.Tags()["appsec.api.redirection.redirect_target"], "redirect_target must reflect the FIRST redirect hop, not the last")
388+
assert.Equal(t, float64(5), serviceSpan.Tags()["_dd.appsec.downstream_request"], "unexpected or missing _dd.appsec.downstream_request tag")
389+
}

internal/appsec/emitter/waf/context.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,14 @@ func (op *ContextOperation) AbsorbDerivatives(derivatives map[string]any) {
168168
continue
169169
}
170170

171+
// First-write-wins, intentionally: ephemeral subcontexts (e.g. per-hop downstream
172+
// evaluation of a redirect chain) don't share the WAF's in-context attribute dedup, so
173+
// without this guard the last hop would overwrite the first. Keeping the first value makes
174+
// appsec.api.redirection.move_target reflect the FIRST redirect hop, not the last.
175+
if _, ok := op.derivatives[k]; ok {
176+
continue
177+
}
178+
171179
op.derivatives[k] = v
172180
}
173181
}

internal/appsec/emitter/waf/context_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,42 @@ func TestFinishedServiceEntrySpanDoesNotReceiveChildDataEvents(t *testing.T) {
6060
}
6161
}
6262

63+
func TestAbsorbDerivativesFirstWriteWins(t *testing.T) {
64+
op := &ContextOperation{}
65+
66+
op.AbsorbDerivatives(map[string]any{"appsec.api.redirection.move_target": "/first", "count": 1})
67+
op.AbsorbDerivatives(map[string]any{"appsec.api.redirection.move_target": "/second", "count": 2, "added": true})
68+
69+
got := op.Derivatives()
70+
if v := got["appsec.api.redirection.move_target"]; v != "/first" {
71+
t.Errorf("move_target = %v, want /first (first write must win)", v)
72+
}
73+
if v := got["count"]; v != 1 {
74+
t.Errorf("count = %v, want 1 (first write must win)", v)
75+
}
76+
if v, ok := got["added"]; !ok || v != true {
77+
t.Errorf("added = %v (present=%v), want true (a new key must still be absorbed)", v, ok)
78+
}
79+
}
80+
81+
func TestAbsorbDerivativesBlockedResponseSchemaStillSkipped(t *testing.T) {
82+
op := &ContextOperation{}
83+
op.SetRequestBlocked()
84+
85+
op.AbsorbDerivatives(map[string]any{
86+
"_dd.appsec.s.res.body": "schema",
87+
"appsec.api.redirection.move_target": "/first",
88+
})
89+
90+
got := op.Derivatives()
91+
if _, ok := got["_dd.appsec.s.res.body"]; ok {
92+
t.Error("response schema derivative must be skipped when the request is blocked")
93+
}
94+
if v := got["appsec.api.redirection.move_target"]; v != "/first" {
95+
t.Errorf("move_target = %v, want /first (non-schema derivatives must still be absorbed when blocked)", v)
96+
}
97+
}
98+
6399
type recordingTagSetter struct {
64100
mu sync.Mutex
65101
tags map[string]any

0 commit comments

Comments
 (0)