diff --git a/.github/workflows/system-tests.yml b/.github/workflows/system-tests.yml index 619dd79db2a..9bdd69dd721 100644 --- a/.github/workflows/system-tests.yml +++ b/.github/workflows/system-tests.yml @@ -141,6 +141,7 @@ jobs: - APPSEC_BLOCKING_FULL_DENYLIST - APPSEC_API_SECURITY - APPSEC_RASP + - APPSEC_RASP_NON_BLOCKING - APPSEC_RUNTIME_ACTIVATION - APM_TRACING_E2E_SINGLE_SPAN - APM_TRACING_E2E_OTEL diff --git a/contrib/net/http/appsec_test.go b/contrib/net/http/appsec_test.go index 3f0ef7c75f0..ff22cfcab5c 100644 --- a/contrib/net/http/appsec_test.go +++ b/contrib/net/http/appsec_test.go @@ -328,3 +328,62 @@ func TestAppsecHTTP30X(t *testing.T) { // But we have not analyzed any of the redirect response bodies assert.NotContains(t, serviceSpan.Tags(), "_dd.appsec.trace.3xx_res_body") } + +// TestAppsecHTTP30XRedirectChainKeepsFirstHop guards against the #4938 regression surfaced by +// system-tests Test_API10_redirect_status: when every hop of a redirect chain returns the SAME +// status (302 here), the same api-010 rule fires on every hop and writes the same redirection +// attribute from each hop's Location header. The value reported must be the FIRST hop's, not the +// last. Per-hop ephemeral WAF subcontexts (#4938) had regressed this to last-write-wins. +func TestAppsecHTTP30XRedirectChainKeepsFirstHop(t *testing.T) { + t.Setenv("DD_APPSEC_RULES", "../../../internal/appsec/testdata/api10.json") + + // Uniform-302 chain: /redirect?totalRedirects=3 -> 2 -> 1 -> 0 -> /final, mirroring the + // system-tests internal_server. api10.json's api-010-110 rule maps 302 -> redirect_target. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasPrefix(r.URL.Path, "/redirect"): + n, _ := strconv.Atoi(r.URL.Query().Get("totalRedirects")) + loc := "/final" + if n > 0 { + loc = fmt.Sprintf("/redirect?totalRedirects=%d", n-1) + } + http.Redirect(w, r, loc, http.StatusFound) + case r.URL.Path == "/final": + w.WriteHeader(http.StatusOK) + default: + require.Failf(t, "unexpected request", "path: %s", r.URL.Path) + } + })) + defer srv.Close() + + httpClient := WrapClient(srv.Client()) + + mt := mocktracer.Start() + defer mt.Stop() + + testutils.StartAppSec(t) + + w := httptest.NewRecorder() + r, err := http.NewRequest("GET", srv.URL+"/redirect?totalRedirects=3", nil) + require.NoError(t, err) + + TraceAndServe(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + resp, err := httpClient.Do(r) + require.NoError(t, err) + if resp != nil && resp.Body != nil { + defer resp.Body.Close() + } + }), w, r, &ServeConfig{ + Service: "service", + Resource: "resource", + }) + + spans := mt.FinishedSpans() + + // 1 handler span + 5 downstream requests (4x 302 + the final 200). + require.Len(t, spans, 6) + serviceSpan := spans[len(spans)-1] + + assert.Equal(t, "/redirect?totalRedirects=2", serviceSpan.Tags()["appsec.api.redirection.redirect_target"], "redirect_target must reflect the FIRST redirect hop, not the last") + assert.Equal(t, float64(5), serviceSpan.Tags()["_dd.appsec.downstream_request"], "unexpected or missing _dd.appsec.downstream_request tag") +} diff --git a/internal/appsec/emitter/waf/context.go b/internal/appsec/emitter/waf/context.go index 73cfe9fa898..24206702a6d 100644 --- a/internal/appsec/emitter/waf/context.go +++ b/internal/appsec/emitter/waf/context.go @@ -168,6 +168,14 @@ func (op *ContextOperation) AbsorbDerivatives(derivatives map[string]any) { continue } + // First-write-wins, intentionally: ephemeral subcontexts (e.g. per-hop downstream + // evaluation of a redirect chain) don't share the WAF's in-context attribute dedup, so + // without this guard the last hop would overwrite the first. Keeping the first value makes + // appsec.api.redirection.move_target reflect the FIRST redirect hop, not the last. + if _, ok := op.derivatives[k]; ok { + continue + } + op.derivatives[k] = v } } diff --git a/internal/appsec/emitter/waf/context_test.go b/internal/appsec/emitter/waf/context_test.go index 2a9aa290faa..93878f29aad 100644 --- a/internal/appsec/emitter/waf/context_test.go +++ b/internal/appsec/emitter/waf/context_test.go @@ -60,6 +60,42 @@ func TestFinishedServiceEntrySpanDoesNotReceiveChildDataEvents(t *testing.T) { } } +func TestAbsorbDerivativesFirstWriteWins(t *testing.T) { + op := &ContextOperation{} + + op.AbsorbDerivatives(map[string]any{"appsec.api.redirection.move_target": "/first", "count": 1}) + op.AbsorbDerivatives(map[string]any{"appsec.api.redirection.move_target": "/second", "count": 2, "added": true}) + + got := op.Derivatives() + if v := got["appsec.api.redirection.move_target"]; v != "/first" { + t.Errorf("move_target = %v, want /first (first write must win)", v) + } + if v := got["count"]; v != 1 { + t.Errorf("count = %v, want 1 (first write must win)", v) + } + if v, ok := got["added"]; !ok || v != true { + t.Errorf("added = %v (present=%v), want true (a new key must still be absorbed)", v, ok) + } +} + +func TestAbsorbDerivativesBlockedResponseSchemaStillSkipped(t *testing.T) { + op := &ContextOperation{} + op.SetRequestBlocked() + + op.AbsorbDerivatives(map[string]any{ + "_dd.appsec.s.res.body": "schema", + "appsec.api.redirection.move_target": "/first", + }) + + got := op.Derivatives() + if _, ok := got["_dd.appsec.s.res.body"]; ok { + t.Error("response schema derivative must be skipped when the request is blocked") + } + if v := got["appsec.api.redirection.move_target"]; v != "/first" { + t.Errorf("move_target = %v, want /first (non-schema derivatives must still be absorbed when blocked)", v) + } +} + type recordingTagSetter struct { mu sync.Mutex tags map[string]any