From 102a21fc4ea2b9a546b1e95aa22a89afd8897342 Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Sun, 20 Sep 2026 08:57:32 +0200 Subject: [PATCH] feat(access-control): carry the obligation across the route-and-DAO split The census measured this family completing 35 questions on NodeGoat with no obligation raised anywhere: every modelled data operation lives in app/data/*-dao.js, none in a route handler file, and operations were scoped to the region's own file. The family had nothing to be right or wrong about. The obligation now follows the one named call the handler writes, out of the region's file and no further. How it is followed is the point. jssrc2cpg does not resolve allocationsDAO.getByUserIdAndThreshold(...) to the DAO body: the callee is an external stub with no code. That stub's fullName records where the member was written, so the implementation is reached by the graph's own provenance rather than by a repository-wide name guess, which is the false positive the file scope was closed against. A control asserts it: a third module defining a member of the same name that nothing imports is not attributed. A row is attributed to the region's DECLARED function, which is what lets a question about displayAllocations match a graph that holds a . A carried row is also checked for a guard at the CALL, because that is where a handler establishes who is asking. The method holding a carried operation joins the question's context scope, so the module holding the query cannot change under a claim that depends on it without the change being attributed. The witness names both sites and the finding is located at the call, which is the line a contributor changed. A carried obligation corroborates, it does not entail, and that was measured rather than assumed. Entailing it reported handleSignup, which looks a user up by name to check the name is free during registration and is public by design, because two authenticated siblings in the same file pass req.session.userId into the same module. The entailed rung's justification is that one FILE contradicts itself, same file and same trust context; carried, the compared operations live in a data-access module that handlers of different trust contexts share. The corroborated band already asks the right residual: does this operation require a check, or is the route legitimately public? Measured on NodeGoat: 15 handlers asked, obligations raised at zero before and six now, three reaching a verdict, all corroborated, none reported without a judge. VAmPI unchanged at two entailed findings, because it keeps handler and query in one file and carries nothing. What this does not reach is recorded, not implied. NodeGoat's documented insecure direct object reference still raises nothing, because its module reads with allocationsCol.find(...) and find is not a modelled operation: Array.prototype.find and jQuery's .find are everywhere, so it needs an argument-shape discriminator and its own measurement (task 14.8). Two of the three corroborated claims are correct code whose identity read a destructuring spelled away, and the cross-file contradiction that would entail the real defect needs a sibling-scope decision (task 14.7). 1,415 tests pass, fifteen skipped; eleven Joern-gated query tests pass in the image. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo --- .kiro/specs/pre-push-safety-net/status.md | 13 +- .kiro/specs/pre-push-safety-net/tasks.md | 21 ++- ...026-09-20-access-control-module-split.json | 175 ++++++++++++++++++ src/openultrasast/cpg/queries/dominance.sc | 114 +++++++++++- src/openultrasast/model/dominance.py | 55 +++++- tests/test_cpg_queries_compile.py | 140 ++++++++++++++ 6 files changed, 502 insertions(+), 16 deletions(-) create mode 100644 benchmarks/measurements/2026-09-20-access-control-module-split.json diff --git a/.kiro/specs/pre-push-safety-net/status.md b/.kiro/specs/pre-push-safety-net/status.md index e3adb7b..ade90a9 100644 --- a/.kiro/specs/pre-push-safety-net/status.md +++ b/.kiro/specs/pre-push-safety-net/status.md @@ -157,7 +157,15 @@ surface a Node server actually calls, and NodeGoat's SSRF is reported at `app/routes/research.js:16` with a witness naming the call. Output encoding now asks about the server half of its own family and establishes a finding at `app/routes/research.js:25`, and it states in its declaration that template-mediated escaping is not something it can decide. Five -of seven families now establish findings; access control remains open as task 14.4. +of seven families now establish findings. + +Access control was the third gap and its structural half is closed. The obligation now follows +the one named call a handler writes into its data-access module, attributed by the graph's own +provenance rather than by name, and located at the call a contributor changed. On NodeGoat it is +raised at six handlers where it was raised nowhere, and three reach a verdict. All three are +corroborated and none is reported without a judge, which is the honest rung: entailing a carried +obligation reported a signup handler that is public by design, because its authenticated +siblings share the same data module. VAmPI is unchanged. Closing the second gap surfaced four defects, three of which were false positives it would otherwise have shipped: a permissive-value set shared across settings whose booleans have @@ -169,4 +177,5 @@ permissive setting; task 14.6 carries it. Evidence: [family detection census](../../../benchmarks/measurements/2026-09-19-family-detection-census.json), [untrusted destination vocabulary](../../../benchmarks/measurements/2026-09-19-untrusted-destination-vocabulary.json), -[output encoding family](../../../benchmarks/measurements/2026-09-19-output-encoding-family.json). +[output encoding family](../../../benchmarks/measurements/2026-09-19-output-encoding-family.json), +[access control across the module split](../../../benchmarks/measurements/2026-09-20-access-control-module-split.json). diff --git a/.kiro/specs/pre-push-safety-net/tasks.md b/.kiro/specs/pre-push-safety-net/tasks.md index a099ce0..f3887b0 100644 --- a/.kiro/specs/pre-push-safety-net/tasks.md +++ b/.kiro/specs/pre-push-safety-net/tasks.md @@ -498,14 +498,33 @@ Preserve all completed task evidence below. - _Boundary: contributor-scan arbitration, Rung ladder_ - _Depends: 14.3_ - _Requirements: 6.3, 8.1_ -- [ ] 14.4 Raise the access-control obligation where the application splits route from query +- [x] 14.4 Raise the access-control obligation where the application splits route from query - **Named by the 14.1 census and measured with the real dominance query.** Every modelled data operation in NodeGoat lives in `app/data/*-dao.js`; none lives in a route handler file. Operations are scoped to the region's own file and access_control regions are the handler files, so no obligation is raised where the question is asked, and inside the DAO files every operation is equally unguarded so the sibling rung has nothing to contradict. The family answers correctly on VAmPI, which keeps handler and query in one file. - The file scope was chosen deliberately, because unscoped rows attributed a model's unguarded query to a handler of the same name. Widening it is not the fix; carrying the obligation along the call the handler actually makes might be. - NodeGoat's two documented defects are also outside what the model can claim: an authenticated handler reading `userId` from `req.params` (`app/routes/allocations.js:17`), and a commented-out `isAdmin` in the route table (`app/routes/index.js:58`). Record which of the two this task is for, and do not silently widen the claim to the other. - Done when a route-and-DAO split raises the obligation at the handler with a witness naming both sites, and a same-name model function is still not attributed to a handler. + - _Verified 2026-09-20:_ 1,415 tests passed, fifteen skipped; eleven Joern-gated query tests pass in the image; Ruff/format/mypy passed. **This task is for the first defect, the route-and-DAO split itself.** The obligation now follows the one named call the handler writes, out of the region's file and no further. Attribution is by the graph's own provenance, not by name: jssrc2cpg leaves the callee an external stub, and that stub's fullName records the file and member where the implementation was written, so a module the handler never imports is never consulted. A row is attributed to the region's DECLARED function, which is what lets a question about `displayAllocations` match a graph that holds a ``. A carried row is also checked for a guard at the CALL, because that is where a handler establishes who is asking, and the method holding the carried operation joins the question's context scope so the query's own module cannot change unattributed. + - Measured on NodeGoat: 15 handlers asked, obligations raised at zero before and at six now, three of which reach a verdict. All three are corroborated and none is reported without a judge. VAmPI is unchanged at two entailed findings, because it keeps handler and query in one file and carries nothing. Evidence: `benchmarks/measurements/2026-09-20-access-control-module-split.json`. + - _A carried obligation corroborates, it does not entail, and that was measured rather than assumed._ Entailing it reported `handleSignup`, which looks a user up by name to check the name is free during registration and is public by design, because two authenticated siblings in the same file pass `req.session.userId` into the same module. The entailed rung's justification is that one FILE contradicts itself, same file and same trust context; carried, the compared operations live in a data-access module that handlers of different trust contexts share. The corroborated band already asks the right residual: does this operation require a check, or is the route legitimately public? + - _What this does not reach, recorded rather than implied:_ NodeGoat's documented IDOR at `app/routes/allocations.js:17` still raises nothing, because its module reads with `allocationsCol.find(...)` and `find` is not a modelled operation (task 14.8). The commented-out `isAdmin` at `app/routes/index.js:58` is not claimable by this abstraction at all, since the evidence that `/benefits` needs a stronger guard than its siblings exists only in a comment. - _Boundary: contributor-scan dominance, Ranker Scope Contract_ - _Depends: 14.1_ - _Requirements: 6.2, 6.3, 8.1_ +- [ ] 14.7 Decide the sibling scope, and see identity that a destructuring spelled away + - **Named by the 14.4 measurement.** Two of NodeGoat's three corroborated claims are correct code: `const { userId } = req.session` followed by `dao.getByUserId(userId, ...)` is identity-constrained, and the destructuring erases the `session.userId` spelling the token list holds, so the guard is invisible at the call. A judge would be asked about handlers that are not defects. + - The contradiction that would make NodeGoat's IDOR entailed is cross-file: `allocations` passes a URL parameter while `contributions` and `profile` pass session identity. The sibling set is one file, and widening it is a decision, not an implementation detail: measured, a route file mixes public with authenticated handlers, so application-wide siblings would contradict every login and signup handler there is. + - Record the measured cost of each candidate before choosing, and do not widen the scope to make a finding appear. + - Done when one record states the candidates and the choice, a destructured identity read discharges the obligation it actually discharges, and no public route is reported. + - _Boundary: contributor-scan dominance, shipped obligation facts_ + - _Depends: 14.4_ + - _Requirements: 6.2, 6.3, 8.1_ +- [ ] 14.8 Model the collection read an application actually writes + - **Named by the 14.4 measurement.** NodeGoat's documented insecure direct object reference reads with `allocationsCol.find(searchCriteria())`, and `find` is absent from an operation vocabulary holding `findOne`, `findAll` and `findById`. It is the commonest read there is, so the family's obligation is missing at exactly the shape it exists for. + - It is absent for a reason: `Array.prototype.find` is everywhere in JavaScript and jQuery's `.find` is everywhere in a vendored bundle. A collection read takes a query document where those take a closure or a selector, so the discriminator is the argument's shape rather than the name. + - Done when a collection read raises the obligation, an array search and a selector do not, and the false-positive count is measured on a repository with vendored bundles rather than argued from the shape alone. + - _Boundary: shipped obligation facts, contributor-scan dominance_ + - _Depends: 14.4_ + - _Requirements: 6.2, 8.1_ - [ ] 14.5 Ask the prototype and deserialization families somewhere they can answer - **Named by the 14.1 census.** Prototype pollution has been asked on one repository and its only report was a false positive; deserialization was never asked at all, because VAmPI holds no such sink and NodeGoat is not a language where the family is declared. Neither has established anything anywhere. - Find or record a subject where the family has a sink, and report what it does. A negative on a subject that contains the mechanism is a result; silence on a subject that does not contain it is not. diff --git a/benchmarks/measurements/2026-09-20-access-control-module-split.json b/benchmarks/measurements/2026-09-20-access-control-module-split.json new file mode 100644 index 0000000..17cd349 --- /dev/null +++ b/benchmarks/measurements/2026-09-20-access-control-module-split.json @@ -0,0 +1,175 @@ +{ + "schema_version": 1, + "experiment": "access-control-across-the-module-split-v1", + "date": "2026-09-20", + "task": "pre-push-safety-net 14.4", + "claim_scope": "Whether the access-control obligation is raised where an application puts its route in one file and its query in another. Detection only, on two development subjects. No capability qualified.", + "before": "Measured 2026-09-19: every modelled data operation in NodeGoat lives in app/data/*-dao.js or artifacts/db-reset.js, none in a route handler file. Operations were scoped to the region's own file and access_control regions are the handler files, so 35 questions completed with no obligation raised anywhere, and the family had nothing to be right or wrong about.", + "change": { + "carrying": "The obligation follows the call the handler writes, one named call out of the region's file. jssrc2cpg does not resolve allocationsDAO.getByUserIdAndThreshold(...) to the DAO body: the callee is an external stub with no code. The stub's fullName records where the member was written (app/data/allocations-dao.js::program:AllocationsDAO:getByUserIdAndThreshold), so the implementation is reached by the graph's own provenance rather than by a repository-wide name guess, which is the false positive the file scope was closed against.", + "attribution": "A row is attributed to the region's DECLARED function. A handler written this.displayAllocations = (req, res) => {} is a in the graph, and a row naming the lambda could never match a question asking about displayAllocations. Same query-to-operation identity problem task 12.3 fixed at the scope end, appearing at the attribution end.", + "guard_at_the_carrier": "A carried obligation is also checked for a guard at the CALL, not only at the operation. When the obligation crosses a module boundary the check that discharges it is overwhelmingly on the handler's side: measured, userDAO.getUserById(req.session.userId, ...) discharges there and nowhere else.", + "witness_names_both_sites": "displayContributions line 17 calls contributionsDAO.getByUserId(...), whose operation at app/data/contributions-dao.js:57 has no discharging guard. The finding is located at the CALL, which is the line a contributor changed and an admission decision can attribute.", + "change_context": "The method holding a carried operation joins the question's context scope, so the module that holds the query cannot change under a claim that depends on it without the change being attributed.", + "rung_cap": "A carried obligation corroborates, it does not entail. The entailed rung's justification is that one FILE contradicts itself: same file, same trust context. Carried, the compared operations live in a data-access module shared by handlers of different trust contexts, and a route file mixing public with authenticated handlers is the normal shape." + }, + "why_the_rung_cap_is_not_a_fudge": { + "measured_false_positive_without_it": { + "site": "app/routes/session.js handleSignup line 208", + "rung_without_the_cap": "model_entailed", + "witness": "calls userDAO.getUserByUserName(userName, ...), whose operation at app/data/user-dao.js:104 has no discharging guard, while 2 siblings are guarded", + "why_it_is_wrong": "The handler looks a user up by name to check the name is free during registration. The route is public by design. The two guarded siblings in the same file pass req.session.userId into the same module, so the asymmetry is real and the defect is not." + }, + "what_the_band_asks_instead": "Does this operation require an authorization check, or is the route legitimately public? That is already the corroborated residual this family sends to the judge." + }, + "measured_on_nodegoat": { + "handlers_asked": 15, + "obligations_raised_before": 0, + "handlers_with_a_carried_obligation": 9, + "verdicts": { + "none": 12, + "model_corroborated": 3 + }, + "reported_without_a_judge": 0, + "per_handler": { + "app/routes/benefits.js:displayBenefits": { + "rows": 0, + "carried": 0, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + }, + "app/routes/benefits.js:updateBenefits": { + "rows": 0, + "carried": 0, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + }, + "app/routes/allocations.js:displayAllocations": { + "rows": 0, + "carried": 0, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + }, + "app/routes/memos.js:addMemos": { + "rows": 0, + "carried": 0, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + }, + "app/routes/memos.js:displayMemos": { + "rows": 0, + "carried": 0, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + }, + "app/routes/research.js:displayResearch": { + "rows": 0, + "carried": 0, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + }, + "app/routes/contributions.js:displayContributions": { + "rows": 1, + "carried": 1, + "verdict": "model_corroborated", + "witness": "displayContributions line 17 calls contributionsDAO.getByUserId(userId, (error, contrib) => { if (error) return next(error); contr, whose operation at app/data/contributions-dao.js:57: obligated operation with no discharging guard, and no guarded sibling", + "reported_without_a_judge": false + }, + "app/routes/contributions.js:handleContributionsUpdate": { + "rows": 1, + "carried": 1, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + }, + "app/routes/session.js:displayLoginPage": { + "rows": 4, + "carried": 4, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + }, + "app/routes/session.js:handleLoginRequest": { + "rows": 4, + "carried": 4, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + }, + "app/routes/session.js:displaySignupPage": { + "rows": 4, + "carried": 4, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + }, + "app/routes/session.js:handleSignup": { + "rows": 4, + "carried": 4, + "verdict": "model_corroborated", + "witness": "handleSignup line 208 calls userDAO.getUserByUserName(userName, (err, user) => { if (err) return next(err); if (us, whose operation at app/data/user-dao.js:104: no discharging guard, while 3 sibling handler(s) reaching the same module are guarded (16, 2, 6); the obligation was carried across a module boundary, so those siblings need not share this route's trust context", + "reported_without_a_judge": false + }, + "app/routes/session.js:displayWelcomePage": { + "rows": 4, + "carried": 4, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + }, + "app/routes/profile.js:displayProfile": { + "rows": 1, + "carried": 1, + "verdict": "model_corroborated", + "witness": "displayProfile line 20 calls profile.getByUserId(parseInt(userId), (err, doc) => { if (err) return next(err); doc.userId = us, whose operation at app/data/profile-dao.js:95: obligated operation with no discharging guard, and no guarded sibling", + "reported_without_a_judge": false + }, + "app/routes/profile.js:handleProfileUpdate": { + "rows": 1, + "carried": 1, + "verdict": "none", + "witness": "", + "reported_without_a_judge": false + } + }, + "census_findings_by_family": { + "access_control": 0, + "config_secrets": 1, + "injection": 3, + "output_encoding": 1, + "path": 0, + "prototype": 0, + "untrusted_destination": 2 + } + }, + "measured_on_vampi": { + "access_control_findings": 2, + "rungs": { + "model_entailed": 2 + }, + "unchanged": true, + "why": "VAmPI keeps handler and query in one file, so no row is carried and the entailed path is untouched." + }, + "what_this_does_not_yet_reach": { + "the_documented_idor": "app/routes/allocations.js:17 takes userId from req.params on an authenticated route, with the fix commented out. It raises no obligation even now, because its data-access module reads with allocationsCol.find(...) and `find` is not a modelled operation. Adding it needs a shape discriminator -- Array.prototype.find and jQuery's .find take a closure or a selector where a collection read takes a query document -- and its own false-positive measurement. Task 14.8.", + "the_commented_out_admin_check": "app/routes/index.js:58 registers /benefits with isLoggedIn and the isAdmin guard commented out. Neither middleware name is in the discharger vocabulary, and the evidence that /benefits needs a stronger guard than its siblings exists only in a comment. Not claimed, and not claimable by this abstraction.", + "identity_through_destructuring": "const { userId } = req.session then dao.getByUserId(userId, ...) is correctly identity-constrained, and the destructuring erases the session.userId spelling the token list holds, so the guard is invisible at the call. Two of NodeGoat's three corroborated claims are this shape. Task 14.7.", + "cross_file_siblings": "The contradiction that would make NodeGoat's IDOR entailed is cross-file: allocations passes a URL parameter while contributions and profile pass session identity. The sibling set is one file. Widening it is a decision with a measured false-positive risk, not an implementation detail. Task 14.7." + }, + "controls": [ + "A route-and-DAO split raises the obligation at the handler, corroborated, with a witness naming the call and the operation's file.", + "A third module defining a member of the SAME NAME that nothing imports is not attributed, which is the collision the file scope was closed against.", + "A sibling handler that passes the caller's own identity into the same module yields no verdict at all." + ], + "limits": [ + "One named call out of the region's file, not a transitive walk: a helper's helper's query is not attributed to a handler that never mentions it.", + "Provenance comes from an external stub's fullName, which is a jssrc2cpg detail; a frontend that resolves the call properly needs no stub, and one that emits neither carries nothing.", + "Two subjects, both used to develop the analyser." + ] +} diff --git a/src/openultrasast/cpg/queries/dominance.sc b/src/openultrasast/cpg/queries/dominance.sc index 4f14ad3..90796b5 100644 --- a/src/openultrasast/cpg/queries/dominance.sc +++ b/src/openultrasast/cpg/queries/dominance.sc @@ -135,11 +135,87 @@ // 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 opCalls = { - val all = cpg.call.filter(c => matchesOperation(c.name, c.code)).l - if (fileS.isEmpty) all else all.filter(_.method.filename.endsWith(fileS)) + val allOperations = cpg.call.filter(c => matchesOperation(c.name, c.code)).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 -------------------------- + // + // Measured on NodeGoat: every modelled data operation lives in `app/data/*-dao.js`, none in a route handler + // file, and the family answered 35 questions there with nothing to say -- the obligation was never raised + // anywhere the question was asked. Widening the file scope is not the fix, because an unscoped row is + // exactly what attributed a model's unguarded query to a handler of the same name. The obligation is + // carried along the call the handler actually writes. + // + // How it is followed matters. jssrc2cpg does not resolve `allocationsDAO.getByUserIdAndThreshold(...)` to + // the DAO's body: the callee is an EXTERNAL stub with no code and no filename. But the stub's fullName + // records where the member was written -- `app/data/allocations-dao.js::program:AllocationsDAO:getByUser...` + // -- so the implementation is reached by the graph's own provenance rather than by guessing a name across + // the repository, which is the false positive this scope was closed to avoid. + def stubTarget(fullName: String): Option[(String, String)] = { + val idx = fullName.indexOf("::program:") + val member = fullName.substring(fullName.lastIndexOf(':') + 1) + if (idx <= 0 || member.isEmpty) None else Some((fullName.substring(0, idx), member)) } + def spans(outer: io.shiftleft.codepropertygraph.generated.nodes.Method, inner: io.shiftleft.codepropertygraph.generated.nodes.Method): Boolean = + outer.filename == inner.filename && + inner.lineNumber.getOrElse(-1) >= outer.lineNumber.getOrElse(0) && + inner.lineNumberEnd.getOrElse(-1) <= outer.lineNumberEnd.getOrElse(Int.MaxValue) + + // The method a MEMBER NAME is written as, in one file. A handler or a DAO method written + // `this.getUserById = (userId, callback) => {}` is a `` in the graph, and the assignment puts the + // lambda's first line on itself -- which is how the written name and the graph's method are joined without + // a repository-wide name lookup. A language that gives its functions real names (every Python handler this + // family already answers) matches on the first branch and never needs the second. + val methodsByFile = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.Method]] + def fileMethods(path: String): List[io.shiftleft.codepropertygraph.generated.nodes.Method] = + methodsByFile.getOrElseUpdate(path, cpg.method.filterNot(_.isExternal).filter(_.filename.endsWith(path)).l) + + def writtenAs(path: String, member: String): List[io.shiftleft.codepropertygraph.generated.nodes.Method] = { + val inFile = fileMethods(path) + val named = inFile.filter(_.name == member) + if (named.nonEmpty) named + else { + val prefixes = List("this." + member, member, "const " + member, "let " + member, "var " + member) + val lines = inFile + .flatMap(_.ast.isCall.nameExact(".assignment").l) + .filter(a => prefixes.exists(p => a.code.startsWith(p + " =") || a.code.startsWith(p + "="))) + .flatMap(_.lineNumber) + .toSet + inFile.filter(m => m.lineNumber.exists(lines.contains)) + } + } + + def operationsWithin(path: String, member: String): List[io.shiftleft.codepropertygraph.generated.nodes.Call] = { + val holders = writtenAs(path, member) + if (holders.isEmpty) Nil + else allOperations.filter(op => op.method.filename.endsWith(path) && holders.exists(h => h.id == op.method.id || spans(h, op.method))) + } + + // One named call out of the scoped file, not a transitive walk. The claim is about the call this handler + // WRITES: a deeper walk would attribute a helper's helper's query to a handler that never mentions it, and + // the witness could no longer name the two sites a reader has to open. + val carried: List[(io.shiftleft.codepropertygraph.generated.nodes.Call, io.shiftleft.codepropertygraph.generated.nodes.Call)] = + if (fileS.isEmpty) Nil + else + fileMethods(fileS).flatMap { m => + m.call.l.flatMap { via => + via.callee.l + .filter(_.isExternal) + .flatMap(stub => stubTarget(stub.fullName)) + .distinct + .filterNot { case (path, _) => path.endsWith(fileS) } + .flatMap { case (path, member) => operationsWithin(path, member).map(op => (op, via)) } + } + }.distinctBy { case (op, via) => (op.id, via.id) } + + // The region asks about a declared function; the graph may hold it as a lambda. Attributing a row to the + // declared name is what lets the arbiter match at all -- the same query-to-operation identity problem task + // 12.3 fixed at the scope end, appearing here at the attribution end. + val labelled = if (function.isEmpty || fileS.isEmpty) Nil else writtenAs(fileS, function) + def attributedName(m: io.shiftleft.codepropertygraph.generated.nodes.Method): String = + if (labelled.exists(l => l.id == m.id || spans(l, m))) function else m.name + // Change-attribution context for THIS family, describing the scope this query actually used. // It is deliberately not taint's projection: dominance compares sibling operations across a // file, so the file's own methods are the scope, the methods holding a matched operation are @@ -154,21 +230,26 @@ val scoped = if (fileS.isEmpty) cpg.method.filterNot(_.isExternal).l else cpg.method.filterNot(_.isExternal).filter(_.filename.endsWith(fileS)).l - val operationIds = opCalls.map(_.method.id).toSet + val operationIds = opCalls.map(_.method.id).toSet ++ carried.map { case (_, via) => via.method.id }.toSet val guardIds = scoped.filter(m => m.ast.isCall.code.l.exists(code => mentionsGuard(code))).map(_.id).toSet -- operationIds val rows = scoped.map { m => val kind = if (operationIds.contains(m.id)) "operation" else if (guardIds.contains(m.id)) "guard" else "sibling" location(m, kind) } + // A carried obligation makes the claim span two files, so the method holding the operation is part of this + // question's scope even though it is not in the region's file. Leaving it out would let the module that + // holds the query change under a claim that depends on it without the change being attributed. + val carriedRows = carried.map { case (op, _) => location(op.method, "carried_operation") }.distinctBy(row => row.value.toString) // An empty scope is reported, never implied: a file the frontend produced no methods for // cannot support a statement about that file contradicting itself. val boundaries = if (scoped.isEmpty) List(ujson.Obj("kind" -> "context_boundary", "reason" -> "context_scope_empty:contributor-scan")) else Nil - ujson.Obj("kind" -> "context_summary") :: (rows ++ boundaries) + ujson.Obj("kind" -> "context_summary") :: (rows ++ carriedRows ++ boundaries) } - val rows = opCalls.map { op => + // `via` is the call that carried the obligation here, absent when the operation is in the region's own file. + def rowFor(op: io.shiftleft.codepropertygraph.generated.nodes.Call, via: Option[io.shiftleft.codepropertygraph.generated.nodes.Call]): ujson.Obj = { val method = op.method val applicable = guardsFor(op.name, op.code) @@ -183,19 +264,36 @@ // 3. dominating guard: a call naming a discharger that actually dominates the operation. val dominating = op.dominatedBy.isCall.code.l.filter(c => mentions(c, applicable)) - val guards = (inArguments ++ inCondition ++ dominating).distinct.map(_.take(120)) + // 4. a guard at the CARRIER. When the obligation crossed a module boundary the check that discharges it is + // overwhelmingly on this side of it: the handler establishes who is asking and passes the result down. + // Reading only the operation's own site would report every correctly guarded handler in the repository. + val atCarrier = via.toList.flatMap { call => + call.argument.code.l.filter(c => mentions(c, applicable)) ++ + call.method.controlStructure.condition.code.l.filter(c => mentions(c, applicable)) ++ + call.dominatedBy.isCall.code.l.filter(c => mentions(c, applicable)) + } + + val guards = (inArguments ++ inCondition ++ dominating ++ atCarrier).distinct.map(_.take(120)) + val attributed = via.map(call => attributedName(call.method)).getOrElse(attributedName(method)) ujson.Obj( "operation" -> op.code.take(200), "opLine" -> op.lineNumber.getOrElse(-1).toString, - "opMethod" -> method.name, + "opMethod" -> attributed, // Where the operation is. Without it an access-control finding reaches a contributor as // `api_views/users.py:?:update_password` -- a location no editor can open. "opFile" -> method.filename, + // The two sites of a carried obligation: the call a reader would open first, and the operation it + // reaches. Empty for an operation the region's own file holds, which is the committed shape. + "viaCall" -> via.map(_.code.take(120).replace("\n", " ")).getOrElse(""), + "viaLine" -> via.flatMap(_.lineNumber).map(_.toString).getOrElse(""), + "viaFile" -> via.map(_.method.filename).getOrElse(""), "dominatingGuards" -> ujson.Arr(guards.map(ujson.Str(_)): _*) ) } + val rows = opCalls.map(op => rowFor(op, None)) ++ carried.map { case (op, via) => rowFor(op, Some(via)) } + if (contextS == "true") rows ++ contextRowsFor() else rows } diff --git a/src/openultrasast/model/dominance.py b/src/openultrasast/model/dominance.py index 34b41cb..0a31f97 100644 --- a/src/openultrasast/model/dominance.py +++ b/src/openultrasast/model/dominance.py @@ -71,18 +71,54 @@ def verdict(cpg: CpgResult, spec: DominanceSpec, *, function: str = "", file: st guarded_methods = sorted({str(row["opMethod"]) for row in siblings if row["guards"]}) target = min(unguarded, key=lambda row: (row["opMethod"], row["opLine"])) - if guarded_methods: + where = _where(target) + # A carried obligation does not ENTAIL, however clean the asymmetry looks. + # + # The rung's justification is that one FILE contradicts itself: same file, same trust context, so an + # unguarded operation beside a guarded one is evidence. Once the obligation is carried across a module + # boundary, the operations being compared live in a data-access module that handlers of DIFFERENT trust + # contexts share, and a route file mixing public with authenticated handlers is the normal shape -- so the + # asymmetry stops implying inconsistency. + # + # Measured on NodeGoat, where this was not hypothetical: `handleSignup` looks a user up by name to check + # the name is free, which is public by design, and two authenticated siblings in the same file pass + # `req.session.userId` into the same module. The asymmetry is real and the defect is not. That question -- + # does this operation require a check, or is the route legitimately public? -- is exactly what the + # corroborated band's residual asks, so a carried row is capped there and the judge decides it. + if guarded_methods and not target.get("viaLine"): witness = ( - f"{target['opMethod']} line {target['opLine']}: no discharging guard, " - f"while {len(guarded_methods)} sibling handler(s) are guarded " - f"({', '.join(guarded_methods[:3])})" + f"{where}: no discharging guard, while {len(guarded_methods)} sibling handler(s) are guarded ({', '.join(guarded_methods[:3])})" ) return Verdict(rung=Rung.ENTAILED, family=spec.family, witness=witness, location=_location(target)) - witness = f"{target['opMethod']} line {target['opLine']}: obligated operation with no discharging guard, and no guarded sibling" + if guarded_methods: + witness = ( + f"{where}: no discharging guard, while {len(guarded_methods)} sibling handler(s) reaching the same " + f"module are guarded ({', '.join(guarded_methods[:3])}); the obligation was carried across a module " + f"boundary, so those siblings need not share this route's trust context" + ) + return Verdict(rung=Rung.CORROBORATED, family=spec.family, witness=witness, location=_location(target)) + + witness = f"{where}: obligated operation with no discharging guard, and no guarded sibling" return Verdict(rung=Rung.CORROBORATED, family=spec.family, witness=witness, location=_location(target)) +def _where(row: Mapping[str, object]) -> str: + """The sites a reader has to open. Two of them when the obligation crossed a module boundary. + + An application that puts its route in one file and its query in another raised no obligation at all until + the query followed the call, and a witness that named only the operation would send a contributor to a + data-access module that is not where the decision was made. Naming only the handler would hide what the + claim is about. So a carried row names both, in the order a reader needs them. + """ + method = str(row["opMethod"]) + via_line = str(row.get("viaLine") or "") + if not via_line: + return f"{method} line {row['opLine']}" + call = str(row.get("viaCall") or "").strip() + return f"{method} line {via_line} calls {call}, whose operation at {row.get('opFile')}:{row['opLine']}" + + def _location(row: Mapping[str, object]) -> str: """``path:line:function`` of the obligated operation, when the engine reported a file. @@ -90,6 +126,12 @@ def _location(row: Mapping[str, object]) -> str: reached a contributor as `api_views/users.py:?:update_password` -- the right file by accident, because the region was asked about that file, and no line at all. """ + # A carried obligation is located at the CALL, not at the operation: that is the line the contributor + # changed and the line an admission decision has to be attributable to. The operation's own site travels + # in the witness. + carried_line = str(row.get("viaLine") or "") + if carried_line.lstrip("-").isdigit() and int(carried_line) >= 0 and row.get("viaFile"): + return f"{row['viaFile']}:{carried_line}:{row.get('opMethod') or '?'}" where = str(row.get("opFile") or "") line = str(row.get("opLine") or "") if not where or not line.lstrip("-").isdigit() or int(line) < 0: @@ -112,6 +154,9 @@ def _operations(rows: object) -> list[dict[str, object]]: "opLine": str(row.get("opLine", "")), "opMethod": str(row.get("opMethod", "")), "opFile": str(row.get("opFile", "")), + "viaCall": str(row.get("viaCall", "")), + "viaLine": str(row.get("viaLine", "")), + "viaFile": str(row.get("viaFile", "")), "guards": [str(g) for g in guards] if isinstance(guards, Sequence) and not isinstance(guards, str) else [], } ) diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index 28fd011..1aad5b7 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -296,3 +296,143 @@ def test_a_qualified_setting_needs_its_receiver(tmp_path: Path) -> None: settings = [str(row.get("setting", "")) for row in (payload.get("0") or []) if row.get("setting")] assert any("marked.setOptions" in row for row in settings), f"the template engine's setting was missed: {settings}" assert all("editor.setOptions" not in row for row in settings), f"an unrelated setOptions was claimed: {settings}" + + +def _route_and_dao(tmp_path: Path) -> Path: + """An application shaped the way the census found NodeGoat: route in one file, query in another. + + `displayNote` takes the record id from the URL; `displayMine` takes it from the caller's identity. Both + reach the same data-access module, so the file holding the query cannot tell them apart and the file + holding the decision holds no operation at all. A third module defines a member of the SAME NAME that + nothing imports, which is the collision the file scope was originally closed to prevent. + """ + routes = tmp_path / "app" / "routes" + data = tmp_path / "app" / "data" + routes.mkdir(parents=True) + data.mkdir(parents=True) + (routes / "notes.js").write_text( + 'const NotesDAO = require("../data/notes-dao").NotesDAO;\n' + "\n" + "function NotesHandler(db) {\n" + " const notesDAO = new NotesDAO(db);\n" + "\n" + " this.displayNote = (req, res) => {\n" + " const noteId = req.params.noteId;\n" + " return notesDAO.getById(noteId, (err, note) => res.json(note));\n" + " };\n" + "\n" + " this.displayMine = (req, res) => {\n" + " return notesDAO.getOwned(req.user.id, (err, notes) => res.json(notes));\n" + " };\n" + "}\n" + "\n" + "module.exports = NotesHandler;\n" + ) + (data / "notes-dao.js").write_text( + "function NotesDAO(db) {\n" + ' const notes = db.collection("notes");\n' + "\n" + " this.getById = (noteId, callback) => {\n" + " notes.findOne({ _id: noteId }, callback);\n" + " };\n" + "\n" + " this.getOwned = (userId, callback) => {\n" + " notes.findOne({ owner: userId }, callback);\n" + " };\n" + "}\n" + "\n" + "module.exports.NotesDAO = NotesDAO;\n" + ) + (data / "audit-dao.js").write_text( + "function AuditDAO(db) {\n" + ' const audit = db.collection("audit");\n' + "\n" + " this.getById = (anyId, callback) => {\n" + " audit.findOne({ _id: anyId }, callback);\n" + " };\n" + "}\n" + "\n" + "module.exports.AuditDAO = AuditDAO;\n" + ) + return tmp_path + + +def _dominance_rows(tmp_path: Path, *, function: str, file: str) -> list[dict[str, object]]: + from openultrasast.cpg.backend import extract_payload + from openultrasast.model.dominance import request_params + from openultrasast.model.specs import dominance_specs + + frontend = __import__("shutil").which("jssrc2cpg") + if frontend is None: + pytest.skip("jssrc2cpg is not installed") + cpg = tmp_path / "cpg.bin" + built = subprocess.run([frontend, str(tmp_path), "-o", str(cpg)], capture_output=True, text=True, timeout=600, check=False) + if not cpg.is_file(): + pytest.skip(f"could not build a sample cpg: {(built.stderr or '')[-200:]}") + spec = dominance_specs(language="javascript")["access_control"] + params = request_params(spec, function=function, file=file) + request = {k: v for k, v in params.items() if isinstance(v, str)} + request["operations"] = ",".join(spec.operations) + request["dischargers"] = ",".join(spec.dischargers) + requests = tmp_path / "requests.json" + requests.write_text(json.dumps({"0": request})) + done = subprocess.run( + [ + _joern() or "joern", + "--script", + str((QUERIES / "dominance.sc").resolve()), + "--param", + f"cpgFile={cpg}", + "--param", + f"requestsFile={requests}", + ], + capture_output=True, + text=True, + timeout=900, + check=False, + ) + assert done.returncode == 0, f"dominance.sc did not run: {(done.stderr or done.stdout or '')[-500:]}" + payload = extract_payload(done.stdout or "") + assert payload is not None, "dominance.sc produced no parseable payload" + return [row for row in (payload.get("0") or []) if row.get("operation")] + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_a_route_and_dao_split_raises_the_obligation_at_the_handler(tmp_path: Path) -> None: + """The obligation follows the call the handler writes, and the witness names both sites.""" + from openultrasast.cpg.backend import CpgResult + from openultrasast.model.dominance import verdict + from openultrasast.model.ladder import Rung + from openultrasast.model.specs import dominance_specs + + rows = _dominance_rows(_route_and_dao(tmp_path), function="displayNote", file="app/routes/notes.js") + assert rows, "the route file raised no obligation at all, which is the gap this closes" + carried = [row for row in rows if row.get("viaLine")] + assert carried, f"no obligation was carried across the module boundary: {rows}" + assert all("notes-dao.js" in str(row["opFile"]) for row in carried), f"an unimported module was attributed: {carried}" + + spec = dominance_specs(language="javascript")["access_control"] + cpg = CpgResult(cpg_path=tmp_path / "cpg.bin", run=lambda q, p: rows) + answer = verdict(cpg, spec, function="displayNote", file="app/routes/notes.js") + assert answer is not None, f"no verdict from rows {rows}" + # CORROBORATED, not entailed, and deliberately: the obligation crossed a module boundary, so the guarded + # sibling need not share this route's trust context. Measured on NodeGoat, where entailing it reported a + # signup handler that is public by design. The residual question the band carries is the right one. + assert answer.rung is Rung.CORROBORATED, f"{answer.rung} from {answer.witness}" + assert "displayNote" in answer.witness, answer.witness + assert "notes-dao.js" in answer.witness, f"the witness does not name the operation's site: {answer.witness}" + assert "getById" in answer.witness, f"the witness does not name the call that carries it: {answer.witness}" + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_an_identity_constrained_handler_is_not_reported(tmp_path: Path) -> None: + """The control for the other direction: the sibling that passes the caller's own identity is guarded.""" + from openultrasast.cpg.backend import CpgResult + from openultrasast.model.dominance import verdict + from openultrasast.model.specs import dominance_specs + + rows = _dominance_rows(_route_and_dao(tmp_path), function="displayMine", file="app/routes/notes.js") + spec = dominance_specs(language="javascript")["access_control"] + cpg = CpgResult(cpg_path=tmp_path / "cpg.bin", run=lambda q, p: rows) + answer = verdict(cpg, spec, function="displayMine", file="app/routes/notes.js") + assert answer is None, f"a handler that passes the caller's own identity was reported: {answer}"