Address CodeQL and Dependabot findings - #18
Merged
Merged
Conversation
CodeQL reported five alerts and Dependabot one advisory on the freshly enabled scanners. Each is handled below with what it does and does not mean. Dependabot: labstack/echo GHSA-vfp3-v2gw-7wfq (high) ----------------------------------------------------- Bumped 4.13.3 -> 4.15.3, an indirect dependency pulled in by Wails. Scope, measured rather than assumed: echo reaches this module only through wailsapp/wails/v2/internal/frontend/devserver, which is behind `//go:build dev`. `go list -deps ./...` links zero echo packages, `go list -tags dev -deps ./...` links one, and `go version -m` on the published v1.3.0 Windows binary finds no echo at all. So shipped builds never contained the vulnerable code and users were not exposed; the reachable surface was the developer machine during `wails dev`. Worth fixing, not worth an emergency release. CodeQL go/allocation-size-overflow (high) — crypto.go ----------------------------------------------------- EncryptBytes sizes its output buffer as headerLen + len(plaintext) + the GCM tag. That is an int addition, so on a 32-bit build a plaintext near 2 GiB would wrap and under-allocate before the copy ever ran. Its only caller passes a CA bundle of a few kilobytes, so this is not reachable today — but it is an exported entry point, and nothing in the signature says otherwise. Bounded at 64 MiB with an explicit error steering callers with real volume to EncryptFile, which streams instead of holding the whole payload. CodeQL go/unhandled-writable-file-close (4 warnings) — crypto.go, config.go ---------------------------------------------------------------------------- All four sit on error paths that close a file after a failed write and return the original error. Discarding the Close error is defensible there, but not free: on a buffered filesystem a deferred write failure surfaces only at Close, so the discarded error can be the one that says the file is incomplete. The temp file is removed straight after, which limits this to losing a diagnosis rather than corrupting anything. They now join both errors, so whichever one carries the useful detail survives. Also fixes the one real gofmt deviation in the tree (a double space before an inline comment in logstore.go). Note that `gofmt -l` flags three further files locally, which is a Windows CRLF artefact, not a formatting problem — the repo stores LF via .gitattributes, and a Linux checkout reports them clean.
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.
The scanners enabled in #10 produced their first findings: five CodeQL alerts
(one high) and one Dependabot advisory (high). Each is handled below, with what
it does — and does not — mean.
Dependabot:
labstack/echoGHSA-vfp3-v2gw-7wfq (high)Bumped 4.13.3 → 4.15.3. An indirect dependency, pulled in by Wails.
Shipped builds never contained the vulnerable code. Echo reaches this module
only through
wailsapp/wails/v2/internal/frontend/devserver, which is behind//go:build dev. Measured rather than assumed:go list -deps ./...go list -tags dev -deps ./...go version -mon the published v1.3.0 Windows binarySo users were never exposed. The reachable surface was the developer machine
during
wails dev. Worth fixing, not worth an emergency release — which is alsowhy this is a normal PR rather than a hotfix.
CodeQL
go/allocation-size-overflow(high) —crypto.goEncryptBytessizes its output buffer asheaderLen + len(plaintext) + gcm.Overhead(). That is anintaddition, so on a 32-bit build a plaintext near2 GiB would wrap and under-allocate before the copy ran.
Not reachable today: the only caller is
CAStore.Save, passing a CA bundle of afew kilobytes. But it is an exported entry point and nothing in the signature
says otherwise, so it is bounded at 64 MiB with an explicit error steering
callers with real volume to
EncryptFile, which streams. Covered by a test thatchecks both the rejection and that a normal payload still round-trips.
CodeQL
go/unhandled-writable-file-close(4 warnings) —crypto.go,config.goAll four sit on error paths that close a file after a failed write and return the
original error. Discarding the
Closeerror is defensible there — but not free:on a buffered filesystem a deferred write failure surfaces only at
Close, sothe discarded error can be precisely the one saying the file is incomplete. The
temp file is removed immediately after, which limits the cost to losing a
diagnosis rather than corrupting anything.
They now
errors.Joinboth, so whichever error carries the useful detailsurvives.
Also
Fixes the one real
gofmtdeviation in the tree — a double space before aninline comment in
logstore.go, which predates this branch.Worth noting:
gofmt -lflags three further files on a Windows checkout, whichis a CRLF artefact rather than a formatting problem (the repo stores LF via
.gitattributes, and a Linux checkout reports them clean). CI does not checkformatting at all — neither does SnmpLens. Adding
gofmt -lto thequalityjob would close that gap in three lines; left out here to keep this PR to the
alerts.
Verification
go buildfor both the release and thedevtag paths,go vet,staticcheckand
go test -race ./...all pass.