feat(quality): add curated .quality/opengrep ruleset (arms lean gate 4) - #200
feat(quality): add curated .quality/opengrep ruleset (arms lean gate 4)#200Prekzursil wants to merge 1 commit into
Conversation
This repo shipped no `.quality/opengrep`, so the lean gate's SAST lane (gate 4
of reusable-quality.yml) was skipped entirely and the `quality` job still
reported success -- a skip-and-pass. This adds the missing pinned, in-repo
ruleset so gate 4 actually runs.
## What lands -- 19 rules across the 3 languages opengrep can see here
- `.quality/opengrep/csharp-security.yaml` (6) -- `BinaryFormatter`/
`SoapFormatter`/`NetDataContractSerializer`/`LosFormatter`; Newtonsoft
`TypeNameHandling != None`; a TLS validation callback returning `true`
unconditionally; MD5/SHA-1; `Process.Start` with a runtime-built command
string; `Assembly.Load*` from a computed path.
- `.quality/opengrep/python-security.yaml` (8) -- `exec`, `eval`,
`subprocess(..., shell=True)`, `os.system`/`os.popen`, unsafe `yaml.load`,
`pickle`/`marshal` loads, md5/sha1, TLS verification disabled.
- `.quality/opengrep/cpp-security.yaml` (3) -- unbounded `strcpy`/`strcat`/
`sprintf`/`gets` with a **non-literal** source, `system`/`popen`/`_wsystem`,
`tmpnam`/`mktemp`/`tempnam`.
- `.quality/opengrep/general-security.yaml` (2) -- committed PEM private key,
AWS access key id.
- `.quality/opengrep/README.md` -- language inventory, the vendored-tree
exclusions with their upstream licences, the suppression table, the measured
coverage residuals, and the both-states proof procedure.
- One inline `# nosemgrep:` suppression in `swfoc_toolkit/live_test.py`
(comments only, no behaviour change).
Scope per language was measured, not guessed: C# 398 files (`src/` 192,
`tests/` 206), Python 60, C/C++ 108, PowerShell 48, Lua 5.
**PowerShell gets no lane** -- opengrep 1.22.0 does not support the language
(`opengrep show supported-languages`). **Lua gets no lane** -- there is no
`load`/`loadstring`/`dofile`/`os.execute`/`io.popen` in the 5 `.lua` files, so a
rule there would be decoration. Both are stated in the README rather than left
to look covered.
## Why curated + pinned, not `--config auto`
A registry-fetched ruleset changes underneath you: green today, red tomorrow
with no change of yours. That externally-refreshing finding set is the treadmill
the lean charter exists to escape. A fixed in-repo ruleset makes the gate
deterministic, so zero is both reachable and stable.
## Verification (measured 2026-08-11, opengrep 1.22.0 -- the CI pin)
Both states were checked; a ruleset that has never gone red is indistinguishable
from an empty file. The fired-rule set is taken from `--json`
`results[].check_id`, because the pretty console output wraps long rule ids and
under-reports which rules fired.
- KNOWN-BAD control (`Vuln.cs` + `vuln.py` + `vuln.cpp` + a fake PEM/AKIA
fixture, outside the repo): **19/19 declared rules fired, 32 findings,
exit 1, 0 engine errors**.
- KNOWN-GOOD control (safe equivalents -- `System.Text.Json`,
`TypeNameHandling.None`, SHA-256, `ProcessStartInfo` + `ArgumentList`,
`ast.literal_eval`, `shell=False`, `yaml.safe_load`, `snprintf`):
**0 findings, exit 0, 0 engine errors**.
- REAL repo, gate-4 invocation replayed verbatim on a native Linux filesystem:
**`Ran 19 rules on 46724 files: 1 finding`, exit 1** before the suppression;
**0 findings, exit 0** after. Wall clock ~140-210 s.
- gitleaks 8.30.1 (gate 5) with the new files present: exit 0; detector
validated -- it exits 1 on the known-bad fixture.
## The 1 real finding, and why it is annotated rather than hidden
`swfoc_toolkit/live_test.py` `launch_game()` --
`subprocess.Popen(["cmd", "/c", f"start steam://rungameid/{STEAM_APP_ID}"], shell=True)`.
The argv is a fixed literal list whose only interpolation is the module-level
`STEAM_APP_ID` constant, so **no untrusted input reaches the shell** -- it is not
a command-injection vulnerability. `shell=True` is nonetheless redundant here
(the list already invokes `cmd /c`) and should be dropped; that is recorded as a
follow-up rather than changed inside a ruleset-adoption PR, because this harness
drives a real game process and cannot be exercised in CI.
The suppression is inline and greppable (`grep -rn nosemgrep`), and the reason is
in the code and in the README table. No `paths: exclude` entry hides it.
Note the Python rules are deliberately **wider** than `ruff.toml`'s
`extend-exclude`: a security lane should not inherit a style lane's scope, so
`swfoc_toolkit/`, `swfoc_lua_bridge/`, `bridge/` and `ghidra_scripts/` are
scanned.
## Vendored trees excluded from the C/C++ rules (named, with licences)
`swfoc_overlay/imgui/**` (Dear ImGui, MIT, (c) 2014-2024 Omar Cornut) and
`swfoc_lua_bridge/minhook/**` (MinHook, (c) 2009-2017 Tsuda Kageyu), both
carrying their upstream `LICENSE.txt` in-tree. This mirrors the DevExtreme
ruleset's `vendor/` exclusion and is not hiding a first-party hit: with the
exclusions removed the additional match count is still zero, because every
`strcpy` in this repo has a string-literal source and the rule already exempts
that shape.
## Gate-authoring traps this PR documents (README)
1. opengrep exits **2** on a rule parse error while still printing `0 findings`
-- a broken rule reads as "clean" if you only look at the count, and reds the
gate permanently in CI. Check the exit code.
2. opengrep reports rule **timeouts**, **partial parses** and **too-big skips**
as `level: warn` and **still exits 0** -- so gate 4 can silently stop covering
a file while staying green. The README enumerates the four measured residuals
on this repo (78 `knowledge-base/` files >1 MB; a Memprof limit on
`lua_bridge.cpp`; PartialParsing in 4 source files; an intermittent, purely
load-dependent 5 s rule timeout on the 230 KB `RuntimeAdapter.cs` -- 0/5
timeouts when that file is scanned alone). The `Process.Start` rule was
rewritten from a `metavariable-pattern` to a flat `pattern-either` to cut that
cost.
3. `nosemgrep` is only honoured on the **same line as the finding or the line
immediately above it**; a reason block ending several lines above suppresses
nothing.
![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: 8f99590695
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # `cmd /c`) and should be dropped -- tracked as a follow-up rather than | ||
| # changed inside a ruleset-adoption PR, because this harness drives a real | ||
| # game process and cannot be exercised in CI. | ||
| # nosemgrep: python-subprocess-shell-true -- literal argv, constant AppID |
There was a problem hiding this comment.
This line suppresses the only opengrep finding documented for the new gate, so CI can report zero findings while launch_game() still uses shell=True; in this repo the quality contract explicitly requires no suppressions/workarounds, so the gate should either fix the Popen call or keep failing instead of being annotated away.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
| # KNOWN-BAD: must exit 1, and every declared rule id must appear | ||
| opengrep scan --config .quality/opengrep --error --json --output bad.json fixtures/bad/ | ||
| # KNOWN-GOOD: must exit 0, 0 findings, and 0 entries in .errors | ||
| opengrep scan --config .quality/opengrep --error --json --output good.json fixtures/good/ |
There was a problem hiding this comment.
These verification commands point at fixtures/bad/ and fixtures/good/, but the commit does not add either directory in the repo, so anyone following the required ruleset proof gets a missing-path failure instead of the known-bad/known-good evidence. Add checked-in fixtures (or update the commands to their real checked-in path) so future rule changes can be verified reproducibly.
AGENTS.md reference: AGENTS.md:L52-L54
Useful? React with 👍 / 👎.
| - pattern: hashlib.md5($X) | ||
| - pattern: hashlib.sha1($X) |
There was a problem hiding this comment.
The direct hashlib.md5($X) / hashlib.sha1($X) patterns require an initial argument, so the common incremental form h = hashlib.md5(); h.update(data) has no expression for $X to bind and the weak-hash rule will not fire. Add no-argument alternatives or switch these to hashlib.md5(...) / hashlib.sha1(...), otherwise the new SAST gate misses weak hashes built via updates.
Useful? React with 👍 / 👎.
| # `cmd /c`) and should be dropped -- tracked as a follow-up rather than | ||
| # changed inside a ruleset-adoption PR, because this harness drives a real | ||
| # game process and cannot be exercised in CI. | ||
| # nosemgrep: python-subprocess-shell-true -- literal argv, constant AppID |
There was a problem hiding this comment.
This line suppresses the only opengrep finding documented for the new gate, so CI can report zero findings while launch_game() still uses shell=True; in this repo the quality contract explicitly requires no suppressions/workarounds, so the gate should either fix the Popen call or keep failing instead of being annotated away.
Useful? React with 👍 / 👎.
| # KNOWN-BAD: must exit 1, and every declared rule id must appear | ||
| opengrep scan --config .quality/opengrep --error --json --output bad.json fixtures/bad/ | ||
| # KNOWN-GOOD: must exit 0, 0 findings, and 0 entries in .errors | ||
| opengrep scan --config .quality/opengrep --error --json --output good.json fixtures/good/ |
There was a problem hiding this comment.
These verification commands point at fixtures/bad/ and fixtures/good/, but the commit does not add either directory in the repo, so anyone following the required ruleset proof gets a missing-path failure instead of the known-bad/known-good evidence. Add checked-in fixtures (or update the commands to their real checked-in path) so future rule changes can be verified reproducibly.
Useful? React with 👍 / 👎.
| - pattern: Process.Start($A + $B, ...) | ||
| - pattern: Process.Start(string.Format(...), ...) | ||
| - pattern: Process.Start(String.Format(...), ...) | ||
| - pattern: Process.Start($"...", ...) |
There was a problem hiding this comment.
These patterns only inspect the first Process.Start parameter, but the two-string overload also executes a literal program with a runtime-built argument string, e.g. Process.Start("cmd.exe", $"/c {userInput}"). That scenario still has the command-injection sink the rule is meant to block, yet the SAST gate would pass it unless second-argument interpolation/concatenation patterns are added.
Useful? React with 👍 / 👎.
| - pattern: MD5.Create(...) | ||
| - pattern: SHA1.Create(...) | ||
| - pattern: new MD5CryptoServiceProvider(...) | ||
| - pattern: new SHA1CryptoServiceProvider(...) | ||
| - pattern: new MD5Managed(...) | ||
| - pattern: new SHA1Managed(...) | ||
| - pattern: HashAlgorithm.Create("MD5") | ||
| - pattern: HashAlgorithm.Create("SHA1") |
There was a problem hiding this comment.
The repo targets .NET 8, where weak hashes can be called as MD5.HashData(...) or SHA1.HashData(...), but this rule only matches Create() and legacy provider constructors. A new use of those static APIs would therefore pass gate 4 even though it is the same MD5/SHA-1 use the rule advertises as blocking.
Useful? React with 👍 / 👎.
| - pattern: $X.ServerCertificateValidationCallback = (...) => true; | ||
| - pattern: $X.ServerCertificateCustomValidationCallback = (...) => true; | ||
| - pattern: ServicePointManager.ServerCertificateValidationCallback = (...) => true; | ||
| - pattern: $X.RemoteCertificateValidationCallback = (...) => true; |
There was a problem hiding this comment.
The TLS rule only matches expression-bodied => true assignments, so common always-accept forms such as assigning HttpClientHandler.DangerousAcceptAnyServerCertificateValidator or using a block lambda with return true; are not reported. In those cases certificate validation is still disabled, but the new SAST lane would stay green.
Useful? React with 👍 / 👎.
| - "*.example" | ||
| - ".env.example" | ||
| - "**/tests/**" | ||
| - "**/fixtures/**" |
There was a problem hiding this comment.
The README asks maintainers to prove every rule with fixtures/bad/, including fake PEM and AKIA controls, but this exclusion skips any path under fixtures/ for the two generic secret rules. Once the documented controls are checked in, those rules still will not fire, so the advertised 19/19 fired-rule proof cannot be reproduced unless the controls live outside the excluded tree or the exclusion is narrowed.
Useful? React with 👍 / 👎.
| - pattern: sprintf($DST, $SRC, ...) | ||
| - pattern: gets($DST) |
There was a problem hiding this comment.
This unsafe-string rule stops at sprintf/gets, so cases the rule text says it covers, such as scanf("%s", buf) or sscanf(input, "%s", buf), are not scanned at all; likewise the C++ spelling std::sprintf(...) is missed even though the rule has std:: variants for strcpy/strcat. Add those patterns or adjust the claim, otherwise gate 4 passes common unbounded native string reads/writes.
Useful? React with 👍 / 👎.
1 participant
feat(quality): add curated .quality/opengrep ruleset (arms lean gate 4)
This repo shipped no
.quality/opengrep, so the lean gate's SAST lane (gate 4of reusable-quality.yml) was skipped entirely and the
qualityjob stillreported success -- a skip-and-pass. This adds the missing pinned, in-repo
ruleset so gate 4 actually runs.
What lands -- 19 rules across the 3 languages opengrep can see here
.quality/opengrep/csharp-security.yaml(6) --BinaryFormatter/SoapFormatter/NetDataContractSerializer/LosFormatter; NewtonsoftTypeNameHandling != None; a TLS validation callback returningtrueunconditionally; MD5/SHA-1;
Process.Startwith a runtime-built commandstring;
Assembly.Load*from a computed path..quality/opengrep/python-security.yaml(8) --exec,eval,subprocess(..., shell=True),os.system/os.popen, unsafeyaml.load,pickle/marshalloads, md5/sha1, TLS verification disabled..quality/opengrep/cpp-security.yaml(3) -- unboundedstrcpy/strcat/sprintf/getswith a non-literal source,system/popen/_wsystem,tmpnam/mktemp/tempnam..quality/opengrep/general-security.yaml(2) -- committed PEM private key,AWS access key id.
.quality/opengrep/README.md-- language inventory, the vendored-treeexclusions with their upstream licences, the suppression table, the measured
coverage residuals, and the both-states proof procedure.
# nosemgrep:suppression inswfoc_toolkit/live_test.py(comments only, no behaviour change).
Scope per language was measured, not guessed: C# 398 files (
src/192,tests/206), Python 60, C/C++ 108, PowerShell 48, Lua 5.PowerShell gets no lane -- opengrep 1.22.0 does not support the language
(
opengrep show supported-languages). Lua gets no lane -- there is noload/loadstring/dofile/os.execute/io.popenin the 5.luafiles, so arule there would be decoration. Both are stated in the README rather than left
to look covered.
Why curated + pinned, not
--config autoA registry-fetched ruleset changes underneath you: green today, red tomorrow
with no change of yours. That externally-refreshing finding set is the treadmill
the lean charter exists to escape. A fixed in-repo ruleset makes the gate
deterministic, so zero is both reachable and stable.
Verification (measured 2026-08-11, opengrep 1.22.0 -- the CI pin)
Both states were checked; a ruleset that has never gone red is indistinguishable
from an empty file. The fired-rule set is taken from
--jsonresults[].check_id, because the pretty console output wraps long rule ids andunder-reports which rules fired.
Vuln.cs+vuln.py+vuln.cpp+ a fake PEM/AKIAfixture, outside the repo): 19/19 declared rules fired, 32 findings,
exit 1, 0 engine errors.
System.Text.Json,TypeNameHandling.None, SHA-256,ProcessStartInfo+ArgumentList,ast.literal_eval,shell=False,yaml.safe_load,snprintf):0 findings, exit 0, 0 engine errors.
Ran 19 rules on 46724 files: 1 finding, exit 1 before the suppression;0 findings, exit 0 after. Wall clock ~140-210 s.
validated -- it exits 1 on the known-bad fixture.
The 1 real finding, and why it is annotated rather than hidden
swfoc_toolkit/live_test.pylaunch_game()--subprocess.Popen(["cmd", "/c", f"start steam://rungameid/{STEAM_APP_ID}"], shell=True).The argv is a fixed literal list whose only interpolation is the module-level
STEAM_APP_IDconstant, so no untrusted input reaches the shell -- it is nota command-injection vulnerability.
shell=Trueis nonetheless redundant here(the list already invokes
cmd /c) and should be dropped; that is recorded as afollow-up rather than changed inside a ruleset-adoption PR, because this harness
drives a real game process and cannot be exercised in CI.
The suppression is inline and greppable (
grep -rn nosemgrep), and the reason isin the code and in the README table. No
paths: excludeentry hides it.Note the Python rules are deliberately wider than
ruff.toml'sextend-exclude: a security lane should not inherit a style lane's scope, soswfoc_toolkit/,swfoc_lua_bridge/,bridge/andghidra_scripts/arescanned.
Vendored trees excluded from the C/C++ rules (named, with licences)
swfoc_overlay/imgui/**(Dear ImGui, MIT, (c) 2014-2024 Omar Cornut) andswfoc_lua_bridge/minhook/**(MinHook, (c) 2009-2017 Tsuda Kageyu), bothcarrying their upstream
LICENSE.txtin-tree. This mirrors the DevExtremeruleset's
vendor/exclusion and is not hiding a first-party hit: with theexclusions removed the additional match count is still zero, because every
strcpyin this repo has a string-literal source and the rule already exemptsthat shape.
Gate-authoring traps this PR documents (README)
0 findings-- a broken rule reads as "clean" if you only look at the count, and reds the
gate permanently in CI. Check the exit code.
as
level: warnand still exits 0 -- so gate 4 can silently stop coveringa file while staying green. The README enumerates the four measured residuals
on this repo (78
knowledge-base/files >1 MB; a Memprof limit onlua_bridge.cpp; PartialParsing in 4 source files; an intermittent, purelyload-dependent 5 s rule timeout on the 230 KB
RuntimeAdapter.cs-- 0/5timeouts when that file is scanned alone). The
Process.Startrule wasrewritten from a
metavariable-patternto a flatpattern-eitherto cut thatcost.
nosemgrepis only honoured on the same line as the finding or the lineimmediately above it; a reason block ending several lines above suppresses
nothing.