Fix go-sarif breaks server-client miss match versions

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

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
+	}
+}

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)
+}