fix: address the two code scanning alerts - #25
Merged
Conversation
The fake server's deepMerge copied every key of the incoming partial straight onto the destination, and that partial is parsed off a websocket frame. A frame carrying __proto__ would have rewritten the prototype instead of setting a property. Nothing can reach this but the tests themselves, so this is not a live exposure -- but the guard is one line, which beats teaching CodeQL to ignore the write. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`res.ok` is true for any 200, including the HTML login page a captive portal or a corporate proxy serves. That page was written straight over a working javascript.d.ts, and the damage surfaces later as every script in the editor lighting up with TypeScript errors -- with nothing pointing back at `iob-sync types`. Check three things before the write: the announced content-length, the actual body size, and that the body contains a declaration at all. Each throws into the existing catch, which already warns and keeps the copy on disk, so a portal response now leaves a good cached file intact. The size cap bounds what reaches the disk, not what reaches memory -- res.text() has already buffered the body by then. Bounding that too means streaming the response, which is more than a hardcoded raw.githubusercontent.com URL warrants. The download had no coverage before; it now has the happy path, the portal case, and the oversize case, against a stubbed fetch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Two CodeQL alerts, with opposite conclusions.
test/fake-server.ts:143— remote property injection (error)Real dataflow.
extendObjectpullspartialout of a JSON-parsed websocket frameand hands it to
deepMerge, which writesdest[key]for every key in it, so aframe carrying
__proto__would have rewritten the prototype rather than settinga property.
Not a live exposure — the fake server exists only inside the test process and only
ever talks to this repo's own client — but the guard is one line, which beats a
suppression.
src/commands/types.ts:126— network data written to file (warning)I suggest dismissing this one as by-design.
js/http-to-file-accessflags anyremote data reaching the filesystem, and downloading
javascript.d.tsinto.iobroker/types/is precisely whatiob-sync typesis for. The URL is ahardcoded HTTPS constant, the destination path is fixed, and a
.d.tsis typeinformation
tscreads but never executes. There is no sanitizer for this rule, sothe alert will probably survive this PR.
It did point sideways at a real bug, though.
res.okis true for any 200 —including the HTML login page a captive portal or corporate proxy serves. That page
was written straight over a working
javascript.d.ts, and the failure surfaceslater as every script in the editor lighting up with TypeScript errors, with nothing
pointing back at
iob-sync types.So, three checks before the write: the announced
content-length, the actual bodysize, and whether the body contains a declaration at all. Each throws into the
existing
catch, which already warns and keeps whatever copy is on disk — so aportal response now leaves a good cached file intact instead of destroying it.
Known limit: the size cap bounds what reaches disk, not what reaches memory —
res.text()has already buffered the body by then. Thecontent-lengthcheckcovers the honest-server case; a server lying about the length could still make the
process buffer a large body. Bounding that properly means streaming the response,
which seemed disproportionate here. Happy to add it if you disagree.
Tests
The download path had no coverage at all — the old file header said so outright. It
now has three cases against a stubbed
fetch: the happy path, the portal caseasserting the good copy survives, and the oversize case asserting nothing is
written.
277 tests pass; ESLint and Prettier clean.
🤖 Generated with Claude Code