mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
fix(cpg): ask the graph before splitting it, not the frontend's log
Sharding was triggered by `Failed to process` warnings. Those warnings are not
evidence about the finished graph:
a build reporting 2 drops -> 3 files, 139 methods, same size as a clean one
The file it warned about was in the graph the whole time. Acting on the
warning is not merely wasteful, it is destructive -- shards cannot see each
other's flows, so splitting a two-file WordPress pair on a false alarm cost it
CVE-2022-25148 outright. That is how this was found: the slices caught it, at
a scale small enough to check by hand, before the whole-repository run could
bury it.
So the build now takes a census of the graph (`census.sc`) when the frontend
warns, and splits only if the graph is actually short of the files handed to
it. A warning is a symptom to investigate, never a verdict. Conservative in
the only direction that matters: if the census cannot be taken, the caller
does the careful thing and splits.
wp-vuln 3 entailed incl. CVE at :225, NO shard (was: 2, sharded)
wp-fixed 2, that one absent
mwwpform 1 / 0
pmpro one-file 18 incl. CVE at :936
pmpro slice 18 incl. CVE at :936, sharded -- genuinely deficient here
The census costs one JVM, and only on a build that warned; a clean build pays
nothing.
849 tests pass; gates byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG
This commit is contained in:
co-authored by
Claude Opus 5
parent
7f83977add
commit
d63be34451
@@ -78,6 +78,8 @@ _FRONTENDS = {
|
||||
# right there) and try again. Compacting the parser's output to a fifth of its size, running the parser
|
||||
# through a shim, and pinning `ForkJoinPool.common.parallelism=1` were each tried and each changed nothing.
|
||||
_PREFER_FRONTEND = frozenset({"php"})
|
||||
# The extensions that count as source for a language, used to ask whether a graph is missing anything.
|
||||
_LANGUAGE_EXTENSIONS: dict[str, tuple[str, ...]] = {"php": (".php",)}
|
||||
FRONTEND_BUILD_ATTEMPTS = 4
|
||||
|
||||
# Request fields that describe the REPOSITORY rather than the region, and are therefore identical in every
|
||||
@@ -254,6 +256,15 @@ class JoernBackend:
|
||||
if not dropped:
|
||||
return (main,), ()
|
||||
|
||||
# A warning is a symptom, not a verdict. `php2cpg` logs `Failed to process` for files that are
|
||||
# nonetheless in the finished graph -- measured: a build reporting two drops produced a graph holding
|
||||
# both of its files and all 139 methods, the same size as a clean one. Splitting on that evidence is
|
||||
# actively harmful, because shards cannot see each other's flows: it cost a two-file WordPress pair
|
||||
# its CVE. So ask the graph before doing anything drastic to it.
|
||||
if self._graph_is_complete(main, root, language):
|
||||
logger.info("the frontend warned about %d file(s) under %s but the graph holds them all", len(dropped), root)
|
||||
return (main,), ()
|
||||
|
||||
# Second pass over the same tree, this time telling the frontend to leave the difficult files alone,
|
||||
# so the rest of the repository is analysed instead of nothing being analysed.
|
||||
remainder = self._build_with_retries(root, main, scratch, language, exclude=dropped)
|
||||
@@ -271,6 +282,27 @@ class JoernBackend:
|
||||
return tuple(shards), tuple(still)
|
||||
return tuple(shards), dropped
|
||||
|
||||
def _graph_is_complete(self, cpg_path: Path, root: Path, language: str) -> bool:
|
||||
"""Does this graph hold at least as many files as the tree has sources?
|
||||
|
||||
Conservative in the only direction that matters: when the census cannot be taken, or the language
|
||||
has no declared extensions, the answer is "no" and the caller does the careful thing.
|
||||
"""
|
||||
extensions = _LANGUAGE_EXTENSIONS.get(language.lower())
|
||||
if not extensions:
|
||||
return False
|
||||
expected = sum(1 for path in root.rglob("*") if path.suffix.lower() in extensions and path.is_file())
|
||||
if not expected:
|
||||
return False
|
||||
payload = self.query(cpg_path, "census", {})
|
||||
if not isinstance(payload, Mapping):
|
||||
return False
|
||||
try:
|
||||
files = int(str(payload.get("files", "0")))
|
||||
except ValueError:
|
||||
return False
|
||||
return files >= expected
|
||||
|
||||
def _island_root(self, root: Path, scratch: Path, files: Sequence[str]) -> Path | None:
|
||||
"""A tree holding only ``files``, at their original relative paths.
|
||||
|
||||
|
||||
@@ -0,0 +1,18 @@
|
||||
// How much of the repository actually made it into the graph (contributor-scan 5.10).
|
||||
//
|
||||
// A frontend can log `Failed to process` for a file that is nonetheless present -- measured: a build
|
||||
// reporting two drops produced a graph holding both of its files and all 139 methods, byte-identical in
|
||||
// size to a clean one. So the warning is a symptom to investigate, never a verdict, and the only thing that
|
||||
// settles whether a graph is deficient is the graph.
|
||||
//
|
||||
// This matters because acting on the warning alone is destructive: it makes the driver split a repository
|
||||
// into shards that cannot see each other's flows, and a two-file WordPress pair lost its CVE that way.
|
||||
//
|
||||
// Parameters: cpgFile.
|
||||
// Output: fenced JSON {files, methods}.
|
||||
@main def exec(cpgFile: String) = {
|
||||
importCpg(cpgFile)
|
||||
println("---OUSAST-CPG-BEGIN---")
|
||||
println(ujson.write(ujson.Obj("files" -> cpg.file.size.toString, "methods" -> cpg.method.size.toString)))
|
||||
println("---OUSAST-CPG-END---")
|
||||
}
|
||||
@@ -456,6 +456,11 @@ def test_files_the_frontend_refuses_get_a_cpg_of_their_own(tmp_path: Path, monke
|
||||
|
||||
def runner(command, **kwargs): # type: ignore[no-untyped-def]
|
||||
commands.append(list(command))
|
||||
if "--script" in command:
|
||||
# The census, asked because the frontend warned. Here the warning is real: the graph is short a
|
||||
# file, so the caller should go on to exclude it and build it separately.
|
||||
body = '---OUSAST-CPG-BEGIN---\n{"files": "1", "methods": "1"}\n---OUSAST-CPG-END---\n'
|
||||
return subprocess.CompletedProcess(args=command, returncode=0, stdout=body, stderr="")
|
||||
Path(command[command.index("-o") + 1]).write_text("cpg")
|
||||
# The whole tree always drops big.php; any build that excludes it, or covers it alone, is clean.
|
||||
excluded = "--exclude" in command
|
||||
@@ -510,3 +515,37 @@ def test_a_shard_that_cannot_answer_does_not_lose_the_others(monkeypatch: pytest
|
||||
|
||||
monkeypatch.setattr(JoernBackend, "query_batch", lambda self, path, query, requests: None)
|
||||
assert backend.query_batch_across([Path("a.bin")], "taint", {}) is None, "no shard answered is not an empty answer"
|
||||
|
||||
|
||||
def test_a_warning_about_a_file_the_graph_actually_holds_does_not_split_it(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""`php2cpg` logs `Failed to process` for files that are nonetheless in the finished graph. Measured: a
|
||||
build reporting two drops produced a graph holding both its files and all 139 methods, the same size as
|
||||
a clean one.
|
||||
|
||||
Acting on the warning alone is destructive rather than merely wasteful -- shards cannot see each other's
|
||||
flows, and a two-file WordPress pair lost its CVE that way. So the graph is asked before it is split.
|
||||
"""
|
||||
import subprocess
|
||||
|
||||
from openultrasast.cpg.backend import JoernBackend
|
||||
|
||||
(tmp_path / "a.php").write_text("<?php function a() {}")
|
||||
(tmp_path / "b.php").write_text("<?php function b() {}")
|
||||
commands: list[list[str]] = []
|
||||
|
||||
def runner(command, **kwargs): # type: ignore[no-untyped-def]
|
||||
commands.append(list(command))
|
||||
if "--script" in command:
|
||||
body = '---OUSAST-CPG-BEGIN---\n{"files": "2", "methods": "9"}\n---OUSAST-CPG-END---\n'
|
||||
return subprocess.CompletedProcess(args=command, returncode=0, stdout=body, stderr="")
|
||||
Path(command[command.index("-o") + 1]).write_text("cpg")
|
||||
return subprocess.CompletedProcess(args=command, returncode=0, stdout="", stderr=f"WARN Failed to process '{tmp_path / 'a.php'}'\n")
|
||||
|
||||
monkeypatch.setenv("OPENULTRASAST_JOERN_PROBE", "on")
|
||||
monkeypatch.setattr("shutil.which", lambda name: f"/opt/joern/{name}")
|
||||
|
||||
result = JoernBackend(runner=runner).build(tmp_path, language="php")
|
||||
|
||||
assert result is not None
|
||||
assert result.unparsed == (), "the graph holds both files, so nothing is missing"
|
||||
assert not any("--exclude" in command for command in commands), "and it was not split"
|
||||
|
||||
Reference in New Issue
Block a user