fix(openfeature): bound context flattening depth and detect cycles#4973
fix(openfeature): bound context flattening depth and detect cycles#4973leoromanovsky wants to merge 4 commits into
Conversation
flattenRecursive walked map[string]any / []any evaluation context with no depth limit and no cycle detection. An attacker-influenced context with deep nesting or a self-referential map could recurse until the Go runtime hit a fatal, unrecoverable stack overflow (a process-crash DoS, worse than a degraded telemetry path since recover() cannot catch it); a wide context was also fully materialized before the field cap applied. Add maxContextDepth (32) plus stack-based cycle detection for the only self-referenceable container types (map[string]any, []any), threaded through the flatten helpers. This hardens both the EVP flagevaluation and the older exposure flattening paths. Output is unchanged for legitimate contexts (depth <= 32, no cycles); shared sub-trees (diamonds) are still fully flattened. dd-trace-rb fixed the same defect class in #5896 (commit f8e718). Codex-scan finding: d4c4823ac5148191bd9869bb77f0227e Refs: APMSP-3616, FFL-2651
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 244502d | Docs | Datadog PR Page | Give us feedback! |
Drop the cycle-detection machinery (reflect-based identity tracking, seen set, enterContainer helper) from the previous commit. The fatal failure is the stack overflow, and the depth cap alone prevents it: a self-referential context simply recurses to maxContextDepth and stops. Any bounded junk keys a cycle produces are pruned by the existing 256-field limit. This keeps the switch and helpers structurally identical to the original, adding only a threaded depth int and one guard.
BenchmarksBenchmark execution time: 2026-07-01 17:10:14 Comparing candidate commit 244502d in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 324 metrics, 2 unstable metrics, 1 flaky benchmarks without significant changes.
|
Satisfies golangci-lint modernize (rangeint); the loop counters were unused.
The depth cap alone fixes the fatal stack overflow but not the memory/CPU amplification the security report also called out: a context that shares a child map/slice across many keys (a DAG) fans out ~2^depth before the 256-field prune runs, and a self-referential map produces bounded-but-junk keys. Restore the two guards from the original finding alongside the depth cap: - cycle detection: track map[string]any / []any identity on the recursion stack and skip references already in progress (breaks true cycles; diamonds still flatten fully since identity is removed on the way out). - maxFlattenFields (65536): a total-field safety ceiling that bounds DAG fan-out. It sits far above the 256-field intake prune, so real contexts are unaffected and the deterministic prune that builds the aggregation bucket key is unchanged; only pathological amplification is truncated. All of this runs on the background aggregation worker, not the evaluation hot path, so there is no per-evaluation cost. Tests are table-driven in flatten_test.go: normal nesting, direct/indirect cycles, diamond (not a cycle), depth-bounded map/array, and field-ceiling fan-out. Refs: APMSP-3616, FFL-2651
leoromanovsky
requested review from
dd-oleksii and
sameerank
and removed request for
a team
|
@codex Review this for correctness and canonical golang code. Validate that this operation, while expensive, is off the flag evaluation hot path and won't directly impact the user experience. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 244502d64f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // builds the aggregation bucket key is unchanged; only pathological amplification is truncated. | ||
| // Flattening runs on the background aggregation worker, not the evaluation hot path, so this | ||
| // adds no per-evaluation cost. | ||
| maxFlattenFields = 1 << 16 // 65536 |
There was a problem hiding this comment.
maxFlattenFields does not only guard the background EVP aggregation path: flattenRecursive is also used by flattenContext, which exposureHook.After calls before appending the exposure event (openfeature/exposure_hook.go:72). The OpenFeature SDK invokes After hooks synchronously before returning evaluation details, so for successful evaluations with doLog enabled and an allocation key, a pathological context can still make the request flatten up to 65,536 entries. That violates the intended off-hot-path guarantee; move exposure flattening to the writer or use a much smaller synchronous snapshot/cap.
Useful? React with 👍 / 👎.
| ptr := reflect.ValueOf(v).Pointer() | ||
| if _, cyclic := seen[ptr]; cyclic { |
There was a problem hiding this comment.
Using only reflect.ValueOf(v).Pointer() as the slice identity makes any subslice that starts at the same backing-array element look cyclic. For example, with s := []any{"leaf", nil}; t := s[:1}; s[1] = t, the new check skips s.1 even though flattening it terminates and previously produced s.1.0. This drops valid context fields when callers reuse prefix subslices; include length/capacity in the stack key or otherwise detect exact slice cycles.
Useful? React with 👍 / 👎.
1 participant
What does this PR do?
Bounds the recursion in OpenFeature evaluation-context flattening (
openfeature/flatten.go) with the three guards the security finding called for:maxContextDepth = 32) — stops unbounded nesting.map[string]any/[]anyidentity on the recursion stack and skips references already in progress. Breaks true cycles; shared sub-trees (diamonds) still flatten fully since identity is removed on the way out.maxFlattenFields = 65536) — bounds fan-out from shared-child (DAG) contexts, which neither depth nor cycle detection stops on its own.This hardens both the EVP
flagevaluationpath (flattenAndPruneContext) and the older exposure path (flattenContext), which both funnel throughflattenRecursive.Motivation
A Codex security scan (finding
d4c4823ac5148191bd9869bb77f0227e, tracked as APMSP-3616 / FFL-2651) flagged unbounded evaluation-context flattening with "no depth limit, cycle detection, or early field cap." The evaluation context is attacker-influenceable:map[string]anyrecursed without limit. In Go a stack overflow is a fatal, unrecoverable runtime error —recover()cannot catch it — so this is a process-crash DoS.dd-trace-rbfixed the same defect class in DataDog/dd-trace-rb#5896. Sibling follow-ups are tracked fordd-trace-jsanddd-trace-py(Java/.NET prune flat only and are not exposed).Decisions
aggregate()on a single goroutine), never on the evaluationFinallyhook, so none of this touches per-evaluation latency. Benchmarks: 0 regressions across 324 metrics.flattenRecursive's public signature is unchanged — conflict-free with fix(openfeature): bound flagevaluation EVP payloads #4934 and safe for both call sites.Testing
openfeature/flatten_test.gois table-driven: normal map/array flattening, direct self-reference, indirecta→b→acycle, diamond (not a cycle), depth-bounded map and array, and shared-subtree fan-out bounded by the field ceiling. Full./openfeature/suite passes;go vetandgolangci-lintclean.