fix(643): validate every field the extraction consumes; fix a vacuous test
Re-review of the previous fix commit found it incomplete and its test vacuous. Both reproduced before fixing. MEDIUM — the validation domain did not match the CONSUMPTION domain. `chunk` emits `(.previous_filename // empty)` for EVERY row regardless of status, but the guard validated that field only when `.status == "renamed"`. So a row marked `modified` — or Gitea's distinct `copied` — carrying a newline in previous_filename was still exempted. Verified EXEMPT for both statuses against the previous commit. Now validated whenever present, with the `renamed` => REQUIRED clause kept on top. MEDIUM — test_newline_in_previous_filename_is_also_rejected asserted the right outcome without exercising the mechanism: its payload's second segment was `ErsatzTV/Program.cs`, which the allow-list rejects on its own, so the test passed with the newline guard entirely removed. That is why the hole above went unnoticed — the same filter-hides-the-defect trap the guard itself is about. Payload changed to a segment that PASSES the allow-list, so the test now discriminates, plus parametrized regressions for modified/copied/added. LOW — `..` components rejected. The allow-list anchors `^docs/`, so `docs/../ErsatzTV/Program.cs` matched it (reproduced). Git will not produce such a path, but this guard exists to fail closed on unexpected 2xx shapes. Added a positive control (a legitimate docs->docs rename still exempts) so the tightened row schema cannot be satisfied by never exempting anything. Severity calibrated in the record: the docs-only exemption ends in `decide allow ""`, a passthrough to the normal permission prompt, NOT an auto-grant. Every bypass here downgrades a mechanical deny/ask to a human prompt; none can silently self-merge. Real, worth fixing, but not what an earlier framing of #643 implied. All mutation-verified; 123 passed under BOTH jq 1.8.2 and jq 1.6. Refs #643, #631
This commit is contained in:
@@ -122,11 +122,27 @@ while [ "$page" -le 40 ]; do
|
||||
# `docs/Program.cs`, both of which pass, while the actual single path ends in `.cs`. Git permits
|
||||
# newlines in filenames, so this is reachable, and it was reproduced against this hook. Failing
|
||||
# closed on control characters is the cheap fix; no decision/docs path ever contains one.
|
||||
#
|
||||
# VALIDATE EVERY FIELD THE EXTRACTION BELOW CONSUMES. `chunk` emits `(.previous_filename //
|
||||
# empty)` for EVERY row regardless of `.status`, so validating that field only on `renamed` rows
|
||||
# left a hole one predicate wide: a row with `status: "modified"` (or Gitea's distinct `copied`)
|
||||
# carrying a newline in `previous_filename` was reproducibly exempted. The rule this encodes:
|
||||
# the validation domain must match the CONSUMPTION domain, not the domain the field is
|
||||
# semantically "supposed to" appear in. The `renamed` => REQUIRED clause is kept on top of the
|
||||
# unconditional if-present check.
|
||||
#
|
||||
# `..` is rejected for the same reason: the allow-list anchors `^docs/`, so
|
||||
# `docs/../ErsatzTV/Program.cs` matches it. Git will not produce such a path, but this guard's
|
||||
# whole job is to fail closed on unexpected 2xx shapes rather than to assume a well-behaved peer.
|
||||
if ! printf '%s' "$raw" \
|
||||
| jq -e 'type == "array" and all(.[];
|
||||
(.filename | type == "string" and length > 0 and (test("[\\r\\n]") | not))
|
||||
| jq -e 'def ok: type == "string" and length > 0
|
||||
and (test("[\\r\\n]") | not)
|
||||
and (split("/") | index("..") | not);
|
||||
type == "array" and all(.[];
|
||||
(.filename | ok)
|
||||
and (.previous_filename == null or (.previous_filename | ok))
|
||||
and (if .status == "renamed"
|
||||
then (.previous_filename | type == "string" and length > 0 and (test("[\\r\\n]") | not))
|
||||
then (.previous_filename | type == "string" and length > 0)
|
||||
else true end))' \
|
||||
>/dev/null 2>&1; then
|
||||
files_complete=no; break
|
||||
|
||||
@@ -77,6 +77,20 @@ Plus a **Medium**: paging is several round-trips, so a force-push between them y
|
||||
belonging to no single commit. The head sha is now re-read after enumeration and the exemption
|
||||
refused if it moved.
|
||||
|
||||
A re-review of that fix then found it **incomplete**, and the test for it **vacuous**: `chunk`
|
||||
consumes `previous_filename` on EVERY row, but the guard validated it only on `renamed` rows, so a
|
||||
`modified`/`copied` row carrying a newline there was still exempted — while the test meant to cover
|
||||
that side used a payload the allow-list rejected anyway, so it passed with the guard removed. **The
|
||||
rule that generalises: validate every field the extraction CONSUMES, not the fields it is
|
||||
semantically supposed to contain — and a test whose payload fails for an unrelated reason asserts
|
||||
nothing.**
|
||||
|
||||
**Severity, stated honestly.** The docs-only exemption ends in `decide allow ""` — a *passthrough*
|
||||
to the normal permission prompt, not an auto-grant (only the satisfied a+b+c path grants). So every
|
||||
bypass above downgrades a mechanical deny/ask to a human prompt; none of them can produce a silent
|
||||
self-merge. That is a real weakening of the gate, and worth fixing, but it is not the
|
||||
"unreviewed code merges itself" scenario an earlier framing of #643 implied.
|
||||
|
||||
**The generalisable lesson is about the SHAPE of this guard, not any one bug.** Every defect here
|
||||
was an *exhaustiveness* failure in an enumeration whose completeness is load-bearing: each looked
|
||||
like a complete list and wasn't. When a security decision depends on having seen ALL of something,
|
||||
|
||||
@@ -362,12 +362,45 @@ def test_newline_in_filename_does_not_split_into_two_passing_paths(hook):
|
||||
|
||||
|
||||
def test_newline_in_previous_filename_is_also_rejected(hook):
|
||||
"""Same hole via the rename side — `previous_filename` is flattened identically."""
|
||||
hook.set_pages([{"filename": "docs/ok.md", "previous_filename": "safe.md\nErsatzTV/Program.cs",
|
||||
"status": "renamed"}])
|
||||
"""Same hole via the rename side — `previous_filename` is flattened identically.
|
||||
|
||||
NOTE the payload's second segment must itself be allow-list-PASSING (`docs/Program.cs`, not
|
||||
`ErsatzTV/Program.cs`). The first version of this test used the latter, which the allow-list
|
||||
rejects on its own merits, so the test passed with the newline guard entirely removed — it
|
||||
asserted the outcome without ever exercising the mechanism. That is the same
|
||||
filter-hides-the-defect trap the guard itself is about."""
|
||||
hook.set_pages([{"filename": "docs/ok.md", "previous_filename": "safe.md\ndocs/Program.cs",
|
||||
"status": "renamed"}], [])
|
||||
assert hook.exempted() is False
|
||||
|
||||
|
||||
@pytest.mark.parametrize("status", ["modified", "copied", "added"])
|
||||
def test_previous_filename_is_validated_on_NON_renamed_rows_too(hook, status):
|
||||
"""The validation domain must match the CONSUMPTION domain.
|
||||
|
||||
`chunk` emits `(.previous_filename // empty)` for EVERY row regardless of `.status`, but the
|
||||
field was validated only when `.status == "renamed"`. A row marked `modified` (or Gitea's
|
||||
distinct `copied`) carrying a newline in `previous_filename` was reproducibly exempted."""
|
||||
hook.set_pages([{"filename": "docs/ok.md", "status": status,
|
||||
"previous_filename": "safe.md\ndocs/Program.cs"}], [])
|
||||
assert hook.exempted() is False
|
||||
|
||||
|
||||
def test_dotdot_path_component_is_rejected(hook):
|
||||
"""The allow-list anchors `^docs/`, so `docs/../ErsatzTV/Program.cs` matches it. Git will not
|
||||
produce such a path, but this guard's job is to fail closed on unexpected 2xx shapes rather
|
||||
than assume a well-behaved peer."""
|
||||
hook.set_pages([{"filename": "docs/../ErsatzTV/Program.cs", "status": "modified"}], [])
|
||||
assert hook.exempted() is False
|
||||
|
||||
|
||||
def test_legitimate_rename_within_docs_still_exempts(hook):
|
||||
"""Positive control: the tightened row schema must not break a real docs-only rename."""
|
||||
hook.set_pages([{"filename": "docs/b.md", "status": "renamed",
|
||||
"previous_filename": "docs/a.md"}], [])
|
||||
assert hook.exempted() is True
|
||||
|
||||
|
||||
def test_short_NONTERMINAL_page_does_not_end_the_enumeration(hook):
|
||||
""""Fewer rows than we asked for" must not be read as "last page".
|
||||
|
||||
|
||||
Reference in New Issue
Block a user