crosswalk: an outside opinion, and three rules with something behind them
Browse filesTwo gaps in `nexa check`, both of them about the corpus rather than the code:
it can only find what it has a rule for, and the worklist that was meant to
say where the missing rules are had been pointing at the wrong place.
AN OUTSIDE OPINION
crosscheck, adversarial, fieldtest and recall all ask whether what we wrote
behaves. None of them can ask whether it is the right set of things to have
written, because a rule nobody thought of is invisible to every check that
reads the rules.
OpenSPP runs semgrep against 44 hand-written Odoo rules of its own. `nexa
crosswalk` compares the two corpora without running semgrep -- not installed
here, and an offline tool will not grow a package index for one question.
Each of their patterns becomes the smallest snippet it would match and OUR
checkers run over that, so every pairing rests on a checker firing rather
than on two rule descriptions sounding alike.
their patterns turned into a probe 146 of 146
both corpora fire on the same construct 6 (10 now, see below)
theirs fires, nothing here does 38
ours no pattern of theirs could match 7 of 23 comparable
The six were eval, exec, pickle, os.popen, shell=True and domain injection
through eval -- every one reached by the two Python rules about executing
input, and not one by an Odoo rule.
There is a second axis that is about code rather than rule text. OpenSPP's
source carries 687 `# nosemgrep:` markers, each a finding that fired AND that
a person read and kept the code over. Of the 631 that name a rule, a checker
here fires on 20. `odoo-sudo-without-context` fires on every `.sudo()`; ours
fires on `.sudo().search(` and clears on a company_id filter. Both are
defensible and they are different rules about the same word, which is what
most of the disagreement turns out to be.
THE COLUMN THE HARVEST WAS MISSING
3.30's worklist said `21x api.depends (nothing caught here)` against a rule
about exactly that, which reads as a missing rule. Reading the diffs -- which
3.30 tells you to do and which nobody had -- most of those commits merely ADD
a compute, and the scope-token denominator counts them anyway. The
interesting one was hdx_cod_source.py, where the checker fired correctly on
the file before the fix and the fix went to a different line. Recall scored
that a miss, and the worklist then said "nothing caught here" about a rule
that was firing.
`measure` records `spoke` now -- rules with a finding in the file that is not
a catch -- and --harvest splits every cluster on it:
missed caught spoke silent
.sudo( 70 6 47 23
api.depends 23 0 16 7
api.constrains 16 0 12 4
ondelete 13 0 10 3
total in scope 154 6 92 62
Ninety-two of 154 misses are not blind spots. That wants a sharper rule, not
a new one, and a flat miss count had been hiding which of the two you have
since 3.25.
THREE RULES
odoo.sql-needs-query-parameters critical
odoo.commit-inside-a-loop-splits-the-transaction high
odoo.depends-must-not-name-unread-fields medium
The first two are the crosswalk gaps that were about correctness rather than
security. Both are AST passes for the same reason from opposite directions: a
commit is the same string wherever it sits, and a parameterised query carries
the same percent sign as an interpolated one. No regex sees either difference.
The commit rule fires once in 229,600 lines of OpenSPP, on a line already
carrying `# nosemgrep: odoo-commit-in-loop - batch consent expiry cron`. Two
analysers, one construct, one file, and a person who had already decided.
The third came from the diffs: two commits removed an @api.depends entry the
body never read. Its checker was narrowed three times against OpenSPP and
each narrowing is a class it now deliberately misses -- a record handed to a
call, a read outside the declarations, a compute counting rows on another
model. The last costs one of the two incidents it was written from, and that
is stated in the checker rather than glossed: once a compute reads another
model, a declared dependency is the author's best approximation rather than a
leftover. 42 findings before narrowing, 11 after, 10 of them true positives
classified by hand.
Field rate unchanged at 2.1 per 1000 lines on OpenSPP. Every version claim
documented in 16.0, 17.0 and 18.0 -- cr.commit is a contributing guideline
rather than an ORM reference topic, and the guidelines say the thing the rule
says. The crosswalk agreement is 10 now because the gap got closed, not
because anything was re-scored.
TWO SMALLER THINGS
`# nosec` joins the suppression markers. Bandit is an independent analyser and
the argument is the one that put nosemgrep on the list; two of the SQL rule's
six findings sat on lines carrying nothing else.
`python bin/recall.py --harvest --json` died part-way through the report on a
commit subject containing an arrow, taking the JSON artifact with it. The
artifact is written before the report now, and stdout is reconfigured rather
than left to raise.
STALE NUMBERS, AGAIN
Two survived the last sweep because they were not derived from anything:
"64/64 rewrites" was 87 before this change and is 98 after, and "0 of 12
satisfied patterns" was already 14.
409 tests. validate, crosscheck, adversarial, coverage, check-sources and the
retrieval eval all clean; 43 of 43 rules retrievable by their own probe.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- ARCHITECTURE.md +144 -8
- README.md +68 -26
- bin/crosswalk.py +767 -0
- bin/layers.py +3 -3
- bin/recall.py +66 -13
- bin/semantics.py +371 -0
- corpus/build_corpus.py +93 -0
- corpus/check_sources.py +6 -0
- corpus/corpus_meta.json +5 -5
- corpus/rules/odoo.json +69 -0
- modelfiles/gen-odoo.Modelfile +43 -0
- nexa.py +41 -1
- tests/test_crosswalk.py +396 -0
- tests/test_layers.py +23 -0
- tests/test_recall.py +93 -5
|
@@ -131,7 +131,7 @@ is silently undone by the next build.
|
|
| 131 |
more: the model cites a real rule and then breaks it. Observed exactly that — the 7B
|
| 132 |
cited `odoo.onchange-is-not-validation` above an `@api.onchange`.
|
| 133 |
|
| 134 |
-
**Consequence.** 32 of
|
| 135 |
an AST pass (3.12) and the remaining three return no verdict rather than a false
|
| 136 |
pass. Patterns match the code with comments stripped (3.13).
|
| 137 |
|
|
@@ -293,7 +293,7 @@ message — and narrowed the checker. The other was real: the `correct` sample o
|
|
| 293 |
so a sample in this corpus did not follow another rule in this corpus. The sample was
|
| 294 |
fixed, exactly as in §3.5.
|
| 295 |
|
| 296 |
-
**Consequence.** Machine-checkable coverage is
|
| 297 |
AST. `nexa coverage` reports the two mechanisms separately, because a rule that
|
| 298 |
compares meaning and a rule that compares text are not the same kind of checked, and
|
| 299 |
one percentage would hide which of the two a `critical` rule is resting on.
|
|
@@ -1175,6 +1175,137 @@ once instead of in whichever was remembered. That is the same argument 3.2 makes
|
|
| 1175 |
about the corpus and the Modelfiles, and it took writing a second feature to
|
| 1176 |
notice the drift had already happened.
|
| 1177 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 1178 |
## 4. Performance traps, all found by measurement
|
| 1179 |
|
| 1180 |
Within four instrumented runs the report showed the 7B at ~10 tok/s against a
|
|
@@ -1246,11 +1377,11 @@ in a corpus; it does not close the reasoning gap.
|
|
| 1246 |
|
| 1247 |
### Checking is partial, and the layers say where
|
| 1248 |
|
| 1249 |
-
- 3 of
|
| 1250 |
false pass, and `nexa coverage` prints the reason for each.
|
| 1251 |
-
-
|
| 1252 |
catches textual signatures, not meaning. Code wrong in a way no pattern describes
|
| 1253 |
-
passes silently.
|
| 1254 |
- Semantic analysis is **Python only**. The standard library has no TypeScript or
|
| 1255 |
JavaScript parser, so TS/JS answers get delimiter balance and nothing deeper, and
|
| 1256 |
the layer reports UNKNOWN rather than PASS for the rest.
|
|
@@ -1322,9 +1453,13 @@ unawares.
|
|
| 1322 |
|
| 1323 |
### Corpus provenance
|
| 1324 |
|
| 1325 |
-
|
| 1326 |
-
deserve more scepticism.
|
| 1327 |
-
production incidents
|
|
|
|
|
|
|
|
|
|
|
|
|
| 1328 |
|
| 1329 |
This is the deepest unresolved problem in the design and no layer addresses it: the
|
| 1330 |
validator and the thing being validated are the same class of system. Provenance
|
|
@@ -1430,6 +1565,7 @@ nexa/
|
|
| 1430 |
│ ├── adversarial.py evasion / counterexample / contradiction probes
|
| 1431 |
│ ├── fieldtest.py every checker against somebody else's source
|
| 1432 |
│ ├── recall.py what the corpus misses, against real bugfixes
|
|
|
|
| 1433 |
│ ├── eval.py the distribution: ask, the pipeline, retrieval, check
|
| 1434 |
│ ├── endpoint.py where Ollama is, and what to say when it is not
|
| 1435 |
│ ├── evidence.py per-run bundles: the record behind a verdict
|
|
|
|
| 131 |
more: the model cites a real rule and then breaks it. Observed exactly that — the 7B
|
| 132 |
cited `odoo.onchange-is-not-validation` above an `@api.onchange`.
|
| 133 |
|
| 134 |
+
**Consequence.** 32 of 43 rules are checkable by pattern. Eight more are checkable by
|
| 135 |
an AST pass (3.12) and the remaining three return no verdict rather than a false
|
| 136 |
pass. Patterns match the code with comments stripped (3.13).
|
| 137 |
|
|
|
|
| 293 |
so a sample in this corpus did not follow another rule in this corpus. The sample was
|
| 294 |
fixed, exactly as in §3.5.
|
| 295 |
|
| 296 |
+
**Consequence.** Machine-checkable coverage is 40 of 43 (93%): 32 by pattern, 8 by
|
| 297 |
AST. `nexa coverage` reports the two mechanisms separately, because a rule that
|
| 298 |
compares meaning and a rule that compares text are not the same kind of checked, and
|
| 299 |
one percentage would hide which of the two a `critical` rule is resting on.
|
|
|
|
| 1175 |
about the corpus and the Modelfiles, and it took writing a second feature to
|
| 1176 |
notice the drift had already happened.
|
| 1177 |
|
| 1178 |
+
### 3.34 Somebody else's ruleset, which is the only outside opinion available
|
| 1179 |
+
|
| 1180 |
+
Every measurement above tests whether what we wrote behaves. `crosscheck` fires the
|
| 1181 |
+
checkers at the corpus's own correct samples, `adversarial` rewrites those samples,
|
| 1182 |
+
`fieldtest` fires them at a stranger's tree, `recall` at a stranger's history. All
|
| 1183 |
+
four ask the same question from different angles.
|
| 1184 |
+
|
| 1185 |
+
None of them can ask whether the corpus is the right set of rules to have written,
|
| 1186 |
+
and that question cannot be answered from inside it: a rule nobody thought of is
|
| 1187 |
+
invisible to every check that reads the rules. Coverage of the rules is not coverage
|
| 1188 |
+
of the subject, and nothing here could tell the difference.
|
| 1189 |
+
|
| 1190 |
+
OpenSPP runs semgrep against a hand-written Odoo ruleset of its own, 44 rules in
|
| 1191 |
+
`.semgrep/odoo-security.yml`. Somebody else decided what was worth checking, on the
|
| 1192 |
+
same platform, without reading ours. `nexa crosswalk` compares the two.
|
| 1193 |
+
|
| 1194 |
+
**Semgrep is never run.** It is not installed here and an offline tool will not grow
|
| 1195 |
+
a package index to answer one question. Instead each of their patterns is turned into
|
| 1196 |
+
the smallest snippet it would match -- `$CR.execute($QUERY % ...)` becomes
|
| 1197 |
+
`_cr.execute(_query % _h1)` -- and OUR checkers are run over that. Every pairing then
|
| 1198 |
+
rests on a checker firing rather than on two rule descriptions sounding alike, which
|
| 1199 |
+
is the failure mode any hand-written mapping between two rulesets would have.
|
| 1200 |
+
|
| 1201 |
+
| | | first run | now |
|
| 1202 |
+
|---|---|---|---|
|
| 1203 |
+
| Their rules read | 44 | | |
|
| 1204 |
+
| Their patterns turned into a probe | 146 of 146 | | |
|
| 1205 |
+
| Both corpora fire on the same construct | | **6** | **10** |
|
| 1206 |
+
| Theirs fires, nothing here does | | 38 | 34 |
|
| 1207 |
+
| Ours no pattern of theirs could match | 7 of 23 comparable | | |
|
| 1208 |
+
|
| 1209 |
+
The first six were `eval`, `exec`, `pickle`, `os.popen`, `shell=True` and domain
|
| 1210 |
+
injection through `eval` -- every one of them reached by the two Python rules about
|
| 1211 |
+
executing input, and none by an Odoo rule. The other four are the rules this run
|
| 1212 |
+
produced, which is the difference between a measurement and a worklist: the number
|
| 1213 |
+
moved because the gap it named got closed, not because anything was re-scored.
|
| 1214 |
+
|
| 1215 |
+
On everything else the two corpora barely intersect, and the honest reading of that
|
| 1216 |
+
is not that either is wrong: theirs is about security and this one is about ORM
|
| 1217 |
+
correctness. A gap on a subject a corpus deliberately does not cover looks exactly
|
| 1218 |
+
like one it forgot, and the report says so rather than scoring it.
|
| 1219 |
+
|
| 1220 |
+
**The evidence that is not about rule text.** OpenSPP's source carries 687
|
| 1221 |
+
`# nosemgrep:` markers. Each one is a finding that fired AND that a person read and
|
| 1222 |
+
decided to keep, which makes it the only record available here of what the other
|
| 1223 |
+
analyser does on real code. It is partial in a known direction: it records what was
|
| 1224 |
+
dismissed and says nothing about what was fixed.
|
| 1225 |
+
|
| 1226 |
+
| | |
|
| 1227 |
+
|---|---|
|
| 1228 |
+
| Lines their analyser fired on | 631 (56 more markers name no rule) |
|
| 1229 |
+
| …where a checker here also fires | **20** |
|
| 1230 |
+
|
| 1231 |
+
Twenty of 631. `odoo-sudo-without-context` fires on every `.sudo()`;
|
| 1232 |
+
`odoo.sudo-bypasses-record-rules` fires on `.sudo().search(` and clears on a
|
| 1233 |
+
`company_id` filter. Both decisions are defensible and they are different rules about
|
| 1234 |
+
the same word, which is what most of the disagreement turns out to be.
|
| 1235 |
+
|
| 1236 |
+
**Two rules came out of it.** `odoo-commit-in-loop` and the SQL-injection family were
|
| 1237 |
+
subjects with a real correctness failure behind them and nothing here that fired.
|
| 1238 |
+
Both are now rules, and both are AST passes rather than patterns for the same reason
|
| 1239 |
+
from opposite directions: a commit is the same string wherever it sits, and a
|
| 1240 |
+
parameterised query carries the same percent sign as an interpolated one. A regex
|
| 1241 |
+
cannot see either difference.
|
| 1242 |
+
|
| 1243 |
+
The commit rule found exactly one hit in 229,600 lines of OpenSPP, and that line
|
| 1244 |
+
already carried `# nosemgrep: odoo-commit-in-loop — batch consent expiry cron`. Two
|
| 1245 |
+
analysers, the same construct, the same file, and a person who had already decided.
|
| 1246 |
+
That is the strongest corroboration anything in this repository has.
|
| 1247 |
+
|
| 1248 |
+
### 3.35 Reading the harvest, and a column it was missing
|
| 1249 |
+
|
| 1250 |
+
3.30 turned the missed list into a worklist clustered by construct, and the ranking
|
| 1251 |
+
said `21x api.depends (nothing caught here)` against a rule about exactly that. The
|
| 1252 |
+
obvious reading is a missing rule, and the obvious reading was wrong.
|
| 1253 |
+
|
| 1254 |
+
Reading the diffs -- which 3.30 says to do and which nobody had -- of the 23 fix
|
| 1255 |
+
commits whose diff touched `@api.depends`, most merely ADD a compute. The decorator is
|
| 1256 |
+
in the diff because a new one was written, not because a depends bug was fixed, and
|
| 1257 |
+
the scope-token denominator counts them anyway. Of the rest, the interesting case was
|
| 1258 |
+
`spp_area_hdx/models/hdx_cod_source.py`: the checker fired correctly on the file
|
| 1259 |
+
before the fix, and the fix went to a different line. Recall scored that a miss, and
|
| 1260 |
+
the worklist then said "nothing caught here" about a rule that was firing.
|
| 1261 |
+
|
| 1262 |
+
**The column.** `measure` now records `spoke` -- rules with a finding in the file that
|
| 1263 |
+
is not a catch -- and `--harvest` splits every cluster on it:
|
| 1264 |
+
|
| 1265 |
+
| | missed | caught | spoke | silent |
|
| 1266 |
+
|---|---|---|---|---|
|
| 1267 |
+
| `.sudo(` | 70 | 6 | 47 | 23 |
|
| 1268 |
+
| `ValidationError` | 39 | 1 | 29 | 10 |
|
| 1269 |
+
| `Many2one` | 26 | 1 | 14 | 12 |
|
| 1270 |
+
| `api.depends` | 23 | 0 | 16 | **7** |
|
| 1271 |
+
| `api.constrains` | 16 | 0 | 12 | 4 |
|
| 1272 |
+
| `ondelete` | 13 | 0 | 10 | 3 |
|
| 1273 |
+
| **total in scope** | **154** | 6 | **92** | **62** |
|
| 1274 |
+
|
| 1275 |
+
Ninety-two of the 154 misses are not blind spots. The corpus was looking straight at
|
| 1276 |
+
those files and pointing somewhere else, and that wants the opposite repair from a
|
| 1277 |
+
rule that does not exist: a sharper rule, not a new one. A flat miss count hides which
|
| 1278 |
+
of the two you have, and it had been hiding it since 3.25.
|
| 1279 |
+
|
| 1280 |
+
**One rule came out of the diffs.** Two commits removed an `@api.depends` entry the
|
| 1281 |
+
body never read -- a leftover from an earlier body in both cases. The harm is mild and
|
| 1282 |
+
stated as mild: a dependency the body cannot use does not make the value stale, it
|
| 1283 |
+
makes it recompute on writes that could not have changed it. The reason it is worth a
|
| 1284 |
+
rule is the second commit, where `@api.depends("topic_ids")` sat on a compute that
|
| 1285 |
+
counted rows on another model. The decorator was the only statement anywhere of what
|
| 1286 |
+
the value was made from, and it said the dependency was accounted for while nothing
|
| 1287 |
+
was watching the value at all.
|
| 1288 |
+
|
| 1289 |
+
The checker was narrowed three times against OpenSPP, and each narrowing is a class it
|
| 1290 |
+
now deliberately misses: a body that hands a record to a call, a body that reads a
|
| 1291 |
+
field the decorator does not declare, and a body that counts rows on another model.
|
| 1292 |
+
The last one costs the second incident -- it is exactly that shape and is no longer
|
| 1293 |
+
reported -- because once a compute reads another model, a declared dependency is the
|
| 1294 |
+
author's best available approximation rather than a leftover, and telling them to
|
| 1295 |
+
delete it removes the only recompute trigger the value has.
|
| 1296 |
+
|
| 1297 |
+
| | |
|
| 1298 |
+
|---|---|
|
| 1299 |
+
| Findings on OpenSPP before narrowing | 42 |
|
| 1300 |
+
| …after | **11** (0.05 per 1000 lines) |
|
| 1301 |
+
| True positives among them, classified by hand | **10 of 11** |
|
| 1302 |
+
|
| 1303 |
+
The one false positive is a compute whose docstring reads "Dummy compute to trigger
|
| 1304 |
+
dependent field recomputation" -- a deliberate canary, depending on fields it does not
|
| 1305 |
+
read on purpose. The checker cannot tell that from the two dead legacy computes it
|
| 1306 |
+
correctly found in the same run, and there is no reason it should: both are findings
|
| 1307 |
+
and only one of them is wanted.
|
| 1308 |
+
|
| 1309 |
## 4. Performance traps, all found by measurement
|
| 1310 |
|
| 1311 |
Within four instrumented runs the report showed the 7B at ~10 tok/s against a
|
|
|
|
| 1377 |
|
| 1378 |
### Checking is partial, and the layers say where
|
| 1379 |
|
| 1380 |
+
- 3 of 43 rules have no automated check at all. They return **no verdict**, never a
|
| 1381 |
false pass, and `nexa coverage` prints the reason for each.
|
| 1382 |
+
- 32 of the 40 checked rules are decided by a **regex over generated code**. That
|
| 1383 |
catches textual signatures, not meaning. Code wrong in a way no pattern describes
|
| 1384 |
+
passes silently. The other 8 are decided by an AST pass (§3.12).
|
| 1385 |
- Semantic analysis is **Python only**. The standard library has no TypeScript or
|
| 1386 |
JavaScript parser, so TS/JS answers get delimiter balance and nothing deeper, and
|
| 1387 |
the layer reports UNKNOWN rather than PASS for the rest.
|
|
|
|
| 1453 |
|
| 1454 |
### Corpus provenance
|
| 1455 |
|
| 1456 |
+
40 of 43 rules cite upstream documentation. The rest rest on general knowledge and
|
| 1457 |
+
deserve more scepticism. Most of the rules were written by an assistant rather than
|
| 1458 |
+
harvested from production incidents, and rules drawn from real incidents are worth
|
| 1459 |
+
more. Three now are: `commit-inside-a-loop` and `sql-needs-query-parameters` came
|
| 1460 |
+
from an external ruleset disagreeing with this one (3.34), and
|
| 1461 |
+
`depends-must-not-name-unread-fields` from a fix commit in OpenSPP's history (3.35).
|
| 1462 |
+
Three of forty-three is not a change of method yet.
|
| 1463 |
|
| 1464 |
This is the deepest unresolved problem in the design and no layer addresses it: the
|
| 1465 |
validator and the thing being validated are the same class of system. Provenance
|
|
|
|
| 1565 |
│ ├── adversarial.py evasion / counterexample / contradiction probes
|
| 1566 |
│ ├── fieldtest.py every checker against somebody else's source
|
| 1567 |
│ ├── recall.py what the corpus misses, against real bugfixes
|
| 1568 |
+
│ ├── crosswalk.py this corpus against somebody else's ruleset
|
| 1569 |
│ ├── eval.py the distribution: ask, the pipeline, retrieval, check
|
| 1570 |
│ ├── endpoint.py where Ollama is, and what to say when it is not
|
| 1571 |
│ ├── evidence.py per-run bundles: the record behind a verdict
|
|
@@ -294,23 +294,23 @@ Same principle as `z.infer<typeof Schema>`: one definition, derived artifacts, n
|
|
| 294 |
|
| 295 |
| | |
|
| 296 |
|---|---|
|
| 297 |
-
| Rules |
|
| 298 |
-
| Critical |
|
| 299 |
-
| Machine-checkable |
|
| 300 |
-
| Traceable to upstream docs |
|
| 301 |
| Pattern false positives | 0 cross-hits across the corpus |
|
| 302 |
-
| Recall under meaning-preserving rewrite |
|
| 303 |
-
| Satisfied patterns bypassable by a comment | 0 of
|
| 304 |
-
| Retrievable by their own probe |
|
| 305 |
| Hit rate on 34,550 lines of reviewed OCA code | 3.9 per 1000 lines |
|
| 306 |
-
| Hit rate on 229,600 lines of OpenSPP (Odoo 19) | 2.1 per 1000 lines, +
|
| 307 |
-
| **Recall against real Odoo bugfixes** | **1 of 56 (OCA), 6 of
|
| 308 |
| Verdict stability, 12 questions x 5 runs | 12/12 same verdict every run |
|
| 309 |
| Escalation rescued a bad draft | 12 of 22, 95% CI 35–73% |
|
| 310 |
| **`nexa check`, 5 files x 5 runs** | **23 findings reported, in 5 of 5 files — every one the corpus can make** |
|
| 311 |
| Rules the model added beyond the checkers | 1, across 25 reviews |
|
| 312 |
|
| 313 |
-
|
| 314 |
context a snippet does not contain, so they return **no verdict** — never a false pass
|
| 315 |
— and `nexa coverage` names each one and why.
|
| 316 |
|
|
@@ -481,31 +481,73 @@ What *is* adopted needed no measuring: a rule the checkers already fired on at l
|
|
| 481 |
nexa recall ../OpenSPP2-19.0 --harvest
|
| 482 |
```
|
| 483 |
|
| 484 |
-
clusters the fixes the corpus
|
| 485 |
-
so the reading order is the writing order. Against **OpenSPP 2
|
| 486 |
-
|
| 487 |
|
| 488 |
| | |
|
| 489 |
|---|---|
|
| 490 |
-
| Fix commits examined |
|
| 491 |
-
| …touching a construct the corpus covers |
|
| 492 |
| Caught | 6 |
|
| 493 |
-
| Recall against in-scope fixes |
|
| 494 |
|
| 495 |
-
Two repositories, two ecosystems, 2–
|
| 496 |
clustering says where to dig:
|
| 497 |
|
| 498 |
```
|
| 499 |
-
|
| 500 |
-
|
| 501 |
-
|
| 502 |
-
|
|
|
|
| 503 |
```
|
| 504 |
|
| 505 |
-
|
| 506 |
-
|
| 507 |
-
|
| 508 |
-
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 509 |
|
| 510 |
### Every version claim is checked or stated
|
| 511 |
|
|
@@ -535,7 +577,7 @@ nexa eval --retrieval
|
|
| 535 |
asks each rule its own probe and reports whether it comes back. No generation, so it
|
| 536 |
needs only the embedding model and finishes in seconds.
|
| 537 |
|
| 538 |
-
All
|
| 539 |
not, and both failed the way a rule does: correctly worded, in the vocabulary of
|
| 540 |
somebody who already knows the answer. `no-monkey-patching` did not answer *"change
|
| 541 |
what a core sale.order method does"*; `unknown-not-any-at-boundaries` did not answer
|
|
|
|
| 294 |
|
| 295 |
| | |
|
| 296 |
|---|---|
|
| 297 |
+
| Rules | 43 — 18 Odoo, 10 TypeScript, 8 Python, 7 JavaScript |
|
| 298 |
+
| Critical | 17 |
|
| 299 |
+
| Machine-checkable | 40 (93%) — 32 by pattern, 8 by AST |
|
| 300 |
+
| Traceable to upstream docs | 40 (93%), each audited for topic fit |
|
| 301 |
| Pattern false positives | 0 cross-hits across the corpus |
|
| 302 |
+
| Recall under meaning-preserving rewrite | 98/98 |
|
| 303 |
+
| Satisfied patterns bypassable by a comment | 0 of 14 |
|
| 304 |
+
| Retrievable by their own probe | 43 of 43, at top 3 |
|
| 305 |
| Hit rate on 34,550 lines of reviewed OCA code | 3.9 per 1000 lines |
|
| 306 |
+
| Hit rate on 229,600 lines of OpenSPP (Odoo 19) | 2.1 per 1000 lines, +24 another tool had already flagged |
|
| 307 |
+
| **Recall against real Odoo bugfixes** | **1 of 56 (OCA), 6 of 160 (OpenSPP). See below.** |
|
| 308 |
| Verdict stability, 12 questions x 5 runs | 12/12 same verdict every run |
|
| 309 |
| Escalation rescued a bad draft | 12 of 22, 95% CI 35–73% |
|
| 310 |
| **`nexa check`, 5 files x 5 runs** | **23 findings reported, in 5 of 5 files — every one the corpus can make** |
|
| 311 |
| Rules the model added beyond the checkers | 1, across 25 reviews |
|
| 312 |
|
| 313 |
+
40 of 43 rules carry an automated check. The remaining three are decidable only with
|
| 314 |
context a snippet does not contain, so they return **no verdict** — never a false pass
|
| 315 |
— and `nexa coverage` names each one and why.
|
| 316 |
|
|
|
|
| 481 |
nexa recall ../OpenSPP2-19.0 --harvest
|
| 482 |
```
|
| 483 |
|
| 484 |
+
clusters the fixes the corpus did not flag on the line of the fix by the construct
|
| 485 |
+
each diff touched, so the reading order is the writing order. Against **OpenSPP 2
|
| 486 |
+
(Odoo 19)** — a second ecosystem on a newer series than the OCA repositories:
|
| 487 |
|
| 488 |
| | |
|
| 489 |
|---|---|
|
| 490 |
+
| Fix commits examined | 406 |
|
| 491 |
+
| …touching a construct the corpus covers | 160 |
|
| 492 |
| Caught | 6 |
|
| 493 |
+
| Recall against in-scope fixes | 4% |
|
| 494 |
|
| 495 |
+
Two repositories, two ecosystems, 2–4% both times. The floor is a floor — and the
|
| 496 |
clustering says where to dig:
|
| 497 |
|
| 498 |
```
|
| 499 |
+
missed caught spoke silent
|
| 500 |
+
.sudo( 70 6 47 23
|
| 501 |
+
api.depends 23 0 16 7
|
| 502 |
+
api.constrains 16 0 12 4
|
| 503 |
+
ondelete 13 0 10 3
|
| 504 |
```
|
| 505 |
|
| 506 |
+
**`spoke` is the column that changed the reading.** A miss is not automatically a
|
| 507 |
+
blind spot: a checker firing somewhere else in the same file means the rule exists
|
| 508 |
+
and is aimed wrong, which wants a sharper rule rather than a new one. Ninety-two of
|
| 509 |
+
the 154 misses are that. The first version of this table reported `21x api.depends
|
| 510 |
+
(nothing caught here)`, which reads as a missing rule — and the checker was in fact
|
| 511 |
+
firing on sixteen of those files while the fixes went elsewhere.
|
| 512 |
+
|
| 513 |
+
Reading the diffs behind the seven that really were silent produced one rule,
|
| 514 |
+
`odoo.depends-must-not-name-unread-fields`. Two of the remaining rules in this corpus
|
| 515 |
+
came from the crosswalk below.
|
| 516 |
+
|
| 517 |
+
### An outside opinion, which is the one thing the corpus cannot give itself
|
| 518 |
+
|
| 519 |
+
```bash
|
| 520 |
+
nexa crosswalk ../OpenSPP2-19.0/.semgrep/odoo-security.yml --checkout ../OpenSPP2-19.0
|
| 521 |
+
```
|
| 522 |
+
|
| 523 |
+
Every other number here tests whether what we wrote behaves. None of them can ask
|
| 524 |
+
whether it is the right set of rules to have written, because a rule nobody thought of
|
| 525 |
+
is invisible to every check that reads the rules. OpenSPP runs semgrep against 44
|
| 526 |
+
hand-written Odoo rules of its own — somebody else's judgement, on the same platform,
|
| 527 |
+
formed without reading ours.
|
| 528 |
+
|
| 529 |
+
Semgrep is never run: each of their patterns is turned into the smallest snippet it
|
| 530 |
+
would match and *our* checkers run over that, so a pairing rests on a checker firing
|
| 531 |
+
rather than on two rule descriptions sounding alike.
|
| 532 |
+
|
| 533 |
+
| | first run | now |
|
| 534 |
+
|---|---|---|
|
| 535 |
+
| Their patterns turned into a probe | 146 of 146 | 146 of 146 |
|
| 536 |
+
| Both corpora fire on the same construct | 6 | 10 |
|
| 537 |
+
| Theirs fires, nothing here does | 38 | 34 |
|
| 538 |
+
| Lines their analyser fired on (`# nosemgrep:` markers) | 631 | 631 |
|
| 539 |
+
| …where a checker here also fires | 20 | 20 |
|
| 540 |
+
|
| 541 |
+
The agreement moved from 6 to 10 because the gap it named got closed, not because
|
| 542 |
+
anything was re-scored — the first six were all reached by the two Python rules about
|
| 543 |
+
executing input, and not one by an Odoo rule.
|
| 544 |
+
|
| 545 |
+
Most of that gap is a different subject, not a missing rule: theirs is about security
|
| 546 |
+
and this one is about ORM correctness. Two entries were neither, and are now rules —
|
| 547 |
+
`odoo.commit-inside-a-loop-splits-the-transaction` and
|
| 548 |
+
`odoo.sql-needs-query-parameters`. The commit rule fires once in 229,600 lines of
|
| 549 |
+
OpenSPP, on a line already carrying `# nosemgrep: odoo-commit-in-loop`: two analysers,
|
| 550 |
+
one construct, and a person who had already decided.
|
| 551 |
|
| 552 |
### Every version claim is checked or stated
|
| 553 |
|
|
|
|
| 577 |
asks each rule its own probe and reports whether it comes back. No generation, so it
|
| 578 |
needs only the embedding model and finishes in seconds.
|
| 579 |
|
| 580 |
+
All 43 rules are retrievable at the operating `TOP_K` of 6, and at top 3. Two were
|
| 581 |
not, and both failed the way a rule does: correctly worded, in the vocabulary of
|
| 582 |
somebody who already knows the answer. `no-monkey-patching` did not answer *"change
|
| 583 |
what a core sale.order method does"*; `unknown-not-any-at-boundaries` did not answer
|
|
@@ -0,0 +1,767 @@
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 1 |
+
#!/usr/bin/env python3
|
| 2 |
+
"""Compare this corpus against somebody else's ruleset for the same stack.
|
| 3 |
+
|
| 4 |
+
WHY THIS EXISTS
|
| 5 |
+
---------------
|
| 6 |
+
Every measurement in this repository is taken against code. `crosscheck` fires
|
| 7 |
+
the checkers at the corpus's own correct samples, `adversarial` rewrites those
|
| 8 |
+
samples, `fieldtest` fires them at a stranger's tree, `recall` fires them at a
|
| 9 |
+
stranger's history. All four ask the same question from different angles: does
|
| 10 |
+
what we wrote behave.
|
| 11 |
+
|
| 12 |
+
None of them asks whether what we wrote is the right set of things to have
|
| 13 |
+
written. That question cannot be answered from inside the corpus, because the
|
| 14 |
+
corpus is the thing under test -- a rule nobody thought of is invisible to every
|
| 15 |
+
check that reads the rules.
|
| 16 |
+
|
| 17 |
+
An external ruleset for the same stack is the one artefact that can answer it.
|
| 18 |
+
Somebody else decided what was worth checking, on the same platform, without
|
| 19 |
+
reading ours. Where the two agree, two independent judgements landed on the same
|
| 20 |
+
construct. Where theirs has a rule and ours has nothing, that is a subject we
|
| 21 |
+
did not think of, named by somebody who did.
|
| 22 |
+
|
| 23 |
+
python crosswalk.py <ruleset.yml> [--checkout DIR] [--json OUT]
|
| 24 |
+
|
| 25 |
+
WHAT IT DOES NOT DO
|
| 26 |
+
-------------------
|
| 27 |
+
It does not run the other analyser. Semgrep is not installed here and adding it
|
| 28 |
+
would make an offline tool need a package index. Instead the ruleset's own
|
| 29 |
+
patterns are turned into the smallest snippet each one would match, and OUR
|
| 30 |
+
checkers are run over that. That direction works because our checkers are what
|
| 31 |
+
we have; the other direction is answered separately and more weakly, and the
|
| 32 |
+
report says which claim rests on which.
|
| 33 |
+
|
| 34 |
+
It is also not a score. A rule of theirs that nothing here fires on is a
|
| 35 |
+
candidate, not a defect: this corpus is about ORM correctness and a security
|
| 36 |
+
ruleset is about security, and the answer to most of that gap is "different
|
| 37 |
+
subject" rather than "missing rule". Reading it is the work.
|
| 38 |
+
|
| 39 |
+
Stdlib only. No model, no network.
|
| 40 |
+
"""
|
| 41 |
+
import argparse
|
| 42 |
+
import ast
|
| 43 |
+
import collections
|
| 44 |
+
import io
|
| 45 |
+
import json
|
| 46 |
+
import os
|
| 47 |
+
import re
|
| 48 |
+
import sys
|
| 49 |
+
|
| 50 |
+
HERE = os.path.dirname(os.path.abspath(__file__))
|
| 51 |
+
sys.path.insert(0, HERE)
|
| 52 |
+
|
| 53 |
+
import corpus # noqa: E402
|
| 54 |
+
import layers # noqa: E402
|
| 55 |
+
|
| 56 |
+
CROSSWALK_VERSION = "1.0.0"
|
| 57 |
+
|
| 58 |
+
# A semgrep rule says which languages it is about. Ours say which stack, and the
|
| 59 |
+
# two do not line up one to one: a `languages: [python]` rule is answerable by
|
| 60 |
+
# anything in the odoo or python stacks, because both of those are Python.
|
| 61 |
+
LANG_STACKS = {
|
| 62 |
+
"python": ("odoo", "python"),
|
| 63 |
+
"py": ("odoo", "python"),
|
| 64 |
+
"js": ("javascript", "typescript"),
|
| 65 |
+
"javascript": ("javascript", "typescript"),
|
| 66 |
+
"ts": ("typescript", "javascript"),
|
| 67 |
+
"typescript": ("typescript", "javascript"),
|
| 68 |
+
}
|
| 69 |
+
|
| 70 |
+
# Directories that are never the code under review. The same list fieldtest
|
| 71 |
+
# walks, for the same reason.
|
| 72 |
+
SKIP_DIRS = frozenset([".git", ".hg", "node_modules", "static", "__pycache__",
|
| 73 |
+
".tox", ".venv", "venv", "dist", "build", "migrations"])
|
| 74 |
+
|
| 75 |
+
# Only the suppression markers that NAME a rule are read here. `# noqa` says a
|
| 76 |
+
# person silenced something; `# nosemgrep: odoo-sudo-without-context` says which
|
| 77 |
+
# finding they silenced, and the name is the part that can be compared with
|
| 78 |
+
# anything.
|
| 79 |
+
SUPPRESSION_RE = re.compile(r"#\s*nosemgrep\s*:?\s*([A-Za-z0-9_.\-, ]*)")
|
| 80 |
+
|
| 81 |
+
|
| 82 |
+
# ------------------------------------------------------------------- reading
|
| 83 |
+
|
| 84 |
+
class ExternalRule(object):
|
| 85 |
+
"""One rule read out of somebody else's ruleset."""
|
| 86 |
+
|
| 87 |
+
__slots__ = ("rid", "severity", "languages", "category", "cwe",
|
| 88 |
+
"patterns", "message")
|
| 89 |
+
|
| 90 |
+
def __init__(self, rid):
|
| 91 |
+
self.rid = rid
|
| 92 |
+
self.severity = ""
|
| 93 |
+
self.languages = []
|
| 94 |
+
self.category = ""
|
| 95 |
+
self.cwe = ""
|
| 96 |
+
self.patterns = []
|
| 97 |
+
self.message = ""
|
| 98 |
+
|
| 99 |
+
def as_dict(self):
|
| 100 |
+
return {"id": self.rid, "severity": self.severity,
|
| 101 |
+
"languages": self.languages, "category": self.category,
|
| 102 |
+
"cwe": self.cwe, "patterns": self.patterns}
|
| 103 |
+
|
| 104 |
+
|
| 105 |
+
# `pattern-not` and friends are recognised and deliberately not read: they say
|
| 106 |
+
# what a rule EXCLUDES, and probing one would test the opposite of the rule.
|
| 107 |
+
_SKIPPED_KEYS = ("pattern-not-inside", "pattern-not", "pattern-regex",
|
| 108 |
+
"metavariable-regex", "metavariable-pattern",
|
| 109 |
+
"focus-metavariable")
|
| 110 |
+
_PATTERN_LINE = re.compile(r"^(\s*)(?:-\s+)?(?:pattern|pattern-inside):\s*(.*)$")
|
| 111 |
+
_KEYED_ITEM = re.compile(r"^\s*-\s*[\w-]+:")
|
| 112 |
+
|
| 113 |
+
|
| 114 |
+
def read_ruleset(path):
|
| 115 |
+
"""Read a semgrep rule file, for the subset of YAML these files use.
|
| 116 |
+
|
| 117 |
+
A real YAML parser is not in the standard library and this tool will not
|
| 118 |
+
grow a dependency for one file format. What is here reads the shape semgrep
|
| 119 |
+
rule files actually have -- a `rules:` list of mappings, scalar values, a
|
| 120 |
+
flow sequence for `languages`, and `|` block scalars for `message` and for
|
| 121 |
+
multi-line patterns -- and reports what it skipped rather than pretending
|
| 122 |
+
the file held only what it understood.
|
| 123 |
+
|
| 124 |
+
Returns (rules, skipped), where skipped is a list of (rule id, key) pairs
|
| 125 |
+
for constructs recognised and deliberately left alone.
|
| 126 |
+
"""
|
| 127 |
+
with io.open(path, encoding="utf-8-sig") as handle:
|
| 128 |
+
lines = handle.read().splitlines()
|
| 129 |
+
|
| 130 |
+
rules, skipped = [], []
|
| 131 |
+
current = None
|
| 132 |
+
i = 0
|
| 133 |
+
while i < len(lines):
|
| 134 |
+
line = lines[i]
|
| 135 |
+
stripped = line.strip()
|
| 136 |
+
if not stripped or stripped.startswith("#"):
|
| 137 |
+
i += 1
|
| 138 |
+
continue
|
| 139 |
+
|
| 140 |
+
found = re.match(r"^\s*-\s+id:\s*(\S+)", line)
|
| 141 |
+
if found:
|
| 142 |
+
current = ExternalRule(found.group(1).strip("\"'"))
|
| 143 |
+
rules.append(current)
|
| 144 |
+
i += 1
|
| 145 |
+
continue
|
| 146 |
+
if current is None:
|
| 147 |
+
i += 1
|
| 148 |
+
continue
|
| 149 |
+
|
| 150 |
+
skip = next((key for key in _SKIPPED_KEYS
|
| 151 |
+
if stripped.startswith(key + ":")), None)
|
| 152 |
+
if skip:
|
| 153 |
+
skipped.append((current.rid, skip))
|
| 154 |
+
if stripped.endswith("|"):
|
| 155 |
+
_, i = _block_scalar(lines, i + 1, _indent(line))
|
| 156 |
+
else:
|
| 157 |
+
i += 1
|
| 158 |
+
continue
|
| 159 |
+
|
| 160 |
+
found = _PATTERN_LINE.match(line)
|
| 161 |
+
if found:
|
| 162 |
+
value = found.group(2).strip()
|
| 163 |
+
if value == "|":
|
| 164 |
+
block, i = _block_scalar(lines, i + 1, len(found.group(1)))
|
| 165 |
+
if block:
|
| 166 |
+
current.patterns.append(block)
|
| 167 |
+
continue
|
| 168 |
+
if value:
|
| 169 |
+
current.patterns.append(value)
|
| 170 |
+
i += 1
|
| 171 |
+
continue
|
| 172 |
+
|
| 173 |
+
# A bare alternative under `pattern-either:`, which is how some rulesets
|
| 174 |
+
# write the second and later branches. Excluded when the item is itself
|
| 175 |
+
# a key, because those have been handled above or are none of our
|
| 176 |
+
# business.
|
| 177 |
+
found = re.match(r"^\s*-\s+(\S.*)$", line)
|
| 178 |
+
if found and current.patterns and not _KEYED_ITEM.match(line):
|
| 179 |
+
body = found.group(1).strip()
|
| 180 |
+
if body and not body.startswith("#"):
|
| 181 |
+
current.patterns.append(body)
|
| 182 |
+
i += 1
|
| 183 |
+
continue
|
| 184 |
+
|
| 185 |
+
found = re.match(r"^\s*severity:\s*(\S+)", line)
|
| 186 |
+
if found:
|
| 187 |
+
current.severity = found.group(1).strip("\"'")
|
| 188 |
+
i += 1
|
| 189 |
+
continue
|
| 190 |
+
found = re.match(r"^\s*languages:\s*\[(.*)\]", line)
|
| 191 |
+
if found:
|
| 192 |
+
current.languages = [part.strip().strip("\"'")
|
| 193 |
+
for part in found.group(1).split(",")
|
| 194 |
+
if part.strip()]
|
| 195 |
+
i += 1
|
| 196 |
+
continue
|
| 197 |
+
found = re.match(r"^\s*(cwe|category):\s*(.+)$", line)
|
| 198 |
+
if found:
|
| 199 |
+
setattr(current, found.group(1), found.group(2).strip().strip("\"'"))
|
| 200 |
+
i += 1
|
| 201 |
+
continue
|
| 202 |
+
found = re.match(r"^(\s*)message:\s*\|\s*$", line)
|
| 203 |
+
if found:
|
| 204 |
+
block, i = _block_scalar(lines, i + 1, len(found.group(1)))
|
| 205 |
+
current.message = block
|
| 206 |
+
continue
|
| 207 |
+
i += 1
|
| 208 |
+
return rules, skipped
|
| 209 |
+
|
| 210 |
+
|
| 211 |
+
def _indent(line):
|
| 212 |
+
return len(line) - len(line.lstrip())
|
| 213 |
+
|
| 214 |
+
|
| 215 |
+
def _block_scalar(lines, start, parent_indent):
|
| 216 |
+
"""The body of a `key: |` block, and the index of the line after it."""
|
| 217 |
+
body, i = [], start
|
| 218 |
+
while i < len(lines):
|
| 219 |
+
line = lines[i]
|
| 220 |
+
if line.strip() and _indent(line) <= parent_indent:
|
| 221 |
+
break
|
| 222 |
+
body.append(line)
|
| 223 |
+
i += 1
|
| 224 |
+
while body and not body[-1].strip():
|
| 225 |
+
body.pop()
|
| 226 |
+
if not body:
|
| 227 |
+
return "", i
|
| 228 |
+
margin = min(_indent(row) for row in body if row.strip())
|
| 229 |
+
return "\n".join(row[margin:] for row in body), i
|
| 230 |
+
|
| 231 |
+
|
| 232 |
+
# ------------------------------------------------------------------- probing
|
| 233 |
+
|
| 234 |
+
_META = re.compile(r"\$([A-Z][A-Z0-9_]*)")
|
| 235 |
+
_FSTRING = re.compile(r'f"([^"]*)"')
|
| 236 |
+
_BARE_STRING = re.compile(r'"\.\.\."')
|
| 237 |
+
|
| 238 |
+
|
| 239 |
+
def probe(pattern):
|
| 240 |
+
"""The smallest snippet a pattern would match, or None.
|
| 241 |
+
|
| 242 |
+
A semgrep pattern is code with holes in it: `$CR.execute($QUERY % ...)` is
|
| 243 |
+
Python with a receiver, an argument and a tail left open. Filling the holes
|
| 244 |
+
with names gives a snippet that is exactly what the rule is about and
|
| 245 |
+
nothing else, which is the fairest input our checkers can be handed -- no
|
| 246 |
+
surrounding code to fire on by accident, and no chance of crediting a rule
|
| 247 |
+
for something a neighbouring line triggered.
|
| 248 |
+
|
| 249 |
+
None means the pattern did not survive being filled in. Every one of those
|
| 250 |
+
is reported rather than quietly dropped: a pattern that cannot be probed is
|
| 251 |
+
a subject this tool has no answer about, which is a different thing from a
|
| 252 |
+
subject where the answer is no.
|
| 253 |
+
"""
|
| 254 |
+
text = pattern.strip()
|
| 255 |
+
if not text or text.startswith("#"):
|
| 256 |
+
return None
|
| 257 |
+
|
| 258 |
+
# Some rulesets quote the whole pattern. Those quotes are YAML's, not the
|
| 259 |
+
# pattern's, and leaving them on turns code into a string literal.
|
| 260 |
+
if len(text) > 1 and text[0] == text[-1] and text[0] in "\"'":
|
| 261 |
+
inner = text[1:-1]
|
| 262 |
+
if "..." in inner or "$" in inner or "(" in inner:
|
| 263 |
+
text = inner
|
| 264 |
+
|
| 265 |
+
text = _META.sub(lambda m: "_" + m.group(1).lower(), text)
|
| 266 |
+
|
| 267 |
+
# f-strings first: an ellipsis inside one is arbitrary TEXT, and the
|
| 268 |
+
# argument-list rules below would turn it into a name.
|
| 269 |
+
text = _FSTRING.sub(_fill_fstring, text)
|
| 270 |
+
text = _BARE_STRING.sub('"s"', text)
|
| 271 |
+
|
| 272 |
+
out = []
|
| 273 |
+
for row in text.split("\n"):
|
| 274 |
+
body = row.strip()
|
| 275 |
+
if body == "...":
|
| 276 |
+
out.append(row[:len(row) - len(body)] + "pass")
|
| 277 |
+
continue
|
| 278 |
+
row = re.sub(r"\bfor\s+\.\.\.\s+in\s+\.\.\.\s*:", "for _i in _seq:", row)
|
| 279 |
+
row = re.sub(r"\bwhile\s+\.\.\.\s*:", "while _c:", row)
|
| 280 |
+
out.append(_fill_ellipsis(row))
|
| 281 |
+
text = "\n".join(out)
|
| 282 |
+
|
| 283 |
+
if "..." in text:
|
| 284 |
+
return None
|
| 285 |
+
# ast.parse and not compile(): `return {...}` and `await x()` are whole
|
| 286 |
+
# patterns in real rulesets and are fragments rather than modules. The
|
| 287 |
+
# parser accepts both -- they fail later, at compile -- and a fragment is
|
| 288 |
+
# exactly what a pattern is, so the parser is the right gate here.
|
| 289 |
+
try:
|
| 290 |
+
ast.parse(text)
|
| 291 |
+
except (SyntaxError, ValueError):
|
| 292 |
+
return None
|
| 293 |
+
return text
|
| 294 |
+
|
| 295 |
+
|
| 296 |
+
def _fill_fstring(match):
|
| 297 |
+
"""`f"... FROM {$TABLE} ..."` is a template with text on either side of a
|
| 298 |
+
hole. The ellipses stand for arbitrary text, so any text will do, as long as
|
| 299 |
+
it is text a tokeniser will accept inside a string."""
|
| 300 |
+
body = match.group(1).replace("...", "a")
|
| 301 |
+
if "{" not in body:
|
| 302 |
+
body = "{_x}" + body
|
| 303 |
+
return 'f"' + body + '"'
|
| 304 |
+
|
| 305 |
+
|
| 306 |
+
def _fill_ellipsis(row):
|
| 307 |
+
"""Replace the argument-list and collection ellipses with names.
|
| 308 |
+
|
| 309 |
+
Position decides what a `...` can legally become, and three positions differ:
|
| 310 |
+
|
| 311 |
+
inside a dict display **name, because a bare name is not an entry
|
| 312 |
+
after a keyword argument **name, because `f(x, k=1, y)` is a syntax
|
| 313 |
+
error and `subprocess.run(..., shell=True,
|
| 314 |
+
...)` is a pattern several rulesets carry
|
| 315 |
+
anywhere else a plain name
|
| 316 |
+
|
| 317 |
+
Getting one wrong produces a snippet that does not parse, and the rule it
|
| 318 |
+
came from is then reported as unanswered when it could have been answered.
|
| 319 |
+
"""
|
| 320 |
+
out, brackets = [], []
|
| 321 |
+
i, hole = 0, 0
|
| 322 |
+
while i < len(row):
|
| 323 |
+
char = row[i]
|
| 324 |
+
if char in "([{":
|
| 325 |
+
brackets.append([char, False])
|
| 326 |
+
out.append(char)
|
| 327 |
+
elif char in ")]}":
|
| 328 |
+
if brackets:
|
| 329 |
+
brackets.pop()
|
| 330 |
+
out.append(char)
|
| 331 |
+
elif char == "=" and brackets and not _is_comparison(row, i):
|
| 332 |
+
brackets[-1][1] = True
|
| 333 |
+
out.append(char)
|
| 334 |
+
elif row.startswith("...", i):
|
| 335 |
+
hole += 1
|
| 336 |
+
kind, keyworded = brackets[-1] if brackets else ("", False)
|
| 337 |
+
if kind == "{" or (kind == "(" and keyworded):
|
| 338 |
+
out.append("**_d{0}".format(hole))
|
| 339 |
+
else:
|
| 340 |
+
out.append("_h{0}".format(hole))
|
| 341 |
+
i += 3
|
| 342 |
+
continue
|
| 343 |
+
else:
|
| 344 |
+
out.append(char)
|
| 345 |
+
i += 1
|
| 346 |
+
return "".join(out)
|
| 347 |
+
|
| 348 |
+
|
| 349 |
+
def _is_comparison(row, i):
|
| 350 |
+
"""`==`, `!=`, `<=`, `>=` are not keyword arguments."""
|
| 351 |
+
return row[i - 1:i] in ("=", "!", "<", ">") or row[i + 1:i + 2] == "="
|
| 352 |
+
|
| 353 |
+
|
| 354 |
+
# ------------------------------------------------------------------ matching
|
| 355 |
+
|
| 356 |
+
# Names that say nothing about what a pattern is about: structural Python every
|
| 357 |
+
# snippet has, and the filler this module puts in itself.
|
| 358 |
+
_STOP = frozenset(["self", "in", "is", "if", "or", "and", "not", "as",
|
| 359 |
+
"_i", "_seq", "_c", "_x"])
|
| 360 |
+
|
| 361 |
+
|
| 362 |
+
def literal_names(pattern):
|
| 363 |
+
"""Every identifier a pattern requires the source to contain, literally.
|
| 364 |
+
|
| 365 |
+
A semgrep pattern matches an AST, and the names in it that are NOT
|
| 366 |
+
metavariables have to be present in the code for a match to happen. Source
|
| 367 |
+
containing none of them cannot match the rule, whatever else is in it.
|
| 368 |
+
|
| 369 |
+
That is a necessary condition and not a sufficient one, which makes it
|
| 370 |
+
useful in exactly one direction: it can rule a match OUT and never in. The
|
| 371 |
+
one hole in it is semgrep's import equivalences -- `from subprocess import
|
| 372 |
+
call` lets `subprocess.call(...)` match a line where the word `subprocess`
|
| 373 |
+
never appears. Wherever this is used, the report says so.
|
| 374 |
+
"""
|
| 375 |
+
text = _META.sub(" ", pattern)
|
| 376 |
+
names = re.findall(r"[A-Za-z_][A-Za-z0-9_]*", text)
|
| 377 |
+
return set(name for name in names if len(name) > 1 and name not in _STOP)
|
| 378 |
+
|
| 379 |
+
|
| 380 |
+
def fires_on(snippet, rules_by_stack, stacks):
|
| 381 |
+
"""Which of our rules fire on one snippet, across the stacks that apply."""
|
| 382 |
+
hit = set()
|
| 383 |
+
for stack in stacks:
|
| 384 |
+
rules = rules_by_stack.get(stack)
|
| 385 |
+
if not rules:
|
| 386 |
+
continue
|
| 387 |
+
for found in layers.scan_source(snippet, rules, stack, errors=[]):
|
| 388 |
+
hit.add(found.rule)
|
| 389 |
+
return hit
|
| 390 |
+
|
| 391 |
+
|
| 392 |
+
def crosswalk(external, our_rules):
|
| 393 |
+
"""Pair their rules with ours by running their patterns past our checkers."""
|
| 394 |
+
by_stack = collections.defaultdict(list)
|
| 395 |
+
for rule in our_rules:
|
| 396 |
+
by_stack[rule["stack"]].append(rule)
|
| 397 |
+
|
| 398 |
+
rows = []
|
| 399 |
+
for ext in external:
|
| 400 |
+
stacks = set()
|
| 401 |
+
for lang in ext.languages or ["python"]:
|
| 402 |
+
stacks.update(LANG_STACKS.get(lang.lower(), ()))
|
| 403 |
+
probes, unprobed = [], []
|
| 404 |
+
for pattern in ext.patterns:
|
| 405 |
+
snippet = probe(pattern)
|
| 406 |
+
if snippet is None:
|
| 407 |
+
unprobed.append(pattern)
|
| 408 |
+
else:
|
| 409 |
+
probes.append(snippet)
|
| 410 |
+
ours = set()
|
| 411 |
+
for snippet in probes:
|
| 412 |
+
ours |= fires_on(snippet, by_stack, sorted(stacks))
|
| 413 |
+
rows.append({
|
| 414 |
+
"their_rule": ext.rid,
|
| 415 |
+
"severity": ext.severity,
|
| 416 |
+
"category": ext.category or ext.cwe or "(none)",
|
| 417 |
+
"languages": ext.languages,
|
| 418 |
+
"patterns": len(ext.patterns),
|
| 419 |
+
"probed": len(probes),
|
| 420 |
+
"unprobed": unprobed,
|
| 421 |
+
"our_rules": sorted(ours),
|
| 422 |
+
})
|
| 423 |
+
return rows
|
| 424 |
+
|
| 425 |
+
|
| 426 |
+
def covered_stacks(external):
|
| 427 |
+
"""The stacks their ruleset is even about.
|
| 428 |
+
|
| 429 |
+
A Python-only ruleset having nothing to say about a TypeScript rule of ours
|
| 430 |
+
is not a finding, it is the languages line at the bottom of every one of
|
| 431 |
+
their rules. Comparing across it would pad the gap with entries whose answer
|
| 432 |
+
was known before the tool ran.
|
| 433 |
+
"""
|
| 434 |
+
stacks = set()
|
| 435 |
+
for ext in external:
|
| 436 |
+
for lang in ext.languages or ["python"]:
|
| 437 |
+
stacks.update(LANG_STACKS.get(lang.lower(), ()))
|
| 438 |
+
return stacks
|
| 439 |
+
|
| 440 |
+
|
| 441 |
+
def unreachable_here(our_rules, external):
|
| 442 |
+
"""Our rules that no pattern of theirs could match.
|
| 443 |
+
|
| 444 |
+
See literal_names: a pattern cannot match source missing one of its literal
|
| 445 |
+
names, so a rule whose own wrong sample is missing a name from every pattern
|
| 446 |
+
they have is a subject their ruleset does not reach.
|
| 447 |
+
|
| 448 |
+
Restricted to the stacks their ruleset covers, so the answer is about what
|
| 449 |
+
they chose not to check rather than about which languages they parse.
|
| 450 |
+
"""
|
| 451 |
+
stacks = covered_stacks(external)
|
| 452 |
+
required = []
|
| 453 |
+
for ext in external:
|
| 454 |
+
for pattern in ext.patterns:
|
| 455 |
+
names = literal_names(pattern)
|
| 456 |
+
if names:
|
| 457 |
+
required.append(names)
|
| 458 |
+
out = []
|
| 459 |
+
for rule in our_rules:
|
| 460 |
+
if stacks and rule["stack"] not in stacks:
|
| 461 |
+
continue
|
| 462 |
+
sample = "{0}\n{1}".format(rule.get("wrong", ""), rule.get("rule", ""))
|
| 463 |
+
words = set(re.findall(r"[A-Za-z_][A-Za-z0-9_]*", sample))
|
| 464 |
+
if not any(names <= words for names in required):
|
| 465 |
+
out.append(rule["id"])
|
| 466 |
+
return out
|
| 467 |
+
|
| 468 |
+
|
| 469 |
+
# ---------------------------------------------------------------- field data
|
| 470 |
+
|
| 471 |
+
def suppressions(root):
|
| 472 |
+
"""Every `# nosemgrep: <id>` in a tree, by rule id, with file and line.
|
| 473 |
+
|
| 474 |
+
The only evidence available here about what the other analyser does on real
|
| 475 |
+
code, and better evidence than it looks: a suppression is a finding that
|
| 476 |
+
fired AND that a person read and decided to keep. It is partial in a known
|
| 477 |
+
direction -- it records what fired and was dismissed, and says nothing about
|
| 478 |
+
what fired and was fixed.
|
| 479 |
+
"""
|
| 480 |
+
found = collections.defaultdict(list)
|
| 481 |
+
for dirpath, dirnames, filenames in os.walk(root):
|
| 482 |
+
dirnames[:] = [name for name in dirnames if name not in SKIP_DIRS]
|
| 483 |
+
for name in sorted(filenames):
|
| 484 |
+
if not name.endswith(".py"):
|
| 485 |
+
continue
|
| 486 |
+
path = os.path.join(dirpath, name)
|
| 487 |
+
try:
|
| 488 |
+
with io.open(path, encoding="utf-8", errors="replace") as handle:
|
| 489 |
+
body = handle.read()
|
| 490 |
+
except OSError:
|
| 491 |
+
continue
|
| 492 |
+
if "nosemgrep" not in body:
|
| 493 |
+
continue
|
| 494 |
+
rel = os.path.relpath(path, root).replace(os.sep, "/")
|
| 495 |
+
for number, line in enumerate(body.splitlines(), 1):
|
| 496 |
+
match = SUPPRESSION_RE.search(line)
|
| 497 |
+
if not match:
|
| 498 |
+
continue
|
| 499 |
+
named = False
|
| 500 |
+
for part in match.group(1).split(","):
|
| 501 |
+
rid = _clean_rule_id(part)
|
| 502 |
+
if rid:
|
| 503 |
+
found[rid].append((rel, number))
|
| 504 |
+
named = True
|
| 505 |
+
if not named:
|
| 506 |
+
found["(unnamed)"].append((rel, number))
|
| 507 |
+
return found
|
| 508 |
+
|
| 509 |
+
|
| 510 |
+
def _clean_rule_id(text):
|
| 511 |
+
"""A rule id out of a suppression comment, or "" if it does not hold one.
|
| 512 |
+
|
| 513 |
+
Real markers carry prose after the id -- `# nosemgrep: odoo-sudo-without-
|
| 514 |
+
context - policy lookup only` -- and some carry a `semgrep.` prefix. Both
|
| 515 |
+
are stripped; anything that is not then a plain rule id is not counted as
|
| 516 |
+
one, because a marker that names nothing cannot be compared with anything.
|
| 517 |
+
"""
|
| 518 |
+
rid = text.strip().split()[0] if text.strip() else ""
|
| 519 |
+
rid = rid.split("#")[0].split("--")[0].strip().rstrip(".:")
|
| 520 |
+
if rid.startswith("semgrep."):
|
| 521 |
+
rid = rid[len("semgrep."):]
|
| 522 |
+
return rid if re.match(r"^[a-z][a-z0-9]*(?:-[a-z0-9]+)+$", rid) else ""
|
| 523 |
+
|
| 524 |
+
|
| 525 |
+
def echoed_on_the_same_line(root, marks, our_rules, stack, limit=400):
|
| 526 |
+
"""Of the lines their analyser fired on, how many do ours fire on too?
|
| 527 |
+
|
| 528 |
+
The strongest comparison this tool can make without running semgrep: the
|
| 529 |
+
same file, the same line number, two analysers. A line both flag is the one
|
| 530 |
+
case here where agreement is a fact about code rather than about rule text.
|
| 531 |
+
"""
|
| 532 |
+
by_file = collections.defaultdict(set)
|
| 533 |
+
for rid, places in marks.items():
|
| 534 |
+
if rid == "(unnamed)":
|
| 535 |
+
continue
|
| 536 |
+
for rel, line in places:
|
| 537 |
+
by_file[rel].add((line, rid))
|
| 538 |
+
|
| 539 |
+
rules = [rule for rule in our_rules if rule["stack"] == stack]
|
| 540 |
+
both, theirs_only = [], 0
|
| 541 |
+
unread = max(0, len(by_file) - limit)
|
| 542 |
+
for rel in sorted(by_file)[:limit]:
|
| 543 |
+
try:
|
| 544 |
+
with io.open(os.path.join(root, rel), encoding="utf-8",
|
| 545 |
+
errors="replace") as handle:
|
| 546 |
+
body = handle.read()
|
| 547 |
+
except OSError:
|
| 548 |
+
continue
|
| 549 |
+
ours = collections.defaultdict(set)
|
| 550 |
+
for found in layers.scan_source(body, rules, stack, errors=[]):
|
| 551 |
+
ours[found.line].add(found.rule)
|
| 552 |
+
for line, rid in sorted(by_file[rel]):
|
| 553 |
+
if ours.get(line):
|
| 554 |
+
both.append((rel, line, rid, sorted(ours[line])))
|
| 555 |
+
else:
|
| 556 |
+
theirs_only += 1
|
| 557 |
+
return both, theirs_only, unread
|
| 558 |
+
|
| 559 |
+
|
| 560 |
+
# ------------------------------------------------------------------- report
|
| 561 |
+
|
| 562 |
+
def report(rows, external, our_rules, skipped, marks, echo, examples=8):
|
| 563 |
+
agreed = [row for row in rows if row["our_rules"]]
|
| 564 |
+
alone = [row for row in rows if not row["our_rules"] and row["probed"]]
|
| 565 |
+
blind = [row for row in rows if not row["probed"]]
|
| 566 |
+
|
| 567 |
+
print("CROSSWALK {0} rules here against {1} of theirs".format(
|
| 568 |
+
len(our_rules), len(rows)))
|
| 569 |
+
print()
|
| 570 |
+
print(" Every other measurement in this repository tests whether what we")
|
| 571 |
+
print(" wrote behaves. None of them can test whether it is the right set of")
|
| 572 |
+
print(" things to have written, because a rule nobody thought of is invisible")
|
| 573 |
+
print(" to every check that reads the rules. Somebody else's ruleset for the")
|
| 574 |
+
print(" same stack is the one artefact that can say.")
|
| 575 |
+
print()
|
| 576 |
+
patterns = sum(row["patterns"] for row in rows)
|
| 577 |
+
probed = sum(row["probed"] for row in rows)
|
| 578 |
+
print(" Their patterns are turned into the smallest snippet each would match")
|
| 579 |
+
print(" and OUR checkers are run over it: {0} of {1} patterns produced one.".format(
|
| 580 |
+
probed, patterns))
|
| 581 |
+
print()
|
| 582 |
+
print(" {0:<46} {1:>5}".format("both corpora fire on the same construct",
|
| 583 |
+
len(agreed)))
|
| 584 |
+
print(" {0:<46} {1:>5}".format("theirs fires, nothing here does", len(alone)))
|
| 585 |
+
print(" {0:<46} {1:>5}".format("no probe could be built, so no answer",
|
| 586 |
+
len(blind)))
|
| 587 |
+
print()
|
| 588 |
+
|
| 589 |
+
if agreed:
|
| 590 |
+
print(" WHERE THE TWO AGREE ({0})".format(len(agreed)))
|
| 591 |
+
print(" Two rulesets written independently landed on the same construct.")
|
| 592 |
+
print(" This is the only external corroboration in the repository.")
|
| 593 |
+
print()
|
| 594 |
+
for row in sorted(agreed, key=lambda r: r["their_rule"]):
|
| 595 |
+
print(" {0}".format(row["their_rule"]))
|
| 596 |
+
for rid in row["our_rules"]:
|
| 597 |
+
print(" {0}".format(rid))
|
| 598 |
+
print()
|
| 599 |
+
|
| 600 |
+
if alone:
|
| 601 |
+
print(" THEIRS, AND NOTHING HERE FIRES ({0})".format(len(alone)))
|
| 602 |
+
print(" A candidate list, not a defect list. This corpus is about ORM")
|
| 603 |
+
print(" correctness and theirs is about security, so most of this gap is")
|
| 604 |
+
print(" a different subject rather than a missing rule. The entries worth")
|
| 605 |
+
print(" arguing over are the ones whose subject IS correctness.")
|
| 606 |
+
print()
|
| 607 |
+
by_category = collections.defaultdict(list)
|
| 608 |
+
for row in alone:
|
| 609 |
+
by_category[row["category"]].append(row)
|
| 610 |
+
for category in sorted(by_category):
|
| 611 |
+
print(" {0}".format(category))
|
| 612 |
+
for row in sorted(by_category[category], key=lambda r: r["their_rule"]):
|
| 613 |
+
print(" {0:<40} {1}".format(row["their_rule"], row["severity"]))
|
| 614 |
+
print()
|
| 615 |
+
|
| 616 |
+
if blind:
|
| 617 |
+
print(" NO PROBE, SO NO ANSWER ({0})".format(len(blind)))
|
| 618 |
+
print(" Their pattern did not survive being filled in. These are")
|
| 619 |
+
print(" unmeasured, which is a different thing from uncovered.")
|
| 620 |
+
for row in sorted(blind, key=lambda r: r["their_rule"])[:examples]:
|
| 621 |
+
print(" {0}".format(row["their_rule"]))
|
| 622 |
+
if len(blind) > examples:
|
| 623 |
+
print(" ... {0} more".format(len(blind) - examples))
|
| 624 |
+
print()
|
| 625 |
+
|
| 626 |
+
if skipped:
|
| 627 |
+
print(" KEYS READ AND NOT USED")
|
| 628 |
+
for key, count in collections.Counter(
|
| 629 |
+
key for _, key in skipped).most_common():
|
| 630 |
+
print(" {0:>4}x {1}".format(count, key))
|
| 631 |
+
print(" pattern-not says what a rule EXCLUDES, and probing one would")
|
| 632 |
+
print(" test the opposite of the rule.")
|
| 633 |
+
print()
|
| 634 |
+
|
| 635 |
+
outside = unreachable_here(our_rules, external)
|
| 636 |
+
if outside:
|
| 637 |
+
stacks = covered_stacks(external)
|
| 638 |
+
comparable = [rule for rule in our_rules if rule["stack"] in stacks]
|
| 639 |
+
print(" OURS, WITH NO PATTERN OF THEIRS THAT COULD MATCH ({0} of {1})".format(
|
| 640 |
+
len(outside), len(comparable)))
|
| 641 |
+
print(" Counted over the stack(s) their ruleset is about: {0}.".format(
|
| 642 |
+
", ".join(sorted(stacks))))
|
| 643 |
+
print(" A pattern cannot match source that is missing one of its literal")
|
| 644 |
+
print(" names. For these rules no pattern of theirs has all its names")
|
| 645 |
+
print(" present in our own wrong sample, so none of their rules is about")
|
| 646 |
+
print(" them. Strong, and not a proof: semgrep's import equivalences can")
|
| 647 |
+
print(" match a name the line does not spell out.")
|
| 648 |
+
print()
|
| 649 |
+
for rid in outside[:examples * 2]:
|
| 650 |
+
print(" {0}".format(rid))
|
| 651 |
+
if len(outside) > examples * 2:
|
| 652 |
+
print(" ... {0} more".format(len(outside) - examples * 2))
|
| 653 |
+
print()
|
| 654 |
+
|
| 655 |
+
if marks:
|
| 656 |
+
total = sum(len(places) for places in marks.values())
|
| 657 |
+
print(" WHAT THEIR ANALYSER ACTUALLY FIRED ON ({0} suppressed findings)".format(
|
| 658 |
+
total))
|
| 659 |
+
print(" Read out of `# nosemgrep:` markers in the checkout. Each one is a")
|
| 660 |
+
print(" finding that fired AND that a person read and kept the code over.")
|
| 661 |
+
print(" Partial in a known direction: it records what was dismissed and")
|
| 662 |
+
print(" says nothing about what was fixed.")
|
| 663 |
+
print()
|
| 664 |
+
known = set(row["their_rule"] for row in rows)
|
| 665 |
+
for rid, places in sorted(marks.items(),
|
| 666 |
+
key=lambda kv: (-len(kv[1]), kv[0])):
|
| 667 |
+
if rid == "(unnamed)":
|
| 668 |
+
tag = " (marker names no rule, so nothing to compare)"
|
| 669 |
+
elif rid in known:
|
| 670 |
+
tag = ""
|
| 671 |
+
else:
|
| 672 |
+
tag = " (not in this ruleset)"
|
| 673 |
+
print(" {0:>5}x {1}{2}".format(len(places), rid, tag))
|
| 674 |
+
print()
|
| 675 |
+
|
| 676 |
+
if echo is not None:
|
| 677 |
+
both, theirs_only, unread = echo
|
| 678 |
+
print(" THE SAME LINE, TWO ANALYSERS")
|
| 679 |
+
print(" The strongest comparison available without running theirs: same")
|
| 680 |
+
print(" file, same line, both corpora with something to say.")
|
| 681 |
+
print()
|
| 682 |
+
print(" {0:<46} {1:>5}".format("lines theirs fired on",
|
| 683 |
+
len(both) + theirs_only))
|
| 684 |
+
print(" {0:<46} {1:>5}".format("...where a checker here also fires",
|
| 685 |
+
len(both)))
|
| 686 |
+
if unread:
|
| 687 |
+
print(" {0:<46} {1:>5}".format("files past the read limit, unmeasured",
|
| 688 |
+
unread))
|
| 689 |
+
print()
|
| 690 |
+
for rel, line, rid, ours in both[:examples]:
|
| 691 |
+
print(" {0}:{1}".format(rel, line))
|
| 692 |
+
print(" theirs {0}".format(rid))
|
| 693 |
+
print(" ours {0}".format(", ".join(ours)))
|
| 694 |
+
if len(both) > examples:
|
| 695 |
+
print(" ... {0} more".format(len(both) - examples))
|
| 696 |
+
print()
|
| 697 |
+
|
| 698 |
+
print(" WHAT THIS DOES NOT MEASURE")
|
| 699 |
+
print(" Their analyser was never run. A probe is the construct their pattern")
|
| 700 |
+
print(" describes, not the code their analyser would flag in a real file, and")
|
| 701 |
+
print(" the two differ wherever a pattern leans on context a probe strips")
|
| 702 |
+
print(" away. Nothing here is a score either: a gap on a subject this corpus")
|
| 703 |
+
print(" deliberately does not cover reads exactly like one it forgot.")
|
| 704 |
+
return 0
|
| 705 |
+
|
| 706 |
+
|
| 707 |
+
def main(argv):
|
| 708 |
+
parser = argparse.ArgumentParser(prog="nexa crosswalk", add_help=True)
|
| 709 |
+
parser.add_argument("ruleset", nargs="?",
|
| 710 |
+
help="an external analyser's rule file (semgrep YAML)")
|
| 711 |
+
parser.add_argument("--checkout",
|
| 712 |
+
help="a source tree the other analyser runs on, read for "
|
| 713 |
+
"its suppression markers")
|
| 714 |
+
parser.add_argument("--stack", default="odoo",
|
| 715 |
+
help="the stack whose checkers run on the checkout")
|
| 716 |
+
parser.add_argument("--examples", type=int, default=8,
|
| 717 |
+
help="entries to print per list (default 8)")
|
| 718 |
+
parser.add_argument("--json", help="write the raw rows here as well")
|
| 719 |
+
args = parser.parse_args(argv)
|
| 720 |
+
|
| 721 |
+
if not args.ruleset:
|
| 722 |
+
print(__doc__)
|
| 723 |
+
return 2
|
| 724 |
+
if not os.path.isfile(args.ruleset):
|
| 725 |
+
print("No such rule file: {0}".format(args.ruleset))
|
| 726 |
+
return 2
|
| 727 |
+
|
| 728 |
+
external, skipped = read_ruleset(args.ruleset)
|
| 729 |
+
if not external:
|
| 730 |
+
print("No rules found in {0}.".format(args.ruleset))
|
| 731 |
+
print("This reads semgrep rule files: a `rules:` list of `- id:` mappings.")
|
| 732 |
+
return 2
|
| 733 |
+
|
| 734 |
+
our_rules = corpus.load_rules()
|
| 735 |
+
rows = crosswalk(external, our_rules)
|
| 736 |
+
|
| 737 |
+
marks, echo = {}, None
|
| 738 |
+
if args.checkout:
|
| 739 |
+
if not os.path.isdir(args.checkout):
|
| 740 |
+
print("No such checkout: {0}".format(args.checkout))
|
| 741 |
+
return 2
|
| 742 |
+
sys.stderr.write("\r reading {0} ...".format(
|
| 743 |
+
os.path.basename(os.path.abspath(args.checkout))))
|
| 744 |
+
sys.stderr.flush()
|
| 745 |
+
marks = suppressions(args.checkout)
|
| 746 |
+
if marks:
|
| 747 |
+
echo = echoed_on_the_same_line(args.checkout, marks, our_rules,
|
| 748 |
+
args.stack)
|
| 749 |
+
sys.stderr.write("\r" + " " * 60 + "\r")
|
| 750 |
+
|
| 751 |
+
code = report(rows, external, our_rules, skipped, marks, echo, args.examples)
|
| 752 |
+
if args.json:
|
| 753 |
+
payload = {"crosswalk_version": CROSSWALK_VERSION,
|
| 754 |
+
"ruleset": os.path.basename(args.ruleset),
|
| 755 |
+
"their_rules": [rule.as_dict() for rule in external],
|
| 756 |
+
"rows": rows,
|
| 757 |
+
"suppressions": dict(marks),
|
| 758 |
+
"same_line": echo[0] if echo else []}
|
| 759 |
+
with io.open(args.json, "w", encoding="utf-8") as handle:
|
| 760 |
+
handle.write(json.dumps(payload, indent=2, ensure_ascii=False))
|
| 761 |
+
print()
|
| 762 |
+
print(" raw rows written to {0}".format(args.json))
|
| 763 |
+
return code
|
| 764 |
+
|
| 765 |
+
|
| 766 |
+
if __name__ == "__main__":
|
| 767 |
+
sys.exit(main(sys.argv[1:]))
|
|
@@ -262,9 +262,9 @@ def check_compliance(text, rules):
|
|
| 262 |
# there is deliberately no nexa-specific marker on this list. A mute button for
|
| 263 |
# our own findings would be the first mechanism here capable of hiding one, and
|
| 264 |
# the whole design is an argument against having any.
|
| 265 |
-
SUPPRESSION_MARKERS = ("noqa", "nosemgrep", "
|
| 266 |
-
"type: ignore", "eslint-disable",
|
| 267 |
-
"@ts-expect-error")
|
| 268 |
|
| 269 |
|
| 270 |
class SourceHit(object):
|
|
|
|
| 262 |
# there is deliberately no nexa-specific marker on this list. A mute button for
|
| 263 |
# our own findings would be the first mechanism here capable of hiding one, and
|
| 264 |
# the whole design is an argument against having any.
|
| 265 |
+
SUPPRESSION_MARKERS = ("noqa", "nosemgrep", "nosec", "pylint: disable",
|
| 266 |
+
"pylint:disable", "type: ignore", "eslint-disable",
|
| 267 |
+
"@ts-ignore", "@ts-expect-error")
|
| 268 |
|
| 269 |
|
| 270 |
class SourceHit(object):
|
|
@@ -204,7 +204,7 @@ def measure(repo, stack, limit, rules):
|
|
| 204 |
commits = fix_commits(repo, stack, limit)
|
| 205 |
rows = []
|
| 206 |
for sha, subject in commits:
|
| 207 |
-
caught, vanished = set(), set()
|
| 208 |
touched = 0
|
| 209 |
for path in changed_files(repo, sha, stack):
|
| 210 |
before = git(repo, "show", "{0}^:{1}".format(sha, path))
|
|
@@ -215,6 +215,7 @@ def measure(repo, stack, limit, rules):
|
|
| 215 |
was = findings(before, rules, stack)
|
| 216 |
now = findings(after, rules, stack)
|
| 217 |
changed = touched_lines(repo, sha, path)
|
|
|
|
| 218 |
for key, lines in was.items():
|
| 219 |
if len(lines) <= len(now.get(key, [])):
|
| 220 |
continue # still there, as often as before
|
|
@@ -240,6 +241,13 @@ def measure(repo, stack, limit, rules):
|
|
| 240 |
# the gap between this and "caught" is the measure of how much a
|
| 241 |
# looser criterion would have flattered the result.
|
| 242 |
"vanished_elsewhere": sorted(vanished - caught),
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 243 |
})
|
| 244 |
return rows
|
| 245 |
|
|
@@ -299,9 +307,16 @@ def report(rows, stack, repos, misses_shown):
|
|
| 299 |
|
| 300 |
missed = [r for r in scoped if not r["caught"]]
|
| 301 |
if missed:
|
|
|
|
| 302 |
print(" MISSED, IN SCOPE ({0} of {1})".format(len(missed), len(scoped)))
|
| 303 |
-
print(" Each of these is a bug somebody fixed that the corpus
|
| 304 |
-
print("
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 305 |
print()
|
| 306 |
for row in missed[:misses_shown]:
|
| 307 |
print(" {0} {1}".format(row["sha"], row["subject"]))
|
|
@@ -359,11 +374,12 @@ def harvest(rows, stack, shown=6, words=12):
|
|
| 359 |
|
| 360 |
print("HARVEST {0}".format(stack))
|
| 361 |
print()
|
| 362 |
-
print(" {0} fix commit(s) the corpus
|
| 363 |
len(missed)))
|
| 364 |
-
print(" the construct each diff touched. Every one is an
|
| 365 |
-
print(" somebody found, fixed and pushed. A rule written from
|
| 366 |
-
print(" has a reason behind it that a rule written from an
|
|
|
|
| 367 |
print()
|
| 368 |
|
| 369 |
by_token = collections.defaultdict(list)
|
|
@@ -376,12 +392,20 @@ def harvest(rows, stack, shown=6, words=12):
|
|
| 376 |
caught_by_token[token] += 1
|
| 377 |
|
| 378 |
print(" CONSTRUCTS, BY HOW MANY MISSED FIXES TOUCHED THEM")
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 379 |
ranked = sorted(by_token.items(), key=lambda kv: (-len(kv[1]), kv[0]))
|
| 380 |
for token, hits in ranked:
|
| 381 |
got = caught_by_token[token]
|
| 382 |
-
|
| 383 |
-
|
| 384 |
-
|
| 385 |
print()
|
| 386 |
|
| 387 |
scopes = set()
|
|
@@ -409,10 +433,14 @@ def harvest(rows, stack, shown=6, words=12):
|
|
| 409 |
|
| 410 |
print(" THE LIST")
|
| 411 |
for token, hits in ranked:
|
|
|
|
| 412 |
print()
|
| 413 |
-
print(" {0} -- {1} missed
|
|
|
|
| 414 |
for row in hits[:shown]:
|
| 415 |
print(" {0} {1}".format(row["sha"], row["subject"][:88]))
|
|
|
|
|
|
|
| 416 |
if len(hits) > shown:
|
| 417 |
print(" ... {0} more".format(len(hits) - shown))
|
| 418 |
print()
|
|
@@ -422,7 +450,26 @@ def harvest(rows, stack, shown=6, words=12):
|
|
| 422 |
return 1
|
| 423 |
|
| 424 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 425 |
def main(argv):
|
|
|
|
| 426 |
parser = argparse.ArgumentParser(prog="nexa recall", add_help=True)
|
| 427 |
parser.add_argument("repos", nargs="*", help="git checkouts WITH history")
|
| 428 |
parser.add_argument("--stack", default="odoo",
|
|
@@ -462,11 +509,17 @@ def main(argv):
|
|
| 462 |
rows.extend(measure(repo, args.stack, args.limit, rules))
|
| 463 |
sys.stderr.write("\r" + " " * 60 + "\r")
|
| 464 |
|
| 465 |
-
|
| 466 |
-
|
|
|
|
|
|
|
| 467 |
if args.json:
|
| 468 |
with io.open(args.json, "w", encoding="utf-8") as fh:
|
| 469 |
fh.write(json.dumps(rows, indent=2, ensure_ascii=False))
|
|
|
|
|
|
|
|
|
|
|
|
|
| 470 |
print(" raw rows written to {0}".format(args.json))
|
| 471 |
return code
|
| 472 |
|
|
|
|
| 204 |
commits = fix_commits(repo, stack, limit)
|
| 205 |
rows = []
|
| 206 |
for sha, subject in commits:
|
| 207 |
+
caught, vanished, spoke = set(), set(), set()
|
| 208 |
touched = 0
|
| 209 |
for path in changed_files(repo, sha, stack):
|
| 210 |
before = git(repo, "show", "{0}^:{1}".format(sha, path))
|
|
|
|
| 215 |
was = findings(before, rules, stack)
|
| 216 |
now = findings(after, rules, stack)
|
| 217 |
changed = touched_lines(repo, sha, path)
|
| 218 |
+
spoke.update(key[0] for key in was)
|
| 219 |
for key, lines in was.items():
|
| 220 |
if len(lines) <= len(now.get(key, [])):
|
| 221 |
continue # still there, as often as before
|
|
|
|
| 241 |
# the gap between this and "caught" is the measure of how much a
|
| 242 |
# looser criterion would have flattered the result.
|
| 243 |
"vanished_elsewhere": sorted(vanished - caught),
|
| 244 |
+
# Rules with a finding in the file BEFORE the fix that is not a
|
| 245 |
+
# catch: still there afterwards, or gone from a line the fix never
|
| 246 |
+
# touched. On a missed commit this separates the corpus being blind
|
| 247 |
+
# to the file from the corpus looking straight at it and pointing
|
| 248 |
+
# somewhere else, and those want opposite repairs -- a rule that
|
| 249 |
+
# does not exist, against a rule that does and is aimed wrong.
|
| 250 |
+
"spoke": sorted(spoke - caught),
|
| 251 |
})
|
| 252 |
return rows
|
| 253 |
|
|
|
|
| 307 |
|
| 308 |
missed = [r for r in scoped if not r["caught"]]
|
| 309 |
if missed:
|
| 310 |
+
spoke = [r for r in missed if r.get("spoke")]
|
| 311 |
print(" MISSED, IN SCOPE ({0} of {1})".format(len(missed), len(scoped)))
|
| 312 |
+
print(" Each of these is a bug somebody fixed that the corpus did not")
|
| 313 |
+
print(" flag on the line of the fix. This is the candidate-rule list.")
|
| 314 |
+
print()
|
| 315 |
+
print(" {0} of them are not blind spots: a checker fired somewhere else".format(
|
| 316 |
+
len(spoke)))
|
| 317 |
+
print(" in one of the files and the fix went to a different line. The rule")
|
| 318 |
+
print(" exists and is aimed wrong, which is not the same repair as a rule")
|
| 319 |
+
print(" that does not exist. --harvest splits the list on that.")
|
| 320 |
print()
|
| 321 |
for row in missed[:misses_shown]:
|
| 322 |
print(" {0} {1}".format(row["sha"], row["subject"]))
|
|
|
|
| 374 |
|
| 375 |
print("HARVEST {0}".format(stack))
|
| 376 |
print()
|
| 377 |
+
print(" {0} fix commit(s) the corpus did not flag on the line of the fix,".format(
|
| 378 |
len(missed)))
|
| 379 |
+
print(" clustered by the construct each diff touched. Every one is an")
|
| 380 |
+
print(" incident: a bug somebody found, fixed and pushed. A rule written from")
|
| 381 |
+
print(" one of these has a reason behind it that a rule written from an")
|
| 382 |
+
print(" opinion does not.")
|
| 383 |
print()
|
| 384 |
|
| 385 |
by_token = collections.defaultdict(list)
|
|
|
|
| 392 |
caught_by_token[token] += 1
|
| 393 |
|
| 394 |
print(" CONSTRUCTS, BY HOW MANY MISSED FIXES TOUCHED THEM")
|
| 395 |
+
print()
|
| 396 |
+
print(" caught the corpus flagged the line the fix changed")
|
| 397 |
+
print(" spoke a checker fired somewhere in one of these files and the")
|
| 398 |
+
print(" fix went elsewhere. Not a catch, and not blindness either:")
|
| 399 |
+
print(" the rule exists and is aimed wrong, which is a different")
|
| 400 |
+
print(" repair from the rule that does not exist. A flat miss")
|
| 401 |
+
print(" count hides which of the two you are looking at.")
|
| 402 |
+
print()
|
| 403 |
ranked = sorted(by_token.items(), key=lambda kv: (-len(kv[1]), kv[0]))
|
| 404 |
for token, hits in ranked:
|
| 405 |
got = caught_by_token[token]
|
| 406 |
+
said = sum(1 for row in hits if row.get("spoke"))
|
| 407 |
+
print(" {0:>4}x {1:<20} {2:>3} caught {3:>3} spoke {4:>3} silent".format(
|
| 408 |
+
len(hits), token, got, said, len(hits) - said))
|
| 409 |
print()
|
| 410 |
|
| 411 |
scopes = set()
|
|
|
|
| 433 |
|
| 434 |
print(" THE LIST")
|
| 435 |
for token, hits in ranked:
|
| 436 |
+
said = sum(1 for row in hits if row.get("spoke"))
|
| 437 |
print()
|
| 438 |
+
print(" {0} -- {1} missed, {2} of them with a checker firing "
|
| 439 |
+
"elsewhere in the file".format(token, len(hits), said))
|
| 440 |
for row in hits[:shown]:
|
| 441 |
print(" {0} {1}".format(row["sha"], row["subject"][:88]))
|
| 442 |
+
for rid in row.get("spoke", []):
|
| 443 |
+
print(" spoke: {0}".format(rid))
|
| 444 |
if len(hits) > shown:
|
| 445 |
print(" ... {0} more".format(len(hits) - shown))
|
| 446 |
print()
|
|
|
|
| 450 |
return 1
|
| 451 |
|
| 452 |
|
| 453 |
+
def _survive_the_console():
|
| 454 |
+
"""A commit subject is somebody else's text and may hold any character.
|
| 455 |
+
|
| 456 |
+
Printing one to a Windows console on a legacy code page raises
|
| 457 |
+
UnicodeEncodeError part-way through the report, which loses the rest of the
|
| 458 |
+
report AND the --json artifact written after it -- measured, on a subject
|
| 459 |
+
containing an arrow. `nexa recall` never meets this because the parent sets
|
| 460 |
+
PYTHONIOENCODING; `python recall.py`, which the docstring above tells people
|
| 461 |
+
to run, does.
|
| 462 |
+
"""
|
| 463 |
+
for attempt in ({"encoding": "utf-8"}, {"errors": "replace"}):
|
| 464 |
+
try:
|
| 465 |
+
sys.stdout.reconfigure(**attempt)
|
| 466 |
+
return
|
| 467 |
+
except (AttributeError, OSError, ValueError):
|
| 468 |
+
continue
|
| 469 |
+
|
| 470 |
+
|
| 471 |
def main(argv):
|
| 472 |
+
_survive_the_console()
|
| 473 |
parser = argparse.ArgumentParser(prog="nexa recall", add_help=True)
|
| 474 |
parser.add_argument("repos", nargs="*", help="git checkouts WITH history")
|
| 475 |
parser.add_argument("--stack", default="odoo",
|
|
|
|
| 509 |
rows.extend(measure(repo, args.stack, args.limit, rules))
|
| 510 |
sys.stderr.write("\r" + " " * 60 + "\r")
|
| 511 |
|
| 512 |
+
# Before the report, not after it. The measurement is the expensive half --
|
| 513 |
+
# minutes of git and checker runs -- and printing it is the fragile half.
|
| 514 |
+
# Written first, a report that cannot reach the screen still leaves the
|
| 515 |
+
# thing the numbers are re-derivable from.
|
| 516 |
if args.json:
|
| 517 |
with io.open(args.json, "w", encoding="utf-8") as fh:
|
| 518 |
fh.write(json.dumps(rows, indent=2, ensure_ascii=False))
|
| 519 |
+
|
| 520 |
+
code = (harvest(rows, args.stack, args.examples) if args.harvest
|
| 521 |
+
else report(rows, args.stack, args.repos, args.misses))
|
| 522 |
+
if args.json:
|
| 523 |
print(" raw rows written to {0}".format(args.json))
|
| 524 |
return code
|
| 525 |
|
|
@@ -811,9 +811,380 @@ def _comodel_of(call):
|
|
| 811 |
return None
|
| 812 |
|
| 813 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 814 |
CHECKS = {
|
| 815 |
"check_company_missing_on_company_owned_relation":
|
| 816 |
check_company_missing_on_company_owned_relation,
|
|
|
|
|
|
|
|
|
|
| 817 |
"constrains_lists_every_field_read": constrains_lists_every_field_read,
|
| 818 |
"depends_covers_compute_reads": depends_covers_compute_reads,
|
| 819 |
"ondelete_leaves_a_dangling_read": ondelete_leaves_a_dangling_read,
|
|
|
|
| 811 |
return None
|
| 812 |
|
| 813 |
|
| 814 |
+
# ------------------------------------------------------- the cursor checkers
|
| 815 |
+
|
| 816 |
+
# The names an Odoo database cursor is reached by. self.env.cr and self._cr are
|
| 817 |
+
# the two the framework provides; `cr` alone is the argument name every
|
| 818 |
+
# migration script and post-init hook uses. Anything else called .execute() or
|
| 819 |
+
# .commit() is somebody else's object and none of our business.
|
| 820 |
+
CURSOR_NAMES = frozenset(["cr", "_cr", "cursor", "_cursor"])
|
| 821 |
+
|
| 822 |
+
|
| 823 |
+
def _cursor_call(node, method):
|
| 824 |
+
"""True when this Call is <something>.cr.<method>(...).
|
| 825 |
+
|
| 826 |
+
The attribute immediately before the method name is what decides it, so
|
| 827 |
+
self.env.cr.execute, self._cr.execute and a bare cr.execute all match while
|
| 828 |
+
connection.execute and session.commit do not.
|
| 829 |
+
"""
|
| 830 |
+
if not isinstance(node, ast.Call):
|
| 831 |
+
return False
|
| 832 |
+
func = node.func
|
| 833 |
+
if not isinstance(func, ast.Attribute) or func.attr != method:
|
| 834 |
+
return False
|
| 835 |
+
owner = func.value
|
| 836 |
+
if isinstance(owner, ast.Attribute):
|
| 837 |
+
return owner.attr in CURSOR_NAMES
|
| 838 |
+
if isinstance(owner, ast.Name):
|
| 839 |
+
return owner.id in CURSOR_NAMES
|
| 840 |
+
return False
|
| 841 |
+
|
| 842 |
+
|
| 843 |
+
def commit_inside_a_loop(code):
|
| 844 |
+
"""odoo.commit-inside-a-loop-leaves-half-written-data
|
| 845 |
+
|
| 846 |
+
A commit ends the transaction the ORM is working inside. Done once per
|
| 847 |
+
iteration, the loop stops being one unit of work: iteration seven raising
|
| 848 |
+
leaves iterations one to six already written, the caller's except clause
|
| 849 |
+
has nothing left to roll back, and the user sees an error over data that
|
| 850 |
+
stayed.
|
| 851 |
+
|
| 852 |
+
Only loops, not every commit. A commit in a cron batch or a migration
|
| 853 |
+
script is a deliberate decision about batch size, and a rule that fired on
|
| 854 |
+
all of them would be argued with rather than read. Inside a loop over a
|
| 855 |
+
recordset it is almost never deliberate -- it is somebody flushing as they
|
| 856 |
+
go -- and that is the shape this reports.
|
| 857 |
+
|
| 858 |
+
The receiver decides, not the method name: `.commit()` on anything that is
|
| 859 |
+
not a cursor belongs to some other library.
|
| 860 |
+
"""
|
| 861 |
+
tree, _ = parse_python(code)
|
| 862 |
+
if tree is None:
|
| 863 |
+
return []
|
| 864 |
+
findings = []
|
| 865 |
+
for loop in ast.walk(tree):
|
| 866 |
+
if not isinstance(loop, (ast.For, ast.AsyncFor, ast.While)):
|
| 867 |
+
continue
|
| 868 |
+
# The loop's own body, not its else clause: an else runs once, after
|
| 869 |
+
# the loop, and a commit there is a commit after a loop.
|
| 870 |
+
for statement in loop.body:
|
| 871 |
+
for node in ast.walk(statement):
|
| 872 |
+
if _cursor_call(node, "commit"):
|
| 873 |
+
findings.append((
|
| 874 |
+
node.lineno,
|
| 875 |
+
"cr.commit() inside a loop -- an exception on a later "
|
| 876 |
+
"iteration leaves the earlier ones written, and the "
|
| 877 |
+
"caller cannot roll the batch back"))
|
| 878 |
+
return sorted(set(findings))
|
| 879 |
+
|
| 880 |
+
|
| 881 |
+
def sql_built_by_string_formatting(code):
|
| 882 |
+
"""odoo.sql-needs-query-parameters
|
| 883 |
+
|
| 884 |
+
cr.execute("... id = %s", (rec.id,)) is correct and contains a percent
|
| 885 |
+
sign. cr.execute("... id = %s" % rec.id) is the bug and contains the same
|
| 886 |
+
percent sign. A regex cannot tell them apart, which is why this is an AST
|
| 887 |
+
pass: the difference is whether the substitution happens in Python before
|
| 888 |
+
the cursor sees the string, and that is a BinOp rather than a character.
|
| 889 |
+
|
| 890 |
+
Four spellings, all of them the same mistake:
|
| 891 |
+
|
| 892 |
+
"..." % value BinOp with Mod
|
| 893 |
+
"...".format(value) a call to .format on a string
|
| 894 |
+
f"...{value}..." JoinedStr
|
| 895 |
+
"..." + value BinOp with Add, one side a string
|
| 896 |
+
|
| 897 |
+
Only the FIRST argument is examined. The second is the parameter tuple and
|
| 898 |
+
building that with formatting is somebody's business, not a SQL injection.
|
| 899 |
+
"""
|
| 900 |
+
tree, _ = parse_python(code)
|
| 901 |
+
if tree is None:
|
| 902 |
+
return []
|
| 903 |
+
findings = []
|
| 904 |
+
for node in ast.walk(tree):
|
| 905 |
+
if not _cursor_call(node, "execute") or not node.args:
|
| 906 |
+
continue
|
| 907 |
+
how = _how_the_query_was_built(node.args[0])
|
| 908 |
+
if how:
|
| 909 |
+
findings.append((
|
| 910 |
+
node.args[0].lineno,
|
| 911 |
+
"the query handed to cr.execute() is built by {0} -- pass the "
|
| 912 |
+
"values as the second argument so the driver quotes them".format(how)))
|
| 913 |
+
return sorted(set(findings))
|
| 914 |
+
|
| 915 |
+
|
| 916 |
+
def _how_the_query_was_built(node):
|
| 917 |
+
"""The name of the string operation that produced this query, or ""."""
|
| 918 |
+
if isinstance(node, ast.JoinedStr):
|
| 919 |
+
return "an f-string"
|
| 920 |
+
if isinstance(node, ast.BinOp):
|
| 921 |
+
if isinstance(node.op, ast.Mod):
|
| 922 |
+
return "percent formatting"
|
| 923 |
+
if isinstance(node.op, ast.Add) and (_is_a_string(node.left)
|
| 924 |
+
or _is_a_string(node.right)):
|
| 925 |
+
return "string concatenation"
|
| 926 |
+
if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) \
|
| 927 |
+
and node.func.attr == "format" and not _is_composed_sql(node.func.value):
|
| 928 |
+
return "str.format()"
|
| 929 |
+
return ""
|
| 930 |
+
|
| 931 |
+
|
| 932 |
+
def _is_composed_sql(node):
|
| 933 |
+
"""psycopg2's sql.SQL("... {}").format(sql.Identifier(name)) is the CORRECT
|
| 934 |
+
way to parameterise a table or column name, and it spells .format().
|
| 935 |
+
|
| 936 |
+
Found by running this over 229,600 lines of OpenSPP: the one finding that
|
| 937 |
+
was wrong rather than merely suppressed was this construct, on a DROP INDEX
|
| 938 |
+
whose identifier the surrounding code had already validated. A checker that
|
| 939 |
+
tells an author to stop using the safe API is worse than one that says
|
| 940 |
+
nothing.
|
| 941 |
+
"""
|
| 942 |
+
if not isinstance(node, ast.Call):
|
| 943 |
+
return False
|
| 944 |
+
func = node.func
|
| 945 |
+
if isinstance(func, ast.Attribute):
|
| 946 |
+
return func.attr in ("SQL", "Identifier", "Composed", "Literal")
|
| 947 |
+
return isinstance(func, ast.Name) and func.id in ("SQL", "Identifier")
|
| 948 |
+
|
| 949 |
+
|
| 950 |
+
def _is_a_string(node):
|
| 951 |
+
return ((isinstance(node, ast.Constant) and isinstance(node.value, str))
|
| 952 |
+
or isinstance(node, ast.JoinedStr))
|
| 953 |
+
|
| 954 |
+
|
| 955 |
+
def depends_names_unread_fields(code):
|
| 956 |
+
"""odoo.depends-must-not-name-unread-fields
|
| 957 |
+
|
| 958 |
+
The other direction of depends_covers_compute_reads, and a milder problem:
|
| 959 |
+
a declared dependency the body never reads does not make the value stale,
|
| 960 |
+
it makes the value recompute on writes that cannot change it.
|
| 961 |
+
|
| 962 |
+
Harvested from OpenSPP's history rather than reasoned about. Two fix
|
| 963 |
+
commits, both of them a leftover:
|
| 964 |
+
|
| 965 |
+
@api.depends("partner_id", "pathway_id", "assessment_date")
|
| 966 |
+
def _compute_name(self): # reads partner_id and pathway_id only
|
| 967 |
+
|
| 968 |
+
@api.depends("topic_ids")
|
| 969 |
+
def _compute_session_count(self): # counts rows on another model
|
| 970 |
+
|
| 971 |
+
The second is why this is worth reporting at all. The decorator is the only
|
| 972 |
+
statement anywhere of what a stored value depends on, and a reader trusts
|
| 973 |
+
it. Left naming a field an older body used to read, it hides that this
|
| 974 |
+
compute has no expressible dependency at all -- it queries a different
|
| 975 |
+
model -- and the staleness that follows looks accounted for.
|
| 976 |
+
|
| 977 |
+
Deliberately permissive, because a wrong entry costs a recompute and a
|
| 978 |
+
false finding costs trust:
|
| 979 |
+
|
| 980 |
+
A string literal anywhere in the body counts as a read. filtered("x"),
|
| 981 |
+
mapped("a.b") and sorted(key=...) name fields without an attribute access
|
| 982 |
+
ever appearing.
|
| 983 |
+
|
| 984 |
+
A declared path is covered by any read that shares a prefix with it in
|
| 985 |
+
either direction, so declaring line_ids while reading line_ids.price is
|
| 986 |
+
fine and so is the reverse.
|
| 987 |
+
|
| 988 |
+
A body that calls a method on self, or hands a record to any call, is
|
| 989 |
+
left alone entirely. The read is then inside the callee, this pass cannot
|
| 990 |
+
follow it, and the honest answer is not to guess. Measured on OpenSPP,
|
| 991 |
+
this is the largest false-positive class: a compute that declares five
|
| 992 |
+
fields and passes the record to a resolver that reads all five.
|
| 993 |
+
|
| 994 |
+
A body that reads a field the decorator does not declare is left alone
|
| 995 |
+
too. The two are already out of step, and that disagreement belongs to
|
| 996 |
+
depends_covers_compute_reads -- reporting it twice from opposite
|
| 997 |
+
directions is two findings about one confusion. It is also how a compute
|
| 998 |
+
reading another stored compute looks from here, and those declare the
|
| 999 |
+
upstream source on purpose.
|
| 1000 |
+
"""
|
| 1001 |
+
tree, _ = parse_python(code)
|
| 1002 |
+
if tree is None:
|
| 1003 |
+
return []
|
| 1004 |
+
findings = []
|
| 1005 |
+
for fn in _functions(tree):
|
| 1006 |
+
declared = _decorator_args(fn, "api", "depends")
|
| 1007 |
+
if not declared:
|
| 1008 |
+
continue
|
| 1009 |
+
if _calls_a_method_on_self(fn) or _hands_a_record_to_a_call(fn) \
|
| 1010 |
+
or _queries_another_model(fn):
|
| 1011 |
+
continue
|
| 1012 |
+
aliases = _aliases(fn)
|
| 1013 |
+
read = set(".".join(path) for path, _ in _field_reads(fn, aliases))
|
| 1014 |
+
produced = set(_assigned_attrs(fn))
|
| 1015 |
+
if _reads_outside_the_declarations(read - produced, declared):
|
| 1016 |
+
continue
|
| 1017 |
+
read |= produced
|
| 1018 |
+
read |= _string_literals(fn)
|
| 1019 |
+
unread = [entry for entry in declared
|
| 1020 |
+
if not _covered_by_a_read(entry, read)]
|
| 1021 |
+
if unread:
|
| 1022 |
+
# One finding per method, not per entry. Three leftovers on one
|
| 1023 |
+
# decorator is one thing to fix, and three lines all pointing at
|
| 1024 |
+
# the same def read as three problems.
|
| 1025 |
+
findings.append((
|
| 1026 |
+
fn.lineno,
|
| 1027 |
+
"{0}() declares {1} in @api.depends and never reads {2} -- the "
|
| 1028 |
+
"value recomputes on writes that cannot change it, and the "
|
| 1029 |
+
"decorator stops describing the method".format(
|
| 1030 |
+
fn.name, ", ".join(repr(str(e)) for e in unread),
|
| 1031 |
+
"them" if len(unread) > 1 else "it")))
|
| 1032 |
+
return sorted(set(findings))
|
| 1033 |
+
|
| 1034 |
+
|
| 1035 |
+
def _reads_outside_the_declarations(read, declared):
|
| 1036 |
+
"""Does the body read a field no declared entry covers?
|
| 1037 |
+
|
| 1038 |
+
Prefix-wise in both directions, so reading partner_id.name against a
|
| 1039 |
+
declared partner_id is inside the declarations and reading
|
| 1040 |
+
intervention_count against a declared intervention_ids is outside them.
|
| 1041 |
+
"""
|
| 1042 |
+
for path in read:
|
| 1043 |
+
if not any(path == entry or path.startswith(entry + ".")
|
| 1044 |
+
or entry.startswith(path + ".") for entry in declared):
|
| 1045 |
+
return True
|
| 1046 |
+
return False
|
| 1047 |
+
|
| 1048 |
+
|
| 1049 |
+
def _hands_a_record_to_a_call(fn):
|
| 1050 |
+
"""Does any call in the body receive a record as an argument?
|
| 1051 |
+
|
| 1052 |
+
resolve(scope) can read every field on scope, so a dependency declared for
|
| 1053 |
+
its benefit is correct and invisible here. A domain built from rec.id is
|
| 1054 |
+
not this: an id is an integer, and no callee reads a field through one.
|
| 1055 |
+
"""
|
| 1056 |
+
aliases = _aliases(fn)
|
| 1057 |
+
for node in _walk_body(fn):
|
| 1058 |
+
if not isinstance(node, ast.Call):
|
| 1059 |
+
continue
|
| 1060 |
+
for argument in list(node.args) + [kw.value for kw in node.keywords]:
|
| 1061 |
+
if _is_record_valued(argument, aliases):
|
| 1062 |
+
return True
|
| 1063 |
+
return False
|
| 1064 |
+
|
| 1065 |
+
|
| 1066 |
+
QUERY_METHODS = frozenset(["search", "search_count", "search_read", "read_group",
|
| 1067 |
+
"_read_group", "execute", "_search"])
|
| 1068 |
+
|
| 1069 |
+
|
| 1070 |
+
def _queries_another_model(fn):
|
| 1071 |
+
"""Does the body count or fetch rows from somewhere other than self?
|
| 1072 |
+
|
| 1073 |
+
The largest class of false finding here by a wide margin: 17 of the 31 this
|
| 1074 |
+
reported on OpenSPP. A compute whose value comes from rows in ANOTHER model
|
| 1075 |
+
has no dependency @api.depends can express -- the ORM cannot watch a table
|
| 1076 |
+
this record has no field for -- so whatever the author declared is their
|
| 1077 |
+
best available approximation, usually the inverse One2many, and it often
|
| 1078 |
+
works. Telling them to delete it removes the only recompute trigger the
|
| 1079 |
+
value has.
|
| 1080 |
+
|
| 1081 |
+
The cost of the exclusion is real and stated rather than hidden: one of the
|
| 1082 |
+
two harvested incidents this checker was written from is exactly this shape
|
| 1083 |
+
-- @api.depends("topic_ids") on a compute that counted rows on another
|
| 1084 |
+
model -- and is no longer reported. Reading it, the author's fix was to
|
| 1085 |
+
decide there was no correct dependency at all, which is a judgement this
|
| 1086 |
+
pass has no way to make for them.
|
| 1087 |
+
"""
|
| 1088 |
+
for node in _walk_body(fn):
|
| 1089 |
+
if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) \
|
| 1090 |
+
and node.func.attr in QUERY_METHODS:
|
| 1091 |
+
return True
|
| 1092 |
+
return False
|
| 1093 |
+
|
| 1094 |
+
|
| 1095 |
+
def _is_record_valued(node, aliases):
|
| 1096 |
+
"""Is this argument a record, or a field of one, rather than a plain value?
|
| 1097 |
+
|
| 1098 |
+
Collection literals are looked inside, because a domain is a list of tuples
|
| 1099 |
+
and a vals dict is a dict of them. Attribute chains are NOT walked into:
|
| 1100 |
+
the Name at the base of rec.id is not itself an argument, and counting it
|
| 1101 |
+
made a search domain look like a record being handed over.
|
| 1102 |
+
"""
|
| 1103 |
+
if isinstance(node, ast.Name):
|
| 1104 |
+
return node.id in aliases
|
| 1105 |
+
if isinstance(node, ast.Attribute):
|
| 1106 |
+
path = _attr_path(node, aliases)
|
| 1107 |
+
return bool(path) and path[0] not in ORM_ATTRS \
|
| 1108 |
+
and not path[0].startswith("_")
|
| 1109 |
+
if isinstance(node, (ast.List, ast.Tuple, ast.Set)):
|
| 1110 |
+
return any(_is_record_valued(item, aliases) for item in node.elts)
|
| 1111 |
+
if isinstance(node, ast.Dict):
|
| 1112 |
+
return any(_is_record_valued(item, aliases)
|
| 1113 |
+
for item in list(node.keys) + list(node.values) if item)
|
| 1114 |
+
return False
|
| 1115 |
+
|
| 1116 |
+
|
| 1117 |
+
def _covered_by_a_read(entry, read):
|
| 1118 |
+
"""A declared path is covered by any read sharing a prefix with it."""
|
| 1119 |
+
for path in read:
|
| 1120 |
+
if path == entry or path.startswith(entry + ".") \
|
| 1121 |
+
or entry.startswith(path + "."):
|
| 1122 |
+
return True
|
| 1123 |
+
return False
|
| 1124 |
+
|
| 1125 |
+
|
| 1126 |
+
def _walk_body(fn):
|
| 1127 |
+
"""Every node in the function's BODY, decorators excluded.
|
| 1128 |
+
|
| 1129 |
+
ast.walk(fn) includes the decorator list, and @api.depends("x") holds the
|
| 1130 |
+
string "x". Counted as evidence the body mentions x, every declared entry
|
| 1131 |
+
proves itself and the checker silently finds nothing -- which is exactly
|
| 1132 |
+
what it did on the incident it was written from.
|
| 1133 |
+
"""
|
| 1134 |
+
for statement in fn.body:
|
| 1135 |
+
for node in ast.walk(statement):
|
| 1136 |
+
yield node
|
| 1137 |
+
|
| 1138 |
+
|
| 1139 |
+
def _string_literals(fn):
|
| 1140 |
+
"""Every string in the body, plus each dotted path inside one.
|
| 1141 |
+
|
| 1142 |
+
filtered("is_late") and mapped("line_ids.price") name fields without an
|
| 1143 |
+
attribute access ever appearing, and a declared dependency on either is
|
| 1144 |
+
correct. Splitting the dotted ones lets a declared prefix match too.
|
| 1145 |
+
"""
|
| 1146 |
+
out = set()
|
| 1147 |
+
for node in _walk_body(fn):
|
| 1148 |
+
if isinstance(node, ast.Constant) and isinstance(node.value, str):
|
| 1149 |
+
text = node.value.strip()
|
| 1150 |
+
if text:
|
| 1151 |
+
out.add(text)
|
| 1152 |
+
out.update(re.findall(r"[A-Za-z_][A-Za-z0-9_.]*", text))
|
| 1153 |
+
return out
|
| 1154 |
+
|
| 1155 |
+
|
| 1156 |
+
def _calls_a_method_on_self(fn):
|
| 1157 |
+
"""Does this body call a method ON the record -- rec._helper(), self._go()?
|
| 1158 |
+
|
| 1159 |
+
The read is then inside the helper, where this pass cannot follow it, and a
|
| 1160 |
+
dependency declared for the helper's benefit is correct. Reporting it would
|
| 1161 |
+
be telling the author to delete a line that is doing its job.
|
| 1162 |
+
|
| 1163 |
+
Length one, deliberately. rec._helper() resolves to the path ('_helper',)
|
| 1164 |
+
and can read anything; rec.line_ids.filtered(...) resolves to ('line_ids',
|
| 1165 |
+
'filtered') and is an ORM call on one field, which reaches no other. Taking
|
| 1166 |
+
both lost the incident this checker was written from, where the body's only
|
| 1167 |
+
call was .replace() on a string.
|
| 1168 |
+
"""
|
| 1169 |
+
aliases = _aliases(fn)
|
| 1170 |
+
for node in _walk_body(fn):
|
| 1171 |
+
if not isinstance(node, ast.Call):
|
| 1172 |
+
continue
|
| 1173 |
+
func = node.func
|
| 1174 |
+
if not isinstance(func, ast.Attribute):
|
| 1175 |
+
continue
|
| 1176 |
+
path = _attr_path(func, aliases)
|
| 1177 |
+
if path is not None and len(path) == 1:
|
| 1178 |
+
return True
|
| 1179 |
+
return False
|
| 1180 |
+
|
| 1181 |
+
|
| 1182 |
CHECKS = {
|
| 1183 |
"check_company_missing_on_company_owned_relation":
|
| 1184 |
check_company_missing_on_company_owned_relation,
|
| 1185 |
+
"commit_inside_a_loop": commit_inside_a_loop,
|
| 1186 |
+
"depends_names_unread_fields": depends_names_unread_fields,
|
| 1187 |
+
"sql_built_by_string_formatting": sql_built_by_string_formatting,
|
| 1188 |
"constrains_lists_every_field_read": constrains_lists_every_field_read,
|
| 1189 |
"depends_covers_compute_reads": depends_covers_compute_reads,
|
| 1190 |
"ondelete_leaves_a_dangling_read": ondelete_leaves_a_dangling_read,
|
|
@@ -354,6 +354,64 @@ SaleOrder._compute_amount = my_replacement""",
|
|
| 354 |
</odoo>""",
|
| 355 |
"tags": ["xml", "data", "upgrade"],
|
| 356 |
},
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 357 |
]
|
| 358 |
|
| 359 |
TYPESCRIPT = [
|
|
@@ -798,6 +856,10 @@ ODOO_SECURITY = "https://www.odoo.com/documentation/18.0/developer/reference/bac
|
|
| 798 |
ODOO_EXTERNAL_API = "https://www.odoo.com/documentation/18.0/developer/reference/external_api.html"
|
| 799 |
ODOO_CONSTRAINTS = "https://www.odoo.com/documentation/18.0/developer/tutorials/server_framework_101/10_constraints.html"
|
| 800 |
ODOO_ONCHANGE = "https://www.odoo.com/documentation/18.0/developer/tutorials/server_framework_101/08_compute_onchange.html"
|
|
|
|
|
|
|
|
|
|
|
|
|
| 801 |
PG_CREATE_TABLE = "https://www.postgresql.org/docs/current/ddl-constraints.html"
|
| 802 |
# The Zod API reference, not the homepage. Four rules cited zod.dev/, which
|
| 803 |
# supports nothing: a landing page is not provenance.
|
|
@@ -925,6 +987,22 @@ ENRICH = {
|
|
| 925 |
# noupdate is a data-file attribute. The ORM reference never mentions it.
|
| 926 |
"source": ODOO_DATA,
|
| 927 |
},
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 928 |
|
| 929 |
# ---- TypeScript -------------------------------------------------------
|
| 930 |
"typescript.types-are-erased-at-runtime": {
|
|
@@ -1110,6 +1188,12 @@ PROBES = {
|
|
| 1110 |
"change what a core sale.order method does without editing the sale module",
|
| 1111 |
"odoo.noupdate-on-user-editable-data":
|
| 1112 |
"the customer's configuration resets itself every time we upgrade the module",
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 1113 |
|
| 1114 |
# ---- TypeScript -------------------------------------------------------
|
| 1115 |
"typescript.types-are-erased-at-runtime":
|
|
@@ -1180,6 +1264,15 @@ SEMANTIC_CHECK = {
|
|
| 1180 |
"odoo.stored-compute-needs-complete-depends": "depends_covers_compute_reads",
|
| 1181 |
"odoo.ondelete-is-a-decision": "ondelete_leaves_a_dangling_read",
|
| 1182 |
"odoo.check-company-on-relations": "check_company_missing_on_company_owned_relation",
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 1183 |
}
|
| 1184 |
|
| 1185 |
|
|
|
|
| 354 |
</odoo>""",
|
| 355 |
"tags": ["xml", "data", "upgrade"],
|
| 356 |
},
|
| 357 |
+
{
|
| 358 |
+
"id": "odoo.commit-inside-a-loop-splits-the-transaction",
|
| 359 |
+
"odoo_versions": ">=16.0",
|
| 360 |
+
"severity": "high",
|
| 361 |
+
"title": "Never commit inside a loop over records",
|
| 362 |
+
"rule": "Let the caller own the transaction. Do not call cr.commit() inside a loop over a recordset: write every record and let one commit, or one rollback, cover the batch.",
|
| 363 |
+
"why": "A commit ends the transaction the ORM is working inside, and Odoo's own guidelines say a manual one causes 'partial commits, and thus partial and unclean rollbacks'. Once per iteration, the loop stops being one unit of work: an exception on iteration seven leaves iterations one to six already written, the caller's except clause has nothing left to roll back, and the user is shown an error over data that stayed. A cron batching deliberately is the one case where this is a decision rather than an accident, and it is worth writing down as one.",
|
| 364 |
+
"enforcement_layer": "build-step",
|
| 365 |
+
"bypassed_by": [],
|
| 366 |
+
"wrong": """def _expire_consents(self):
|
| 367 |
+
for rec in self:
|
| 368 |
+
rec.state = 'expired'
|
| 369 |
+
self.env.cr.commit()""",
|
| 370 |
+
"correct": """def _expire_consents(self):
|
| 371 |
+
for rec in self:
|
| 372 |
+
rec.state = 'expired'
|
| 373 |
+
# No commit. The caller owns the transaction, so a failure anywhere in
|
| 374 |
+
# the batch rolls the whole batch back.""",
|
| 375 |
+
"tags": ["transaction", "commit", "partial-write", "atomicity", "batch"],
|
| 376 |
+
},
|
| 377 |
+
{
|
| 378 |
+
"id": "odoo.sql-needs-query-parameters",
|
| 379 |
+
"odoo_versions": ">=16.0",
|
| 380 |
+
"severity": "critical",
|
| 381 |
+
"title": "Pass values to cr.execute as parameters, never formatted into the query",
|
| 382 |
+
"rule": "Build every query as a constant string with %s placeholders and hand the values to cr.execute as its second argument. Never use %, .format(), an f-string or + to put a value into SQL.",
|
| 383 |
+
"why": "The driver quotes and types a parameter; Python formatting does neither. A value containing an apostrophe ends the string literal and what follows is executed as SQL, which is how an ordinary surname becomes an injection. It is also wrong before anybody is malicious: None formats as the text None rather than NULL, a date formats as a Python repr, and a number formatted into a text column compares as text. The query runs, returns rows, and is quietly the wrong query.",
|
| 384 |
+
"enforcement_layer": "build-step",
|
| 385 |
+
"bypassed_by": [],
|
| 386 |
+
"wrong": """self.env.cr.execute(
|
| 387 |
+
"SELECT id FROM res_partner WHERE company_id = %s" % company.id
|
| 388 |
+
)""",
|
| 389 |
+
"correct": """self.env.cr.execute(
|
| 390 |
+
"SELECT id FROM res_partner WHERE company_id = %s", (company.id,)
|
| 391 |
+
)""",
|
| 392 |
+
"tags": ["sql", "injection", "cr-execute", "query-parameters", "security"],
|
| 393 |
+
},
|
| 394 |
+
{
|
| 395 |
+
"id": "odoo.depends-must-not-name-unread-fields",
|
| 396 |
+
"odoo_versions": ">=16.0",
|
| 397 |
+
"severity": "medium",
|
| 398 |
+
"title": "Every api.depends entry should be a field the compute actually reads",
|
| 399 |
+
"rule": "Remove @api.depends entries the method body never reads. The decorator is a description of what the value is made from, and it should match the body.",
|
| 400 |
+
"why": "A dependency the body cannot use does not make the value stale, it makes the value recompute on writes that could not have changed it, and on a stored compute that is a database write per record per unrelated edit. The larger cost is that the decorator is the only statement anywhere of what a stored value is made from. Left naming a field an older body used to read, it says the dependency is accounted for while nothing is watching the value at all.",
|
| 401 |
+
"enforcement_layer": "python-constraint",
|
| 402 |
+
"bypassed_by": [],
|
| 403 |
+
"wrong": """@api.depends('deployment_date', 'actual_return_date', 'status')
|
| 404 |
+
def _compute_days_deployed(self):
|
| 405 |
+
for rec in self:
|
| 406 |
+
end = rec.actual_return_date or fields.Date.today()
|
| 407 |
+
rec.days_deployed = (end - rec.deployment_date).days""",
|
| 408 |
+
"correct": """@api.depends('deployment_date', 'actual_return_date')
|
| 409 |
+
def _compute_days_deployed(self):
|
| 410 |
+
for rec in self:
|
| 411 |
+
end = rec.actual_return_date or fields.Date.today()
|
| 412 |
+
rec.days_deployed = (end - rec.deployment_date).days""",
|
| 413 |
+
"tags": ["compute", "depends", "recompute", "stale-data", "decorator"],
|
| 414 |
+
},
|
| 415 |
]
|
| 416 |
|
| 417 |
TYPESCRIPT = [
|
|
|
|
| 856 |
ODOO_EXTERNAL_API = "https://www.odoo.com/documentation/18.0/developer/reference/external_api.html"
|
| 857 |
ODOO_CONSTRAINTS = "https://www.odoo.com/documentation/18.0/developer/tutorials/server_framework_101/10_constraints.html"
|
| 858 |
ODOO_ONCHANGE = "https://www.odoo.com/documentation/18.0/developer/tutorials/server_framework_101/08_compute_onchange.html"
|
| 859 |
+
# Transaction handling is a contributing guideline rather than an ORM reference
|
| 860 |
+
# topic: the ORM page does not use the word commit in any series. The guidelines
|
| 861 |
+
# page does, and says the thing the rule says.
|
| 862 |
+
ODOO_GUIDELINES = "https://www.odoo.com/documentation/18.0/contributing/development/coding_guidelines.html"
|
| 863 |
PG_CREATE_TABLE = "https://www.postgresql.org/docs/current/ddl-constraints.html"
|
| 864 |
# The Zod API reference, not the homepage. Four rules cited zod.dev/, which
|
| 865 |
# supports nothing: a landing page is not provenance.
|
|
|
|
| 987 |
# noupdate is a data-file attribute. The ORM reference never mentions it.
|
| 988 |
"source": ODOO_DATA,
|
| 989 |
},
|
| 990 |
+
"odoo.commit-inside-a-loop-splits-the-transaction": {
|
| 991 |
+
# No violation_pattern on purpose. `cr.commit()` is one string whether
|
| 992 |
+
# it sits in a loop or after one, and the difference between those is
|
| 993 |
+
# the entire rule.
|
| 994 |
+
"source": ODOO_GUIDELINES,
|
| 995 |
+
},
|
| 996 |
+
"odoo.sql-needs-query-parameters": {
|
| 997 |
+
# Also deliberately AST-only, and this one is the clearest case in the
|
| 998 |
+
# corpus for it: the CORRECT form contains a percent sign too. A regex
|
| 999 |
+
# that fires on `%` in a query fires on every parameterised query in
|
| 1000 |
+
# the codebase, and one that does not fire on `%` cannot see the bug.
|
| 1001 |
+
"source": ODOO_ORM,
|
| 1002 |
+
},
|
| 1003 |
+
"odoo.depends-must-not-name-unread-fields": {
|
| 1004 |
+
"source": ODOO_ORM,
|
| 1005 |
+
},
|
| 1006 |
|
| 1007 |
# ---- TypeScript -------------------------------------------------------
|
| 1008 |
"typescript.types-are-erased-at-runtime": {
|
|
|
|
| 1188 |
"change what a core sale.order method does without editing the sale module",
|
| 1189 |
"odoo.noupdate-on-user-editable-data":
|
| 1190 |
"the customer's configuration resets itself every time we upgrade the module",
|
| 1191 |
+
"odoo.commit-inside-a-loop-splits-the-transaction":
|
| 1192 |
+
"half my records were updated and the rest were not after the job raised",
|
| 1193 |
+
"odoo.sql-needs-query-parameters":
|
| 1194 |
+
"a raw query breaks when a partner name contains an apostrophe",
|
| 1195 |
+
"odoo.depends-must-not-name-unread-fields":
|
| 1196 |
+
"my api.depends lists a field the compute never uses",
|
| 1197 |
|
| 1198 |
# ---- TypeScript -------------------------------------------------------
|
| 1199 |
"typescript.types-are-erased-at-runtime":
|
|
|
|
| 1264 |
"odoo.stored-compute-needs-complete-depends": "depends_covers_compute_reads",
|
| 1265 |
"odoo.ondelete-is-a-decision": "ondelete_leaves_a_dangling_read",
|
| 1266 |
"odoo.check-company-on-relations": "check_company_missing_on_company_owned_relation",
|
| 1267 |
+
# The three below are AST passes for the same reason from a different
|
| 1268 |
+
# angle: the violation and the correct form are the same text, and only
|
| 1269 |
+
# the structure around it differs. A commit is a commit wherever it sits;
|
| 1270 |
+
# a parameterised query carries the same percent sign as an interpolated
|
| 1271 |
+
# one; and an unread dependency is a string that looks exactly like a read
|
| 1272 |
+
# one until you compare it against the body.
|
| 1273 |
+
"odoo.commit-inside-a-loop-splits-the-transaction": "commit_inside_a_loop",
|
| 1274 |
+
"odoo.sql-needs-query-parameters": "sql_built_by_string_formatting",
|
| 1275 |
+
"odoo.depends-must-not-name-unread-fields": "depends_names_unread_fields",
|
| 1276 |
}
|
| 1277 |
|
| 1278 |
|
|
@@ -55,6 +55,12 @@ SYMBOLS = {
|
|
| 55 |
"odoo.sudo-bypasses-record-rules": "sudo",
|
| 56 |
"odoo.no-monkey-patching": "_inherit",
|
| 57 |
"odoo.noupdate-on-user-editable-data": "noupdate",
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 58 |
}
|
| 59 |
|
| 60 |
# Where an API lives when the rule cites a tutorial page. The tutorials were
|
|
|
|
| 55 |
"odoo.sudo-bypasses-record-rules": "sudo",
|
| 56 |
"odoo.no-monkey-patching": "_inherit",
|
| 57 |
"odoo.noupdate-on-user-editable-data": "noupdate",
|
| 58 |
+
# "cr.commit", not "commit": the guidelines page says "commit" eighteen
|
| 59 |
+
# times and seventeen of them are about git. The dotted spelling appears
|
| 60 |
+
# once, in the paragraph this rule is built on.
|
| 61 |
+
"odoo.commit-inside-a-loop-splits-the-transaction": "cr.commit",
|
| 62 |
+
"odoo.sql-needs-query-parameters": "execute",
|
| 63 |
+
"odoo.depends-must-not-name-unread-fields": "depends",
|
| 64 |
}
|
| 65 |
|
| 66 |
# Where an API lives when the rule cites a tutorial page. The tutorials were
|
|
@@ -1,14 +1,14 @@
|
|
| 1 |
{
|
| 2 |
-
"built_utc": "2026-09-
|
| 3 |
-
"rules":
|
| 4 |
-
"machine_checkable":
|
| 5 |
"by_pattern": 32,
|
| 6 |
-
"by_semantic_check":
|
| 7 |
"checked_against": {
|
| 8 |
"odoo": "18.0",
|
| 9 |
"python": "3.12",
|
| 10 |
"typescript": "5.x",
|
| 11 |
"javascript": "ES2023"
|
| 12 |
},
|
| 13 |
-
"corpus_sha256": "
|
| 14 |
}
|
|
|
|
| 1 |
{
|
| 2 |
+
"built_utc": "2026-09-14T15:27:55.137727+00:00",
|
| 3 |
+
"rules": 43,
|
| 4 |
+
"machine_checkable": 40,
|
| 5 |
"by_pattern": 32,
|
| 6 |
+
"by_semantic_check": 8,
|
| 7 |
"checked_against": {
|
| 8 |
"odoo": "18.0",
|
| 9 |
"python": "3.12",
|
| 10 |
"typescript": "5.x",
|
| 11 |
"javascript": "ES2023"
|
| 12 |
},
|
| 13 |
+
"corpus_sha256": "82ea99467831cb210b5385d96cdd2629a4fa5c85e1a256ed57184ea71f469e51"
|
| 14 |
}
|
|
@@ -350,5 +350,74 @@
|
|
| 350 |
"satisfied_pattern": "noupdate",
|
| 351 |
"source": "https://www.odoo.com/documentation/18.0/developer/reference/backend/data.html",
|
| 352 |
"probe": "the customer's configuration resets itself every time we upgrade the module"
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 353 |
}
|
| 354 |
]
|
|
|
|
| 350 |
"satisfied_pattern": "noupdate",
|
| 351 |
"source": "https://www.odoo.com/documentation/18.0/developer/reference/backend/data.html",
|
| 352 |
"probe": "the customer's configuration resets itself every time we upgrade the module"
|
| 353 |
+
},
|
| 354 |
+
{
|
| 355 |
+
"id": "odoo.commit-inside-a-loop-splits-the-transaction",
|
| 356 |
+
"odoo_versions": ">=16.0",
|
| 357 |
+
"severity": "high",
|
| 358 |
+
"title": "Never commit inside a loop over records",
|
| 359 |
+
"rule": "Let the caller own the transaction. Do not call cr.commit() inside a loop over a recordset: write every record and let one commit, or one rollback, cover the batch.",
|
| 360 |
+
"why": "A commit ends the transaction the ORM is working inside, and Odoo's own guidelines say a manual one causes 'partial commits, and thus partial and unclean rollbacks'. Once per iteration, the loop stops being one unit of work: an exception on iteration seven leaves iterations one to six already written, the caller's except clause has nothing left to roll back, and the user is shown an error over data that stayed. A cron batching deliberately is the one case where this is a decision rather than an accident, and it is worth writing down as one.",
|
| 361 |
+
"enforcement_layer": "build-step",
|
| 362 |
+
"bypassed_by": [],
|
| 363 |
+
"wrong": "def _expire_consents(self):\n for rec in self:\n rec.state = 'expired'\n self.env.cr.commit()",
|
| 364 |
+
"correct": "def _expire_consents(self):\n for rec in self:\n rec.state = 'expired'\n # No commit. The caller owns the transaction, so a failure anywhere in\n # the batch rolls the whole batch back.",
|
| 365 |
+
"tags": [
|
| 366 |
+
"transaction",
|
| 367 |
+
"commit",
|
| 368 |
+
"partial-write",
|
| 369 |
+
"atomicity",
|
| 370 |
+
"batch"
|
| 371 |
+
],
|
| 372 |
+
"stack": "odoo",
|
| 373 |
+
"source": "https://www.odoo.com/documentation/18.0/contributing/development/coding_guidelines.html",
|
| 374 |
+
"probe": "half my records were updated and the rest were not after the job raised",
|
| 375 |
+
"semantic_check": "commit_inside_a_loop"
|
| 376 |
+
},
|
| 377 |
+
{
|
| 378 |
+
"id": "odoo.sql-needs-query-parameters",
|
| 379 |
+
"odoo_versions": ">=16.0",
|
| 380 |
+
"severity": "critical",
|
| 381 |
+
"title": "Pass values to cr.execute as parameters, never formatted into the query",
|
| 382 |
+
"rule": "Build every query as a constant string with %s placeholders and hand the values to cr.execute as its second argument. Never use %, .format(), an f-string or + to put a value into SQL.",
|
| 383 |
+
"why": "The driver quotes and types a parameter; Python formatting does neither. A value containing an apostrophe ends the string literal and what follows is executed as SQL, which is how an ordinary surname becomes an injection. It is also wrong before anybody is malicious: None formats as the text None rather than NULL, a date formats as a Python repr, and a number formatted into a text column compares as text. The query runs, returns rows, and is quietly the wrong query.",
|
| 384 |
+
"enforcement_layer": "build-step",
|
| 385 |
+
"bypassed_by": [],
|
| 386 |
+
"wrong": "self.env.cr.execute(\n \"SELECT id FROM res_partner WHERE company_id = %s\" % company.id\n)",
|
| 387 |
+
"correct": "self.env.cr.execute(\n \"SELECT id FROM res_partner WHERE company_id = %s\", (company.id,)\n)",
|
| 388 |
+
"tags": [
|
| 389 |
+
"sql",
|
| 390 |
+
"injection",
|
| 391 |
+
"cr-execute",
|
| 392 |
+
"query-parameters",
|
| 393 |
+
"security"
|
| 394 |
+
],
|
| 395 |
+
"stack": "odoo",
|
| 396 |
+
"source": "https://www.odoo.com/documentation/18.0/developer/reference/backend/orm.html",
|
| 397 |
+
"probe": "a raw query breaks when a partner name contains an apostrophe",
|
| 398 |
+
"semantic_check": "sql_built_by_string_formatting"
|
| 399 |
+
},
|
| 400 |
+
{
|
| 401 |
+
"id": "odoo.depends-must-not-name-unread-fields",
|
| 402 |
+
"odoo_versions": ">=16.0",
|
| 403 |
+
"severity": "medium",
|
| 404 |
+
"title": "Every api.depends entry should be a field the compute actually reads",
|
| 405 |
+
"rule": "Remove @api.depends entries the method body never reads. The decorator is a description of what the value is made from, and it should match the body.",
|
| 406 |
+
"why": "A dependency the body cannot use does not make the value stale, it makes the value recompute on writes that could not have changed it, and on a stored compute that is a database write per record per unrelated edit. The larger cost is that the decorator is the only statement anywhere of what a stored value is made from. Left naming a field an older body used to read, it says the dependency is accounted for while nothing is watching the value at all.",
|
| 407 |
+
"enforcement_layer": "python-constraint",
|
| 408 |
+
"bypassed_by": [],
|
| 409 |
+
"wrong": "@api.depends('deployment_date', 'actual_return_date', 'status')\ndef _compute_days_deployed(self):\n for rec in self:\n end = rec.actual_return_date or fields.Date.today()\n rec.days_deployed = (end - rec.deployment_date).days",
|
| 410 |
+
"correct": "@api.depends('deployment_date', 'actual_return_date')\ndef _compute_days_deployed(self):\n for rec in self:\n end = rec.actual_return_date or fields.Date.today()\n rec.days_deployed = (end - rec.deployment_date).days",
|
| 411 |
+
"tags": [
|
| 412 |
+
"compute",
|
| 413 |
+
"depends",
|
| 414 |
+
"recompute",
|
| 415 |
+
"stale-data",
|
| 416 |
+
"decorator"
|
| 417 |
+
],
|
| 418 |
+
"stack": "odoo",
|
| 419 |
+
"source": "https://www.odoo.com/documentation/18.0/developer/reference/backend/orm.html",
|
| 420 |
+
"probe": "my api.depends lists a field the compute never uses",
|
| 421 |
+
"semantic_check": "depends_names_unread_fields"
|
| 422 |
}
|
| 423 |
]
|
|
@@ -116,6 +116,18 @@ garnish on top of the real rule. Agreeing here ships bad data to production.
|
|
| 116 |
def migrate(cr, version):
|
| 117 |
cr.execute("UPDATE my_model SET ref = 'LEGACY-' || id WHERE ref IS NULL")
|
| 118 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 119 |
[odoo.stored-compute-needs-complete-depends] (critical) Stored computed fields need a complete api.depends
|
| 120 |
Rule: Every field read inside a stored compute must appear in @api.depends, including fields reached through a relation.
|
| 121 |
Why: A stored computed field with an incomplete depends is written once and never recomputed. The value goes stale silently, survives restarts, and diverges from reality. Nothing errors, so it surfaces only when a report disagrees with the source data.
|
|
@@ -141,6 +153,21 @@ garnish on top of the real rule. Agreeing here ships bad data to production.
|
|
| 141 |
('company_id', 'in', [False] + self.env.company.ids),
|
| 142 |
])
|
| 143 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 144 |
[odoo.constrains-iterate-recordset] (high) Constraints receive a recordset, never a single record
|
| 145 |
Rule: Always iterate with 'for rec in self:' inside @api.constrains and @api.depends methods.
|
| 146 |
Why: Batch create and multi-record write pass a multi-record recordset. Reading self.field on more than one record raises a singleton error, so a bulk import fails only for some batches and looks intermittent.
|
|
@@ -235,6 +262,22 @@ garnish on top of the real rule. Agreeing here ships bad data to production.
|
|
| 235 |
if self._has_cycle():
|
| 236 |
raise ValidationError('You cannot create recursive hierarchies.')
|
| 237 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 238 |
[odoo.noupdate-on-user-editable-data] (medium) XML data users may edit needs noupdate
|
| 239 |
Rule: Wrap XML data records that users are expected to modify in a data element with noupdate set to 1.
|
| 240 |
Why: Without noupdate, every module upgrade overwrites the record with the version in XML, discarding the customer's configuration. It surfaces weeks later as 'our settings reset themselves'.
|
|
|
|
| 116 |
def migrate(cr, version):
|
| 117 |
cr.execute("UPDATE my_model SET ref = 'LEGACY-' || id WHERE ref IS NULL")
|
| 118 |
|
| 119 |
+
[odoo.sql-needs-query-parameters] (critical) Pass values to cr.execute as parameters, never formatted into the query
|
| 120 |
+
Rule: Build every query as a constant string with %s placeholders and hand the values to cr.execute as its second argument. Never use %, .format(), an f-string or + to put a value into SQL.
|
| 121 |
+
Why: The driver quotes and types a parameter; Python formatting does neither. A value containing an apostrophe ends the string literal and what follows is executed as SQL, which is how an ordinary surname becomes an injection. It is also wrong before anybody is malicious: None formats as the text None rather than NULL, a date formats as a Python repr, and a number formatted into a text column compares as text. The query runs, returns rows, and is quietly the wrong query.
|
| 122 |
+
Wrong:
|
| 123 |
+
self.env.cr.execute(
|
| 124 |
+
"SELECT id FROM res_partner WHERE company_id = %s" % company.id
|
| 125 |
+
)
|
| 126 |
+
Correct:
|
| 127 |
+
self.env.cr.execute(
|
| 128 |
+
"SELECT id FROM res_partner WHERE company_id = %s", (company.id,)
|
| 129 |
+
)
|
| 130 |
+
|
| 131 |
[odoo.stored-compute-needs-complete-depends] (critical) Stored computed fields need a complete api.depends
|
| 132 |
Rule: Every field read inside a stored compute must appear in @api.depends, including fields reached through a relation.
|
| 133 |
Why: A stored computed field with an incomplete depends is written once and never recomputed. The value goes stale silently, survives restarts, and diverges from reality. Nothing errors, so it surfaces only when a report disagrees with the source data.
|
|
|
|
| 153 |
('company_id', 'in', [False] + self.env.company.ids),
|
| 154 |
])
|
| 155 |
|
| 156 |
+
[odoo.commit-inside-a-loop-splits-the-transaction] (high) Never commit inside a loop over records
|
| 157 |
+
Rule: Let the caller own the transaction. Do not call cr.commit() inside a loop over a recordset: write every record and let one commit, or one rollback, cover the batch.
|
| 158 |
+
Why: A commit ends the transaction the ORM is working inside, and Odoo's own guidelines say a manual one causes 'partial commits, and thus partial and unclean rollbacks'. Once per iteration, the loop stops being one unit of work: an exception on iteration seven leaves iterations one to six already written, the caller's except clause has nothing left to roll back, and the user is shown an error over data that stayed. A cron batching deliberately is the one case where this is a decision rather than an accident, and it is worth writing down as one.
|
| 159 |
+
Wrong:
|
| 160 |
+
def _expire_consents(self):
|
| 161 |
+
for rec in self:
|
| 162 |
+
rec.state = 'expired'
|
| 163 |
+
self.env.cr.commit()
|
| 164 |
+
Correct:
|
| 165 |
+
def _expire_consents(self):
|
| 166 |
+
for rec in self:
|
| 167 |
+
rec.state = 'expired'
|
| 168 |
+
# No commit. The caller owns the transaction, so a failure anywhere in
|
| 169 |
+
# the batch rolls the whole batch back.
|
| 170 |
+
|
| 171 |
[odoo.constrains-iterate-recordset] (high) Constraints receive a recordset, never a single record
|
| 172 |
Rule: Always iterate with 'for rec in self:' inside @api.constrains and @api.depends methods.
|
| 173 |
Why: Batch create and multi-record write pass a multi-record recordset. Reading self.field on more than one record raises a singleton error, so a bulk import fails only for some batches and looks intermittent.
|
|
|
|
| 262 |
if self._has_cycle():
|
| 263 |
raise ValidationError('You cannot create recursive hierarchies.')
|
| 264 |
|
| 265 |
+
[odoo.depends-must-not-name-unread-fields] (medium) Every api.depends entry should be a field the compute actually reads
|
| 266 |
+
Rule: Remove @api.depends entries the method body never reads. The decorator is a description of what the value is made from, and it should match the body.
|
| 267 |
+
Why: A dependency the body cannot use does not make the value stale, it makes the value recompute on writes that could not have changed it, and on a stored compute that is a database write per record per unrelated edit. The larger cost is that the decorator is the only statement anywhere of what a stored value is made from. Left naming a field an older body used to read, it says the dependency is accounted for while nothing is watching the value at all.
|
| 268 |
+
Wrong:
|
| 269 |
+
@api.depends('deployment_date', 'actual_return_date', 'status')
|
| 270 |
+
def _compute_days_deployed(self):
|
| 271 |
+
for rec in self:
|
| 272 |
+
end = rec.actual_return_date or fields.Date.today()
|
| 273 |
+
rec.days_deployed = (end - rec.deployment_date).days
|
| 274 |
+
Correct:
|
| 275 |
+
@api.depends('deployment_date', 'actual_return_date')
|
| 276 |
+
def _compute_days_deployed(self):
|
| 277 |
+
for rec in self:
|
| 278 |
+
end = rec.actual_return_date or fields.Date.today()
|
| 279 |
+
rec.days_deployed = (end - rec.deployment_date).days
|
| 280 |
+
|
| 281 |
[odoo.noupdate-on-user-editable-data] (medium) XML data users may edit needs noupdate
|
| 282 |
Rule: Wrap XML data records that users are expected to modify in a data element with noupdate set to 1.
|
| 283 |
Why: Without noupdate, every module upgrade overwrites the record with the version in XML, discarding the customer's configuration. It surfaces weeks later as 'our settings reset themselves'.
|
|
@@ -297,6 +297,42 @@ COMMANDS = {
|
|
| 297 |
"\n"
|
| 298 |
"No model, no network.",
|
| 299 |
),
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 300 |
"coverage": (
|
| 301 |
"Show what is machine-checkable and what is not",
|
| 302 |
"nexa coverage",
|
|
@@ -429,7 +465,7 @@ COMMANDS = {
|
|
| 429 |
GROUPS = [
|
| 430 |
("ASK", ["ask", "run", "check"]),
|
| 431 |
("CORPUS", ["rules", "build", "validate", "crosscheck", "coverage",
|
| 432 |
-
"adversarial", "fieldtest", "recall"]),
|
| 433 |
("SYSTEM", ["doctor", "models", "report", "evidence", "index", "advise",
|
| 434 |
"eval"]),
|
| 435 |
]
|
|
@@ -630,6 +666,10 @@ def main(argv):
|
|
| 630 |
if not rest:
|
| 631 |
return usage_error("recall")
|
| 632 |
return run_module("recall.py", rest, styled=True)
|
|
|
|
|
|
|
|
|
|
|
|
|
| 633 |
if cmd == "evidence":
|
| 634 |
return run_module("evidence.py", rest, styled=True)
|
| 635 |
if cmd == "report":
|
|
|
|
| 297 |
"\n"
|
| 298 |
"No model, no network.",
|
| 299 |
),
|
| 300 |
+
"crosswalk": (
|
| 301 |
+
"Compare the corpus against somebody else's ruleset",
|
| 302 |
+
"nexa crosswalk <ruleset.yml> [--checkout DIR] [--stack odoo]",
|
| 303 |
+
"crosscheck, adversarial, fieldtest and recall all ask whether what we\n"
|
| 304 |
+
"wrote behaves. None of them can ask whether it is the right set of\n"
|
| 305 |
+
"things to have written: a rule nobody thought of is invisible to every\n"
|
| 306 |
+
"check that reads the rules.\n"
|
| 307 |
+
"\n"
|
| 308 |
+
"An external ruleset for the same stack is the one artefact that can\n"
|
| 309 |
+
"say. Somebody else decided what was worth checking, on the same\n"
|
| 310 |
+
"platform, without reading ours. Where the two agree, two independent\n"
|
| 311 |
+
"judgements landed on the same construct, and that is the only external\n"
|
| 312 |
+
"corroboration in this repository.\n"
|
| 313 |
+
"\n"
|
| 314 |
+
"Their analyser is never run -- semgrep is not installed here and an\n"
|
| 315 |
+
"offline tool will not grow a package index. Each of their patterns is\n"
|
| 316 |
+
"turned into the smallest snippet it would match and OUR checkers run\n"
|
| 317 |
+
"over that, so every claim rests on a checker firing rather than on two\n"
|
| 318 |
+
"rule descriptions sounding alike.\n"
|
| 319 |
+
"\n"
|
| 320 |
+
" --checkout a tree their analyser runs on. Its `# nosemgrep:` markers\n"
|
| 321 |
+
" say which of their rules fired on real code and were read\n"
|
| 322 |
+
" and kept by a person, and the report says how many of\n"
|
| 323 |
+
" those same lines a checker here also fires on.\n"
|
| 324 |
+
"\n"
|
| 325 |
+
" --json every row, their rule ids included.\n"
|
| 326 |
+
"\n"
|
| 327 |
+
"Not a score. This corpus is about ORM correctness and a security\n"
|
| 328 |
+
"ruleset is about security, so most of the gap is a different subject\n"
|
| 329 |
+
"rather than a missing rule, and reading it is the work.\n"
|
| 330 |
+
"\n"
|
| 331 |
+
" nexa crosswalk ../their-repo/.semgrep/odoo-security.yml\n"
|
| 332 |
+
" nexa crosswalk rules.yml --checkout ../their-repo\n"
|
| 333 |
+
"\n"
|
| 334 |
+
"No model, no network.",
|
| 335 |
+
),
|
| 336 |
"coverage": (
|
| 337 |
"Show what is machine-checkable and what is not",
|
| 338 |
"nexa coverage",
|
|
|
|
| 465 |
GROUPS = [
|
| 466 |
("ASK", ["ask", "run", "check"]),
|
| 467 |
("CORPUS", ["rules", "build", "validate", "crosscheck", "coverage",
|
| 468 |
+
"adversarial", "fieldtest", "recall", "crosswalk"]),
|
| 469 |
("SYSTEM", ["doctor", "models", "report", "evidence", "index", "advise",
|
| 470 |
"eval"]),
|
| 471 |
]
|
|
|
|
| 666 |
if not rest:
|
| 667 |
return usage_error("recall")
|
| 668 |
return run_module("recall.py", rest, styled=True)
|
| 669 |
+
if cmd == "crosswalk":
|
| 670 |
+
if not rest:
|
| 671 |
+
return usage_error("crosswalk")
|
| 672 |
+
return run_module("crosswalk.py", rest, styled=True)
|
| 673 |
if cmd == "evidence":
|
| 674 |
return run_module("evidence.py", rest, styled=True)
|
| 675 |
if cmd == "report":
|
|
@@ -0,0 +1,396 @@
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 1 |
+
"""nexa crosswalk: comparing this corpus against somebody else's ruleset.
|
| 2 |
+
|
| 3 |
+
The fixtures are small semgrep rule files written here, not a copy of anybody's
|
| 4 |
+
real one. What is under test is the machinery -- what gets read out of the YAML,
|
| 5 |
+
what a pattern turns into, and which claims the report is entitled to make --
|
| 6 |
+
and all of that has to be decidable without a network or a semgrep install.
|
| 7 |
+
|
| 8 |
+
The load-bearing claim of the whole tool is that a pairing rests on a checker
|
| 9 |
+
FIRING on a snippet, never on two rule descriptions sounding alike. Several of
|
| 10 |
+
these tests exist to pin that, from both directions.
|
| 11 |
+
"""
|
| 12 |
+
import io
|
| 13 |
+
import os
|
| 14 |
+
|
| 15 |
+
import pytest
|
| 16 |
+
|
| 17 |
+
import crosswalk
|
| 18 |
+
|
| 19 |
+
|
| 20 |
+
def ruleset(tmp_path, body, name="rules.yml"):
|
| 21 |
+
path = tmp_path / name
|
| 22 |
+
path.write_text(body, encoding="utf-8")
|
| 23 |
+
return str(path)
|
| 24 |
+
|
| 25 |
+
|
| 26 |
+
SIMPLE = """rules:
|
| 27 |
+
- id: their-eval
|
| 28 |
+
patterns:
|
| 29 |
+
- pattern-either:
|
| 30 |
+
- pattern: eval(...)
|
| 31 |
+
- pattern: exec(...)
|
| 32 |
+
message: |
|
| 33 |
+
Do not evaluate strings.
|
| 34 |
+
Second line of the message.
|
| 35 |
+
severity: ERROR
|
| 36 |
+
languages: [python]
|
| 37 |
+
metadata:
|
| 38 |
+
cwe: "CWE-95"
|
| 39 |
+
category: security
|
| 40 |
+
|
| 41 |
+
- id: their-loop-commit
|
| 42 |
+
patterns:
|
| 43 |
+
- pattern-either:
|
| 44 |
+
- pattern: |
|
| 45 |
+
for ... in ...:
|
| 46 |
+
...
|
| 47 |
+
$CR.commit()
|
| 48 |
+
...
|
| 49 |
+
message: |
|
| 50 |
+
Committing in a loop leaves partial state.
|
| 51 |
+
severity: WARNING
|
| 52 |
+
languages: [python]
|
| 53 |
+
metadata:
|
| 54 |
+
category: reliability
|
| 55 |
+
|
| 56 |
+
- id: their-weak-random
|
| 57 |
+
patterns:
|
| 58 |
+
- pattern-either:
|
| 59 |
+
- pattern: random.random()
|
| 60 |
+
- pattern: random.choice(...)
|
| 61 |
+
message: |
|
| 62 |
+
Do not use the random module for security decisions.
|
| 63 |
+
severity: ERROR
|
| 64 |
+
languages: [python]
|
| 65 |
+
metadata:
|
| 66 |
+
cwe: "CWE-330"
|
| 67 |
+
"""
|
| 68 |
+
|
| 69 |
+
|
| 70 |
+
# ------------------------------------------------------------------- reading
|
| 71 |
+
|
| 72 |
+
def test_rules_and_their_metadata_are_read(tmp_path):
|
| 73 |
+
rules, _ = crosswalk.read_ruleset(ruleset(tmp_path, SIMPLE))
|
| 74 |
+
assert [rule.rid for rule in rules] == ["their-eval", "their-loop-commit",
|
| 75 |
+
"their-weak-random"]
|
| 76 |
+
first = rules[0]
|
| 77 |
+
assert first.severity == "ERROR"
|
| 78 |
+
assert first.languages == ["python"]
|
| 79 |
+
assert first.cwe == "CWE-95"
|
| 80 |
+
assert first.category == "security"
|
| 81 |
+
assert first.patterns == ["eval(...)", "exec(...)"]
|
| 82 |
+
|
| 83 |
+
|
| 84 |
+
def test_a_block_scalar_pattern_keeps_its_lines(tmp_path):
|
| 85 |
+
rules, _ = crosswalk.read_ruleset(ruleset(tmp_path, SIMPLE))
|
| 86 |
+
pattern = rules[1].patterns[0]
|
| 87 |
+
assert pattern.startswith("for ... in ...:")
|
| 88 |
+
assert "$CR.commit()" in pattern
|
| 89 |
+
assert pattern.count("\n") == 3
|
| 90 |
+
|
| 91 |
+
|
| 92 |
+
def test_a_block_message_does_not_leak_into_the_patterns(tmp_path):
|
| 93 |
+
"""A `message: |` body is prose sitting at the same indent as a pattern
|
| 94 |
+
alternative. Reading it as one would pair rules on English."""
|
| 95 |
+
rules, _ = crosswalk.read_ruleset(ruleset(tmp_path, SIMPLE))
|
| 96 |
+
for rule in rules:
|
| 97 |
+
for pattern in rule.patterns:
|
| 98 |
+
assert "Do not evaluate" not in pattern
|
| 99 |
+
assert "partial state" not in pattern
|
| 100 |
+
assert "Second line of the message." in rules[0].message
|
| 101 |
+
|
| 102 |
+
|
| 103 |
+
def test_pattern_not_is_recognised_and_refused(tmp_path):
|
| 104 |
+
"""It says what a rule EXCLUDES. Probing it would test the opposite of the
|
| 105 |
+
rule, so it is reported as skipped rather than silently read or ignored."""
|
| 106 |
+
body = """rules:
|
| 107 |
+
- id: their-savepoint
|
| 108 |
+
pattern: |
|
| 109 |
+
$S = self.env.cr.savepoint()
|
| 110 |
+
...
|
| 111 |
+
pattern-not: |
|
| 112 |
+
$S = self.env.cr.savepoint()
|
| 113 |
+
...
|
| 114 |
+
$S.close()
|
| 115 |
+
severity: INFO
|
| 116 |
+
languages: [python]
|
| 117 |
+
"""
|
| 118 |
+
rules, skipped = crosswalk.read_ruleset(ruleset(tmp_path, body))
|
| 119 |
+
assert skipped == [("their-savepoint", "pattern-not")]
|
| 120 |
+
assert len(rules[0].patterns) == 1
|
| 121 |
+
assert "close()" not in rules[0].patterns[0]
|
| 122 |
+
|
| 123 |
+
|
| 124 |
+
def test_a_file_with_no_rules_is_not_read_as_an_empty_corpus(tmp_path, capsys):
|
| 125 |
+
path = ruleset(tmp_path, "# just a comment\n")
|
| 126 |
+
assert crosswalk.main([path]) == 2
|
| 127 |
+
assert "No rules found" in capsys.readouterr().out
|
| 128 |
+
|
| 129 |
+
|
| 130 |
+
def test_a_missing_file_is_a_bad_argument(capsys):
|
| 131 |
+
assert crosswalk.main(["no-such-file.yml"]) == 2
|
| 132 |
+
assert "No such rule file" in capsys.readouterr().out
|
| 133 |
+
|
| 134 |
+
|
| 135 |
+
# ------------------------------------------------------------------- probing
|
| 136 |
+
|
| 137 |
+
@pytest.mark.parametrize("pattern, wanted", [
|
| 138 |
+
("eval(...)", "eval(_h1)"),
|
| 139 |
+
("$CR.execute($QUERY % ...)", "_cr.execute(_query % _h1)"),
|
| 140 |
+
("self.env[$MODEL].sudo()", "self.env[_model].sudo()"),
|
| 141 |
+
("compile(..., ..., \"exec\")", "compile(_h1, _h2, \"exec\")"),
|
| 142 |
+
])
|
| 143 |
+
def test_holes_are_filled_with_names(pattern, wanted):
|
| 144 |
+
assert crosswalk.probe(pattern) == wanted
|
| 145 |
+
|
| 146 |
+
|
| 147 |
+
def test_an_ellipsis_in_a_dict_becomes_a_splat():
|
| 148 |
+
"""`{..., 'id': $R.id, ...}` is a dict display, and a bare name is not a
|
| 149 |
+
dict entry. Filling it wrong produces a snippet that does not parse, which
|
| 150 |
+
would be reported as no-answer on a rule we could have answered."""
|
| 151 |
+
assert crosswalk.probe("$R.write({..., 'state': 'done', ...})") == \
|
| 152 |
+
"_r.write({**_d1, 'state': 'done', **_d2})"
|
| 153 |
+
|
| 154 |
+
|
| 155 |
+
def test_an_ellipsis_inside_an_fstring_is_text_not_an_argument():
|
| 156 |
+
probe = crosswalk.probe('$CR.execute(f"... FROM {$TABLE} ...")')
|
| 157 |
+
assert probe == '_cr.execute(f"a FROM {_table} a")'
|
| 158 |
+
|
| 159 |
+
|
| 160 |
+
def test_a_statement_ellipsis_becomes_a_statement():
|
| 161 |
+
probe = crosswalk.probe("for ... in ...:\n ...\n $CR.commit()\n ...")
|
| 162 |
+
assert probe == "for _i in _seq:\n pass\n _cr.commit()\n pass"
|
| 163 |
+
|
| 164 |
+
|
| 165 |
+
def test_an_ellipsis_after_a_keyword_argument_becomes_a_splat():
|
| 166 |
+
"""`f(x, shell=True, y)` is a syntax error, so filling the trailing hole
|
| 167 |
+
with a plain name loses the rule rather than answering it. Three of the
|
| 168 |
+
forty-four rules crosswalk was first run against are this shape."""
|
| 169 |
+
assert crosswalk.probe("subprocess.run(..., shell=True, ...)") == \
|
| 170 |
+
"subprocess.run(_h1, shell=True, **_d2)"
|
| 171 |
+
|
| 172 |
+
|
| 173 |
+
def test_a_yaml_quoted_pattern_loses_the_quotes_not_the_code():
|
| 174 |
+
"""A ruleset that writes `- pattern: "return {...}"` means code. Left
|
| 175 |
+
quoted, it probes as a string literal and pairs with nothing."""
|
| 176 |
+
probe = crosswalk.probe("\"return {..., 'id': $RECORD.id, ...}\"")
|
| 177 |
+
assert probe == "return {**_d1, 'id': _record.id, **_d2}"
|
| 178 |
+
|
| 179 |
+
|
| 180 |
+
def test_a_fragment_is_probed_as_a_fragment():
|
| 181 |
+
"""A pattern is a fragment, not a module. `return {...}` and `await x()`
|
| 182 |
+
are whole patterns in real rulesets, and gating on compile() rather than on
|
| 183 |
+
the parser would report no answer on rules we can answer."""
|
| 184 |
+
assert crosswalk.probe("await $X.save()") == "await _x.save()"
|
| 185 |
+
assert crosswalk.probe("yield $X") == "yield _x"
|
| 186 |
+
|
| 187 |
+
|
| 188 |
+
def test_a_pattern_that_cannot_be_filled_in_returns_none():
|
| 189 |
+
assert crosswalk.probe("$X ... $Y ...:::") is None
|
| 190 |
+
assert crosswalk.probe("") is None
|
| 191 |
+
assert crosswalk.probe("# a comment") is None
|
| 192 |
+
|
| 193 |
+
|
| 194 |
+
def test_every_probe_parses_as_python(tmp_path):
|
| 195 |
+
"""The one invariant the whole comparison rests on: a probe that does not
|
| 196 |
+
parse cannot be handed to a checker, and a probe that parses into something
|
| 197 |
+
other than the construct would pair rules on an accident."""
|
| 198 |
+
import ast
|
| 199 |
+
rules, _ = crosswalk.read_ruleset(ruleset(tmp_path, SIMPLE))
|
| 200 |
+
for rule in rules:
|
| 201 |
+
for pattern in rule.patterns:
|
| 202 |
+
probe = crosswalk.probe(pattern)
|
| 203 |
+
assert probe is not None, pattern
|
| 204 |
+
ast.parse(probe)
|
| 205 |
+
|
| 206 |
+
|
| 207 |
+
# ------------------------------------------------------------------ matching
|
| 208 |
+
|
| 209 |
+
def test_a_pairing_rests_on_a_checker_firing(tmp_path, all_rules):
|
| 210 |
+
"""`eval(...)` and python.eval-and-pickle-execute-their-input are paired
|
| 211 |
+
because the checker fires on the snippet, not because both mention eval."""
|
| 212 |
+
rules, _ = crosswalk.read_ruleset(ruleset(tmp_path, SIMPLE))
|
| 213 |
+
rows = crosswalk.crosswalk(rules, all_rules)
|
| 214 |
+
by_id = {row["their_rule"]: row for row in rows}
|
| 215 |
+
assert "python.eval-and-pickle-execute-their-input" in \
|
| 216 |
+
by_id["their-eval"]["our_rules"]
|
| 217 |
+
|
| 218 |
+
|
| 219 |
+
def test_a_construct_no_checker_has_pairs_with_nothing(tmp_path, all_rules):
|
| 220 |
+
"""Weak randomness in a security decision is a real defect and this corpus
|
| 221 |
+
has nothing about it: it is about ORM correctness, not cryptography. The
|
| 222 |
+
report has to say so rather than reach for something close."""
|
| 223 |
+
rules, _ = crosswalk.read_ruleset(ruleset(tmp_path, SIMPLE))
|
| 224 |
+
rows = crosswalk.crosswalk(rules, all_rules)
|
| 225 |
+
by_id = {row["their_rule"]: row for row in rows}
|
| 226 |
+
assert by_id["their-weak-random"]["our_rules"] == []
|
| 227 |
+
assert by_id["their-weak-random"]["probed"] == 2, "probed, and still no pair"
|
| 228 |
+
|
| 229 |
+
|
| 230 |
+
def test_a_gap_this_tool_found_and_closed_now_pairs(all_rules, tmp_path):
|
| 231 |
+
"""The first crosswalk run reported `odoo-commit-in-loop` under "theirs
|
| 232 |
+
fires, nothing here does". That gap is why odoo.commit-inside-a-loop
|
| 233 |
+
exists, and this is the assertion that it stays closed: a corpus can lose
|
| 234 |
+
a rule as easily as it gained one.
|
| 235 |
+
"""
|
| 236 |
+
rules, _ = crosswalk.read_ruleset(ruleset(tmp_path, SIMPLE))
|
| 237 |
+
rows = crosswalk.crosswalk(rules, all_rules)
|
| 238 |
+
by_id = {row["their_rule"]: row for row in rows}
|
| 239 |
+
assert by_id["their-loop-commit"]["our_rules"] == \
|
| 240 |
+
["odoo.commit-inside-a-loop-splits-the-transaction"]
|
| 241 |
+
|
| 242 |
+
|
| 243 |
+
def test_probed_and_unprobed_are_counted_apart(tmp_path, all_rules):
|
| 244 |
+
"""A pattern that could not be filled in is a subject with no answer, which
|
| 245 |
+
is a different thing from a subject where the answer is no."""
|
| 246 |
+
body = SIMPLE + """
|
| 247 |
+
- id: their-unprobeable
|
| 248 |
+
patterns:
|
| 249 |
+
- pattern: $X ...:::
|
| 250 |
+
severity: INFO
|
| 251 |
+
languages: [python]
|
| 252 |
+
"""
|
| 253 |
+
rules, _ = crosswalk.read_ruleset(ruleset(tmp_path, body))
|
| 254 |
+
rows = crosswalk.crosswalk(rules, all_rules)
|
| 255 |
+
row = [r for r in rows if r["their_rule"] == "their-unprobeable"][0]
|
| 256 |
+
assert row["probed"] == 0
|
| 257 |
+
assert row["unprobed"] == ["$X ...:::"]
|
| 258 |
+
|
| 259 |
+
|
| 260 |
+
def test_literal_names_drop_the_metavariables():
|
| 261 |
+
assert crosswalk.literal_names("$CR.execute($QUERY % ...)") == {"execute"}
|
| 262 |
+
assert crosswalk.literal_names("pickle.loads(...)") == {"pickle", "loads"}
|
| 263 |
+
|
| 264 |
+
|
| 265 |
+
def test_the_reverse_direction_is_restricted_to_their_languages(tmp_path,
|
| 266 |
+
all_rules):
|
| 267 |
+
"""A Python-only ruleset having nothing to say about a TypeScript rule is
|
| 268 |
+
the languages line at the bottom of their file, not a finding."""
|
| 269 |
+
rules, _ = crosswalk.read_ruleset(ruleset(tmp_path, SIMPLE))
|
| 270 |
+
assert crosswalk.covered_stacks(rules) == {"odoo", "python"}
|
| 271 |
+
outside = crosswalk.unreachable_here(all_rules, rules)
|
| 272 |
+
assert outside, "a two-rule ruleset reaches almost nothing"
|
| 273 |
+
assert not [rid for rid in outside
|
| 274 |
+
if rid.startswith(("typescript.", "javascript."))]
|
| 275 |
+
|
| 276 |
+
|
| 277 |
+
def test_a_rule_their_patterns_name_is_not_reported_as_unreachable(tmp_path,
|
| 278 |
+
all_rules):
|
| 279 |
+
"""python.eval-and-pickle-execute-their-input's own wrong sample contains
|
| 280 |
+
`eval`, so `eval(...)` could match it. Claiming otherwise would be a false
|
| 281 |
+
negative in the one direction this test can be sound about."""
|
| 282 |
+
rules, _ = crosswalk.read_ruleset(ruleset(tmp_path, SIMPLE))
|
| 283 |
+
outside = crosswalk.unreachable_here(all_rules, rules)
|
| 284 |
+
assert "python.eval-and-pickle-execute-their-input" not in outside
|
| 285 |
+
|
| 286 |
+
|
| 287 |
+
# ---------------------------------------------------------------- field data
|
| 288 |
+
|
| 289 |
+
MARKED = '''from odoo import models
|
| 290 |
+
|
| 291 |
+
|
| 292 |
+
class Thing(models.Model):
|
| 293 |
+
def go(self):
|
| 294 |
+
return self.env["res.users"].sudo().search([]) # nosemgrep: odoo-sudo-without-context
|
| 295 |
+
|
| 296 |
+
def other(self):
|
| 297 |
+
return self.env["x"].sudo() # nosemgrep: semgrep.odoo-sudo-without-context -- reviewed
|
| 298 |
+
|
| 299 |
+
def third(self):
|
| 300 |
+
return 1 # nosemgrep
|
| 301 |
+
'''
|
| 302 |
+
|
| 303 |
+
|
| 304 |
+
def test_suppression_markers_are_read_with_their_rule_ids(tmp_path):
|
| 305 |
+
(tmp_path / "mod").mkdir()
|
| 306 |
+
(tmp_path / "mod" / "a.py").write_text(MARKED, encoding="utf-8")
|
| 307 |
+
marks = crosswalk.suppressions(str(tmp_path))
|
| 308 |
+
assert len(marks["odoo-sudo-without-context"]) == 2
|
| 309 |
+
assert marks["odoo-sudo-without-context"][0] == ("mod/a.py", 6)
|
| 310 |
+
|
| 311 |
+
|
| 312 |
+
def test_a_marker_naming_nothing_is_not_counted_as_a_rule(tmp_path):
|
| 313 |
+
"""`# nosemgrep` on its own says a person silenced something and not which
|
| 314 |
+
finding. There is nothing there to compare with anything."""
|
| 315 |
+
(tmp_path / "a.py").write_text(MARKED, encoding="utf-8")
|
| 316 |
+
marks = crosswalk.suppressions(str(tmp_path))
|
| 317 |
+
assert len(marks["(unnamed)"]) == 1
|
| 318 |
+
assert not [rid for rid in marks if rid.startswith("semgrep.")]
|
| 319 |
+
|
| 320 |
+
|
| 321 |
+
@pytest.mark.parametrize("comment, wanted", [
|
| 322 |
+
("odoo-sudo-without-context", "odoo-sudo-without-context"),
|
| 323 |
+
("semgrep.odoo-sudo-without-context", "odoo-sudo-without-context"),
|
| 324 |
+
("odoo-sudo-without-context - policy lookup only", "odoo-sudo-without-context"),
|
| 325 |
+
("odoo-sudo-without-context.", "odoo-sudo-without-context"),
|
| 326 |
+
("not user input", ""),
|
| 327 |
+
("", ""),
|
| 328 |
+
])
|
| 329 |
+
def test_rule_ids_are_recovered_from_prose(comment, wanted):
|
| 330 |
+
assert crosswalk._clean_rule_id(comment) == wanted
|
| 331 |
+
|
| 332 |
+
|
| 333 |
+
def test_the_same_line_comparison_is_a_fact_about_code(tmp_path, all_rules):
|
| 334 |
+
"""The strongest claim the tool makes: same file, same line, two analysers.
|
| 335 |
+
It has to come from running our checkers over the real file, not from the
|
| 336 |
+
rule ids matching up."""
|
| 337 |
+
(tmp_path / "a.py").write_text(MARKED, encoding="utf-8")
|
| 338 |
+
marks = crosswalk.suppressions(str(tmp_path))
|
| 339 |
+
both, theirs_only, unread = crosswalk.echoed_on_the_same_line(
|
| 340 |
+
str(tmp_path), marks, all_rules, "odoo")
|
| 341 |
+
assert unread == 0
|
| 342 |
+
assert ("a.py", 6, "odoo-sudo-without-context",
|
| 343 |
+
["odoo.sudo-bypasses-record-rules"]) in both
|
| 344 |
+
assert theirs_only == 1, "line 9 is a bare .sudo(), which ours does not flag"
|
| 345 |
+
|
| 346 |
+
|
| 347 |
+
def test_files_past_the_read_limit_are_reported_not_dropped(tmp_path, all_rules):
|
| 348 |
+
for n in range(3):
|
| 349 |
+
(tmp_path / "f{0}.py".format(n)).write_text(MARKED, encoding="utf-8")
|
| 350 |
+
marks = crosswalk.suppressions(str(tmp_path))
|
| 351 |
+
_, _, unread = crosswalk.echoed_on_the_same_line(
|
| 352 |
+
str(tmp_path), marks, all_rules, "odoo", limit=1)
|
| 353 |
+
assert unread == 2
|
| 354 |
+
|
| 355 |
+
|
| 356 |
+
# ------------------------------------------------------------------- report
|
| 357 |
+
|
| 358 |
+
def test_the_report_states_what_it_did_not_measure(tmp_path, capsys):
|
| 359 |
+
path = ruleset(tmp_path, SIMPLE)
|
| 360 |
+
assert crosswalk.main([path]) == 0
|
| 361 |
+
out = capsys.readouterr().out
|
| 362 |
+
assert "WHAT THIS DOES NOT MEASURE" in out
|
| 363 |
+
assert "Their analyser was never run" in out
|
| 364 |
+
assert "Nothing here is a score" in out
|
| 365 |
+
|
| 366 |
+
|
| 367 |
+
def test_the_report_separates_no_answer_from_no_coverage(tmp_path, capsys):
|
| 368 |
+
body = SIMPLE + """
|
| 369 |
+
- id: their-unprobeable
|
| 370 |
+
patterns:
|
| 371 |
+
- pattern: $X ...:::
|
| 372 |
+
severity: INFO
|
| 373 |
+
languages: [python]
|
| 374 |
+
"""
|
| 375 |
+
crosswalk.main([ruleset(tmp_path, body)])
|
| 376 |
+
out = capsys.readouterr().out
|
| 377 |
+
assert "theirs fires, nothing here does" in out
|
| 378 |
+
assert "no probe could be built, so no answer" in out
|
| 379 |
+
assert "unmeasured, which is a different thing from uncovered" in out
|
| 380 |
+
|
| 381 |
+
|
| 382 |
+
def test_the_json_artifact_carries_the_rows(tmp_path):
|
| 383 |
+
path = ruleset(tmp_path, SIMPLE)
|
| 384 |
+
out = tmp_path / "crosswalk.json"
|
| 385 |
+
assert crosswalk.main([path, "--json", str(out)]) == 0
|
| 386 |
+
import json
|
| 387 |
+
payload = json.loads(io.open(str(out), encoding="utf-8").read())
|
| 388 |
+
assert payload["ruleset"] == "rules.yml"
|
| 389 |
+
assert len(payload["rows"]) == 3
|
| 390 |
+
assert payload["their_rules"][0]["id"] == "their-eval"
|
| 391 |
+
|
| 392 |
+
|
| 393 |
+
def test_a_checkout_that_is_not_there_is_a_bad_argument(tmp_path, capsys):
|
| 394 |
+
path = ruleset(tmp_path, SIMPLE)
|
| 395 |
+
assert crosswalk.main([path, "--checkout", os.path.join(str(tmp_path), "nope")]) == 2
|
| 396 |
+
assert "No such checkout" in capsys.readouterr().out
|
|
@@ -289,3 +289,26 @@ def test_a_missing_layer_still_reads_as_one(odoo_rules):
|
|
| 289 |
unknown = [layers.LayerResult("domain", layers.UNKNOWN, [], [], [])]
|
| 290 |
decision = layers.decide(unknown)
|
| 291 |
assert decision.insufficient == ["the domain layer could not run"]
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 289 |
unknown = [layers.LayerResult("domain", layers.UNKNOWN, [], [], [])]
|
| 290 |
decision = layers.decide(unknown)
|
| 291 |
assert decision.insufficient == ["the domain layer could not run"]
|
| 292 |
+
|
| 293 |
+
|
| 294 |
+
def test_bandit_marker_is_read_like_the_others():
|
| 295 |
+
"""`# nosec B608` is bandit's, and the argument for reading it is the one
|
| 296 |
+
that put nosemgrep on the list: an independent analyser raised something
|
| 297 |
+
and a person decided to keep the code.
|
| 298 |
+
|
| 299 |
+
Measured while adding odoo.sql-needs-query-parameters: two of its six
|
| 300 |
+
findings on OpenSPP sat on lines carrying nothing but `# nosec B608`, and
|
| 301 |
+
were being re-raised as news on every run.
|
| 302 |
+
"""
|
| 303 |
+
import corpus
|
| 304 |
+
rules = [r for r in corpus.load_rules()
|
| 305 |
+
if r["id"] == "odoo.sql-needs-query-parameters"]
|
| 306 |
+
body = (
|
| 307 |
+
"def run(self, table):\n"
|
| 308 |
+
" self.env.cr.execute(\n"
|
| 309 |
+
" f\"SELECT COUNT(*) FROM {table}\", # nosec B608\n"
|
| 310 |
+
" )\n"
|
| 311 |
+
)
|
| 312 |
+
hits = layers.scan_source(body, rules, "odoo", errors=[])
|
| 313 |
+
assert len(hits) == 1
|
| 314 |
+
assert hits[0].suppressed, "a bandit marker is somebody's decision too"
|
|
@@ -208,10 +208,11 @@ def test_the_report_states_what_it_did_not_measure(repo, capsys, rules):
|
|
| 208 |
|
| 209 |
# -------------------------------------------------------------- the harvest
|
| 210 |
|
| 211 |
-
def missed_row(sha, subject, tokens, caught=()):
|
| 212 |
return {"repo": "r", "sha": sha, "subject": subject, "files": 1,
|
| 213 |
"scope_tokens": list(tokens), "in_scope": True,
|
| 214 |
-
"caught": list(caught), "vanished_elsewhere": []
|
|
|
|
| 215 |
|
| 216 |
|
| 217 |
def test_harvest_clusters_the_misses_by_construct(capsys):
|
|
@@ -226,7 +227,7 @@ def test_harvest_clusters_the_misses_by_construct(capsys):
|
|
| 226 |
missed_row("a3", "fix(z): wrong company", ["check_company"])]
|
| 227 |
recall.harvest(rows, "odoo")
|
| 228 |
out = capsys.readouterr().out
|
| 229 |
-
assert "3 fix commit(s) the corpus
|
| 230 |
assert "2x api.depends" in out
|
| 231 |
assert "1x check_company" in out
|
| 232 |
assert out.index("api.depends") < out.index("check_company"), "ranked by count"
|
|
@@ -243,8 +244,10 @@ def test_harvest_says_where_the_corpus_does_work(capsys):
|
|
| 243 |
"caught": ["odoo.sudo-bypasses-record-rules"], "vanished_elsewhere": []}]
|
| 244 |
recall.harvest(rows, "odoo")
|
| 245 |
out = capsys.readouterr().out
|
| 246 |
-
|
| 247 |
-
|
|
|
|
|
|
|
| 248 |
|
| 249 |
|
| 250 |
def test_harvest_drops_the_module_names_and_the_process_words(capsys):
|
|
@@ -269,3 +272,88 @@ def test_harvest_says_so_when_nothing_was_missed(capsys):
|
|
| 269 |
"caught": ["odoo.sudo-bypasses-record-rules"], "vanished_elsewhere": []}]
|
| 270 |
assert recall.harvest(rows, "odoo") == 0
|
| 271 |
assert "Nothing missed in scope" in capsys.readouterr().out
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 208 |
|
| 209 |
# -------------------------------------------------------------- the harvest
|
| 210 |
|
| 211 |
+
def missed_row(sha, subject, tokens, caught=(), spoke=()):
|
| 212 |
return {"repo": "r", "sha": sha, "subject": subject, "files": 1,
|
| 213 |
"scope_tokens": list(tokens), "in_scope": True,
|
| 214 |
+
"caught": list(caught), "vanished_elsewhere": [],
|
| 215 |
+
"spoke": list(spoke)}
|
| 216 |
|
| 217 |
|
| 218 |
def test_harvest_clusters_the_misses_by_construct(capsys):
|
|
|
|
| 227 |
missed_row("a3", "fix(z): wrong company", ["check_company"])]
|
| 228 |
recall.harvest(rows, "odoo")
|
| 229 |
out = capsys.readouterr().out
|
| 230 |
+
assert "3 fix commit(s) the corpus did not flag on the line of the fix" in out
|
| 231 |
assert "2x api.depends" in out
|
| 232 |
assert "1x check_company" in out
|
| 233 |
assert out.index("api.depends") < out.index("check_company"), "ranked by count"
|
|
|
|
| 244 |
"caught": ["odoo.sudo-bypasses-record-rules"], "vanished_elsewhere": []}]
|
| 245 |
recall.harvest(rows, "odoo")
|
| 246 |
out = capsys.readouterr().out
|
| 247 |
+
sudo = [row for row in out.splitlines() if ".sudo(" in row][0]
|
| 248 |
+
stale = [row for row in out.splitlines() if "api.depends" in row][0]
|
| 249 |
+
assert "1 caught" in sudo
|
| 250 |
+
assert "0 caught" in stale
|
| 251 |
|
| 252 |
|
| 253 |
def test_harvest_drops_the_module_names_and_the_process_words(capsys):
|
|
|
|
| 272 |
"caught": ["odoo.sudo-bypasses-record-rules"], "vanished_elsewhere": []}]
|
| 273 |
assert recall.harvest(rows, "odoo") == 0
|
| 274 |
assert "Nothing missed in scope" in capsys.readouterr().out
|
| 275 |
+
|
| 276 |
+
|
| 277 |
+
# ------------------------------------------------- silent, or aimed elsewhere
|
| 278 |
+
|
| 279 |
+
# A stored compute reading a field its @api.depends does not declare, in a file
|
| 280 |
+
# that ALSO has a dangling Many2one read. depends_covers_compute_reads fires on
|
| 281 |
+
# the compute; ondelete_leaves_a_dangling_read fires on the relation. A fix to
|
| 282 |
+
# one leaves the other in place.
|
| 283 |
+
TWO_DEFECTS = '''from odoo import api, fields, models
|
| 284 |
+
|
| 285 |
+
|
| 286 |
+
class Thing(models.Model):
|
| 287 |
+
_name = "my.thing"
|
| 288 |
+
|
| 289 |
+
task_id = fields.Many2one("my.task")
|
| 290 |
+
total = fields.Float(compute="_compute_total", store=True)
|
| 291 |
+
|
| 292 |
+
@api.depends("line_ids")
|
| 293 |
+
def _compute_total(self):
|
| 294 |
+
for rec in self:
|
| 295 |
+
rec.total = sum(line.price for line in rec.line_ids)
|
| 296 |
+
|
| 297 |
+
def _label(self):
|
| 298 |
+
for rec in self:
|
| 299 |
+
rec.name = rec.task_id.filepath
|
| 300 |
+
'''
|
| 301 |
+
|
| 302 |
+
# The Many2one gets its ondelete. The incomplete depends is untouched, so the
|
| 303 |
+
# corpus was looking straight at this file and pointing at the other defect.
|
| 304 |
+
ONE_FIXED = TWO_DEFECTS.replace('fields.Many2one("my.task")',
|
| 305 |
+
'fields.Many2one("my.task", ondelete="restrict")')
|
| 306 |
+
|
| 307 |
+
|
| 308 |
+
def test_a_finding_the_fix_ignored_is_recorded_as_spoken(repo, rules):
|
| 309 |
+
"""Not a catch, and not blindness either. The corpus had a finding in this
|
| 310 |
+
file before the fix and still has it after, which is the corpus being aimed
|
| 311 |
+
at a different line rather than having nothing to say."""
|
| 312 |
+
commit(repo, "models/a.py", TWO_DEFECTS, "[ADD] a")
|
| 313 |
+
commit(repo, "models/a.py", ONE_FIXED, "[FIX] ondelete on task_id")
|
| 314 |
+
rows = recall.measure(str(repo), "odoo", 0, rules)
|
| 315 |
+
assert rows[0]["caught"] == ["odoo.ondelete-is-a-decision"]
|
| 316 |
+
assert "odoo.stored-compute-needs-complete-depends" in rows[0]["spoke"]
|
| 317 |
+
|
| 318 |
+
|
| 319 |
+
def test_a_catch_is_not_also_counted_as_spoken(repo, rules):
|
| 320 |
+
"""`spoke` is what the fix did NOT address. A rule in both columns would
|
| 321 |
+
make the split useless for deciding which repair a construct needs."""
|
| 322 |
+
commit(repo, "models/a.py", DANGLING, "[ADD] a")
|
| 323 |
+
commit(repo, "models/a.py", FIXED, "[FIX] ondelete on task_id")
|
| 324 |
+
rows = recall.measure(str(repo), "odoo", 0, rules)
|
| 325 |
+
assert rows[0]["caught"] == ["odoo.ondelete-is-a-decision"]
|
| 326 |
+
assert rows[0]["spoke"] == []
|
| 327 |
+
|
| 328 |
+
|
| 329 |
+
def test_a_clean_file_leaves_spoke_empty(repo, rules):
|
| 330 |
+
commit(repo, "models/a.py", "VALUE = 1\n", "[ADD] a")
|
| 331 |
+
commit(repo, "models/a.py", "VALUE = 2\n# api.depends\n", "[FIX] the value")
|
| 332 |
+
rows = recall.measure(str(repo), "odoo", 0, rules)
|
| 333 |
+
assert rows[0]["spoke"] == []
|
| 334 |
+
|
| 335 |
+
|
| 336 |
+
def test_harvest_splits_the_misses_into_silent_and_aimed_elsewhere(capsys):
|
| 337 |
+
"""Twenty-three missed fixes against @api.depends reads as a missing rule.
|
| 338 |
+
Nine of them having a checker fire elsewhere in the file reads as a rule
|
| 339 |
+
that exists and is pointed the wrong way, and those want opposite repairs.
|
| 340 |
+
"""
|
| 341 |
+
rows = [missed_row("a1", "fix(x): stale total", ["api.depends"],
|
| 342 |
+
spoke=["odoo.stored-compute-needs-complete-depends"]),
|
| 343 |
+
missed_row("a2", "fix(y): stale price", ["api.depends"]),
|
| 344 |
+
missed_row("a3", "fix(z): wrong company", ["check_company"])]
|
| 345 |
+
recall.harvest(rows, "odoo")
|
| 346 |
+
out = capsys.readouterr().out
|
| 347 |
+
assert "2x api.depends" in out
|
| 348 |
+
assert "1 spoke" in out
|
| 349 |
+
assert "1 silent" in out
|
| 350 |
+
assert "spoke: odoo.stored-compute-needs-complete-depends" in out
|
| 351 |
+
|
| 352 |
+
|
| 353 |
+
def test_the_report_says_how_many_misses_were_not_blind_spots(capsys):
|
| 354 |
+
rows = [missed_row("a1", "fix(x): stale total", ["api.depends"],
|
| 355 |
+
spoke=["odoo.stored-compute-needs-complete-depends"]),
|
| 356 |
+
missed_row("a2", "fix(y): stale price", ["api.depends"])]
|
| 357 |
+
recall.report(rows, "odoo", ["r"], 25)
|
| 358 |
+
out = capsys.readouterr().out
|
| 359 |
+
assert "1 of them are not blind spots" in out
|