Skip to content

fix(detection): scan the Cookie header one name=value pair at a time - #162

Merged
rennf93 merged 2 commits into
Guard-Core:masterfrom
HardMax71:fix/cookie-header-per-pair
Oct 15, 2026
Merged

rennf93 merged 2 commits into
Guard-Core:masterfrom
HardMax71:fix/cookie-header-per-pair

Conversation

@HardMax71

Copy link
Copy Markdown
Contributor

Delivers issue: #161

Description

_scan_headers now scans the Cookie header one name=value pair at a time, the same way query parameters are scanned one by one. Before, the whole header was one value, so the ; between cookies counted as command separators in the semantic score, and an ordinary logged-in header reached 0.71 and was rejected (repro in #161).

Each pair goes through the same sensitive, excluded or normal header path as before, so every regex and semantic check still runs on each cookie, and the log redaction for cookie is unchanged. Only spaces around a pair are trimmed, so a CR or LF stays in the scanned value; empty pairs are skipped. Each pair counts against detection_max_scan_values, as each query parameter does.

The sync tree is regenerated with scripts/unasync.py, and docs/configuration/detection-tuning.md now says how the Cookie header is scanned.


Motivation and Context

Repro in #161.


Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change
  • Performance improvement
  • Code cleanup or refactoring

How Has This Been Tested

  • tests/test_detection/test_cookie_header_pairs.py (5 tests plus the generated sync copy): the logged-in cookie test fails on master and passes here; XSS, SQLi and CRLF inside one cookie are still detected.
  • Full suite with the dev, otel and logfire extras: 21,426 passed, 4 skipped. I left out tests/test_handlers_integration.py and tests/test_redis/ (and their sync copies), because they connect to a fixed localhost:6379 that is used by something else on my machine.
  • The detection tests with -W error --cov-branch: 8,116 passed, and the new lines are fully covered.
  • ruff, mypy, vulture, xenon, pymarkdownlnt and scripts/unasync.py --check: clean.
  • The repro from the issue prints True ... (score: 0.71) on master and False here.

Screenshots (if appropriate)


Checklist

  • My code follows the code style of this project (Mypy, Ruff)
  • I have added tests to cover my changes
  • All new and existing tests passed
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have checked that my changes don't introduce any new warnings or errors
  • I have updated the version number if necessary
  • I have added any new dependencies to the appropriate requirements file

The whole Cookie header was scanned as one value, so the ';' between
cookies counted as command separators in the semantic score and an
ordinary logged-in header with a JWT reached 0.71. Each pair now goes
through the same header scan on its own, like query parameters.
@HardMax71
HardMax71 requested a review from rennf93 as a code owner October 14, 2026 21:23
@github-actions github-actions Bot added documentation Docs, README, CHANGELOG, governance files area: sync Touches guard_core/sync/ (sync mirror generated by unasync) tests Test suite changes labels Oct 14, 2026
@HardMax71

Copy link
Copy Markdown
Contributor Author

The only CI failure is test_lazy_init_returns_quickly_with_blocking_background_init on 3.11 (0.623 < 0.5), and the other versions were cancelled after it. The same test failed on master in run 38593971238 (0.688 < 0.5, 3.14), so it looks like runner load rather than this change. The rest of the job passed with 100% coverage, and so did every gate. Could you re-run it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: sync Touches guard_core/sync/ (sync mirror generated by unasync) documentation Docs, README, CHANGELOG, governance files tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants