Fix the four open CodeQL alerts - #2
Merged
Merged
Conversation
CodeQL flagged the conversion in readSchemaVersion, and it is a real defect rather than a lint nit. strconv.Atoi returns a 64-bit int that was narrowed to core.ColID (uint32), and the loop discarded the parse error while the unmarshal above it discarded its own. The consequence is column id reuse. nextColID derives the next id from v.Columns and v.Dropped together, so the Dropped half exists precisely to stop an id an earlier epoch retired from being handed out again (§10.5 rule 2). An entry that truncates or fails to parse silently vanishes from that set, and the next ALTER can then reissue an id whose sidecar column still holds the old column's values. Sidecar columns are append-only and named by column id, so the new column would write into the retired one's history. Now parsed with ParseUint(k, 10, 32) — the exact width of core.ColID, so a key that does not fit is an error instead of a different number — and both errors are returned in the style of the colsJSON error just above. The write side already round-trips safely: a uint32 always fits the int that Itoa takes. Covered by test/integration, which is where the store's coverage lives; internal/store has no unit tests of its own. Passes on PostgreSQL 16, PostgreSQL 17 and MySQL 8.4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CodeQL opened one alert per job in ci.yml: build, property and frozen all ran with whatever the default GITHUB_TOKEN scope happens to be, rather than a scope the workflow states. None of the three needs write. They check out the repository, build, and run tests; nothing pushes, comments, or publishes. sdk.yml and sdk-release.yml already declare their own permissions, which is why only this file was flagged. Declared once at the workflow level instead of three times per job. That closes all three alerts and, more usefully, means a job added to this file later inherits read-only by default and has to ask for write explicitly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CodeQL opened a new high alert on the previous commit, at the write side of the same round-trip: dropped[strconv.Itoa(int(id))]. The commit message asserted that half was already safe because "a uint32 always fits the int that Itoa takes". That is true on a 64-bit build and false on a 32-bit one, where int is 32 bits and a ColID above 2^31-1 converts to a negative number. Itoa would then write a key with a minus sign that the new ParseUint on the read side rejects, turning a silent truncation into a hard read failure of the schema version. The alert only appeared now because ParseUint is a source the query tracks and Atoi's int result was not, so fixing the read side is what exposed the write side rather than what broke it. Both conversions now widen instead of narrowing, which is safe on every platform: FormatUint(uint64(id), 10) as the exact mirror of ParseUint(k, 10, 32), and int64 for the mask width, which is only ever written to a column and never read back into Go. Worth recording that 32-bit is theoretical for this project today: GOARCH=386 does not build at all, on pre-existing MaxSeqValue overflows in retention.go that have nothing to do with schema. That makes this a latent bug rather than a live one, and the correct conversion is free either way. Integration suite passes on PostgreSQL 16, PostgreSQL 17 and MySQL 8.4. 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
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.
Closes the four code scanning alerts open on
main. Two independent fixes, one commit each.Parse dropped column ids at the width they are stored in (#4, High)
readSchemaVersionusedstrconv.Atoi— a 64-bitint— and narrowed theresult to
core.ColID(uint32), discarding the parse error; thedroppedunmarshal just above discarded its own error too.
This is a real defect, not a lint nit.
nextColIDderives the next column idfrom
v.Columnsandv.Droppedtogether, so theDroppedhalf existsprecisely to stop an id an earlier epoch retired from being reissued (§10.5
rule 2). An entry that truncates or fails to parse silently vanishes from that
set, and the next
ALTERcan then hand out an id whose sidecar column stillholds the retired column's values. Sidecar columns are append-only and named by
column id, so the new column would write into the old one's history.
Now
ParseUint(k, 10, 32)— the exact width ofcore.ColID, so a key that doesnot fit is an error rather than a different number — with both errors returned
in the style of the
colsJSONerror above it. The write side alreadyround-trips safely: a
uint32always fits theintthatItoatakes.Drop the CI token to read-only (#1, #2, #3, Medium)
build,propertyandfrozenran with whatever the defaultGITHUB_TOKENscope happens to be. None of them needs write — they check out, build, and run
tests. Declared
permissions: contents: readonce at the workflow level ratherthan three times per job, so a job added later inherits read-only and has to ask
for write explicitly.
sdk.ymlandsdk-release.ymlalready declare their own,which is why only
ci.ymlwas flagged.Verification
go build ./...clean andmake lintpasses, including theinternal/modelimport rule. The 80-test integration suite passes against PostgreSQL 16,
PostgreSQL 17 and MySQL 8.4 — that is where the store's coverage lives, since
internal/storehas no unit tests of its own.🤖 Generated with Claude Code