Repository navigation
ci: scan the consumed branch with CodeQL and OSV-Scanner - #1
Merged
Merged
Conversation
PR SummaryMedium Risk Overview Wrong or limited until follow-up: push and pull_request on security workflows do not run on main; schedule only applies from default branch. OSV uses freshly generated lockfile, not a committed one. Scanner findings do not fail jobs. rust.yml still uses unpinned actions/checkout@v2 while security workflows pin SHAs. Dependabot no longer opens Rust dependency PRs here. Reviewed by Cursor Bugbot for commit 05a7fcf. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
kalyazin
force-pushed
the
impl-d035/security
branch
5 times, most recently
from
September 17, 2026 15:45
dc629f7 to
b0f43e0
Compare
kalyazin
force-pushed
the
impl-d035/security
branch
4 times, most recently
from
September 17, 2026 16:27
8626ab7 to
5b176ed
Compare
kalyazin
force-pushed
the
impl-d035/security
branch
4 times, most recently
from
September 18, 2026 09:55
05ff3a6 to
aeea5c0
Compare
kalyazin
force-pushed
the
impl-d035/security
branch
11 times, most recently
from
September 18, 2026 21:24
30c9f4c to
5e18f51
Compare
kalyazin
force-pushed
the
impl-d035/security
branch
4 times, most recently
from
September 22, 2026 15:02
c133215 to
590919d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 590919d. Configure here.
kalyazin
force-pushed
the
impl-d035/security
branch
15 times, most recently
from
September 24, 2026 09:43
3a07e8b to
3caa9a4
Compare
The branch consumers build from had no scanning of its own: code scanning ran default setup on main only, and the alerts that read the dependency graph read what the manifests declare, the repository checking in no lockfile. CodeQL runs per language with its query suite named rather than left to the default, so changing which suite is asked for is a visible edit. What is inside one is not pinned here: the action takes the newest CodeQL version GitHub's feature flags enable, reading its own defaults.json only when none is, and the suite resolves out of that bundle rather than from the registry. The OSV job generates the lockfile — the repository gitignores it, and the scanner's own walk skips ignored files — and then names it explicitly. The scanner runs as an unwrapped container pinned by digest, because the published action pins its image by a version tag rather than a digest. Its exit code is the only thing separating a clean scan from a dead one: an unreachable api.osv.dev still writes a well-formed zero-result SARIF, and uploading that files a clean analysis of the branch. So the step gates on the code and accepts only 0 or 1. --config=/dev/null stops a config file committed beside the lockfile filtering the results away. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
The consuming fork's lockfile owns dependency versions, so cargo bumps here are noise. The action pins still need moving. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
The workflow declares none, so its jobs run on whatever the repository's default happens to be — a setting no file in the branch can read, and one click from read/write. Neither job needs more than the checkout. This is what lets the check added next require the same of every workflow here: without it the rule that nothing but the scan uploads to code scanning rests on a setting rather than on the branch. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
A scan is emptied from whichever level nobody is reading: a filter that stops it running, a permission it no longer holds, a category that collides with another upload, an exit code accepted too widely. None of those show red anywhere, so the shape is asserted on every push and pull request to this branch. Keys and step lists are exact allowlists, not denylists — a deliberate change is expected to fail here and to update the matching constant in the same commit. Two readers back it. The workflows are parsed the way GitHub parses them, refusing a duplicate key in any casing and resolving scalars with its matchers rather than YAML 1.1's, because a file GitHub refuses runs nothing while every assertion still passes. And actionlint is asked whether GitHub accepts the files at all, with -config-file /dev/null so a config committed beside them cannot switch its rules off. It runs as the runner over a read-only mount, because a linter only reads and the mount is what decides that rather than the image's good behaviour. Action revisions are pinned by value rather than by shape: a commit pushed to a fork is served by the repository it was forked from, so a 40-hex that looks like an upstream release can fetch anything. Each launch names its interpreter, so what CI executes does not depend on a first line no check reads: a shebang replaced by /bin/true turns a guard into a no-op that reports success, and the launch this commit adds names python3 rather than relying on one. A link is followed by everything that reads these files, so every one of them is checked for being one: a link can name a path outside the checkout, and then the tree these tests assert is not the tree that ships. And a sibling workflow with no permissions block of its own runs on the repository's default token, so each is required to declare one. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
The gate decides whether a scan that could not run is allowed to publish a clean-looking result, and it was a shell body embedded in YAML — so the only way to assert its behaviour was to pin the body's text and re-execute it from the workflow's own shape test, under a stubbed docker. As a script it can be tested directly: scan.test.sh feeds it every exit code, checks that the ones meaning "did not scan" fail the job, checks the argv the scanner receives and the lockfile it is handed at the moment of the call, and checks that the results are not rewritten after it, on both codes that pass. Being a script is also what makes the container's reach assertable. It is given the lockfile read-only and an output directory of its own, under the runner's user rather than root, because the checkout's post step runs git in that workspace with the token in its environment, so an image able to write there is inside the reach of a step this file does not control. The argv is pinned, so widening the mount again is red, and the script lint's container runs as the same user, over the whole checkout read-only rather than the one file. What comes back out is checked too. The image decides what is left in its output directory, so the results are copied only when they are a file the scanner wrote rather than a link it aimed at one of the runner's. A body that leaves YAML also leaves actionlint's reach, so the runner lints the scripts with shellcheck, and the shape test requires each script it names to be linted and to have a sibling test that a step runs. The runner's bodies are an allowlist for the same reason its steps are: a step running anything else runs before them and can rewrite what they assert. The shape test reads scalars the way GitHub does rather than the way PyYAML would: a timeout written 0400 is 400 to GitHub and 256 to PyYAML, while hex and 0o octal are refused by the pinned actionlint in the same job, as are cron field ranges like */0. The stub records what the scanner receives at the moment it is called, so a body that swaps the lockfile around the call and puts it back is not invisible to a check made afterwards, and it writes more than the results into its output directory, so what crosses back into the workspace is asserted to be only them; it also leaves a link there, and nothing at all, because both are what an image can hand back in place of a scan. Every call is recorded rather than the last, and exactly one is expected, since a container started before the pinned one reaches the workspace while the argv still reads as pinned. The output directory has to be a different path on each run, because a fixed one can be waited at. After each exit code the test also looks for a process still holding that directory, because work backgrounded and not waited for reaches the results after the gate has passed on them; four decoys prove the four ways it looks. That is aimed at a later edit leaving something running, not at an author, who has easier ways and whom no test here constrains. The image the test names is deliberately not the one the workflow ships, so a body that hardcodes a digest rather than reading the one it is given is red, and the link left at the results path is tested live as well as dangling, because cp refuses the second and writes through the first. shellcheck runs with --norc for the same reason actionlint runs with -config-file /dev/null: a config file committed beside them would decide what the lint reports. The scripts and their tests are a pinned list, so a script named there is linted, run, and has a test of its own. It is a guard against drift rather than against whoever writes the diff, who can edit the list in the same commit. shellcheck is pinned by the digest its own tag resolves to, so the version named in the comment is one a reader can verify. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
What the workflow declares and what code scanning received are different claims, and only the second is evidence. A scan that runs and uploads nothing, or whose category merges with another tool's so that both are filed as one, is invisible to anything that reads the workflow file. A third job waits for both uploads and asserts that the commit this run scanned carries analyses under exactly the three expected categories, retrying while indexing catches up. It holds read on security-events, not write: it must not be able to delete what it is checking. It runs unless the run was cancelled, because needs: alone would skip it when a scan fails, and a skipped job reports success to whatever requires it. verify-analyses.test.sh stands gh in front of canned analyses and pins the verdict for each way the expected set can be wrong: a category filed on an earlier commit or another ref, a set split over pages, indexing that lands an attempt late, a call that fails once, a call that fails every time, and an API that stops answering after it has answered. An error on one attempt has to be retried; an error on every attempt still fails the job. The interval between attempts is recorded rather than waited out, and the test cannot speak to a body written to recognise its own launch and skip in the job, which review decides rather than a sibling test. An API that never answered is reported as such rather than as an empty set of analyses, and the fixtures carry an unexpected category at each end of the sort order and a substitution at each end, where a comparison that tolerates a longer list, or counts rather than compares, would pass. A commit sharing seven hex with the scanned one carries the whole expected set in another, and a category differing only in case appears beside the expected one as well as in its place, so an abbreviated match and a case-folded comparison are each red. The analyses are read with --paginate: the ref lists every analysis ever filed on it, newest first, and this pull request's ref passed a hundred in a little over a day, so a single page is a truncated answer that hides an older commit's analyses. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
kalyazin
force-pushed
the
impl-d035/security
branch
from
September 28, 2026 11:23
3caa9a4 to
05a7fcf
Compare
kalyazin
marked this pull request as ready for review
September 28, 2026 11:48
bchalios
approved these changes
Sep 28, 2026
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
2 participants
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.
Adds a security workflow to
feat_write_protection, which carries no analysis today: CodeQL for rust and c-cpp with the query suite named rather than inherited, OSV-Scanner over a generated lockfile, and a third job that checks the analyses actually arrived. A second workflow runs the script tests and asserts the shape of both workflow files against exact allowlists — pinned action revisions and image digests, branch filters, per-job permissions, upload categories, and which script each step runs.Findings never fail a job. They land in the scanned ref's alert list and the job still succeeds. A red job means the scan did not run, its results were not its own, or its analyses were not seen in code scanning under exactly
/language:rust,/language:c-cppandosv-scanner— never that something was found. Code scanning's own check does fail a pull request on error, critical or high alerts in changed lines; gating the lockfile scan as well means failing the job on exit 1.The exit code is the verdict, not the results file's content. A scanner that cannot reach api.osv.dev still writes a well-formed zero-result SARIF, byte-identical to a clean scan's, which on upload files as a clean analysis of the branch.
.github/scripts/scan.shaccepts 0 and 1 and nothing else; its test feeds it 2, 127 and 255 under a stubbed docker and checks the argv, the lockfile it is handed at the moment of the call, and that the results are not rewritten after it.The crate ships no lockfile and
.gitignoreexcludes one, so the job generates it and hands the scanner that single file by path, mounted read-only. The scan therefore judges the newest versions the manifests allow, not the versions the repository consuming this branch pins in its own lockfile.Alerts land on the scanned ref. The Security tab's alerts page defaults to the default branch, so until this branch is it, reaching an advisory means the branch filter or the check's "View all branch alerts" link.
What is left is not in any file here. Making this the default branch moves four things at once — the weekly schedule, the
dependabot.ymlDependabot reads, alert visibility, and the Actions-tab button. It has to be done in Settings rather than by renaming the branch: a rename rewrites no file, so the branch filters and the name the shape test pins would still name the old one. Dependabot version updates must be switched on separately; a checked-independabot.ymldoes not enable them on a fork.Verified on this head: eight check runs green, three analyses filed on the pull-request merge ref under the three expected categories. Earlier, a probe head that uploaded a fourth category drove the verify job red on it while all three scan jobs stayed green.