security: triage the five CodeQL alerts, two of which are real

[aenawi]

CodeQL's first run reported five high-severity alerts. Two are real, three are false
positives. Each verdict below is about where the value comes from, not about how noisy the
rule is in general.

Real: go/incorrect-integer-conversion at internal/config/config.go:115 and :122 (#3, #4)

UHP_MAX_CONCURRENT_RUNS and UHP_TASK_MAX_STEP are parsed with strconv.ParseInt into
an int64 and were converted straight to the int their Config fields are declared as.
The value is operator-controlled — an environment variable read at startup — so no
privilege boundary is crossed and this is not an attack path. It is still a real defect,
because int is as wide as the machine: the conversion is the identity on the amd64 and
arm64 builds this project ships, and a silent truncation on a 32-bit build, which
go install produces happily and nothing here argues against.

The failure direction is what makes it worth fixing rather than filing. On 32-bit,
UHP_TASK_MAX_STEP=4294967296 keeps only its low half and arrives as 0, and
2147483648 arrives negative — and zero and negative are both how TaskMaxStep and
MaxConcurrentRuns spell no limit of this deployment's own. An operator reaching for an
enormous ceiling would get none at all, which is the direction #72 says a bound must never
fail in.

Both now go through getEnvIntCapped, saturating at math.MaxInt32 on every platform
rather than at math.MaxInt, so the setting means the same thing regardless of the word
size it was compiled for. MaxBodyBytes keeps getEnvInt, its field being int64.

The regression test cannot observe the truncation on the machine running it, so it pins
the cap instead — the property that makes the two platforms agree. It fails on three of
its five cases before the change, each returning the unclamped value.

False positive: go/uncontrolled-allocation-size at internal/store/sqlite.go:532 (#5)

CodeQL is right about the source. ?limit= is read off the query string with
strconv.Atoi in handleListSessions, the error is discarded, and it reaches the store
with no range applied at the transport — the value is a stranger's, and it sizes a make().

It is nonetheless bounded. ListSessions clamps to [1,100] in its first four lines,
limit is not reassigned between there and the allocation, and the worst case is 100
pointers. Both stores clamp identically. What CodeQL misses is that the sanitiser is a
reassignment inside the if rather than an early return, so the path where the value is
already in range is followed without recording that being in range is what put it there.

No code change, but the clamp now carries a comment: it read like a default page size,
when it is in fact the only thing between one request and eight gigabytes of preallocated
pointers. A later refactor widening it has to argue with that comment first.

False positive: go/allocation-size-overflow at uhp/uhpgo/uhpgo.go:120 (#1, #2)

make([]byte, 0, len(wire)+len(ext)), one alert per operand. Both operands are the
lengths of []byte values json.Marshal has already returned and which are live in
memory at that line. Two allocations that already succeeded cannot sum past the integer
type that measures either one without the process having exhausted its address space
first — the arithmetic is bounded by the allocator, whoever controls the harness content.
Even granting the impossible, a negative capacity panics in make; it does not
under-allocate, so there is no memory-safety consequence on the other side either. No
inline suppression exists for Go, so these are dismissed via the API with that reasoning.

Verification

make verify (golangci-lint 0 issues, full suite green under -race, script tests, build)
and make security-push (gosec and gitleaks clean). govulncheck's stdlib findings are
against the local 1.25.0 toolchain and unrelated; CI builds with 1.26.

Whether CodeQL should start gating is a separate question, answered in a comment here once
this run is green rather than changed in this PR.

[aenawi]

All five are now resolved: #3 and #4 closed as fixed by the analysis on main, #1, #2 and #5 dismissed as false positives with their reasoning. Zero open alerts.

Should CodeQL start failing the build?

Yes — the reason it does not is spent. The workflow comment says the backlog is why findings go to the Security tab rather than the build, and that it should gate "once the backlog is triaged". The backlog is zero, so gating from here can only break on something new, which is the signal worth having and not the day-one red build the comment was avoiding. gosec already hard-fails in make security-push, so this is consistency rather than a new posture.

Three things to get right when doing it, none of which are a workflow edit:

It is a ruleset setting, not codeql.yml. Analyze Go is already a required check, and passing it only means the analysis ran — it says nothing about what it found. Gating is the "require code scanning results" rule on the branch ruleset, naming the CodeQL tool and a threshold. Editing the workflow would not do it.

Set the threshold at security-severity high, not at all alerts. All five triaged here were high. Dropping the bar to note-level would put the tone-ish Go queries in front of every PR, which is how a gate gets switched off.

The false-positive rate is the ongoing cost, and it is not small. Three of five were false positives, and Go has no inline suppression — every one costs a PATCH to the alerts API by someone with security-events: write, and the reasoning lives in the API rather than next to the code. That is survivable at this size and worth saying out loud, because it is the thing that makes a gate annoying rather than the alerts themselves. Note that all three false positives were in pre-existing code; the rate on newly written code is the one that will actually be paid, and this run says nothing about it.

Not changed here, per the scope of this PR. Related: #119.

[aenawi]

Follow-up: the gate is now live, so the comment above is history rather than a proposal.

One correction to what it said. Gating is a rulesets feature and this repository had no rulesets — main was on classic branch protection, which has no code-scanning rule at all. So it is a new ruleset layered on top of the classic protection rather than an edit to it; both apply and the more restrictive wins.

Ruleset 22523328, "CodeQL results must be clean on main", active, targeting refs/heads/main, tool CodeQL at security_alerts_threshold: high_or_higher and alerts_threshold: errors. No bypass actors — it applies to admins too, on the same reasoning that gives gosec no excludes. The escape hatch for a false positive is the one already exercised here: dismiss it with a reason, which is a record rather than a hole.

Verified rather than assumed, in #123: a branch introducing one deliberate go/uncontrolled-allocation-size was refused. The probe was chosen so the refusal could only come from one place — golangci-lint and gosec both pass an unbounded make(), gosec having no rule for allocation size — and the result separates the two things this comment claimed were different:

build       pass      image      pass      quality  pass
scan        pass      Analyze Go pass
CodeQL      FAIL      merge state: BLOCKED

Analyze Go green and CodeQL red at the same time is the distinction exactly: the required check says the analysis ran, and the new one says what it found. #123 is closed unmerged and its branch deleted; zero open alerts.