Audit fixes from 31 new projects: security recall and precision, deprecated copies, test pairs - #25
Open
tauanbinato wants to merge 27 commits into
Open
tauanbinato wants to merge 27 commits into
tauanbinato wants to merge 27 commits into
Conversation
A dry run left files that need a file-purpose answer unplanned, even when the cache held that answer, so the units a run then sends for them were missing and requests that depend on them (tests, outlines) differed. On devise the estimate counted 106 of 1,532 first-pass requests as new while the run sent none. The preview now records a cached purpose and plans the file as a run does.
Only Django views and PHP pages naming unserialize were asked whether they load data with a deserializer that can build any object. A Flask route passing pickle.loads(request.get_data()) was asked only about query, command, code and markup text, and was clear. Code whose source names such a deserializer (Python's pickle, marshal, shelve, jsonpickle and yaml.load; Ruby's Marshal.load and YAML.load; Java's ObjectInputStream, XMLDecoder, XStream and SnakeYAML; node-serialize) now gets the deserializer form of the presence question and its language's deserialize check in the trace. Other requests are byte-identical, so their cached answers stay valid: across 65 projects only gson (3) and pygoat (1) re-asked a request. pygoat's insecure deserialization lab went from a note to a review, and a seeded Flask canary's pickle route from clear to a review.
"8 new review finding(s)" and "1 file(s) not judged" read as form letters. The gate's reasons, the notes line, the GitHub summary and the baseline commands now say "1 finding" or "2 findings".
A FastAPI handler that returns str(exc) for the LookupError its service raised with the program's own text was a review: the exception check asks about "an exception it did not raise itself", and the handler did not. Twelve handlers of one project were reviews this way. The sensitive-data trace now lists the errors the functions it calls create, two calls deep in its own file or files it imports, and asks the exception check about whose text a response carries: a library's, the database's or the runtime's, or the program's own. On 65 projects only error-detail traces were asked again (/bin/zsh.01): ten of the twelve reviews became notes or considers, two tools returning the text of every exception they catch became reviews, and nothing else changed.
Labeled by hand on 25 projects, 3 of 32 file-organization findings on files under 250 lines were right (a 138-line module, a 175-line test helper file, a 134-line types file were told to split), against 21 of 29 on longer files. Such findings are now notes. A module Choice that picks a group holding three quarters or more of the outline's members no longer names it: moving 14 of a file's 15 members, or 9 of its 11 tests, moves the file rather than splitting it. A consider left naming no group is a note, as before, and a review says to split the whole file (a 754-line routes file whose group held 26 of 29 members). On the labeled projects, file-organization reviews went from 50% to 67% precision and considers from 46% to 71%; 18 wrong findings became notes and 3 right ones did. Nothing is asked again.
A finding reads [maintainability/file-organization], but --rule file-organization was an unknown rule: only the full ID, the key (file_organization) and groups were accepted, and the error listed only the groups. The part of the ID after the group now names the rule everywhere a rule is named (--rule, --skip-rule, --fail-on, [rules], allow comments, the MCP tool), jevgate.schema.json lists it, and an unknown name points to `jevgate rules`.
Any directory named example or demo made a file example code, including the packages below a JVM source root: Spring Initializr names a new project's package com.example.demo, so every finding of such a project was capped (injection at a consider, weak password hashing at a note) and its hardcoded values were not judged. A seeded Spring controller in com.example.shop had SQL injection, deserialization and path traversal at consider and MD5 password hashing and a Random reset token at note; it now has all five at review. Directories below src/<set>/java, kotlin, scala or groovy are packages and no longer mark example code; directories above the source root, such as examples/, still do. Across 65 projects only gson's test-shrinker module (sample classes in com.example) changed, with two hardcoded-value considers and one shared-logic review of its per-case harness.
XXE (CWE-611) was never asked: a pygoat view turning on external general entities before parsing the request body was a note found only by its CSRF check, and a Spring controller parsing its body with a default DocumentBuilderFactory was clear. Code that parses XML with a parser able to resolve external entities now gets that clause in its presence question and an xxe check in its trace: it names lxml, SAX, pulldom, DocumentBuilderFactory, SAXParserFactory, XMLInputFactory, XmlDocument, SimpleXML, libxmljs or Nokogiri, or its file imports one and it calls a parse method. Other requests are byte-identical: across 65 projects only lobsters (5) and pygoat (1) re-asked a request, the pygoat lab became a review, and seeded Python and Java canaries now report their XXE routes.
JevGate reviewing this branch found a test repeating the steps of traced_checks, and considered preview's branching, callee_errors mixing two jobs, three-quarter checks written as arithmetic twice and three tests building the same scripted answers. The injection plan of a one-file project, a cached file purpose, a function's callees, the three-quarter comparison and the answers that move G1 are now named once each. Behavior is unchanged.
Imports::reach looked for a target's file stem in import lines, which Go never writes: a Go import names a package directory, and files of one package use each other without importing. The import block's paths were not even read, since only lines starting with "import " were kept. So callers, callees and the files that use a Go file were never found: an injection's recheck with its callers was never asked, and govwa's SQL injection through a query helper that its handlers call with a cookie and a query parameter stayed a consider. A Go file now reaches every file of its directory and of the packages its import paths name. Across 65 projects only the six Go projects re-asked requests ($0.01): govwa's injection became a review, callers cleared one injection note in wtf and one error-detail note in wild-workouts, and nothing new was raised.
Any syntax error skipped a whole file, but tree-sitter's grammars miss some valid code: tree-sitter-typescript reads a call signature that starts with <T> on the line after another as its continuation, which left four of zustand's store files unjudged, and tree-sitter-go flags a const group closed on a raw string's line, which hid govwa's MD5 password hashing in user/user.go. A file whose errors are at most three regions and an eighth of its source is now parsed; the definitions holding an error, and anything inside an error node, are left out. Generator templates (under a templates directory, or holding ERB tags or dotnet-new //#if conditions) keep the strict rule, since their placeholders are not the language's syntax: devise's and CleanArchitecture's templates stay skipped. Whether errors are tolerable depends on the path, so it is checked on cached trees too. Across 65 projects only zustand and govwa re-asked requests ($0.02): govwa's login now has its MD5 password hashing as a review, and zustand's tests resolve createStore as their code under test.
A Laravel app's config/*.php files come from the framework and its packages (vendor:publish), with their documentation written as comments: alternative drivers left commented out, option descriptions, defaults. On linkace and laravel-realworld all six comment considers in config/ were the publisher's text, and 14 of those files' comments stayed undecided. With an artisan script at the project root, files directly in config/ are no longer read for comments; nothing is asked again.
Copies inside test cases were one level lower, but copies in a test file's fixtures, helpers and setup were judged like application code. Labeled by hand on 25 projects, 13 of 19 shared-logic reviews of such copies were a level too strong (repeated in-memory context setup, a test client's wrappers), while 11 of 12 considers were right as they were. They are now at most a consider, so a review is for duplicated application code. Nothing is asked again; across 65 projects 39 reviews became considers.
Their composition changed on this branch: copies in test support code are at most a consider, and a Laravel app's published configuration is not read for comments.
With the errors of a function's callees in view, the exception check asked about the text of an exception that a library, the database or the runtime raised. On starlette, a benchmark app returning a framework's bad-request message (MultiPartException) became a review, since the framework is a library from its point of view. The check now asks whether the text describes an internal failure (a database, network, file system or library error, or a stack trace) or explains invalid input or a missing record, whoever raised it. Only traces with callee errors are asked again ($0.005 on ten projects): the benchmark's reviews are considers again, two tools returning their errors to a local agent became notes, and one more handler returning its service's not-found message is clear.
A job's two questions (can a run script's expression hold outside text; does it run pull request code with secrets) had no follow-up, so 14 of 48 jobs across 65 projects stayed undecided between 0.2 and 0.6: a release job's tag names, a job uploading a pull request's coverage report. A job left undecided is now asked, in a recheck, which of its listed expressions holds outside text, and for workflows run on pull_request_target or workflow_run what code the job runs (the base branch's, the pull request's, or none). Both Choices can only clear: an undecided concern is clear when the Choice puts 0.80 on no expression, or on the base branch's code or none. For $0.001, 7 of the 14 jobs were settled and no finding changed.
basejump's accept_invitation and lookup_invitation find an invitation by its secret token and act for whoever holds it, the usual way a Supabase app lets an invited user join; both were reviews for checking no auth.uid(). The unchecked-caller question now names acting only for the holder of a token looked up by value as a check. Only SECURITY DEFINER units are asked again ($0.001): the two reviews are gone, and the two functions stay undecided rather than clear.
The message of an error a function creates was its first positional
argument, so HTTPException(500, f"engine error: {e}") had the message
500: a leak's finding said "The message is 500.", and the trace asked
which message carries another error's text about the status code. A
leading status (a number, status.HTTP_…, HTTPStatus.…, http.Status…) is
now passed over for the next positional argument. On the user's
chesstal backend both leaks now quote the f-string that carries the
engine's error; nothing else changed.
A deployment value among a file's constants was reported as "one of
this file's constants", located at every constant: a reader had to
search 14 constants for the three URLs holding the author's account
name. After such a review or consider, a locate Choice names the
constant, and the finding points at it and names it ("The constant is
`API_URL`."). Its level does not change. Across 71 projects, 20 of
23 such findings are now named, for $0.001.
Ruby pairs were asked whether each test checks something the other does
not before a review ("one adds nothing"); pairs in other languages were
not, so tests of two overloads with equivalent inputs (javapoet's
writeTo(Path) and writeTo(File)) and of two public methods (a mount
manager's fileExists and has) were reviews. On 31 labeled projects, six
of the ten test-redundancy reviews were wrong.
Outside Ruby, a pair that would be a review is now asked that question
after the fact, beside its answers, and a pair where each test checks
something the other does not is a consider. Pairs that read the same
apart from their names are not asked. The six wrong reviews are gone and
the four right ones stay.
flysystem keeps its phpseclib 2 SFTP adapter, marked @deprecated, beside the phpseclib 3 adapter that replaces it, and the clone finder paired the two in seven reviews and a consider, all wrong: the old copy goes with the next major version, so sharing code with its replacement is not worth it. A copy inside a function or type marked deprecated (a @deprecated tag, annotation or decorator, #[deprecated], [Obsolete], or a Deprecated: comment above it) is now no candidate. Only a declaration's header up to its body and the lines above it are read, so a file's root, a parameter named `deprecated` and a mark named `deprecated_lifespan` do not count. Across 71 projects, only flysystem's findings changed.
The error-detail trace lists the errors a function's callees raise, and
its exception check already asks whose text a response carries. The
Choice over the messages the function creates did not know about them, so
a FastAPI route that turns its service's LookupError("Pesquisa não
encontrada") into a 404 was still "putting the text of an error the
program did not create" into a message, at 0.93, and stayed a consider.
With callee errors in view, the Choice's note says that passing on their
text is the program's own when only those errors can be caught there. On
71 projects, two wrong considers and a wrong review of that project, and
a local tool's consider about an API error it returns to the user's own
agent, became notes; nothing else changed.
JevGate's review of this branch found the constants locate request repeating the value locate request's steps (a review), and considers on `located_constant` and `located_value`, `plan_pairs`, the magic 8 in `tolerable`, the organization tests' setup, the security question list of the question tests, and the deserializer checks growing questions/security.rs past a thousand lines. The locate requests share `locate_request` and `option_ids`, and the located option is read by `located_option`; `plan_pairs` builds its questions and state in helpers; the error share is named; the tests share their setup and check list; the deserializer and XML parser checks move to questions/deserializers.rs. Requests are unchanged: a corpus run over 71 projects was answered from the cache with no finding changed.
When the Choice over a function's error messages found another error's text in one, an error-detail answer leaning toward a client (undecided, at 0.50 or more) became a consider: "puts the text of a library or database error into an error message, which likely reaches a remote client". Labeled by hand on 71 projects, 1 of 28 such considers outside example code was right. A central handler replaced the text with a generic message (wtf's Error), the error was one written for users (Supabase Auth's "Invalid login credentials" shown in a toast), or no remote client read it (just's datetime errors, a Tauri app's webview). The lean is now a note like any other undecided lean, worded with the error text. 33 considers became notes, 27 of them wrong; five of the six right ones were oauth2-server example files whose leaks their route handlers still report. The number of undecided units is unchanged and nothing is asked again.
JevGate's review of this branch found `finding` mixing separate jobs (a consider): besides dispatching on the unit's detail, it located the constant or value a hardcoded-value finding names, quoted the error message a security finding carries, and listed a workflow job's expressions. Those arms move to `values_finding`, `security_finding` and `job_wording`. Findings are unchanged on 71 projects, messages included.
tauanbinato
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes from an audit of 0.19.0 on 31 projects JevGate had not seen:
Every review and consider on those 31 projects was labeled by hand. The audit also used four seeded-issue canaries (Python, TypeScript, Go and Java shops) and the documented vulnerabilities of three intentionally vulnerable apps. The 40 original projects are rerun at each step from the answer cache to catch regressions.
Builds on #24 (its commit is the first one here). Each commit is one change with its tests, docs and changelog line.
Results
Recall on the vulnerable apps went up:
The two canary misses are a mock-only test that is debatable. Undecided units are unchanged on the corpus: 2,318 before the last precision changes and after them.
Changes
Security recall
pickle,marshal,shelve,jsonpickle,yaml.load,Marshal.load,ObjectInputStream,XMLDecoder, XStream, SnakeYAML and node-serialize.user/user.gounjudged.exampleordemo(Spring Initializr'scom.example.demo) are source, not example code.Security precision
Maintainability and tests precision
config/*.phpfiles are not read for comments.CLI
--dry-runplans cached file purposes (Plan cached file purposes in a dry run #24).--rule file-organization).The last three commits fix what JevGate's own review flagged on this branch. The review now passes with no review or consider findings.
Measured and not adopted
Splitting the clone caps into separate application and test budgets added 20 right and 11 wrong shared-logic reviews, and 26 right and 36 wrong considers. That lowered the rule's precision from 75% to 71% for reviews and from 67% to 61% for considers, so it is left out.
Remaining false-positive patterns
Cost
Each fix cost $0.00–$0.03 of Jev credit over all 71 projects, since cached answers stay valid unless a question's text changes. The whole audit cost about $2.5, mostly the first runs of 0.19.0 on the new projects.