Repository navigation
fix(sarif): emit URI-valued OWASP help metadata - #1273
seonghobae wants to merge 4 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review Please review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested='e97f7b20a87f671e3ebfd4ef695c4d41f9dd2f87'
base='e71d37e7c58118e6764c96ab7c4492fe33eed6f8'
echo '== checkout =='
git rev-parse HEAD
git status --short
git cat-file -t "$requested"
git merge-base --is-ancestor "$base" "$requested" && echo "base-is-ancestor=yes"
echo '== exact diff name/status =='
git diff --name-status "$base" "$requested"
echo '== exact diff =='
git diff --no-ext-diff --unified=80 "$base" "$requested"
echo '== SARIF candidate files =='
fd -HI -t f -i 'sarif' . | sort
echo '== SARIF-related symbols and helpUri references =='
rg -n -i -C 4 '(findings_to_sarif|helpUri|help\.text|reportingDescriptor|CATEGORY_REFERENCE_DEFAULTS|OWASP)' . \
-g '!**/.git/**' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' \
| head -n 900Length of output: 84164 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== module outlines =='
ast-grep outline appguardrail_core/sarif.py --items all
ast-grep outline appguardrail_core/findings.py --items all
echo '== production SARIF writer =='
cat -n appguardrail_core/sarif.py
echo '== category defaults and reference normalization definitions =='
rg -n -C 12 '\bCATEGORY_REFERENCE_DEFAULTS\b|\breferences\b|def normalize_findings\b|def normalize_finding\b' \
appguardrail_core scanner -g '*.py'
echo '== exact category-default declarations and values =='
rg -n -U -C 3 'CATEGORY_REFERENCE_DEFAULTS\s*=\s*\{' appguardrail_core scanner -g '*.py'
echo '== production SARIF writer callers =='
rg -n -C 5 '\bfindings_to_sarif\s*\(' . \
-g '*.py' -g '!tests/**' -g '!**/.git/**'
echo '== URI parser boundary probe (standard library only) =='
python - <<'PY'
from urllib.parse import urlsplit
cases = [
"https://example.com/path",
" HTTPS://example.com/path ",
"https:example.com/path",
"https:///path",
"https://",
"https://example.com path",
"https://example.com/%ZZ",
"https://user:pass@example.com/path",
"https://[::1]/path",
"https://exa mple.com/path",
"https://example.com/\nnext",
"https://example.com/\x00next",
]
for value in cases:
try:
parsed = urlsplit(value.strip())
accepted = parsed.scheme in {"http", "https"} and bool(parsed.netloc)
print(repr(value), "=>", repr(parsed), "accepted=", accepted)
except ValueError as exc:
print(repr(value), "=> ValueError:", str(exc))
PYLength of output: 44859 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== production metadata and finding normalization =='
sed -n '1,190p' appguardrail_core/rules.py | cat -n
sed -n '1,125p' appguardrail_core/findings.py | cat -n
echo '== static comparison: default OWASP labels vs mapped labels =='
python - <<'PY'
import ast
from pathlib import Path
def assignment_dict(path, name):
tree = ast.parse(Path(path).read_text(encoding="utf-8"), filename=path)
for node in tree.body:
if (
isinstance(node, ast.Assign)
and any(isinstance(target, ast.Name) and target.id == name for target in node.targets)
):
return ast.literal_eval(node.value)
raise SystemExit(f"Missing {name} in {path}")
defaults = assignment_dict("appguardrail_core/rules.py", "CATEGORY_REFERENCE_DEFAULTS")
mappings = assignment_dict("appguardrail_core/sarif.py", "_OWASP_HELP_URIS")
default_owasp = {
item
for refs in defaults.values()
for item in refs
if item.startswith("OWASP ")
}
mapped = set(mappings)
print("default_owasp=", sorted(default_owasp))
print("mapped=", sorted(mapped))
print("unmapped_defaults=", sorted(default_owasp - mapped))
print("unused_mappings=", sorted(mapped - default_owasp))
PY
echo '== malformed URI probe (standard library only) =='
python - <<'PY'
from urllib.parse import urlsplit
for value in (
"https://[",
"https://[not-an-ipv6]",
"https://[::1",
"https://example.com/%",
"https://example.com/%2",
"https://example.com/\x00",
):
try:
parsed = urlsplit(value)
accepted = parsed.scheme in {"http", "https"} and bool(parsed.netloc)
print(repr(value), "accepted=", accepted, "parsed=", repr(parsed))
except ValueError as exc:
print(repr(value), "raises=", repr(str(exc)))
PY
echo '== existing test coverage for URI rejection, exceptions, and all mappings =='
rg -n -C 4 '(helpUri|_help_uri|urlsplit|OWASP A0[1-9]|OWASP A10|references)' tests/test_sarif.py tests \
-g '*.py' | head -n 500Length of output: 38705
I verified that all seven OWASP labels in
I did not execute repository code or tests.
|
Restore the audited #1273 helpUri repair, rule-index regression, and scoped changelog fragments after the concurrent optimization rewrite removed them. Preserve the already-verified O(M × R) to O(M + R) rule-index implementation and measurement boundary.
seonghobae
added
bug
ChatGPT Codex Connector
|
@coderabbitai review Please review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested='e97f7b20a87f671e3ebfd4ef695c4d41f9dd2f87'
base='e71d37e7c58118e6764c96ab7c4492fe33eed6f8'
echo '== exact revision =='
git rev-parse HEAD
git cat-file -e "${requested}^{commit}"
git merge-base --is-ancestor "$base" "$requested" && echo 'base-is-ancestor=yes'
echo '== changed files =='
git diff --name-status "$base" "$requested"
echo '== diff =='
git diff --no-ext-diff --unified=100 "$base" "$requested" -- \
appguardrail_core/sarif.py appguardrail_core/rules.py appguardrail_core/findings.py tests/test_sarif.py CHANGELOG.md
echo '== SARIF module outline =='
git show "${requested}:appguardrail_core/sarif.py" > /tmp/sarif.py
ast-grep outline /tmp/sarif.py --items all
echo '== SARIF production writer =='
git show "${requested}:appguardrail_core/sarif.py" | cat -n
echo '== reference sources and normalization =='
for path in appguardrail_core/rules.py appguardrail_core/findings.py; do
if git cat-file -e "${requested}:${path}" 2>/dev/null; then
echo "--- ${path} ---"
git show "${requested}:${path}" | rg -n -C 12 'CATEGORY_REFERENCE_DEFAULTS|def normalize_finding|references|reference' || true
fi
done
echo '== SARIF tests at exact revision =='
if git cat-file -e "${requested}:tests/test_sarif.py" 2>/dev/null; then
git show "${requested}:tests/test_sarif.py" | cat -n
fi
echo '== static default-to-URI mapping comparison =='
python - <<'PY'
import ast, subprocess
rev = "e97f7b20a87f671e3ebfd4ef695c4d41f9dd2f87"
def source(path):
return subprocess.check_output(["git", "show", f"{rev}:{path}"], text=True)
def value(path, name):
tree = ast.parse(source(path), filename=path)
for node in tree.body:
if isinstance(node, (ast.Assign, ast.AnnAssign)):
targets = node.targets if isinstance(node, ast.Assign) else [node.target]
if any(isinstance(t, ast.Name) and t.id == name for t in targets):
return ast.literal_eval(node.value)
raise RuntimeError(f"{name} missing in {path}")
defaults = value("appguardrail_core/rules.py", "CATEGORY_REFERENCE_DEFAULTS")
mappings = value("appguardrail_core/sarif.py", "_OWASP_HELP_URIS")
labels = {x for refs in defaults.values() for x in refs if isinstance(x, str) and x.startswith("OWASP ")}
print("unmapped default labels:", sorted(labels - set(mappings)))
print("unused mappings:", sorted(set(mappings) - labels))
PY
echo '== all exact-commit SARIF writer call sites =='
git grep -n -I -E 'findings_to_sarif[[:space:]]*\(' "$requested" -- '*.py' ':!tests/**' || trueLength of output: 36979
The OWASP mapping coverage is complete for the current
|
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
CHANGELOG.d/1216-sarif-help-uri.md— repository behaviorappguardrail_core/sarif.py— Python module behaviortests/test_sarif.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: 1216-sarif-help-uri.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: 1216-sarif-help-uri.md"]
R1 --> V1["required checks"]
Evidence --> S2["Python: sarif.py"]
S2 --> I2["Python module behavior"]
I2 --> R2["Review risk: Python: sarif.py"]
R2 --> V2["pytest plus coverage"]
Evidence --> S3["Test: test_sarif.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_sarif.py"]
R3 --> V3["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
e97f7b20a87f671e3ebfd4ef695c4d41f9dd2f87 - Workflow run: 35509162862
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: 1216-sarif-help-uri.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: 1216-sarif-help-uri.md"]
R1 --> V1["required checks"]
Evidence --> S2["Python: sarif.py"]
S2 --> I2["Python module behavior"]
I2 --> R2["Review risk: Python: sarif.py"]
R2 --> V2["pytest plus coverage"]
Evidence --> S3["Test: test_sarif.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_sarif.py"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
|
2026-09-26 current-head gate 재검증:
Required CodeQL failure가 해소되고 동일 exact head에서 새 terminal evidence와 review gate가 충족될 때까지 not-ready 후보입니다. Force Push, source-neutral retrigger, finding suppression 또는 protection 우회는 하지 않습니다. |
|
2026-09-26 fresh P1 repair on exact head
The canonical writer now skips invalid candidates, continues to a later valid HTTP(S) reference, and retains all original labels in |
Preserve the exact #1216 SARIF implementation and regression while integrating protected develop@2949d30718752ea5915c7713ba227e8c19d9e5bf through an ordinary two-parent commit. The branches touch disjoint paths.
Exact-head Actions infrastructure RCAHead Failed-run IDs: All failed jobs were re-requested in place. Merge remains blocked until the rerun conclusions and current-head review gates are complete; no gate was suppressed or weakened. |
|
Rerun follow-up: attempt 2 created new job IDs ( |
This branch has not been deployed
1 participant
Current exact authority — 2026-10-03 current-base integration
develop@2949d30718752ea5915c7713ba227e8c19d9e5bf7f22a760ec76f1e61373608c2c9c967e6f9bd3d6a2b9370a27c25a010d3d3f5131e776d682a80d054c45491dc1441c2b6fae0a3ec290a982a2de15adand protecteddevelop; 4 ahead / 0 behindCHANGELOG.d/1216-sarif-help-uri.md,appguardrail_core/sarif.py, andtests/test_sarif.pydevelopand all three SARIF blobs are retained; the two sides touched disjoint paths37104562819, Security Process37104562837, Retention37104562863, OpenSSF37104562830, Pinned HTTPS37104562813, and Tests37104562899are terminal FAILURE. Their 8 jobs havesteps=null; Tests jobs111150456440and111150456625returnBlobNotFoundon log retrieval. This is runner/admission evidence, not an executed SARIF-test verdict. Predecessor successes and CodeQL failure do not transfere97f7b20..., not this headThe previous wrong-base condition is repaired without Force Push or destructive rebase. Keep Draft until the unchanged exact head has terminal executable required Checks, successful CodeQL analysis where required, and a qualifying independent current-head approval. Do not use predecessor evidence, source-neutral wake commits, self-approval, or protection bypass.
Current authority
develop@e71d37e7c58118e6764c96ab7c4492fe33eed6f84c45491dc1441c2b6fae0a3ec290a982a2de15add67ee23c4a59a0fae169d394f38fe72d40487e2ee97f7b20a87f671e3ebfd4ef695c4d41f9dd2f87Root cause
The first repair mapped known OWASP labels and accepted existing HTTP(S) references, but treated
urllib.parse.urlsplit()as both parser and validator. An untrusted reference such ashttps://[raisesValueErrorand aborted the entire SARIF render. References containing whitespace, C0/DEL control characters, or incomplete percent escapes were accepted asreportingDescriptor.helpUrieven though they are not valid URI metadata.The exact-head writer now:
%not followed by two hexadecimal digits;help.text.This is the minimal canonical-writer repair; no new validator dependency or parallel SARIF path was added.
Reproducible RED → GREEN
Against predecessor
e97f7b20...:ValueError: Invalid IPv6 URL;helpUri;Against exact head
4c45491d...:FP/FN boundary
Positive accepted metadata is limited to known OWASP mappings and syntactically admitted absolute HTTP(S) references with a non-empty hostname. Invalid candidates are omitted from
helpUriwhile remaining readable inhelp.text; later valid references are still considered. This repair does not claim DNS resolution, reachability, ownership, content trust, or arbitrary-scheme validation.Exact-head workflows
36237290226— queued36237290236— queued36237290243— queued36237290261— queued36237290255— queued36237290257— queued36237290256— queued36237290275— queued36237290266— queuedQueued is not GREEN. The predecessor CodeQL failure and CHANGES_REQUESTED review are stale for this new head but remain historical evidence; a fresh terminal exact-head verdict and qualifying independent review are required.
Delivery gate
Keep Draft until one unchanged exact head has terminal Tests, Security Scan, SAST, CodeQL, coverage/governance checks, a fresh qualifying review, and no unresolved valid finding. Do not force-push, destructively rebase, self-approve, source-neutrally retrigger, weaken gates, merge, tag, release, or claim protected functionality before ordinary integration.
Relates to #1216.