feat(access-control): model the collection read, by argument shape rather than by name

NodeGoat's documented insecure direct object reference reads with
allocationsCol.find(searchCriteria()), and `find` was absent from an operation table holding
findOne, findById, findAll and findByPk. The obligation was missing at the commonest read there
is, so that defect raised nothing at any point in the pipeline.

It was absent for a good reason, and the reason is the argument, not the name. Measured on
wp-statistics, a WordPress plugin that ships jQuery: its assets hold 88 calls named find, of
which 70 pass a jQuery selector, 13 pass a predicate to Array.prototype.find and 5 pass a
selector built at run time by a template literal or a concatenation. None is a collection read.

So the obligation is claimed only where the first argument has the shape of a query document.
The operation fact states which argument with query_document_arg, and the arbiter that can see
argument shapes decides it: no arguments at all, an object literal, or a builder call are
claimed; a method reference, a literal, an operator call and a bare identifier are not. Each
exclusion is one of the measured classes above rather than a guess.

Zero of those 88 calls became an obligated operation, and the zero is measured rather than
empty: the same run's census reports 60 files and 4,552 methods, and the files holding those
calls are in it.

On NodeGoat all three find calls are collection reads and all three are claimed, which raises
the obligation at four more handlers -- the allocations handler that takes userId from req.params
on an authenticated route, the benefits pages whose isAdmin guard is commented out in the route
table, and the memo listing. Seven of fifteen handlers now reach a verdict, all corroborated,
none reported without a judge, and the census is unchanged on both subjects.

A bare identifier is deliberately not claimed: col.find(query) and arr.find(pred) are the same
shape and neither repository measured holds one, so this is a stated false negative rather than
an unmeasured false-positive risk.

