fix(security): act on the first CodeQL baseline (TOCTOU + unquoted setup path) - #51
Merged
Merged
Conversation
The tool documents "文件已存在，不覆盖", and enforced it by `access()`-ing the path and then writing with a plain `writeFile`. Those two steps are a TOCTOU: between the check and the write, a concurrent call — or a symlink planted at the path — can create the target, and the write then clobbers it silently. The promise was therefore only true when nothing else touched the path in between. Found by the CodeQL baseline added in #50 (`js/file-system-race`, the only one of the 29 first-baseline alerts that sat in this repository's own runtime code rather than generated docs, test fixtures or by-design flows). Create with `flag: 'wx'` (O_CREAT|O_EXCL) instead, which makes existence-check and creation one atomic syscall: the kernel refuses when the path is occupied, including by a dangling symlink. `EEXIST` maps back to the same FILE_EXISTS result the `access()` branch returned, so callers see no change. Covered by two new tests: an existing file keeps its original bytes, and a symlink at the target path does not get followed out of the output root.
`resolveSetup` built `source ${auto} && ` from the auto-detected path with no
quoting, while the `ros2_workspace use` path (`setSessionRosSetup`) has always
run its input through `shq()`. Both end up prepended to a `bash -lc` string, so
the two disagreed about the same hazard: a `workspaceRoot` carrying a space
broke the command, and a metacharacter would have been interpolated straight
into the shell string.
Surfaced by the CodeQL baseline (#50) as
`js/shell-command-constructed-from-input` on the auto-detect branch.
Quote both auto-detect returns with `shq()`. `sourcePath` still carries the raw
filesystem path — consumers `existsSync` it — while the prefix carries the same
path as one safely-quoted shell word.
The existing fallback test asserted the old unquoted prefix, so it is updated
to expect a quoted one, and a new test uses a workspace root containing both a
space and a `;` to pin the property.
The first baseline (from #50) reported 29 alerts, 12 of them in `docs/archify/bridge.html` — a 643 KB GENERATED bundle produced by the archify skill, not source anyone here reviews. That volume buries the alerts that are actually about this repository, which is the entire value of the baseline. Add a CodeQL config with `paths-ignore: docs/archify/**` and point `init` at it. Nothing else is excluded and no query filters are set, so the rest of the tree stays in scope. The two real findings in our own runtime code from that same baseline are fixed in the preceding commits, which is what makes excluding the generated file a triage decision rather than sweeping something under it.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The `CodeQL` pull-request check failed this PR with "1 new alert including 1
high severity security vulnerability", annotated at `runner.spec.ts:73`:
Insecure temporary file — Insecure creation of file in the os temp dir.
That is the new quoting test, which built a fixed
`/tmp/dsh-runner auto ws;${process.pid}` path. Correct catch, and about my own
change rather than pre-existing code: a predictable name under the temp dir is
world-influenceable, and two concurrent runs of the suite would collide on it.
Use `mkdtempSync(path.join(tmpdir(), 'dsh-runner auto ws;'))`, which keeps both
the space and the `;` in the generated path — the properties the test exists to
exercise — and adds a `finally` that removes the tree.
The pre-existing fixed paths in `tempSetup()` are left alone: they are on main
already, were triaged as test-only fixtures, and are not this PR's business.
Adds §21 and refreshes the header: this round's start/end snapshots, the step-2 re-verification of issue #40 (still accurate; 6 dead links, not 5), the four PRs, the Dependabot triage, the phoenix loop with the gen-24 restart request, the full security table, and nine findings. Three things in it are worth reading even if the rest is skimmed: - §21.3.1 — the `__pycache__` publish guard was applied only to the three packages that happened to have one, with measured before/after tarball evidence that `moveit` leaked and that `safety`'s narrower guard missed a bare `.pyc`; - §21.3.3 — the first CodeQL baseline returned 29 alerts and two real defects, both fixed, plus the reasoning for deliberately NOT fixing the three ReDoS ones in the same change; - §21.5 — why the obvious fix for the "rebuilt lib, no way to tell whether the process reloaded" blind spot would not have caught this round's own drift, and so was designed and documented rather than half-built. Also records two methodology lessons: `gh issue view` fails on gh 2.45.0 (Projects classic deprecation) and needs `gh api`, and a `*.py` glob silently misses this repository's six extensionless Python scripts.
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.
Acting on the first CodeQL baseline (#50). That run produced 29 alerts; this PR fixes the two that are real defects in this repository's own runtime code and stops the generated-artifact noise that was hiding them. Baseline now has a triage instead of a number.
Fixed
1.
js/file-system-race—ros2_interface_createcould be made to overwritepackages/core/src/tools.tsThe tool documents "文件已存在，不覆盖" and enforced it with
access()followed by a plainwriteFile. That is a TOCTOU: between the check and the write a concurrent call — or a symlink planted at the path — can create the target, and the write silently clobbers it.Now created with
flag: 'wx'(O_CREAT|O_EXCL), which makes the check and the creation one atomic syscall.EEXISTmaps back to the sameFILE_EXISTSresult, so callers see no change. Two new tests: an existing file keeps its original bytes, and a symlink at the target is not followed out of the output root.2.
js/shell-command-constructed-from-input— auto-detected setup path was unquotedpackages/common/src/runner.tsresolveSetupbuiltsource ${auto} &&from the auto-detected path with no quoting, whileros2_workspace usehas always run its input throughshq(). Both are prepended to abash -lcstring, so the two disagreed about the same hazard: aworkspaceRootwith a space broke the command, and a metacharacter would have been interpolated into the shell string.Both auto-detect returns now use
shq().sourcePathstill carries the raw filesystem path (consumersexistsSyncit); the prefix carries it as one quoted shell word. The old fallback test asserted the unquoted form, so it is updated, and a new test uses a root containing a space and a;.Triage of the other 27
js/remote-property-injectiondocs/archify/bridge.htmljs/insecure-temporary-filescripts/verification/measure.mjsmkdtempdir; no production path. Not this repository's runtime surfacejs/polynomial-redosparse.ts×2,toolkit.tsjs/file-access-to-httpvision.tsjs/shell-command-injection-from-environmentrunner.ts:38shq()-quoted by callersjs/file-system-racetools.ts:432On the ReDoS alerts, deliberately not fixed here. All three are
(\w+):/(\S+)(?:…)?$style patterns overros2CLI output. Real exploitability is low — the input is a local CLI's stdout — but it is not zero forparseSafetyEcho, whose input is an echoed ROS topic a publisher could craft. Rewriting the parsers to be provably linear changes the shape of parsing that the rest of the tool set depends on, and that deserves its own change with its own tests rather than being smuggled into a security PR. Left visible on purpose.Verification
pnpm run typecheckpnpm run testpnpm audit --prod --audit-level high --registry=https://registry.npmjs.org