[codex] Harden DDoS protection controls - #2
Merged
Merged
Conversation
added 26 commits
June 25, 2026 13:13
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR updates filter signatures and validation, tightens HTTP framing and client-IP parsing, bounds TCP relay activity notifications, hardens provider filter sanitization, and expands benchmark assertions, runner behavior, tests, docs, artifacts, and one CI invocation. ChangesRequest hardening and tooling updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/filter.rs (1)
2016-2030: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the new signature test to a concrete digest.
This only proves length plus path normalization. A different 128-bit hash, or a basis tweak that still makes Lines 2023-2028 pass, would slip through without catching the compatibility regression this PR is trying to lock down.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/filter.rs` around lines 2016 - 2030, The new request signature test is too loose because it only checks length, normalization, and ASCII hex, so it can miss a compatibility regression in request_signature or legacy_request_signature. Update request_signature_uses_blake3_fingerprint_with_legacy_helper to assert the exact expected digest for a fixed input, using request_signature and legacy_request_signature as the targets, so the test pins the new BLAKE3-based output to a concrete value and will fail if the hashing basis changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/filter.rs`:
- Around line 481-493: The merged snapshot in FilterEngine::reload/load path
only checks duplicate rule IDs via validate_unique_filter_rule_ids, so malformed
or unsafe rules can still be published. After loading and sorting `loaded`,
validate each rule with validate_filter_rule before building the runtime state,
ideally alongside the existing merged-filter checks in this same block, and
reject the reload if any rule fails. Keep the ID dedupe and activation deadline
retention logic unchanged, but ensure the final `loaded` collection is fully
validated before it is used by FilterEngine::new callers.
In `@src/http_proxy.rs`:
- Around line 1525-1533: The Content-Length validation in
validate_parsed_content_length only checks for trimmed ASCII digits, so
oversized values can slip through and bypass the fast-fail path. Update this
helper to also reject values that do not fit in u64 by parsing the trimmed
string as an integer and returning invalid_reason on parse overflow or failure,
so both request and response handling consistently rejects impossible
Content-Length values.
In `@tools/codex_analyzer.py`:
- Around line 827-829: `filter_has_matcher` is treating a condition with only an
empty `methods` array as a valid runtime rule, so update `sanitize_filter` to
omit the `methods` key when the cleaned list is empty. Make sure the matcher
check in `filter_has_matcher` continues to drop any rule that has no real
matcher after sanitization, so `runtime/filters.json` never includes degenerate
conditions.
---
Nitpick comments:
In `@src/filter.rs`:
- Around line 2016-2030: The new request signature test is too loose because it
only checks length, normalization, and ASCII hex, so it can miss a compatibility
regression in request_signature or legacy_request_signature. Update
request_signature_uses_blake3_fingerprint_with_legacy_helper to assert the exact
expected digest for a fixed input, using request_signature and
legacy_request_signature as the targets, so the test pins the new BLAKE3-based
output to a concrete value and will fail if the hashing basis changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3778a086-ada2-4696-81cc-d769841a7aea
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlREADME.mdbenchmark_results/defense_artifacts_manifest.jsondocs/AI_PROVIDERS.mddocs/ARCHITECTURE.mddocs/EDGE_PROTECTION.mddocs/OPERATIONS.mdsrc/adaptive.rssrc/filter.rssrc/http_proxy.rssrc/tcp_proxy.rstools/assert_local_bench.pytools/codex_analyzer.pytools/run_local_bench.pytools/test_ai_tools.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/run_edge_namespace_smoke.py (1)
135-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffWhitespace normalization is applied inconsistently.
The SYN/Xmas/new-non-SYN matchers were migrated to
compact_stdout, but the remaining substring checks (e.g.tcp_invalid_null_drop_presentat Line 171‑172,update@tcp4_syn_rate``/limit rate over 5000...at Lines 178‑182, `udp_protected_port_drop_present` at Line 194, the connlimit and ICMP checks) still match against raw `stdout`. If real `nft list` output renders these rules with different inter-token spacing than the canonical single-spaced form, the migrated checks would still pass while these raw checks could spuriously report `False`. Consider matching all rule-presence substrings against `compact_stdout` for uniform robustness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/run_edge_namespace_smoke.py` around lines 135 - 206, Whitespace normalization is applied inconsistently in this smoke test: some rule matchers use compact_stdout while several remaining presence checks still compare against raw stdout. Update the rule-presence assertions in run_edge_namespace_smoke.py to use compact_stdout consistently for all nft substring checks, including the tcp_invalid_null_drop_present, syn backstop, connlimit, udp, and ICMP matches, so spacing differences in nft output do not cause false negatives. Refer to the existing compact_stdout normalization and the helper booleans in the return dict to keep the matching style uniform.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tools/run_edge_namespace_smoke.py`:
- Around line 135-206: Whitespace normalization is applied inconsistently in
this smoke test: some rule matchers use compact_stdout while several remaining
presence checks still compare against raw stdout. Update the rule-presence
assertions in run_edge_namespace_smoke.py to use compact_stdout consistently for
all nft substring checks, including the tcp_invalid_null_drop_present, syn
backstop, connlimit, udp, and ICMP matches, so spacing differences in nft output
do not cause false negatives. Refer to the existing compact_stdout normalization
and the helper booleans in the return dict to keep the matching style uniform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4379678-0bb3-4130-b377-42bcae9dd870
📒 Files selected for processing (2)
tools/run_edge_namespace_smoke.pytools/test_ai_tools.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
cargo fmt --checkcargo clippy --all-targets -- -D warningscargo test --quietPYTHONPATH=tools python3 -m unittest discover -s tools -p 'test_*.py'python3 tools/validate_edge_templates.pypython3 tools/assert_defense_bench.py --audit-tracked-artifacts benchmark_results --artifact-manifest benchmark_results/defense_artifacts_manifest.jsoncargo auditgit diff --checkNotes
Codex-Hardeningand does not merge it.Summary by CodeRabbit
New Features
Bug Fixes
Transfer-Encodingvalidation (limits empty elements), whitespace-tolerantContent-Length, improved upstream framing failures as502 Bad Gateway, port-aware host checks, and stricter trusted-client IP handling.Documentation