From 206763b7f1ae5e290f935552d4c84829cd9217a5 Mon Sep 17 00:00:00 2001 From: attiasas Date: Wed, 19 Aug 2026 16:05:32 +0300 Subject: [PATCH] Fix go-sarif breaks server-client miss match versions --- commands/upload/uploadcdx.go | 5 + utils/formats/sarifutils/sarifutils.go | 106 ++++++++++- utils/formats/sarifutils/sarifutils_test.go | 184 ++++++++++++++++++++ 3 files changed, 288 insertions(+), 7 deletions(-) diff --git a/commands/upload/uploadcdx.go b/commands/upload/uploadcdx.go index d10870a77..2821c8c19 100644 --- a/commands/upload/uploadcdx.go +++ b/commands/upload/uploadcdx.go @@ -16,6 +16,7 @@ import ( "github.com/jfrog/jfrog-cli-security/utils" "github.com/jfrog/jfrog-cli-security/utils/artifactory" "github.com/jfrog/jfrog-cli-security/utils/formats/cdxutils" + "github.com/jfrog/jfrog-cli-security/utils/formats/sarifutils" "github.com/jfrog/jfrog-cli-security/utils/xray" "github.com/jfrog/jfrog-cli-security/utils/xray/artifact" ) @@ -120,6 +121,10 @@ func (ucc *UploadCycloneDxCommand) Upload() (artifactPath string, err error) { if err != nil { return "", fmt.Errorf("failed to convert CycloneDx content to JSON: %w", err) } + // Xray uses legacy SARIF format, so we need to strip the unset indexes + if outputBytes, err = sarifutils.StripUnsetIndexes(outputBytes); err != nil { + return "", fmt.Errorf("failed to sanitize CycloneDx SARIF indexes: %w", err) + } if ucc.fileToUpload, err = utils.DumpCdxJsonContentToFile(outputBytes, tempDir, ucc.filePrefix, 0); err != nil { return "", fmt.Errorf("failed to save CycloneDx content to file: %w", err) } diff --git a/utils/formats/sarifutils/sarifutils.go b/utils/formats/sarifutils/sarifutils.go index b67601e1a..fd5feb106 100644 --- a/utils/formats/sarifutils/sarifutils.go +++ b/utils/formats/sarifutils/sarifutils.go @@ -1,6 +1,8 @@ package sarifutils import ( + "bytes" + "encoding/json" "fmt" "path/filepath" "strings" @@ -341,11 +343,32 @@ func copyCodeFlow(flow *sarif.CodeFlow) *sarif.CodeFlow { func copyThreadFlow(threadFlow *sarif.ThreadFlow) *sarif.ThreadFlow { copied := &sarif.ThreadFlow{} for _, location := range threadFlow.Locations { - copied.Locations = append(copied.Locations, sarif.NewThreadFlowLocation().WithLocation(CopyLocation(location.Location))) + copied.Locations = append(copied.Locations, copyThreadFlowLocation(location)) } return copied } +func copyThreadFlowLocation(location *sarif.ThreadFlowLocation) *sarif.ThreadFlowLocation { + if location == nil { + return nil + } + return &sarif.ThreadFlowLocation{ + ExecutionOrder: location.ExecutionOrder, + Importance: location.Importance, + Index: location.Index, + Kinds: location.Kinds, + Location: CopyLocation(location.Location), + Module: copyStrAttribute(location.Module), + NestingLevel: location.NestingLevel, + Properties: location.Properties, + State: location.State, + Stack: location.Stack, + Taxa: location.Taxa, + WebRequest: location.WebRequest, + WebResponse: location.WebResponse, + } +} + func copyMsgAttribute(attr *sarif.Message) *sarif.Message { if attr == nil { return nil @@ -387,18 +410,20 @@ func CopyLocation(location *sarif.Location) *sarif.Location { return nil } copied := sarif.NewLocation() - copied.ID = 0 + copied.ID = location.ID if location.PhysicalLocation != nil { copied.PhysicalLocation = sarif.NewPhysicalLocation() if location.PhysicalLocation.ArtifactLocation != nil { - copied.PhysicalLocation.WithArtifactLocation(sarif.NewArtifactLocation().WithURI(GetLocationFileName(location))) - copied.PhysicalLocation.WithRegion(sarif.NewRegion(). - WithCharOffset(0). - WithByteOffset(0). + copied.PhysicalLocation.WithArtifactLocation(sarif.NewArtifactLocation().WithURI(GetLocationFileName(location)).WithIndex(location.PhysicalLocation.ArtifactLocation.Index)) + region := sarif.NewRegion(). WithStartLine(GetLocationStartLine(location)). WithStartColumn(GetLocationStartColumn(location)). WithEndLine(GetLocationEndLine(location)). - WithEndColumn(GetLocationEndColumn(location))) + WithEndColumn(GetLocationEndColumn(location)) + if srcRegion := location.PhysicalLocation.Region; srcRegion != nil { + region.WithCharOffset(srcRegion.CharOffset).WithByteOffset(srcRegion.ByteOffset) + } + copied.PhysicalLocation.WithRegion(region) if snippet := GetLocationSnippetText(location); len(snippet) > 0 { copied.PhysicalLocation.Region.WithSnippet(sarif.NewArtifactContent().WithText(snippet)) } @@ -407,6 +432,8 @@ func CopyLocation(location *sarif.Location) *sarif.Location { copied.Properties = location.Properties for _, logicalLocation := range location.LogicalLocations { logicalCopy := sarif.NewLogicalLocation().WithProperties(logicalLocation.Properties) + logicalCopy.Index = logicalLocation.Index + logicalCopy.ParentIndex = logicalLocation.ParentIndex if logicalLocation.Name != nil { logicalCopy.WithName(*logicalLocation.Name) } @@ -965,3 +992,68 @@ func getResultIdByLocation(result *sarif.Result) string { } return GetResultRuleId(result) + result.Level + GetResultMsgText(result) + GetLocationId(result.Locations[0]) } + +// SARIF fields that go-sarif v3 encodes as signed ints with -1 meaning "unset". +// go-sarif v1.1.1 (Xray) models the same fields as *uint, so a negative value is invalid JSON for that consumer. +var unsignedSarifIndexKeys = map[string]struct{}{ + "index": {}, + "parentIndex": {}, + "ruleIndex": {}, + "invocationIndex": {}, + "resultGraphIndex": {}, + "runGraphIndex": {}, + "executionOrder": {}, + "absoluteAddress": {}, + "id": {}, +} + +// StripUnsetIndexes removes negative SARIF index-like fields from JSON so the payload +// can be decoded by go-sarif v1 (*uint) consumers. Zero and positive values are kept. +func StripUnsetIndexes(content []byte) ([]byte, error) { + decoder := json.NewDecoder(bytes.NewReader(content)) + decoder.UseNumber() + var decoded any + if err := decoder.Decode(&decoded); err != nil { + return nil, fmt.Errorf("decode json for index sanitization: %w", err) + } + stripNegativeIndexFields(decoded) + sanitized, err := json.MarshalIndent(decoded, "", " ") + if err != nil { + return nil, fmt.Errorf("marshal sanitized json: %w", err) + } + return sanitized, nil +} + +func stripNegativeIndexFields(node any) { + switch value := node.(type) { + case map[string]any: + for key, child := range value { + if _, isIndexKey := unsignedSarifIndexKeys[key]; isIndexKey && isNegativeJSONNumber(child) { + delete(value, key) + continue + } + stripNegativeIndexFields(child) + } + case []any: + for _, child := range value { + stripNegativeIndexFields(child) + } + } +} + +func isNegativeJSONNumber(value any) bool { + switch number := value.(type) { + case json.Number: + if asInt, err := number.Int64(); err == nil { + return asInt < 0 + } + asFloat, err := number.Float64() + return err == nil && asFloat < 0 + case float64: + return number < 0 + case int: + return number < 0 + default: + return false + } +} diff --git a/utils/formats/sarifutils/sarifutils_test.go b/utils/formats/sarifutils/sarifutils_test.go index 569b85879..39bec1643 100644 --- a/utils/formats/sarifutils/sarifutils_test.go +++ b/utils/formats/sarifutils/sarifutils_test.go @@ -1,6 +1,7 @@ package sarifutils import ( + "encoding/json" "path/filepath" "testing" @@ -708,3 +709,186 @@ func TestGroupResultsByLocation(t *testing.T) { assert.ElementsMatch(t, test.expectedOutput.Results, grouped[0].Results) } } + +// GroupResultsByLocation copies results via go-sarif constructors, which default index fields to -1. +// Analyzer Manager output unmarshals omitted indexes as 0; the copy must keep those values so uploaded CDX matches --output-dir dumps. +func TestCopyLocationPreservesSourceIdAndRegionOffsets(t *testing.T) { + location := CreateLocation("file.go", 1, 2, 3, 4, "snippet") + location.ID = -1 + location.PhysicalLocation.Region.CharOffset = -1 + location.PhysicalLocation.Region.ByteOffset = -1 + + copied := CopyLocation(location) + require.NotNil(t, copied) + assert.Equal(t, -1, copied.ID, "must not replace constructor sentinel with 0") + assert.Equal(t, -1, copied.PhysicalLocation.Region.CharOffset) + assert.Equal(t, -1, copied.PhysicalLocation.Region.ByteOffset) + + location.ID = 7 + location.PhysicalLocation.Region.CharOffset = 10 + location.PhysicalLocation.Region.ByteOffset = 20 + copied = CopyLocation(location) + assert.Equal(t, 7, copied.ID) + assert.Equal(t, 10, copied.PhysicalLocation.Region.CharOffset) + assert.Equal(t, 20, copied.PhysicalLocation.Region.ByteOffset) +} + +func TestGroupResultsByLocationPreservesZeroIndexes(t *testing.T) { + location := CreateLocation("src/main/java/com/example/HelloWorld.java", 5, 29, 5, 42, "String[] args") + location.PhysicalLocation.ArtifactLocation.Index = 0 + location.LogicalLocations = []*sarif.LogicalLocation{{ + FullyQualifiedName: ptrTo("com.example.HelloWorld.main"), + Index: 0, + ParentIndex: 0, + }} + + threadFlowLocation := &sarif.ThreadFlowLocation{ + ExecutionOrder: 0, + Importance: "", + Index: 0, + Location: location, + } + result := CreateResultWithLocations("result-msg", "rule1", "error", location).WithCodeFlows([]*sarif.CodeFlow{ + {ThreadFlows: []*sarif.ThreadFlow{{Locations: []*sarif.ThreadFlowLocation{threadFlowLocation}}}}, + }) + + grouped := GroupResultsByLocation([]*sarif.Run{CreateRunWithDummyResults(result)}) + require.Len(t, grouped, 1) + require.Len(t, grouped[0].Results, 1) + + copiedLocation := grouped[0].Results[0].Locations[0] + assert.Equal(t, 0, copiedLocation.PhysicalLocation.ArtifactLocation.Index) + require.Len(t, copiedLocation.LogicalLocations, 1) + assert.Equal(t, 0, copiedLocation.LogicalLocations[0].Index) + assert.Equal(t, 0, copiedLocation.LogicalLocations[0].ParentIndex) + + copiedThreadFlowLocation := grouped[0].Results[0].CodeFlows[0].ThreadFlows[0].Locations[0] + assert.Equal(t, 0, copiedThreadFlowLocation.Index) + assert.Equal(t, 0, copiedThreadFlowLocation.ExecutionOrder) + assert.Equal(t, "", copiedThreadFlowLocation.Importance) +} + +func ptrTo[T any](v T) *T { + return &v +} + +// Mirrors go-sarif v1.1.1 index fields (*uint + omitempty). Negative sentinels from v3 fail to decode. +type legacySarifPayload struct { + Runs []legacyRun `json:"runs"` +} + +type legacyRun struct { + Results []legacyResult `json:"results"` +} + +type legacyResult struct { + RuleIndex *uint `json:"ruleIndex,omitempty"` + Locations []legacyLocation `json:"locations"` + CodeFlows []legacyCodeFlow `json:"codeFlows,omitempty"` +} + +type legacyCodeFlow struct { + ThreadFlows []legacyThreadFlow `json:"threadFlows"` +} + +type legacyThreadFlow struct { + Locations []legacyThreadFlowLocation `json:"locations"` +} + +type legacyThreadFlowLocation struct { + Index *uint `json:"index,omitempty"` + ExecutionOrder *uint `json:"executionOrder,omitempty"` + Location *legacyLocation `json:"location,omitempty"` +} + +type legacyLocation struct { + ID *uint `json:"id,omitempty"` + PhysicalLocation *legacyPhysicalLocation `json:"physicalLocation,omitempty"` + LogicalLocations []legacyLogicalLocation `json:"logicalLocations,omitempty"` +} + +type legacyPhysicalLocation struct { + ArtifactLocation *legacyArtifactLocation `json:"artifactLocation,omitempty"` +} + +type legacyArtifactLocation struct { + URI string `json:"uri,omitempty"` + Index *uint `json:"index,omitempty"` +} + +type legacyLogicalLocation struct { + Index *uint `json:"index,omitempty"` + ParentIndex *uint `json:"parentIndex,omitempty"` +} + +func TestStripUnsetIndexesKeepsPayloadDecodableByLegacyConsumers(t *testing.T) { + payload := []byte(`{ + "runs": [{ + "results": [{ + "ruleIndex": -1, + "locations": [{ + "id": -1, + "physicalLocation": { + "artifactLocation": {"uri": "file.go", "index": -1} + }, + "logicalLocations": [{"index": -1, "parentIndex": -1}] + }], + "codeFlows": [{ + "threadFlows": [{ + "locations": [{ + "index": -1, + "executionOrder": -1, + "location": { + "physicalLocation": { + "artifactLocation": {"uri": "file.go", "index": 0} + } + } + }] + }] + }] + }, { + "ruleIndex": 2, + "locations": [{ + "id": 0, + "physicalLocation": { + "artifactLocation": {"uri": "other.go", "index": 5} + } + }] + }] + }] + }`) + + var before legacySarifPayload + require.Error(t, json.Unmarshal(payload, &before), "negative indexes must fail go-sarif v1 *uint decoding") + + sanitized, err := StripUnsetIndexes(payload) + require.NoError(t, err) + + var after legacySarifPayload + require.NoError(t, json.Unmarshal(sanitized, &after)) + require.Len(t, after.Runs, 1) + require.Len(t, after.Runs[0].Results, 2) + + first := after.Runs[0].Results[0] + assert.Nil(t, first.RuleIndex) + require.Len(t, first.Locations, 1) + assert.Nil(t, first.Locations[0].ID) + assert.Nil(t, first.Locations[0].PhysicalLocation.ArtifactLocation.Index) + require.Len(t, first.Locations[0].LogicalLocations, 1) + assert.Nil(t, first.Locations[0].LogicalLocations[0].Index) + assert.Nil(t, first.Locations[0].LogicalLocations[0].ParentIndex) + require.Len(t, first.CodeFlows, 1) + threadLoc := first.CodeFlows[0].ThreadFlows[0].Locations[0] + assert.Nil(t, threadLoc.Index) + assert.Nil(t, threadLoc.ExecutionOrder) + require.NotNil(t, threadLoc.Location.PhysicalLocation.ArtifactLocation.Index) + assert.Equal(t, uint(0), *threadLoc.Location.PhysicalLocation.ArtifactLocation.Index) + + second := after.Runs[0].Results[1] + require.NotNil(t, second.RuleIndex) + assert.Equal(t, uint(2), *second.RuleIndex) + require.NotNil(t, second.Locations[0].ID) + assert.Equal(t, uint(0), *second.Locations[0].ID) + require.NotNil(t, second.Locations[0].PhysicalLocation.ArtifactLocation.Index) + assert.Equal(t, uint(5), *second.Locations[0].PhysicalLocation.ArtifactLocation.Index) +}