From 080b46f3efeeaadbe9093c5b8d554f8f3f681219 Mon Sep 17 00:00:00 2001 From: Fabio Bonelli Date: Fri, 4 Sep 2026 06:49:20 +0200 Subject: [PATCH] fix: write an analysis namespace another writer changed meanwhile The handler compared each incoming namespace with a copy of the row read before the merge and dropped the ones that looked unchanged. A namespace another writer changed between that read and the merge was never written, and the client got 200 with the other writer's value. The merge now compares against the row it updates: an unchanged namespace keeps its timestamp, a changed one is written. --- CHANGELOG.md | 2 + internal/database/analysis.go | 35 +++++++++++-- internal/database/analysis_test.go | 56 +++++++++++++++++++++ internal/handlers/catalogs.go | 4 +- internal/handlers/software.go | 36 ++------------ software_test.go | 79 ++++++++++++++++++++++++++++++ 6 files changed, 171 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a597ba8..afb123f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 still notified the webhooks of an update that never happened. - On SQLite, a stored analysis namespace that was not a JSON object made every later `PATCH` on that row answer `500`. +- A `PATCH` on the analysis dropped a namespace that another writer had + changed since the client read it. ### Security diff --git a/internal/database/analysis.go b/internal/database/analysis.go index 37fbcd0..b014603 100644 --- a/internal/database/analysis.go +++ b/internal/database/analysis.go @@ -62,6 +62,12 @@ func MergeAnalysis( return merged, nil } +// analysisMergeExpression builds the SET expression of the analysis merge. +// Each incoming namespace is compared with the stored one, "t" excluded: +// equal keeps the stored namespace and its timestamp, different writes the +// incoming one. The comparison runs on the row being updated, so a +// namespace another writer changed after the client read it is still +// written. func analysisMergeExpression(dialect Dialect, patch common.AnalysisData) (clause.Expr, error) { patchJSON, err := json.Marshal(patch) if err != nil { @@ -70,8 +76,18 @@ func analysisMergeExpression(dialect Dialect, patch common.AnalysisData) (clause switch dialect { case Postgres: + // Postgres refuses to delete a key from a jsonb scalar, so when + // either side is not an object the incoming value is taken as is. return gorm.Expr( - "COALESCE(analysis, '{}'::jsonb) || CAST(? AS jsonb)", + `COALESCE(analysis, '{}'::jsonb) || ( + SELECT COALESCE(jsonb_object_agg(patch.key, CASE + WHEN jsonb_typeof(analysis -> patch.key) <> 'object' + OR jsonb_typeof(patch.value) <> 'object' THEN patch.value + WHEN (analysis -> patch.key) - 't' = patch.value - 't' THEN analysis -> patch.key + ELSE patch.value + END), '{}'::jsonb) + FROM jsonb_each(CAST(? AS jsonb)) AS patch + )`, string(patchJSON), ), nil case SQLite: @@ -82,18 +98,27 @@ func analysisMergeExpression(dialect Dialect, patch common.AnalysisData) (clause // string comes back unquoted, true and false as 1 and 0. The -> // operator returns every value as JSON text, so the namespace is read // with it from the document instead. + // SQLite compares the two namespaces as text, so the keys must be in + // the same order on both sides. A namespace written through this API + // keeps the key order of the request, so it matches. One stored in + // another order counts as changed and only gets a fresh timestamp. return gorm.Expr( `( WITH patch AS MATERIALIZED ( SELECT key, json -> fullkey AS value FROM json_each(json(?)) + ), stored AS MATERIALIZED ( + SELECT key, json -> fullkey AS value FROM json_each(COALESCE(analysis, '{}')) ) SELECT json_group_object(merged.key, json(merged.value)) FROM ( - SELECT key, json -> fullkey AS value - FROM json_each(COALESCE(analysis, '{}')) - WHERE key NOT IN (SELECT key FROM patch) + SELECT key, value FROM stored WHERE key NOT IN (SELECT key FROM patch) UNION ALL - SELECT key, value FROM patch + SELECT patch.key, CASE + WHEN json(json_remove(stored.value, '$.t')) = json(json_remove(patch.value, '$.t')) + THEN stored.value + ELSE patch.value + END + FROM patch LEFT JOIN stored ON stored.key = patch.key ) AS merged )`, string(patchJSON), diff --git a/internal/database/analysis_test.go b/internal/database/analysis_test.go index a5463b9..eff3906 100644 --- a/internal/database/analysis_test.go +++ b/internal/database/analysis_test.go @@ -318,3 +318,59 @@ func decodeValue(t *testing.T, value json.RawMessage) any { return decoded } + +func TestMergeAnalysisKeepsTheStoredNamespaceWhenOnlyTheTimestampDiffers(t *testing.T) { + gormdb := openAnalysisTestDatabase(t) + recordID := utils.UUIDv4() + + // The stored and the incoming namespace use the same key order, as the + // API writes them, so the textual comparison on SQLite matches. + require.NoError(t, gormdb.Create(&analysisMergeRecord{ + ID: recordID, + Analysis: common.AnalysisData{ + "scanner": json.RawMessage(`{"v":1,"score":90,"t":"2020-01-01T00:00:00Z"}`), + }, + }).Error) + t.Cleanup(func() { + require.NoError(t, gormdb.Delete(&analysisMergeRecord{}, "id = ?", recordID).Error) + }) + + target := analysisMergeRecord{ID: recordID} + merged, err := MergeAnalysis(gormdb, &target, common.AnalysisData{ + "scanner": json.RawMessage(`{"v":1,"score":90,"t":"2026-09-04T10:00:00Z"}`), + }) + require.NoError(t, err) + assert.JSONEq(t, `{"v":1,"score":90,"t":"2020-01-01T00:00:00Z"}`, string(merged["scanner"])) + + var stored analysisMergeRecord + + require.NoError(t, gormdb.First(&stored, "id = ?", recordID).Error) + assert.JSONEq(t, `{"v":1,"score":90,"t":"2020-01-01T00:00:00Z"}`, string(stored.Analysis["scanner"])) +} + +func TestMergeAnalysisWritesTheIncomingNamespaceWhenTheValueDiffers(t *testing.T) { + gormdb := openAnalysisTestDatabase(t) + recordID := utils.UUIDv4() + + require.NoError(t, gormdb.Create(&analysisMergeRecord{ + ID: recordID, + Analysis: common.AnalysisData{ + "scanner": json.RawMessage(`{"v":1,"score":50,"t":"2020-01-01T00:00:00Z"}`), + }, + }).Error) + t.Cleanup(func() { + require.NoError(t, gormdb.Delete(&analysisMergeRecord{}, "id = ?", recordID).Error) + }) + + target := analysisMergeRecord{ID: recordID} + merged, err := MergeAnalysis(gormdb, &target, common.AnalysisData{ + "scanner": json.RawMessage(`{"v":1,"score":90,"t":"2026-09-04T10:00:00Z"}`), + }) + require.NoError(t, err) + assert.JSONEq(t, `{"v":1,"score":90,"t":"2026-09-04T10:00:00Z"}`, string(merged["scanner"])) + + var stored analysisMergeRecord + + require.NoError(t, gormdb.First(&stored, "id = ?", recordID).Error) + assert.JSONEq(t, `{"v":1,"score":90,"t":"2026-09-04T10:00:00Z"}`, string(stored.Analysis["scanner"])) +} diff --git a/internal/handlers/catalogs.go b/internal/handlers/catalogs.go index 9daaa99..ebfebd8 100644 --- a/internal/handlers/catalogs.go +++ b/internal/handlers/catalogs.go @@ -604,9 +604,7 @@ func (c *Catalog) PatchCatalogAnalysis(ctx *fiber.Ctx) error { return common.Error(fiber.StatusUnprocessableEntity, errMsg, "invalid or malformed JSON") } - // Validate and timestamp only the changed namespaces. The returned map - // is a database patch, not a copy of the complete analysis document. - patch, err := timestampTouchedAnalysis(catalog.Analysis, incoming, time.Now()) + patch, err := common.WithTimestamps(incoming, time.Now()) if err != nil { return common.Error(fiber.StatusUnprocessableEntity, errMsg, err.Error()) } diff --git a/internal/handlers/software.go b/internal/handlers/software.go index 81661e5..dd90a70 100644 --- a/internal/handlers/software.go +++ b/internal/handlers/software.go @@ -338,28 +338,18 @@ func (p *Software) GetSoftwareAnalysis(ctx *fiber.Ctx) error { func (p *Software) PatchSoftwareAnalysis(ctx *fiber.Ctx) error { const errMsg = "can't update Software analysis" - found, err := findOne[models.Software](p.db, ctx.Params("id"), findOptions{ - title: errMsg, - name: softwareEntityName, - }) - if err != nil { - return err - } - - software := *found - var incoming common.AnalysisData if err := json.Unmarshal(ctx.Body(), &incoming); err != nil { return common.Error(fiber.StatusUnprocessableEntity, errMsg, "invalid or malformed JSON") } - // Validate and timestamp only the changed namespaces. The returned map - // is a database patch, not a copy of the complete analysis document. - patch, err := timestampTouchedAnalysis(software.Analysis, incoming, time.Now()) + patch, err := common.WithTimestamps(incoming, time.Now()) if err != nil { return common.Error(fiber.StatusUnprocessableEntity, errMsg, err.Error()) } + software := models.Software{ID: ctx.Params("id")} + merged, err := database.MergeAnalysis(writeDB(ctx, p.db), &software, patch) if err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { @@ -372,26 +362,6 @@ func (p *Software) PatchSoftwareAnalysis(ctx *fiber.Ctx) error { return ctx.JSON(merged) } -// timestampTouchedAnalysis returns the added or changed namespaces with a -// fresh timestamp. Namespaces absent from updated are deliberately omitted: -// the database merges this patch atomically with the value it currently has. -func timestampTouchedAnalysis( - original, updated common.AnalysisData, - now time.Time, -) (common.AnalysisData, error) { - touched := make(common.AnalysisData) - - for namespace, value := range updated { - originalValue, exists := original[namespace] - if !exists || string(originalValue) != string(value) { - touched[namespace] = value - } - } - - //nolint:wrapcheck // validation errors are safe API messages with their namespace prefix - return common.WithTimestamps(touched, now) -} - // softwareURLFilter narrows stmt to the software owning the ?url query // parameter. found is false when no software has that url, so the caller // answers with an empty list instead of running the query. title is the diff --git a/software_test.go b/software_test.go index fa912e0..a069c7e 100644 --- a/software_test.go +++ b/software_test.go @@ -1914,6 +1914,85 @@ func TestSoftwareAnalysisDBChecks(t *testing.T) { assert.Contains(t, analysis, "scanner-one") assert.Contains(t, analysis, "scanner-two") }) + + t.Run("PATCH analysis keeps the timestamp of a namespace it does not change", func(t *testing.T) { + loadFixtures(t) + + const softwareID = "59803fb7-8eec-4fe5-a354-8926009c364a" + const storedAt = "2020-01-01T00:00:00Z" + + writeAnalysis(t, softwareID, `{"scanner":{"v":1,"score":90,"t":"`+storedAt+`"}}`) + + code, body := patch(t, "/v1/software/"+softwareID+"/analysis", + "application/merge-patch+json", `{"scanner": {"v": 1, "score": 90}}`) + + assert.Equal(t, 200, code, "body: %s", body) + + scanner := storedNamespace(t, softwareID, "scanner") + assert.Equal(t, storedAt, scanner["t"]) + assert.Equal(t, float64(90), scanner["score"]) + }) + + t.Run("PATCH analysis writes a namespace another writer changed", func(t *testing.T) { + loadFixtures(t) + + const softwareID = "59803fb7-8eec-4fe5-a354-8926009c364a" + const body = `{"scanner": {"v": 1, "score": 90}}` + + code, response := patch(t, "/v1/software/"+softwareID+"/analysis", "application/merge-patch+json", body) + require.Equal(t, 200, code, "body: %s", response) + + seeded := assertRFC3339(t, storedNamespace(t, softwareID, "scanner")["t"]) + + writeAnalysis(t, softwareID, `{"scanner":{"v":1,"score":50,"t":"2020-01-01T00:00:00Z"}}`) + + code, response = patch(t, "/v1/software/"+softwareID+"/analysis", "application/merge-patch+json", body) + assert.Equal(t, 200, code, "body: %s", response) + + scanner := storedNamespace(t, softwareID, "scanner") + assert.Equal(t, float64(90), scanner["score"]) + assert.False(t, assertRFC3339(t, scanner["t"]).Before(seeded)) + }) + + t.Run("PATCH analysis with an identical body keeps the timestamp", func(t *testing.T) { + loadFixtures(t) + + const softwareID = "59803fb7-8eec-4fe5-a354-8926009c364a" + const body = `{"scanner": {"v": 1, "score": 90}}` + + code, response := patch(t, "/v1/software/"+softwareID+"/analysis", "application/merge-patch+json", body) + require.Equal(t, 200, code, "body: %s", response) + + first := storedNamespace(t, softwareID, "scanner")["t"] + + code, response = patch(t, "/v1/software/"+softwareID+"/analysis", "application/merge-patch+json", body) + assert.Equal(t, 200, code, "body: %s", response) + + assert.Equal(t, first, storedNamespace(t, softwareID, "scanner")["t"]) + }) +} + +// writeAnalysis sets the analysis column of a software row directly, as +// another writer would between the client's read and its PATCH. +func writeAnalysis(t *testing.T, softwareID, document string) { + t.Helper() + + query := "UPDATE software SET analysis = " + placeholder(1) + " WHERE id = " + placeholder(2) + + _, err := db.Exec(query, document, softwareID) + require.NoError(t, err) +} + +// storedNamespace decodes one namespace out of the analysis column of a +// software row. +func storedNamespace(t *testing.T, softwareID, namespace string) map[string]any { + t.Helper() + + var analysis map[string]map[string]any + + require.NoError(t, json.Unmarshal([]byte(dbValue(t, "software", "analysis", "id", softwareID)), &analysis)) + + return analysis[namespace] } func TestSoftwareDeleteDBChecks(t *testing.T) {