1,415 tests pass, sixteen skipped; twelve Joern-gated query tests pass in the image.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo
This commit is contained in:
Marius Ciepluch
2026-09-20 12:10:27 +02:00
co-authored by Claude Opus 5
parent 102a21fc4e
commit c0427e6ceb
10 changed files with 322 additions and 5 deletions
+40 -2
View File
@@ -42,6 +42,7 @@
file: String = "",
operationRequires: String = "",
dischargersByKind: String = "",
documentShape: String = "",
requests: String = "",
requestsFile: String = "",
contextEvidence: String = ""
@@ -68,6 +69,7 @@
fileS: String,
requiresS: String,
byKindS: String,
shapeS: String,
contextS: String = ""
): List[ujson.Obj] = {
@@ -122,6 +124,38 @@
// first repository measured came from exactly that, including three handlers that do call an
// authorization guard. A qualified token means the qualification matters, so the receiver must appear
// too; the desugared chain still carries it.
// Calls whose obligation depends on the ARGUMENT rather than the name. `find` is the commonest collection
// read there is, and also `Array.prototype.find` and jQuery's `.find`, so the name alone cannot carry it.
//
// Measured on one WordPress plugin's shipped assets, where 88 calls are named `find`:
//
// 70 a jQuery selector `this.notice.find('.processed')` LITERAL
// 13 an array predicate `datasets.find(ds => ds.label === x)` METHOD_REF
// 5 a selector built at run time ``el.find(`option[value="${v}"]`)`` CALL, an operator
// 0 a collection read
//
// and on NodeGoat, where all three `find` calls are collection reads: two object literals and one builder
// call. So the shapes separate cleanly, and each exclusion is a measured class rather than a guess:
// a method reference is a predicate, a literal is a selector, and an operator call is a string being built.
// A bare IDENTIFIER is deliberately NOT claimed -- `col.find(query)` and `arr.find(pred)` are the same shape
// and neither repository measured has one, so it is a stated false negative rather than an unmeasured risk.
val shapeNames = split(shapeS)
def isQueryDocument(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean = {
val real = c.argument.argumentIndexGt(0).l
real.headOption match {
case None => true // `col.find()` reads the whole collection, which is the obligation at its widest
case Some(node) =>
node.label match {
case "METHOD_REF" => false
case "LITERAL" => false
case "IDENTIFIER" => false
case "CALL" => !node.asInstanceOf[io.shiftleft.codepropertygraph.generated.nodes.Call].name.startsWith("<operator")
case _ => true // an object literal, which jssrc2cpg gives as a BLOCK
}
}
}
def matchesOperation(name: String, code: String): Boolean =
opNames.exists { n =>
if (name == n) true
@@ -131,11 +165,14 @@
}
}
def isOperation(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean =
matchesOperation(c.name, c.code) && (!shapeNames.contains(c.name) || isQueryDocument(c))
// Scoped to the region's own file. `delete_user` exists in both the API handler and the model of the
// repository this was measured on, and unscoped rows attributed the model's unguarded query to the
// handler. The sibling comparison is a statement about a FILE contradicting itself, which is what the
// rung's justification says, so the file is also the right scope for it.
val allOperations = cpg.call.filter(c => matchesOperation(c.name, c.code)).l
val allOperations = cpg.call.filter(isOperation).l
val opCalls = if (fileS.isEmpty) allOperations else allOperations.filter(_.method.filename.endsWith(fileS))
// ---- The module boundary an application puts between its route and its query --------------------------
@@ -310,6 +347,7 @@
field("file"),
field("operationRequires"),
field("dischargersByKind"),
if (field("documentShape").nonEmpty) field("documentShape") else documentShape,
field("contextEvidence")
): _*
)
@@ -322,7 +360,7 @@
val census = ujson.Arr(ujson.Obj("methods" -> cpg.method.size.toString, "files" -> cpg.file.size.toString, "file_names" -> ujson.Arr.from(cpg.file.name.l)))
println(ujson.write(ujson.Obj.from(answers.toSeq :+ ("__census__" -> census))))
} else {
println(ujson.write(ujson.Arr(rowsFor(operations, dischargers, function, file, operationRequires, dischargersByKind, contextEvidence): _*)))
println(ujson.write(ujson.Arr(rowsFor(operations, dischargers, function, file, operationRequires, dischargersByKind, documentShape, contextEvidence): _*)))
}
println("---OUSAST-CPG-END---")
}
+2
View File
@@ -40,6 +40,8 @@ def request_params(spec: DominanceSpec, *, function: str = "", file: str = "") -
"dischargers": spec.dischargers,
"function": function,
"file": file,
# The calls whose obligation depends on the argument's shape rather than on the name alone.
"documentShape": ",".join(spec.document_shape),
"operationRequires": json.dumps({k: list(v) for k, v in spec.requirements.items()}, sort_keys=True) if spec.requirements else "",
"dischargersByKind": json.dumps({k: list(v) for k, v in spec.dischargers_by_kind.items()}, sort_keys=True)
if spec.dischargers_by_kind
+5
View File
@@ -154,6 +154,10 @@ class DominanceSpec:
dischargers: tuple[str, ...]
requirements: Mapping[str, tuple[str, ...]] = field(default_factory=dict)
dischargers_by_kind: Mapping[str, tuple[str, ...]] = field(default_factory=dict)
# Operations whose obligation depends on the ARGUMENT, not only the name. `find` is a collection read, an
# array search and a jQuery selector, and only the first is obligated; the facts say which argument has to
# look like a query document and the query decides whether it does.
document_shape: tuple[str, ...] = ()
@dataclass(frozen=True)
@@ -318,6 +322,7 @@ def dominance_specs(*, language: str, facts: ObligationFacts | None = None) -> M
dischargers=dischargers,
requirements=requirements,
dischargers_by_kind=by_kind,
document_shape=tuple(sorted({call for fact in guarded if fact.query_document_arg for call in fact.calls})),
)
}
@@ -9,6 +9,23 @@ calls = ["findOne", "findById", "findMany", "findFirst", "findUnique", "findAll"
requires = ["identity_constraint", "ownership_check"]
sensitivity = "high"
[[operation]]
# CWE-639's commonest read, and the one the family was missing. NodeGoat's insecure direct object reference
# reads with `allocationsCol.find(searchCriteria())`, and `find` was absent from a table holding `findOne`,
# `findById` and `findAll` -- the obligation missing at exactly the shape it exists for.
#
# It was absent for a reason, and the reason is the argument, not the name. Measured on one WordPress plugin's
# shipped assets, 88 calls are named `find`: 70 pass a jQuery selector, 13 pass a predicate to
# `Array.prototype.find`, and 5 pass a template literal or a concatenation, which is a selector built at
# runtime. None is a collection read. So this operation is claimed only where the first argument has the shape
# of a query document, which `query_document_arg` says and the arbiter decides.
id = "collection-read"
kind = "protected_read"
calls = ["find"]
query_document_arg = 1
requires = ["identity_constraint", "ownership_check"]
sensitivity = "high"
[[operation]]
id = "orm-write"
kind = "protected_write"
@@ -9,6 +9,23 @@ calls = ["findOne", "findById", "findMany", "findFirst", "findUnique", "findAll"
requires = ["identity_constraint", "ownership_check"]
sensitivity = "high"
[[operation]]
# CWE-639's commonest read, and the one the family was missing. NodeGoat's insecure direct object reference
# reads with `allocationsCol.find(searchCriteria())`, and `find` was absent from a table holding `findOne`,
# `findById` and `findAll` -- the obligation missing at exactly the shape it exists for.
#
# It was absent for a reason, and the reason is the argument, not the name. Measured on one WordPress plugin's
# shipped assets, 88 calls are named `find`: 70 pass a jQuery selector, 13 pass a predicate to
# `Array.prototype.find`, and 5 pass a template literal or a concatenation, which is a selector built at
# runtime. None is a collection read. So this operation is claimed only where the first argument has the shape
# of a query document, which `query_document_arg` says and the arbiter decides.
id = "collection-read"
kind = "protected_read"
calls = ["find"]
query_document_arg = 1
requires = ["identity_constraint", "ownership_check"]
sensitivity = "high"
[[operation]]
id = "orm-write"
kind = "protected_write"
@@ -18,7 +18,7 @@ SENSITIVITIES = ("low", "medium", "high")
DEFAULT_OBLIGATION_FACTS_DIR = Path(__file__).resolve().parents[2] / "ruleset" / "obligations"
_LANGUAGE_ALIASES = {"c_cpp": "c", "cpp": "c", "python_web": "python", "node": "javascript", "ts": "typescript"}
_OPERATION_FIELDS = frozenset({"id", "kind", "calls", "resource_arg", "requires", "sensitivity"})
_OPERATION_FIELDS = frozenset({"id", "kind", "calls", "resource_arg", "requires", "sensitivity", "query_document_arg"})
_DISCHARGER_FIELDS = frozenset(
{"id", "kind", "calls", "decorators", "identity_sources", "constraint_params", "request_sources", "permissive_values"}
)
@@ -37,6 +37,12 @@ class OperationFact:
resource_arg: int | None # argument or receiver position naming the resource (model, table, path); None = receiver
requires: tuple[str, ...] # DISCHARGER_KINDS that discharge this operation
sensitivity: str # SENSITIVITIES
# A call whose NAME is shared with something ordinary, obligated only in the argument shape of a data
# operation. `find` is the commonest collection read there is and also `Array.prototype.find` and jQuery's
# `.find`, so the name alone cannot carry the obligation: measured on one plugin's assets, 83 of 88 `find`
# calls were a selector or a predicate. The position is 1-based over the real arguments; the arbiter that
# can see argument shapes decides it, and a language whose calls are unambiguous never sets this.
query_document_arg: int | None = None
@dataclass(frozen=True)
@@ -96,6 +102,9 @@ def load_obligation_facts(directory: Path | None = None) -> ObligationFacts:
resource_arg = row.get("resource_arg")
if resource_arg is not None and not isinstance(resource_arg, int):
raise ObligationFactsError(f"{path}: operation {row.get('id')!r} resource_arg must be an integer")
document_arg = row.get("query_document_arg")
if document_arg is not None and (not isinstance(document_arg, int) or document_arg < 1):
raise ObligationFactsError(f"{path}: operation {row.get('id')!r} query_document_arg must be a positive integer")
operations.append(
OperationFact(
id=str(row["id"]),
@@ -105,6 +114,7 @@ def load_obligation_facts(directory: Path | None = None) -> ObligationFacts:
resource_arg=resource_arg,
requires=requires,
sensitivity=sensitivity,
query_document_arg=document_arg,
)
)
for row in _rows(payload.get("discharger"), path):