Repository navigation
fix: resolve 15 CodeQL alerts and a proxy sample-count wrap - #29
Merged
Merged
Conversation
A proxy's per-site sample count (sn) grows by one per observation and is never capped, but proxyapi carries it as an int32: past 2^31-1 observations ListProxies/GetProxy returned a negative or tiny number (4294967297 came out as 1). CodeQL flagged the int(int64) at proxy/hot.go but not the int32() at proxyapi/convert.go where the wrap actually happens, because its flow stops at the first conversion it reports. Saturate where the counters are read back from Valkey, with the clampInt32 the snapshot writer already uses: the proxy site counters in proxy/hot.go and the identity counters in hotstate/reader.go (global samples, active leases, per-group samples and failure streak). The identity API already clamped its own copies, so for it this only makes 32-bit builds correct. Resolves code-scanning alerts #4-#9 (go/incorrect-integer-conversion).
Neither conversion could wrap, but CodeQL could not see the bound. - A proxy import's max_concurrency was parsed with Atoi and only range checked later, by Attributes.validate. Parsing it with bit size 32 makes the later int32 conversions provably lossless; a value outside the int32 range now fails while parsing, with the same message the range check gives. - ListSecretAccessLogs bounded the version with min(), which CodeQL does not model. It now uses the package's clampInt32. Resolves code-scanning alerts #3, #10 and #11 (go/incorrect-integer-conversion).
parseAcquire and reapBatch narrowed int64 reply fields to int: the lease count, payload and type versions, the rebind counter and the reaper's batch count. Every one fits in 32 bits by construction, so nothing wraps on the 64-bit images, but on a 32-bit build a value like 4294967297 would become 1 and, for the lease count, pass the reply-length check. A small atoi helper parses with bit size 32, which saturates instead of wrapping. Resolves code-scanning alerts #12-#16 (go/incorrect-integer-conversion).
The FastAPI example returned str(UpstreamError) to its HTTP caller. That text is only "request to the target failed: <httpx class>", so no message or traceback leaked, but it is exception data on an unauthenticated endpoint of code people copy. The reason is now the fixed text; the class still goes to the log, and Spinneret already gets the error_kind with the report. Resolves code-scanning alert #1 (py/stack-trace-exposure).
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.
What and why
CodeQL had 16 open alerts on main. None of them is exploitable. The release images are linux/amd64 and linux/arm64 only, where
intis 64 bits. Every flagged value is either already bounded or written by this service's own Lua scripts. Triaging them still turned up one real bug on one of the same data flows. Fixing the other alerts costs only a few lines each, which is better than 15 dismissals.The bug. A proxy's per-site sample count (
sn) grows by one per observation and is never capped, butproxyapicarries it as anint32. Past 2^31−1 observations,ListProxiesandGetProxyreturn a negative or tiny number: 4294967297 comes out as 1. CodeQL flagged theint(int64)inproxy/hot.gobut not theint32(s.Samples)atproxyapi/convert.go:50, which is where the wrap actually happens. Its flow stops at the first conversion it reports.hotstate/reader.go,proxy/hot.goclampInt32where they are read back from Valkey. This fixes the wrap above.api/secretapi/secrets.gomin(v, math.MaxInt32)with the package'sclampInt32. CodeQL does not modelmin.proxy/importparse.gomax_concurrencywith bit size 32. A value outside the int32 range now fails while parsing, with the range check's own message.scheduler/args.go,scheduler/reap.goatoihelper that parses Lua reply fields with bit size 32. On a 32-bit build it saturates instead of wrapping.examples/fastapi-crawler/app/main.pyreasoninstead ofstr(err). The exception class still goes to the log.This PR does not change #2 (
js/insecure-randomnessinweb/e2e/access.spec.ts). There,Math.random()only makes e2e resource names unique; CodeQL flags it because the variable is calledusername. The plan is to dismiss it as "Used in tests".How it was verified
go build ./...passes on darwin/arm64 and withGOOS=linux GOARCH=386.go vetpasses on the four packages on both. golangci-lint v2.13.2 reports 0 issues across the module (--build-tags e2e).go test -short -racepasses on secretapi, proxy, hotstate and scheduler. The container-backedTestIdentityHotStateandTestListProxiesHotState(PostgreSQL + Valkey) also pass.TestParseLinesgets an extra parsed row for line 9.examples/fastapi-crawler:pytestgives 15 passed and 3 skipped (the integration tests needEXAMPLE_URL).Checklist
documents/en/anddocuments/zh/both updated, and they say the same thing: not needed, since they only quotecrawler.py, which is unchangedCHANGELOG.mdupdated if a user can see the changemake generate, not edited by hand: no generated code touched