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) {