From eacc5236d193bc30973c9c5d3b2c6258d18e9e08 Mon Sep 17 00:00:00 2001 From: Pranav M S Date: Wed, 2 Sep 2026 21:55:28 +0530 Subject: [PATCH 1/3] Parse dropped column ids at the width they are stored in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/store/schema.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/internal/store/schema.go b/internal/store/schema.go index c0b15fc..8962e9c 100644 --- a/internal/store/schema.go +++ b/internal/store/schema.go @@ -96,12 +96,20 @@ func (s *Store) loadSchemaAt(ctx context.Context, t *Table, branchID uuid.UUID, } dropped := map[string]int64{} if len(droppedJSON) > 0 { - _ = json.Unmarshal(droppedJSON, &dropped) + if err := json.Unmarshal(droppedJSON, &dropped); err != nil { + return nil, fmt.Errorf("schema version %d has an unreadable dropped set: %w", e, err) + } } for k, at := range dropped { - if id, err := strconv.Atoi(k); err == nil { - v.Dropped[core.ColID(id)] = at + // Parse at the width of core.ColID rather than through int: nextColID + // derives the next column id from v.Dropped, so an entry lost to a + // truncating conversion or a bad key would let it reissue an id an + // earlier epoch already used (§10.5 rule 2). + id, err := strconv.ParseUint(k, 10, 32) + if err != nil { + return nil, fmt.Errorf("schema version %d has a bad dropped column id %q: %w", e, k, err) } + v.Dropped[core.ColID(id)] = at } return v, nil } From c5b3b2d5bdc693fe56e405ed52b2dc989dbd59ae Mon Sep 17 00:00:00 2001 From: Pranav M S Date: Wed, 2 Sep 2026 21:55:36 +0530 Subject: [PATCH 2/3] Drop the CI token to read-only, which is all any job there needs 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 --- .github/workflows/ci.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 46ef9fa..df4c91d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,6 +5,12 @@ on: branches: [main] pull_request: +# Every job here only reads the checkout and runs tests. Granting the default +# token read-only at the workflow level means a new job cannot silently inherit +# write scope on the repository. +permissions: + contents: read + jobs: build: runs-on: ubuntu-latest From cb28b4686eb4d3aafd6577ffe338686a1338209c Mon Sep 17 00:00:00 2001 From: Pranav M S Date: Wed, 2 Sep 2026 22:02:22 +0530 Subject: [PATCH 3/3] Keep column ids off int on the write side too 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 --- internal/store/schema.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/store/schema.go b/internal/store/schema.go index 8962e9c..2f7bb36 100644 --- a/internal/store/schema.go +++ b/internal/store/schema.go @@ -235,7 +235,11 @@ func (s *Store) writeSchemaVersion(ctx context.Context, tx adapter.Tx, t *Table, } dropped := map[string]int64{} for id, at := range v.Dropped { - dropped[strconv.Itoa(int(id))] = at + // FormatUint, not Itoa(int(id)): core.ColID is uint32, and int is 32 bits + // on a 32-bit build, so routing through it could write a negative key that + // the ParseUint on the read side would then reject. This is the exact + // mirror of that parse. + dropped[strconv.FormatUint(uint64(id), 10)] = at } dj, _ := json.Marshal(dropped) @@ -250,7 +254,7 @@ func (s *Store) writeSchemaVersion(ctx context.Context, tx adapter.Tx, t *Table, // The mask width is recorded WITH the version, because changed_cols is over // column ids and only grows; comparing masks across epochs zero-extends the // shorter one (§10.5). - width := int(nextColID(v)) + width := int64(nextColID(v)) return tx.Exec(ctx, s.ad.InsertOnConflict("datagit_schema_version", []string{"table_id", "branch_id", "epoch", "columns", "dropped", "digest",