Close the mirror gate's last hole, add reference-integrity and oval_pattern guards

Open
#177 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
68/100
Issue type
Feature
Clarity
Mostly clear
Activity status
Active
Tech stack
shell

Research direction

Start with tests/oscap-offline/internal/mirrors/mirrors_test.go, then read tests/stamps/run.sh and run_test.sh for the existing guards and test flow. Run the relevant mirror and stamp tests first. Done means datastream-only entries fail, references resolve after normalizing ./, and an unknown OVAL object id is covered by a test that confirms the script aborts.

Written by the indexing model from the issue text.

Description

Follow-ups from the review of #172 (merged; the review found zero in-scope findings). Each has been verified against main at 1e979cb, with notes below where verification changed the picture.

1. report.Unmirrored is logged, not failed

tests/oscap-offline/internal/mirrors/mirrors_test.go fails on report.Functional and report.Descriptive but only t.Logfs report.Unmirrored. The stub was that list's one permanent entry, so this is the last hole in the mirror gate: oscap-playground's has_stub_oval metadata flag could reintroduce the component on a regeneration and nothing would fail.

Verified: the list is empty on mainTestRepositoryMirrorsMatch reports compared 8 standalone OVAL file(s) with no "present only in the datastream" lines. So flipping it to t.Errorf is a no-op today and a guard from here on.

Worth deciding deliberately: it would also mean any future datastream-only definition has to gain a standalone counterpart before it can land. Given #163 and #164 moved deliberately toward full mirroring, that seems like the intent rather than a side effect.

2. Reference-integrity check over check-content-ref/@href

Nothing currently resolves every check-content-ref/@href through cat:catalog and fails on an unresolved one. mirrors.Check only covers the standalone-file-without-datastream-block direction, and the full-datastream validation in .github/workflows/create-release.yaml is commented out. tests/e2e/fixtures/*/expected.txt asserts 8 lines across 4 rule ids out of 198 rules, so a deletion touching any of the ~180 unasserted, unmirrored rules would go unnoticed — which is what makes this worth having.

Correction to the review's stated payoff. It held that such a check "would immediately surface the pre-existing, unrelated ./AslrCheck.sh mismatch (ssg-chainguard-gpos-ds.xml:6079, :6098 — no matching cat:uri)". That reference is not broken. There is a catalog entry:

AslrCheck.sh -> #scap_org.open-scap_cref_AslrCheck

The href reads ./AslrCheck.sh and the cat:uri name is AslrCheck.sh, so the strings differ, but OpenSCAP normalizes the prefix and resolves it. Confirmed by evaluating the rule: xccdf_mil.disa.stig_rule_SV-203754r958928_rule returns pass, i.e. the script was found and run.

./AslrCheck.sh is also the only href in the datastream carrying a ./ prefix; the other eight are bare and match their catalog names exactly.

So the requirement for this check is that it normalize ./ prefixes before comparing, or it will report a false positive on a working reference on its first run. A naive exact-match version was written while verifying this and did exactly that.

3. oval_pattern has no test coverage, and fails closed only via set -e

tests/stamps/run.sh:79-93's oval_pattern exits non-zero for an unknown object id, and run_test.sh has no coverage of it at all.

The margin is thinner than the review suggested. pattern="$(oval_pattern "${obj_id}")" feeds grep -Pq -- "${pattern}" at :177-178, and grep -Pq -- "" matches anything — so an empty pattern would pass the guard silently rather than failing it. The only thing preventing that is set -euo pipefail at :35, under which a failing command substitution in an assignment aborts the script (verified).

There is no bug today. But the fail-closed behaviour rests entirely on set -e: wrapping that call in a conditional, or appending || true, would silently turn a trust-store guard into a no-op with no test to catch it. A run_test.sh case asserting oval_pattern exits non-zero on an unknown id — and that the script aborts rather than reaching grep — is worth having for that reason specifically.

Not included

The review also raised a process point: agents citing container-local artifact paths (randomized claude-guard-diffmap-*.json mount roots) as corroborating evidence, which produced a false accusation of a fabricated path between agents. Reviews should cite repo-relative paths only. That is tooling guidance rather than a repo change, so it is recorded here but has no task attached.

Dominant language
HTML
Stars
25
Forks
15
Avg merge
3d 8h
Merged PRs (30d)
21

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Similar issues

More Security issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.