test: hypothesis property tests + atheris fuzzer overhaul - #28
Conversation
Adds tests/test_properties.py with 23 property-based tests covering every public API invariant: clean() safety/idempotency/NFKC-stability, walk() mutation/sanitization/depth/cycles, detect_scripts/is_mixed_script consistency, jinja2_escaper delimiter safety, path_escaper traversal prevention, and decode_evasion robustness. Atheris fuzzer updated: - fuzz_walk registered and wired to CLI (was dead code) - --target= flag for selecting fuzz target - NFKC-stability and homoglyph-free invariants added to fuzz_clean - Leaf string verification added to fuzz_walk - CI matrix expanded to run both fuzz_clean and fuzz_walk Infrastructure: - hypothesis>=6.0.0 added to dev dependencies - Hypothesis profiles: dev (50), ci (500), security (10000)
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,321 @@ | |||
| # SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
🔵 LOW: Comprehensive property-based test coverage using Hypothesis
Confidence: 95%
New class-based property tests for `clean()`, `walk()`, `jinja2_escaper`, `path_escaper`, and `decode_evasion` covering idempotency, type preservation, absence of dangerous characters, and contract-based properties.
Introduces over 20 property-based tests for all major invariants of the public API using Hypothesis. These tests cover edge conditions, method contracts, and adversarial cases, demonstrating attention to both correctness and security-hardening of critical paths.
Suggestion: Continue maintaining property-based tests alongside feature changes. Consider increasing max_examples in CI/security profiles for even greater bug surface coverage when appropriate.
— You wrote more property tests than most teams manage all year. ...noted. 🧐
| @@ -0,0 +1,29 @@ | |||
| # SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
🔵 LOW: Structured Hypothesis profile management for CI and dev workflow separation
Confidence: 95%
conftest.py: dev (50), ci (500), and security (10,000) example caps with HealthCheck suppression.
Defines distinct Hypothesis profiles for dev, CI, and security runs in conftest.py, allowing test runs to be tailored for velocity, coverage, or adversarial depth. This reduces developer friction while enabling deep coverage when needed.
Suggestion: Document in CONTRIBUTING how and when to use 'security' profile for pre-release verification, as it may be missed by casual contributors.
— Someone finally read the Hypothesis docs. Practical, not overkill.
| @@ -1,52 +1,55 @@ | |||
| # SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
🔵 LOW: Atheris fuzzer overhaul with properly wired up multiple fuzz targets
Confidence: 90%
fuzz_clean.py: Selection logic for --target=, updated job matrix, new checks for homoglyphs and NFKC-stability inside fuzzers.
Refactors the fuzz harness to accept a --target option, supporting both clean() and walk() invariant checks. This closes a gap where fuzz_walk was defined but never executed. Both targets now verify key invariants including idempotency, NFKC normalization, homoglyph and invisible character removal, and type preservation.
Suggestion: Consider documenting expected coverage and any remaining black-box areas that are not exercised by Atheris. Ensure CI handles and reports failures from all enabled targets.
— The fuzzer is doing real work now. Surprised it took a cat for this to happen.
✅ Grippy Review — PASSScore: 100/100 | Findings: 5 total (3 inline, 2 summary-only) Delta: 3 new Summary-only findings (2) — scored but not inline-eligible🔵 LOW: Expands fuzzing CI matrix to run both fuzz_clean and fuzz_walk📁 The CI matrix now covers both fuzz_clean and fuzz_walk by setting the matrix targets and connecting the target to the fuzzer runner CLI. This increases execution reliability of all fuzz harnesses and closes the gap where walk invariants were not checked during PRs. Suggestion: Monitor execution time/costs in CI; if the matrix slows down overall test turnaround, consider sharding or optimizing example counts. 🔵 LOW: Dev dependency and lock package alignment for Hypothesis📁 Hypothesis is now explicitly tracked as a dev dependency in both pyproject and uv.lock, enabling deterministic installations for test and development environments. Suggestion: Track dependency updates for Hypothesis so that tests remain stable over time. Be alert for potential breaking changes between minor versions. Commit: 41e2854 |
![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: 41e28540b9
ℹ️ 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 (@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 (@codex) address that feedback".
| """Property-based tests for clean() invariants.""" | ||
|
|
||
| @given(text=unicode_text) | ||
| @settings(max_examples=50) |
There was a problem hiding this comment.
The per-test @settings(max_examples=50) (and 20) decorators override profile-level settings, so the ci/security profiles registered in tests/conftest.py never take effect for these tests. In practice, pytest --hypothesis-profile=ci will still run these properties at the hardcoded counts, which silently prevents the intended higher-coverage CI/release runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds property-based testing (Hypothesis) and improves the Atheris fuzzing setup to strengthen invariant coverage across navi-sanitize’s public API, while updating CI to run multiple fuzz targets.
Changes:
- Add Hypothesis property tests covering invariants for
clean(),walk(), escapers, script detection, anddecode_evasion(). - Register Hypothesis profiles via pytest
conftest.pyand add Hypothesis as a dev dependency (incl. lockfile updates). - Overhaul the Atheris harness to support
--target=...selection and expand CI to run bothfuzz_cleanandfuzz_walk.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_properties.py |
New Hypothesis property tests for public API invariants. |
tests/conftest.py |
Adds Hypothesis profiles intended for dev/ci/security runs. |
fuzz/fuzz_clean.py |
Adds multi-target selection and stronger invariants for fuzzing clean()/walk(). |
.github/workflows/fuzz.yml |
Expands CI fuzz matrix to run fuzz_clean and fuzz_walk. |
pyproject.toml |
Adds Hypothesis to dev dependency group. |
uv.lock |
Locks Hypothesis (+ transitive sortedcontainers) for reproducible installs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @given(text=unicode_text) | ||
| @settings(max_examples=50) | ||
| def test_always_returns_str(self, text: str) -> None: | ||
| assert isinstance(clean(text), str) | ||
|
|
||
| @given(text=hostile_text) | ||
| @settings(max_examples=50) | ||
| def test_no_null_bytes(self, text: str) -> None: | ||
| assert "\x00" not in clean(text) | ||
|
|
||
| @given(text=hostile_text) | ||
| @settings(max_examples=50) | ||
| def test_no_invisible_characters(self, text: str) -> None: | ||
| assert not INVISIBLE_RE.search(clean(text)) | ||
|
|
||
| @given(text=hostile_text) | ||
| @settings(max_examples=50) | ||
| def test_no_homoglyphs(self, text: str) -> None: | ||
| result = clean(text) | ||
| remaining = set(result) & set(HOMOGLYPH_MAP) | ||
| assert not remaining, f"Homoglyphs remain: {remaining!r}" | ||
|
|
||
| @given(text=unicode_text) | ||
| @settings(max_examples=50) | ||
| def test_nfkc_stable(self, text: str) -> None: | ||
| result = clean(text) | ||
| assert unicodedata.normalize("NFKC", result) == result | ||
|
|
||
| @given(text=unicode_text) | ||
| @settings(max_examples=50) | ||
| def test_idempotent(self, text: str) -> None: | ||
| first = clean(text) | ||
| assert clean(first) == first | ||
|
|
||
| @given(text=hostile_text) | ||
| @settings(max_examples=50) | ||
| def test_idempotent_hostile(self, text: str) -> None: | ||
| first = clean(text) |
There was a problem hiding this comment.
Each property test is decorated with @settings(max_examples=50), which hard-codes the example count and prevents the Hypothesis profiles (ci/security) from increasing coverage as described in the module docstring and tests/conftest.py. Consider removing the per-test @settings(...) decorators (or not overriding max_examples) so --hypothesis-profile=ci / security actually takes effect.
| def _collect_leaf_strings(data: object) -> list[str]: | ||
| """Extract all leaf strings from a dict/list structure.""" | ||
| result: list[str] = [] | ||
| stack: list[object] = [data] | ||
| seen: set[int] = set() | ||
| while stack: | ||
| item = stack.pop() | ||
| obj_id = id(item) | ||
| if obj_id in seen: | ||
| continue | ||
| if isinstance(item, str): | ||
| result.append(item) | ||
| elif isinstance(item, dict): | ||
| seen.add(obj_id) | ||
| stack.extend(item.values()) | ||
| elif isinstance(item, list): | ||
| seen.add(obj_id) | ||
| stack.extend(item) | ||
| return result |
There was a problem hiding this comment.
walk() sanitizes dict keys as well as values (see walk() implementation); however, _collect_leaf_strings() only traverses dict.values(), so these property tests never assert that sanitized keys are free of nulls/invisibles/homoglyphs and NFKC-stable. Consider extending the traversal to include dict keys (when they are str) so the tests cover key sanitization too.
| suppress_health_check=[HealthCheck.too_slow], | ||
| ) | ||
|
|
||
| settings.load_profile("dev") |
There was a problem hiding this comment.
tests/conftest.py unconditionally calls settings.load_profile("dev"). This makes it easy for the default to override other intended profiles and conflicts with the docs that suggest selecting ci/security via --hypothesis-profile=.... Consider only loading a default profile when no explicit profile was requested (e.g., let Hypothesis/pytest load the CLI-selected profile, or load based on an env var/pytest option).
| settings.load_profile("dev") | |
| def pytest_configure(config) -> None: | |
| """Load the default Hypothesis profile only when none was requested.""" | |
| if not config.getoption("hypothesis_profile"): | |
| settings.load_profile("dev") |
| # Invariant: all leaf strings satisfy clean() postconditions | ||
| stack: list[object] = [result] | ||
| seen: set[int] = set() | ||
| while stack: | ||
| item = stack.pop() | ||
| obj_id = id(item) | ||
| if obj_id in seen: | ||
| continue | ||
| if isinstance(item, str): | ||
| assert "\x00" not in item | ||
| assert not INVISIBLE_RE.search(item) | ||
| assert not (set(item) & set(HOMOGLYPH_MAP)) | ||
| assert unicodedata.normalize("NFKC", item) == item | ||
| elif isinstance(item, dict): | ||
| seen.add(obj_id) | ||
| stack.extend(item.values()) | ||
| elif isinstance(item, list): | ||
| seen.add(obj_id) | ||
| stack.extend(item) |
There was a problem hiding this comment.
In fuzz_walk, the invariant check only traverses dict.values(). Since walk() also sanitizes dict keys, the fuzzer currently won’t catch regressions where key sanitization reintroduces nulls/invisibles/homoglyphs. Consider also validating string keys while traversing dictionaries.
| if escaper is None: | ||
| # Invariant: no null bytes | ||
| assert "\x00" not in result | ||
|
|
||
| # Invariant: no invisible characters in output (escaper=None path) | ||
| if escaper is None: | ||
| # Invariant: no invisible characters | ||
| assert not INVISIBLE_RE.search(result) | ||
|
|
||
| # Invariant: idempotent — second pass is a no-op | ||
| if escaper is None: | ||
| # Invariant: no homoglyphs from the map | ||
| assert not (set(result) & set(HOMOGLYPH_MAP)) | ||
|
|
||
| # Invariant: NFKC-stable | ||
| assert unicodedata.normalize("NFKC", result) == result | ||
|
|
||
| # Invariant: idempotent | ||
| assert clean(result) == result |
There was a problem hiding this comment.
The homoglyph checks repeatedly build set(HOMOGLYPH_MAP) inside the fuzz loop (set(result) & set(HOMOGLYPH_MAP)), which adds avoidable overhead in a performance-sensitive harness. Consider precomputing a HOMOGLYPHS = frozenset(HOMOGLYPH_MAP) once at module import and reusing it for membership/intersection checks.
Summary
fuzz_cleanandfuzz_walkProperty tests (
tests/test_properties.py)clean()walk()detect_scriptsis_mixed_scriptjinja2_escaperpath_escaperdecode_evasionHypothesis profiles
uv run pytest(default)--hypothesis-profile=ciAtheris fuzzer changes
--target=CLI flag for target selection (was hardcoded to fuzz_clean)fuzz_walkwired up: builds structured data from FuzzedDataProvider, verifies all leaf strings[fuzz_clean, fuzz_walk]Test plan
🐈 Waffler demanded this before the release. The cat was right.