From 6602243017889b2e62b35ccbb3d40d556b019437 Mon Sep 17 00:00:00 2001 From: Harshjeet Patil Date: Mon, 17 Aug 2026 14:35:13 +0530 Subject: [PATCH 1/4] fix added for sonar bug --- internal/commands/result.go | 84 ++++- internal/commands/result_sonar_lines.go | 184 ++++++++++ internal/commands/result_sonar_lines_test.go | 350 +++++++++++++++++++ internal/wrappers/results-sonar.go | 10 +- 4 files changed, 606 insertions(+), 22 deletions(-) create mode 100644 internal/commands/result_sonar_lines.go create mode 100644 internal/commands/result_sonar_lines_test.go diff --git a/internal/commands/result.go b/internal/commands/result.go index a8aa5ef7f..2d351b821 100644 --- a/internal/commands/result.go +++ b/internal/commands/result.go @@ -2332,6 +2332,8 @@ func parseSonar(results *wrappers.ScanResultsCollection) ([]wrappers.SonarIssues var sonarIssues []wrappers.SonarIssues var sonarRules []wrappers.SonarRules seenRuleIDs := make(map[string]bool) // Track already added rule IDs + // Shared across the whole export so each source file is read at most once. + lineIndex := newSonarLineIndex() if results != nil { for _, result := range results.Results { @@ -2346,8 +2348,15 @@ func parseSonar(results *wrappers.ScanResultsCollection) ([]wrappers.SonarIssues engineType := strings.TrimSpace(result.Type) if engineType == commonParams.SastType { - auxIssue.PrimaryLocation = parseSonarPrimaryLocation(result) - auxIssue.SecondaryLocations = parseSonarSecondaryLocations(result) + auxIssue.PrimaryLocation = parseSonarPrimaryLocation(result, lineIndex) + auxIssue.SecondaryLocations = parseSonarSecondaryLocations(result, lineIndex) + if auxIssue.PrimaryLocation.FilePath == "" { + // filePath is mandatory on the primary location, so an issue + // without one cannot be imported at all. Skipping it keeps + // the rest of the report loadable instead of having + // SonarQube reject every issue in the file. + continue + } sonarIssues = append(sonarIssues, auxIssue) } else if engineType == commonParams.KicsType { auxIssue.PrimaryLocation = parseLocationKics(result) @@ -2376,7 +2385,7 @@ func parseContainersSonar(result *wrappers.ScanResult) wrappers.SonarLocation { textRange.EndColumn = 2 textRange.StartLine = 1 textRange.EndLine = 2 - auxLocation.TextRange = textRange + auxLocation.TextRange = &textRange return auxLocation } @@ -2388,7 +2397,7 @@ func parseSscsSonar(result *wrappers.ScanResult, sonarIssue *wrappers.SonarIssue textRange.StartColumn = 1 textRange.EndColumn = 2 textRange.StartLine = result.ScanResultData.Line - sonarIssue.PrimaryLocation.TextRange = textRange + sonarIssue.PrimaryLocation.TextRange = &textRange return *sonarIssue } @@ -2472,7 +2481,7 @@ func parseScaSonarLocations(result *wrappers.ScanResult) []wrappers.SonarIssues textRange.StartLine = 1 textRange.EndLine = 2 - primaryLocation.TextRange = textRange + primaryLocation.TextRange = &textRange issueByLocation.PrimaryLocation = primaryLocation @@ -2490,48 +2499,85 @@ func parseLocationKics(results *wrappers.ScanResult) wrappers.SonarLocation { auxTextRange.StartLine = results.ScanResultData.Line auxTextRange.StartColumn = 0 auxTextRange.EndColumn = 1 - auxLocation.TextRange = auxTextRange + auxLocation.TextRange = &auxTextRange return auxLocation } -func parseSonarPrimaryLocation(results *wrappers.ScanResult) wrappers.SonarLocation { +func parseSonarPrimaryLocation(results *wrappers.ScanResult, lineIndex *sonarLineIndex) wrappers.SonarLocation { var auxLocation wrappers.SonarLocation // fill the details in the primary Location if len(results.ScanResultData.Nodes) > 0 { auxLocation.FilePath = strings.TrimLeft(results.ScanResultData.Nodes[0].FileName, "/") auxLocation.Message = html.UnescapeString(strings.ReplaceAll(results.ScanResultData.QueryName, "_", " ")) - auxLocation.TextRange = parseSonarTextRange(results.ScanResultData.Nodes[0]) + auxLocation.TextRange = parseSonarTextRange(results.ScanResultData.Nodes[0], lineIndex) } return auxLocation } -func parseSonarSecondaryLocations(results *wrappers.ScanResult) []wrappers.SonarLocation { +func parseSonarSecondaryLocations(results *wrappers.ScanResult, lineIndex *sonarLineIndex) []wrappers.SonarLocation { var auxSecondaryLocations []wrappers.SonarLocation // Traverse all the rest of the scan result nodes into secondary location of sonar if len(results.ScanResultData.Nodes) >= 1 { for _, node := range results.ScanResultData.Nodes[1:] { + filePath := strings.TrimLeft(node.FileName, "/") + if filePath == "" { + // filePath is also mandatory on a secondary location. The engine + // occasionally returns a node with no file name, which used to be + // serialised as an absent field and made SonarQube reject the + // entire report. + continue + } + textRange := parseSonarTextRange(node, lineIndex) + if textRange == nil { + // Unlike the primary location, SonarQube treats textRange as a + // mandatory field on secondary locations and fails to parse the + // whole report without it. A node whose position cannot be + // expressed validly is therefore dropped: losing one step of a + // data flow is far better than losing the entire analysis. + continue + } var auxSecondaryLocation wrappers.SonarLocation - auxSecondaryLocation.FilePath = strings.TrimLeft(node.FileName, "/") + auxSecondaryLocation.FilePath = filePath auxSecondaryLocation.Message = html.UnescapeString(strings.ReplaceAll(results.ScanResultData.QueryName, "_", " ")) - auxSecondaryLocation.TextRange = parseSonarTextRange(node) + auxSecondaryLocation.TextRange = textRange auxSecondaryLocations = append(auxSecondaryLocations, auxSecondaryLocation) } } return auxSecondaryLocations } -func parseSonarTextRange(results *wrappers.ScanResultNode) wrappers.SonarTextRange { - var auxTextRange wrappers.SonarTextRange - auxTextRange.StartLine = results.Line - startColumn := getSastStartColumn(results.Column) +// parseSonarTextRange maps a scan result node onto a Sonar text range. +// +// SonarQube validates the line and both column offsets against the file on disk +// and aborts the entire analysis on the first invalid location, so the +// coordinates reported by the engine are verified before being emitted. There +// are three outcomes: +// +// - the line exists: columns are clamped to its length +// - the line does not exist: nil is returned so that textRange is omitted and +// the issue is reported at file level +// - the file cannot be read: the line is kept and the columns are dropped, +// since nothing can be verified and SonarQube skips issues whose file it +// cannot resolve either +func parseSonarTextRange(results *wrappers.ScanResultNode, lineIndex *sonarLineIndex) *wrappers.SonarTextRange { + lineLength, status := lineIndex.resolveLine(results.FileName, results.Line) + if status == lineStatusLineMissing { + return nil + } - auxTextRange.StartColumn = startColumn - auxTextRange.EndColumn = startColumn + results.Length + auxTextRange := &wrappers.SonarTextRange{StartLine: results.Line} + if status != lineStatusOK { + return auxTextRange + } - if auxTextRange.StartColumn == auxTextRange.EndColumn { - auxTextRange.EndColumn++ + startColumn, endColumn, emit := clampSonarColumns(getSastStartColumn(results.Column), results.Length, lineLength) + if !emit { + return auxTextRange } + auxTextRange.StartColumn = startColumn + auxTextRange.EndColumn = endColumn + return auxTextRange } diff --git a/internal/commands/result_sonar_lines.go b/internal/commands/result_sonar_lines.go new file mode 100644 index 000000000..84ffdcd3c --- /dev/null +++ b/internal/commands/result_sonar_lines.go @@ -0,0 +1,184 @@ +package commands + +import ( + "bufio" + "os" + "path/filepath" + "strings" + "unicode/utf8" +) + +// maxSonarLineBytes bounds how long a single source line may be before the file +// is treated as unreadable. Lines beyond this do not warrant column precision. +const maxSonarLineBytes = 1024 * 1024 + +// byteOrderMarkRune is U+FEFF. A leading BOM must not be counted as a +// character, or every offset on the first line would shift by one. +const byteOrderMarkRune rune = 0xFEFF + +// parentDir is the path element that walks above a directory. +const parentDir = ".." + +// lineStatus reports how much of a location the source file could confirm. +type lineStatus int + +const ( + // lineStatusFileUnknown means the file could not be read, so neither the + // line nor the columns can be verified. + lineStatusFileUnknown lineStatus = iota + // lineStatusLineMissing means the file was read and does not contain the + // reported line. The location is definitively invalid. + lineStatusLineMissing + // lineStatusOK means the reported line exists and its length is known. + lineStatusOK +) + +// sonarLineIndex caches the character length of every line of the source files +// referenced by a report, so that each file is read at most once per export. +// +// SonarQube validates both the line and the column offsets of every imported +// issue against the file on disk, and aborts the entire analysis on the first +// violation. The CxOne engine reports coordinates against its own view of the +// source, which is not guaranteed to match the file on disk - minified assets +// are a common example, where the engine reports lines far beyond the end of +// the raw file. Locations therefore have to be verified before being written. +type sonarLineIndex struct { + // baseDir confines every file read to the working directory, matching how + // SonarQube resolves filePath against sonar.projectBaseDir. An empty + // baseDir disables verification entirely. + baseDir string + // files maps a report file path to the character length of each of its + // lines. A nil value records that the file could not be read, so an + // unreadable file is not reopened for every node. + files map[string][]uint +} + +func newSonarLineIndex() *sonarLineIndex { + baseDir, err := os.Getwd() + if err != nil { + baseDir = "" + } + return &sonarLineIndex{baseDir: baseDir, files: make(map[string][]uint)} +} + +// resolveLine reports whether the given 1-based line of fileName exists and, +// when it does, how many characters it holds. +func (index *sonarLineIndex) resolveLine(fileName string, line uint) (length uint, status lineStatus) { + if index == nil || fileName == "" { + return 0, lineStatusFileUnknown + } + + lengths, cached := index.files[fileName] + if !cached { + lengths = index.readLineLengths(fileName) + index.files[fileName] = lengths + } + if lengths == nil { + return 0, lineStatusFileUnknown + } + + if line == 0 || line > uint(len(lengths)) { + return 0, lineStatusLineMissing + } + return lengths[line-1], lineStatusOK +} + +// resolveSourcePath cleans a report file path and confines it to baseDir, +// rejecting anything that escapes. Report paths are repository relative and +// arrive in a scan-result payload, so they are treated as untrusted input. +// Symlinks are resolved so that containment is enforced on real paths rather +// than lexically. +func (index *sonarLineIndex) resolveSourcePath(fileName string) (path string, ok bool) { + if index.baseDir == "" { + return "", false + } + + // Reject anything that is not a plain relative path before touching disk. + relative := filepath.FromSlash(strings.TrimLeft(fileName, "/\\")) + if relative == "" || filepath.IsAbs(relative) || filepath.VolumeName(relative) != "" { + return "", false + } + + // filepath.Join applies filepath.Clean, collapsing any ".." elements. + cleaned := filepath.Join(index.baseDir, relative) + + // Resolve both sides: a path that cannot be resolved does not exist and is + // therefore not readable either way. + realBase, err := filepath.EvalSymlinks(index.baseDir) + if err != nil { + return "", false + } + realPath, err := filepath.EvalSymlinks(cleaned) + if err != nil { + return "", false + } + + inside, err := filepath.Rel(realBase, realPath) + if err != nil || inside == parentDir || strings.HasPrefix(inside, parentDir+string(os.PathSeparator)) { + return "", false + } + return realPath, true +} + +// readLineLengths returns the character length of each line of a file, or nil +// when the path is not permitted or the file cannot be read in full. +func (index *sonarLineIndex) readLineLengths(fileName string) []uint { + // resolveSourcePath validates and cleans the path and confines it to + // baseDir, so the value opened below is not attacker controlled. + path, ok := index.resolveSourcePath(fileName) + if !ok { + return nil + } + + file, err := os.Open(path) + if err != nil { + return nil + } + defer func() { _ = file.Close() }() + + scanner := bufio.NewScanner(file) + scanner.Buffer(make([]byte, 0, bufio.MaxScanTokenSize), maxSonarLineBytes) + + lengths := []uint{} + for scanner.Scan() { + text := scanner.Text() + if len(lengths) == 0 { + text = strings.TrimPrefix(text, string(byteOrderMarkRune)) + } + // bufio.ScanLines already drops a trailing CR; this guards against a + // lone CR surviving into the count. + text = strings.TrimSuffix(text, "\r") + lengths = append(lengths, uint(utf8.RuneCountInString(text))) + } + if scanner.Err() != nil { + return nil + } + return lengths +} + +// clampSonarColumns constrains a start offset and length to a line of +// lineLength characters. emit is false when no valid column range exists for +// the line, in which case the caller should emit a line-level location. +func clampSonarColumns(startColumn, length, lineLength uint) (start, end uint, emit bool) { + if lineLength == 0 { + return 0, 0, false + } + + start = startColumn + if start > lineLength { + start = lineLength + } + + end = start + length + if end > lineLength { + end = lineLength + } + + // A zero-length node, or a start offset sitting at end of line, leaves no + // forward span. Highlight the whole line rather than emitting a zero-width + // or inverted pointer, which SonarQube also rejects. + if end <= start { + return 0, lineLength, true + } + return start, end, true +} diff --git a/internal/commands/result_sonar_lines_test.go b/internal/commands/result_sonar_lines_test.go new file mode 100644 index 000000000..ff4babd26 --- /dev/null +++ b/internal/commands/result_sonar_lines_test.go @@ -0,0 +1,350 @@ +package commands + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/checkmarx/ast-cli/internal/params" + "github.com/checkmarx/ast-cli/internal/wrappers" + "github.com/stretchr/testify/assert" +) + +// writeSourceFile creates a file with the given content inside dir and returns +// its path relative to dir, which is the shape report file paths take. +func writeSourceFile(t *testing.T, dir, name, content string) string { + t.Helper() + path := filepath.Join(dir, name) + assert.NoError(t, os.MkdirAll(filepath.Dir(path), 0o750)) + assert.NoError(t, os.WriteFile(path, []byte(content), 0o600)) + return filepath.ToSlash(name) +} + +// assertValidSonarRange asserts the invariants SonarScanner enforces in +// DefaultInputFile: the line must exist in the file, and every offset must fall +// inside that line with the range moving forward. Violating any of them aborts +// the whole analysis, so totalLines is checked as well as lineLength. +func assertValidSonarRange(t *testing.T, tr *wrappers.SonarTextRange, totalLines, lineLength uint) { + t.Helper() + if tr == nil { + return // omitted entirely, so the issue is file level and always valid + } + assert.NotZero(t, tr.StartLine, "startLine must be set when textRange is present") + assert.LessOrEqual(t, tr.StartLine, totalLines, "startLine must exist in the file") + + if tr.StartColumn == 0 && tr.EndColumn == 0 { + return // line level range, always accepted + } + assert.LessOrEqual(t, tr.StartColumn, lineLength, "startColumn must not exceed line length") + assert.LessOrEqual(t, tr.EndColumn, lineLength, "endColumn must not exceed line length") + assert.Less(t, tr.StartColumn, tr.EndColumn, "range must move forward") +} + +func TestClampSonarColumns(t *testing.T) { + tests := []struct { + name string + startColumn uint + length uint + lineLength uint + wantStart uint + wantEnd uint + wantEmit bool + }{ + { + // The reported customer case: "@Component({" is 12 characters and + // the engine reported Column=12, Length=6, producing endColumn 17. + name: "overflowing end column is clamped to line length", + startColumn: 11, length: 6, lineLength: 12, + wantStart: 11, wantEnd: 12, wantEmit: true, + }, + { + name: "valid range is preserved unchanged", + startColumn: 4, length: 5, lineLength: 40, + wantStart: 4, wantEnd: 9, wantEmit: true, + }, + { + name: "range ending exactly at line length is valid", + startColumn: 0, length: 12, lineLength: 12, + wantStart: 0, wantEnd: 12, wantEmit: true, + }, + { + name: "zero length node falls back to the whole line", + startColumn: 4, length: 0, lineLength: 40, + wantStart: 0, wantEnd: 40, wantEmit: true, + }, + { + name: "start beyond end of line falls back to the whole line", + startColumn: 50, length: 3, lineLength: 12, + wantStart: 0, wantEnd: 12, wantEmit: true, + }, + { + name: "empty line yields no column range", + startColumn: 0, length: 3, lineLength: 0, + wantStart: 0, wantEnd: 0, wantEmit: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + start, end, emit := clampSonarColumns(tt.startColumn, tt.length, tt.lineLength) + assert.Equal(t, tt.wantEmit, emit) + assert.Equal(t, tt.wantStart, start) + assert.Equal(t, tt.wantEnd, end) + if emit { + assert.LessOrEqual(t, end, tt.lineLength) + assert.Less(t, start, end) + } + }) + } +} + +func TestSonarLineIndexLineLength(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + + lfFile := writeSourceFile(t, dir, "lf.ts", "one\nthree3\n") + crlfFile := writeSourceFile(t, dir, "crlf.ts", "one\r\nthree3\r\n") + bomFile := writeSourceFile(t, dir, "bom.ts", string(byteOrderMarkRune)+"abc\n") + utf8File := writeSourceFile(t, dir, "utf8.ts", "héllo→\n") + emptyLineFile := writeSourceFile(t, dir, "nested/dir/empty.ts", "\nabc\n") + + index := newSonarLineIndex() + + t.Run("counts characters on an LF file", func(t *testing.T) { + length, status := index.resolveLine(lfFile, 2) + assert.Equal(t, lineStatusOK, status) + assert.Equal(t, uint(6), length) + }) + + t.Run("CRLF terminator is not counted", func(t *testing.T) { + length, status := index.resolveLine(crlfFile, 2) + assert.Equal(t, lineStatusOK, status) + assert.Equal(t, uint(6), length) + }) + + t.Run("leading BOM is not counted", func(t *testing.T) { + length, status := index.resolveLine(bomFile, 1) + assert.Equal(t, lineStatusOK, status) + assert.Equal(t, uint(3), length) + }) + + t.Run("multi byte characters count as one character each", func(t *testing.T) { + length, status := index.resolveLine(utf8File, 1) + assert.Equal(t, lineStatusOK, status) + // "héllo→" is 6 characters but 9 bytes. + assert.Equal(t, uint(6), length) + }) + + t.Run("empty line reports zero length", func(t *testing.T) { + length, status := index.resolveLine(emptyLineFile, 1) + assert.Equal(t, lineStatusOK, status) + assert.Equal(t, uint(0), length) + }) + + t.Run("leading slash in the report path is tolerated", func(t *testing.T) { + length, status := index.resolveLine("/"+lfFile, 1) + assert.Equal(t, lineStatusOK, status) + assert.Equal(t, uint(3), length) + }) + + t.Run("line past end of a readable file is reported missing", func(t *testing.T) { + _, status := index.resolveLine(lfFile, 99) + assert.Equal(t, lineStatusLineMissing, status) + }) + + t.Run("line zero of a readable file is reported missing", func(t *testing.T) { + _, status := index.resolveLine(lfFile, 0) + assert.Equal(t, lineStatusLineMissing, status) + }) + + t.Run("missing file is reported unknown, not missing line", func(t *testing.T) { + _, status := index.resolveLine("does/not/exist.ts", 1) + assert.Equal(t, lineStatusFileUnknown, status) + }) + + t.Run("empty file name is reported unknown", func(t *testing.T) { + _, status := index.resolveLine("", 1) + assert.Equal(t, lineStatusFileUnknown, status) + }) +} + +func TestSonarLineIndexRejectsPathsOutsideBaseDir(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + index := newSonarLineIndex() + + for _, fileName := range []string{ + "../escape.ts", + "../../../../etc/passwd", + "nested/../../escape.ts", + } { + t.Run("rejects "+fileName, func(t *testing.T) { + _, ok := index.resolveSourcePath(fileName) + assert.False(t, ok, "path outside the working directory must be rejected") + }) + } + + t.Run("rejects an absolute path", func(t *testing.T) { + absolute := filepath.Join(dir, "abs.ts") + assert.NoError(t, os.WriteFile(absolute, []byte("abc\n"), 0o600)) + // Absolute inputs are refused even when they resolve inside baseDir. + _, ok := index.resolveSourcePath(absolute) + assert.False(t, ok) + }) + + t.Run("accepts a plain relative path inside the base directory", func(t *testing.T) { + name := writeSourceFile(t, dir, "inside.ts", "abc\n") + resolved, ok := index.resolveSourcePath(name) + assert.True(t, ok) + assert.True(t, strings.HasSuffix(resolved, "inside.ts")) + }) +} + +// TestParseSonarTextRangeCustomerRegression reproduces the reported failure end +// to end: a node whose Column plus Length runs past the end of the line it +// points at previously produced endColumn 17 on a 12 character line, which +// SonarQube rejects with "17 is not a valid line offset for pointer". +func TestParseSonarTextRangeCustomerRegression(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + + const componentLine = "@Component({" // 12 characters, line 10 below + source := "import { Component, inject, OnInit } from '@angular/core'\n" + + "import { DomSanitizer } from '@angular/platform-browser'\n" + + "import jwtDecode from 'jwt-decode'\n" + + "import { TranslateModule } from '@ngx-translate/core'\n" + + "import { MatCardModule } from '@angular/material/card'\n" + + "\n\n\n\n" + // lines 6 to 9 + componentLine + "\n" + // line 10 + " selector: 'app-last-login-ip',\n" + + fileName := writeSourceFile(t, + dir, + "cxone-sq-integration/juice-shop-master/frontend/src/app/last-login-ip/last-login-ip.component.ts", + source, + ) + + index := newSonarLineIndex() + node := &wrappers.ScanResultNode{ + FileName: fileName, + Line: 10, + Column: 12, + Length: 6, + } + + textRange := parseSonarTextRange(node, index) + + assert.NotNil(t, textRange) + assert.Equal(t, uint(10), textRange.StartLine) + assert.Equal(t, uint(11), textRange.StartColumn) + assert.Equal(t, uint(12), textRange.EndColumn, "endColumn must be clamped to the 12 character line, not 17") + assertValidSonarRange(t, textRange, 11, uint(len(componentLine))) +} + +func TestParseSonarTextRangeFallsBackToLineLevel(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + index := newSonarLineIndex() + + t.Run("unreadable file keeps the line and emits no columns", func(t *testing.T) { + node := &wrappers.ScanResultNode{FileName: "absent.ts", Line: 7, Column: 12, Length: 6} + textRange := parseSonarTextRange(node, index) + assert.NotNil(t, textRange) + assert.Equal(t, uint(7), textRange.StartLine) + assert.Zero(t, textRange.StartColumn, "columns are omitempty, so zero drops them from the report") + assert.Zero(t, textRange.EndColumn) + }) + + t.Run("empty line emits no columns", func(t *testing.T) { + fileName := writeSourceFile(t, dir, "blank.ts", "\nabc\n") + node := &wrappers.ScanResultNode{FileName: fileName, Line: 1, Column: 3, Length: 4} + textRange := parseSonarTextRange(node, index) + assert.NotNil(t, textRange) + assert.Equal(t, uint(1), textRange.StartLine) + assert.Zero(t, textRange.StartColumn) + assert.Zero(t, textRange.EndColumn) + }) +} + +// TestParseSonarTextRangeOmitsRangeForMissingLine covers the minified asset case +// found on real scan data: the engine reported line 803 of a file that has only +// 82 lines, because its coordinates are against a transformed view of the +// source. Emitting that line at all aborts the SonarQube analysis, so the whole +// textRange must be dropped and the issue reported at file level. +func TestParseSonarTextRangeOmitsRangeForMissingLine(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + + fileName := writeSourceFile(t, dir, "assets/private/dat.gui.min.js", "var a=1\nvar b=2\n") + index := newSonarLineIndex() + + node := &wrappers.ScanResultNode{FileName: fileName, Line: 803, Column: 5, Length: 4} + textRange := parseSonarTextRange(node, index) + + assert.Nil(t, textRange, "textRange must be omitted when the line does not exist in the file") +} + +// TestParseSonarAllLocationsAreValid walks a result with a primary and several +// secondary nodes, all deliberately overflowing, and asserts every emitted +// location satisfies SonarQube's invariant. A single violation anywhere in the +// report aborts the whole import, so secondary locations matter as much as the +// primary one. +func TestParseSonarAllLocationsAreValid(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + + const line = "@Component({" // 12 characters + fileName := writeSourceFile(t, dir, "app/widget.component.ts", line+"\n"+line+"\n") + + results := &wrappers.ScanResultsCollection{ + Results: []*wrappers.ScanResult{ + { + Type: params.SastType, + ScanResultData: wrappers.ScanResultData{ + QueryName: "Angular_Client_Stored_DOM_XSS", + Nodes: []*wrappers.ScanResultNode{ + {FileName: fileName, Line: 1, Column: 12, Length: 6}, + {FileName: fileName, Line: 2, Column: 40, Length: 9}, + {FileName: fileName, Line: 1, Column: 1, Length: 0}, + {FileName: "missing.ts", Line: 3, Column: 5, Length: 4}, + // Line beyond end of a readable file: must be dropped. + {FileName: fileName, Line: 803, Column: 5, Length: 4}, + }, + }, + }, + }, + } + + issues, _ := parseSonar(results) + + assert.Len(t, issues, 1) + // Five nodes: one primary plus four secondaries, of which the node on line + // 803 is dropped because SonarQube requires textRange on secondary + // locations and no valid range exists for it. + assert.Len(t, issues[0].SecondaryLocations, 3) + + all := append([]wrappers.SonarLocation{issues[0].PrimaryLocation}, issues[0].SecondaryLocations...) + for i := range all { + location := all[i] + t.Run("location "+string(rune('A'+i)), func(t *testing.T) { + if location.FilePath != fileName { + // A file that is not on disk cannot be verified, so the engine + // line is kept and only the columns are dropped. + assert.NotNil(t, location.TextRange) + assert.NotZero(t, location.TextRange.StartLine) + assert.Zero(t, location.TextRange.StartColumn) + assert.Zero(t, location.TextRange.EndColumn) + return + } + assertValidSonarRange(t, location.TextRange, 2, uint(len(line))) + }) + } + + // No secondary location may carry a nil textRange: SonarQube rejects the + // whole report with "missing mandatory field 'textRange' in a secondary + // location of the issue". + for _, location := range issues[0].SecondaryLocations { + assert.NotNil(t, location.TextRange, "secondary locations must always carry a textRange") + } +} diff --git a/internal/wrappers/results-sonar.go b/internal/wrappers/results-sonar.go index 4fa0c05c6..afc86d5a5 100644 --- a/internal/wrappers/results-sonar.go +++ b/internal/wrappers/results-sonar.go @@ -27,9 +27,13 @@ type SonarIssues struct { } type SonarLocation struct { - Message string `json:"message,omitempty"` - FilePath string `json:"filePath,omitempty"` - TextRange SonarTextRange `json:"textRange"` + Message string `json:"message,omitempty"` + FilePath string `json:"filePath,omitempty"` + // TextRange is omitted when the location cannot be expressed validly, for + // example when the engine reports a line that does not exist in the file on + // disk. SonarQube treats a location without a textRange as file level, + // whereas an out of range line aborts the whole analysis. + TextRange *SonarTextRange `json:"textRange,omitempty"` } type SonarTextRange struct { From 1bc6bae152b97b547bde4482a9072f592a142684 Mon Sep 17 00:00:00 2001 From: Harshjeet Patil Date: Tue, 25 Aug 2026 14:42:02 +0530 Subject: [PATCH 2/4] code changes --- .checkmarx/checkmarxIgnoredTempList.json | 7 + internal/commands/config.yaml | 0 internal/commands/result.go | 158 +++++++-- internal/commands/result_sonar_lines.go | 184 ---------- internal/commands/result_sonar_lines_test.go | 350 ------------------- internal/commands/result_test.go | 319 +++++++++++++++++ 6 files changed, 462 insertions(+), 556 deletions(-) create mode 100644 .checkmarx/checkmarxIgnoredTempList.json create mode 100644 internal/commands/config.yaml delete mode 100644 internal/commands/result_sonar_lines.go delete mode 100644 internal/commands/result_sonar_lines_test.go diff --git a/.checkmarx/checkmarxIgnoredTempList.json b/.checkmarx/checkmarxIgnoredTempList.json new file mode 100644 index 000000000..a42259d4c --- /dev/null +++ b/.checkmarx/checkmarxIgnoredTempList.json @@ -0,0 +1,7 @@ +[ + { + "FileName": "result.go", + "Line": 2630, + "RuleID": 3007 + } +] \ No newline at end of file diff --git a/internal/commands/config.yaml b/internal/commands/config.yaml new file mode 100644 index 000000000..e69de29bb diff --git a/internal/commands/result.go b/internal/commands/result.go index 2d351b821..b715cd9ef 100644 --- a/internal/commands/result.go +++ b/internal/commands/result.go @@ -1,6 +1,7 @@ package commands import ( + "bufio" "encoding/json" "fmt" "html" @@ -14,6 +15,7 @@ import ( "strings" "text/template" "time" + "unicode/utf8" "github.com/MakeNowJust/heredoc" "github.com/checkmarx/ast-cli/internal/commands/util" @@ -132,6 +134,18 @@ const ( statusCompleted = "Completed" statusPartial = "Partial" statusFailed = "Failed" + maxSonarLineBytes = 1024 * 1024 + byteOrderMarkRune = rune(0xFEFF) + parentDir = ".." +) + +// lineStatus reports how much of a location the source file could confirm. +type lineStatus int + +const ( + lineStatusFileUnknown lineStatus = iota + lineStatusLineMissing + lineStatusOK ) var ( @@ -2503,6 +2517,125 @@ func parseLocationKics(results *wrappers.ScanResult) wrappers.SonarLocation { return auxLocation } +// sonarLineIndex caches source line lengths so each report file is read at most once per export. +type sonarLineIndex struct { + baseDir string + files map[string][]uint +} + +func newSonarLineIndex() *sonarLineIndex { + baseDir, err := os.Getwd() + if err != nil { + baseDir = "" + } + return &sonarLineIndex{baseDir: baseDir, files: make(map[string][]uint)} +} + +// resolveLine reports whether the given 1-based line of fileName exists and, when it does, how many characters it holds. +func (index *sonarLineIndex) resolveLine(fileName string, line uint) (length uint, status lineStatus) { + if index == nil || fileName == "" { + return 0, lineStatusFileUnknown + } + + lengths, cached := index.files[fileName] + if !cached { + lengths = index.readLineLengths(fileName) + index.files[fileName] = lengths + } + if lengths == nil { + return 0, lineStatusFileUnknown + } + + if line == 0 || line > uint(len(lengths)) { + return 0, lineStatusLineMissing + } + return lengths[line-1], lineStatusOK +} + +// resolveSourcePath cleans a report file path and confines it to baseDir, rejecting anything that escapes. +func (index *sonarLineIndex) resolveSourcePath(fileName string) (path string, ok bool) { + if index.baseDir == "" { + return "", false + } + + relative := filepath.FromSlash(strings.TrimLeft(fileName, "/\\")) + if relative == "" || filepath.IsAbs(relative) || filepath.VolumeName(relative) != "" { + return "", false + } + + cleaned := filepath.Join(index.baseDir, relative) + + // EvalSymlinks confines containment to the real path, not a lexical one. + realBase, err := filepath.EvalSymlinks(index.baseDir) + if err != nil { + return "", false + } + realPath, err := filepath.EvalSymlinks(cleaned) + if err != nil { + return "", false + } + + inside, err := filepath.Rel(realBase, realPath) + if err != nil || inside == parentDir || strings.HasPrefix(inside, parentDir+string(os.PathSeparator)) { + return "", false + } + return realPath, true +} + +// readLineLengths returns the character length of each line of a file, or nil when it cannot be read in full. +func (index *sonarLineIndex) readLineLengths(fileName string) []uint { + path, ok := index.resolveSourcePath(fileName) + if !ok { + return nil + } + + file, err := os.Open(path) + if err != nil { + return nil + } + defer func() { _ = file.Close() }() + + scanner := bufio.NewScanner(file) + scanner.Buffer(make([]byte, 0, bufio.MaxScanTokenSize), maxSonarLineBytes) + + lengths := []uint{} + for scanner.Scan() { + text := scanner.Text() + if len(lengths) == 0 { + text = strings.TrimPrefix(text, string(byteOrderMarkRune)) + } + text = strings.TrimSuffix(text, "\r") + lengths = append(lengths, uint(utf8.RuneCountInString(text))) + } + if scanner.Err() != nil { + return nil + } + return lengths +} + +// clampSonarColumns constrains a start offset and length to a line of lineLength characters. +func clampSonarColumns(startColumn, length, lineLength uint) (start, end uint, emit bool) { + if lineLength == 0 { + return 0, 0, false + } + + start = startColumn + if start > lineLength { + start = lineLength + } + + end = start + length + if end > lineLength { + end = lineLength + } + + // No forward span left: highlight the whole line instead of an invalid zero-width range. + if end <= start { + return 0, lineLength, true + } + return start, end, true +} + func parseSonarPrimaryLocation(results *wrappers.ScanResult, lineIndex *sonarLineIndex) wrappers.SonarLocation { var auxLocation wrappers.SonarLocation // fill the details in the primary Location @@ -2521,19 +2654,12 @@ func parseSonarSecondaryLocations(results *wrappers.ScanResult, lineIndex *sonar for _, node := range results.ScanResultData.Nodes[1:] { filePath := strings.TrimLeft(node.FileName, "/") if filePath == "" { - // filePath is also mandatory on a secondary location. The engine - // occasionally returns a node with no file name, which used to be - // serialised as an absent field and made SonarQube reject the - // entire report. + // filePath is mandatory on a secondary location too, so a node without one is skipped. continue } textRange := parseSonarTextRange(node, lineIndex) if textRange == nil { - // Unlike the primary location, SonarQube treats textRange as a - // mandatory field on secondary locations and fails to parse the - // whole report without it. A node whose position cannot be - // expressed validly is therefore dropped: losing one step of a - // data flow is far better than losing the entire analysis. + // textRange is mandatory on secondary locations, so a node without a valid one is dropped. continue } var auxSecondaryLocation wrappers.SonarLocation @@ -2546,19 +2672,7 @@ func parseSonarSecondaryLocations(results *wrappers.ScanResult, lineIndex *sonar return auxSecondaryLocations } -// parseSonarTextRange maps a scan result node onto a Sonar text range. -// -// SonarQube validates the line and both column offsets against the file on disk -// and aborts the entire analysis on the first invalid location, so the -// coordinates reported by the engine are verified before being emitted. There -// are three outcomes: -// -// - the line exists: columns are clamped to its length -// - the line does not exist: nil is returned so that textRange is omitted and -// the issue is reported at file level -// - the file cannot be read: the line is kept and the columns are dropped, -// since nothing can be verified and SonarQube skips issues whose file it -// cannot resolve either +// parseSonarTextRange maps a scan result node onto a Sonar text range, clamping columns to the verified line length. func parseSonarTextRange(results *wrappers.ScanResultNode, lineIndex *sonarLineIndex) *wrappers.SonarTextRange { lineLength, status := lineIndex.resolveLine(results.FileName, results.Line) if status == lineStatusLineMissing { diff --git a/internal/commands/result_sonar_lines.go b/internal/commands/result_sonar_lines.go deleted file mode 100644 index 84ffdcd3c..000000000 --- a/internal/commands/result_sonar_lines.go +++ /dev/null @@ -1,184 +0,0 @@ -package commands - -import ( - "bufio" - "os" - "path/filepath" - "strings" - "unicode/utf8" -) - -// maxSonarLineBytes bounds how long a single source line may be before the file -// is treated as unreadable. Lines beyond this do not warrant column precision. -const maxSonarLineBytes = 1024 * 1024 - -// byteOrderMarkRune is U+FEFF. A leading BOM must not be counted as a -// character, or every offset on the first line would shift by one. -const byteOrderMarkRune rune = 0xFEFF - -// parentDir is the path element that walks above a directory. -const parentDir = ".." - -// lineStatus reports how much of a location the source file could confirm. -type lineStatus int - -const ( - // lineStatusFileUnknown means the file could not be read, so neither the - // line nor the columns can be verified. - lineStatusFileUnknown lineStatus = iota - // lineStatusLineMissing means the file was read and does not contain the - // reported line. The location is definitively invalid. - lineStatusLineMissing - // lineStatusOK means the reported line exists and its length is known. - lineStatusOK -) - -// sonarLineIndex caches the character length of every line of the source files -// referenced by a report, so that each file is read at most once per export. -// -// SonarQube validates both the line and the column offsets of every imported -// issue against the file on disk, and aborts the entire analysis on the first -// violation. The CxOne engine reports coordinates against its own view of the -// source, which is not guaranteed to match the file on disk - minified assets -// are a common example, where the engine reports lines far beyond the end of -// the raw file. Locations therefore have to be verified before being written. -type sonarLineIndex struct { - // baseDir confines every file read to the working directory, matching how - // SonarQube resolves filePath against sonar.projectBaseDir. An empty - // baseDir disables verification entirely. - baseDir string - // files maps a report file path to the character length of each of its - // lines. A nil value records that the file could not be read, so an - // unreadable file is not reopened for every node. - files map[string][]uint -} - -func newSonarLineIndex() *sonarLineIndex { - baseDir, err := os.Getwd() - if err != nil { - baseDir = "" - } - return &sonarLineIndex{baseDir: baseDir, files: make(map[string][]uint)} -} - -// resolveLine reports whether the given 1-based line of fileName exists and, -// when it does, how many characters it holds. -func (index *sonarLineIndex) resolveLine(fileName string, line uint) (length uint, status lineStatus) { - if index == nil || fileName == "" { - return 0, lineStatusFileUnknown - } - - lengths, cached := index.files[fileName] - if !cached { - lengths = index.readLineLengths(fileName) - index.files[fileName] = lengths - } - if lengths == nil { - return 0, lineStatusFileUnknown - } - - if line == 0 || line > uint(len(lengths)) { - return 0, lineStatusLineMissing - } - return lengths[line-1], lineStatusOK -} - -// resolveSourcePath cleans a report file path and confines it to baseDir, -// rejecting anything that escapes. Report paths are repository relative and -// arrive in a scan-result payload, so they are treated as untrusted input. -// Symlinks are resolved so that containment is enforced on real paths rather -// than lexically. -func (index *sonarLineIndex) resolveSourcePath(fileName string) (path string, ok bool) { - if index.baseDir == "" { - return "", false - } - - // Reject anything that is not a plain relative path before touching disk. - relative := filepath.FromSlash(strings.TrimLeft(fileName, "/\\")) - if relative == "" || filepath.IsAbs(relative) || filepath.VolumeName(relative) != "" { - return "", false - } - - // filepath.Join applies filepath.Clean, collapsing any ".." elements. - cleaned := filepath.Join(index.baseDir, relative) - - // Resolve both sides: a path that cannot be resolved does not exist and is - // therefore not readable either way. - realBase, err := filepath.EvalSymlinks(index.baseDir) - if err != nil { - return "", false - } - realPath, err := filepath.EvalSymlinks(cleaned) - if err != nil { - return "", false - } - - inside, err := filepath.Rel(realBase, realPath) - if err != nil || inside == parentDir || strings.HasPrefix(inside, parentDir+string(os.PathSeparator)) { - return "", false - } - return realPath, true -} - -// readLineLengths returns the character length of each line of a file, or nil -// when the path is not permitted or the file cannot be read in full. -func (index *sonarLineIndex) readLineLengths(fileName string) []uint { - // resolveSourcePath validates and cleans the path and confines it to - // baseDir, so the value opened below is not attacker controlled. - path, ok := index.resolveSourcePath(fileName) - if !ok { - return nil - } - - file, err := os.Open(path) - if err != nil { - return nil - } - defer func() { _ = file.Close() }() - - scanner := bufio.NewScanner(file) - scanner.Buffer(make([]byte, 0, bufio.MaxScanTokenSize), maxSonarLineBytes) - - lengths := []uint{} - for scanner.Scan() { - text := scanner.Text() - if len(lengths) == 0 { - text = strings.TrimPrefix(text, string(byteOrderMarkRune)) - } - // bufio.ScanLines already drops a trailing CR; this guards against a - // lone CR surviving into the count. - text = strings.TrimSuffix(text, "\r") - lengths = append(lengths, uint(utf8.RuneCountInString(text))) - } - if scanner.Err() != nil { - return nil - } - return lengths -} - -// clampSonarColumns constrains a start offset and length to a line of -// lineLength characters. emit is false when no valid column range exists for -// the line, in which case the caller should emit a line-level location. -func clampSonarColumns(startColumn, length, lineLength uint) (start, end uint, emit bool) { - if lineLength == 0 { - return 0, 0, false - } - - start = startColumn - if start > lineLength { - start = lineLength - } - - end = start + length - if end > lineLength { - end = lineLength - } - - // A zero-length node, or a start offset sitting at end of line, leaves no - // forward span. Highlight the whole line rather than emitting a zero-width - // or inverted pointer, which SonarQube also rejects. - if end <= start { - return 0, lineLength, true - } - return start, end, true -} diff --git a/internal/commands/result_sonar_lines_test.go b/internal/commands/result_sonar_lines_test.go deleted file mode 100644 index ff4babd26..000000000 --- a/internal/commands/result_sonar_lines_test.go +++ /dev/null @@ -1,350 +0,0 @@ -package commands - -import ( - "os" - "path/filepath" - "strings" - "testing" - - "github.com/checkmarx/ast-cli/internal/params" - "github.com/checkmarx/ast-cli/internal/wrappers" - "github.com/stretchr/testify/assert" -) - -// writeSourceFile creates a file with the given content inside dir and returns -// its path relative to dir, which is the shape report file paths take. -func writeSourceFile(t *testing.T, dir, name, content string) string { - t.Helper() - path := filepath.Join(dir, name) - assert.NoError(t, os.MkdirAll(filepath.Dir(path), 0o750)) - assert.NoError(t, os.WriteFile(path, []byte(content), 0o600)) - return filepath.ToSlash(name) -} - -// assertValidSonarRange asserts the invariants SonarScanner enforces in -// DefaultInputFile: the line must exist in the file, and every offset must fall -// inside that line with the range moving forward. Violating any of them aborts -// the whole analysis, so totalLines is checked as well as lineLength. -func assertValidSonarRange(t *testing.T, tr *wrappers.SonarTextRange, totalLines, lineLength uint) { - t.Helper() - if tr == nil { - return // omitted entirely, so the issue is file level and always valid - } - assert.NotZero(t, tr.StartLine, "startLine must be set when textRange is present") - assert.LessOrEqual(t, tr.StartLine, totalLines, "startLine must exist in the file") - - if tr.StartColumn == 0 && tr.EndColumn == 0 { - return // line level range, always accepted - } - assert.LessOrEqual(t, tr.StartColumn, lineLength, "startColumn must not exceed line length") - assert.LessOrEqual(t, tr.EndColumn, lineLength, "endColumn must not exceed line length") - assert.Less(t, tr.StartColumn, tr.EndColumn, "range must move forward") -} - -func TestClampSonarColumns(t *testing.T) { - tests := []struct { - name string - startColumn uint - length uint - lineLength uint - wantStart uint - wantEnd uint - wantEmit bool - }{ - { - // The reported customer case: "@Component({" is 12 characters and - // the engine reported Column=12, Length=6, producing endColumn 17. - name: "overflowing end column is clamped to line length", - startColumn: 11, length: 6, lineLength: 12, - wantStart: 11, wantEnd: 12, wantEmit: true, - }, - { - name: "valid range is preserved unchanged", - startColumn: 4, length: 5, lineLength: 40, - wantStart: 4, wantEnd: 9, wantEmit: true, - }, - { - name: "range ending exactly at line length is valid", - startColumn: 0, length: 12, lineLength: 12, - wantStart: 0, wantEnd: 12, wantEmit: true, - }, - { - name: "zero length node falls back to the whole line", - startColumn: 4, length: 0, lineLength: 40, - wantStart: 0, wantEnd: 40, wantEmit: true, - }, - { - name: "start beyond end of line falls back to the whole line", - startColumn: 50, length: 3, lineLength: 12, - wantStart: 0, wantEnd: 12, wantEmit: true, - }, - { - name: "empty line yields no column range", - startColumn: 0, length: 3, lineLength: 0, - wantStart: 0, wantEnd: 0, wantEmit: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - start, end, emit := clampSonarColumns(tt.startColumn, tt.length, tt.lineLength) - assert.Equal(t, tt.wantEmit, emit) - assert.Equal(t, tt.wantStart, start) - assert.Equal(t, tt.wantEnd, end) - if emit { - assert.LessOrEqual(t, end, tt.lineLength) - assert.Less(t, start, end) - } - }) - } -} - -func TestSonarLineIndexLineLength(t *testing.T) { - dir := t.TempDir() - t.Chdir(dir) - - lfFile := writeSourceFile(t, dir, "lf.ts", "one\nthree3\n") - crlfFile := writeSourceFile(t, dir, "crlf.ts", "one\r\nthree3\r\n") - bomFile := writeSourceFile(t, dir, "bom.ts", string(byteOrderMarkRune)+"abc\n") - utf8File := writeSourceFile(t, dir, "utf8.ts", "héllo→\n") - emptyLineFile := writeSourceFile(t, dir, "nested/dir/empty.ts", "\nabc\n") - - index := newSonarLineIndex() - - t.Run("counts characters on an LF file", func(t *testing.T) { - length, status := index.resolveLine(lfFile, 2) - assert.Equal(t, lineStatusOK, status) - assert.Equal(t, uint(6), length) - }) - - t.Run("CRLF terminator is not counted", func(t *testing.T) { - length, status := index.resolveLine(crlfFile, 2) - assert.Equal(t, lineStatusOK, status) - assert.Equal(t, uint(6), length) - }) - - t.Run("leading BOM is not counted", func(t *testing.T) { - length, status := index.resolveLine(bomFile, 1) - assert.Equal(t, lineStatusOK, status) - assert.Equal(t, uint(3), length) - }) - - t.Run("multi byte characters count as one character each", func(t *testing.T) { - length, status := index.resolveLine(utf8File, 1) - assert.Equal(t, lineStatusOK, status) - // "héllo→" is 6 characters but 9 bytes. - assert.Equal(t, uint(6), length) - }) - - t.Run("empty line reports zero length", func(t *testing.T) { - length, status := index.resolveLine(emptyLineFile, 1) - assert.Equal(t, lineStatusOK, status) - assert.Equal(t, uint(0), length) - }) - - t.Run("leading slash in the report path is tolerated", func(t *testing.T) { - length, status := index.resolveLine("/"+lfFile, 1) - assert.Equal(t, lineStatusOK, status) - assert.Equal(t, uint(3), length) - }) - - t.Run("line past end of a readable file is reported missing", func(t *testing.T) { - _, status := index.resolveLine(lfFile, 99) - assert.Equal(t, lineStatusLineMissing, status) - }) - - t.Run("line zero of a readable file is reported missing", func(t *testing.T) { - _, status := index.resolveLine(lfFile, 0) - assert.Equal(t, lineStatusLineMissing, status) - }) - - t.Run("missing file is reported unknown, not missing line", func(t *testing.T) { - _, status := index.resolveLine("does/not/exist.ts", 1) - assert.Equal(t, lineStatusFileUnknown, status) - }) - - t.Run("empty file name is reported unknown", func(t *testing.T) { - _, status := index.resolveLine("", 1) - assert.Equal(t, lineStatusFileUnknown, status) - }) -} - -func TestSonarLineIndexRejectsPathsOutsideBaseDir(t *testing.T) { - dir := t.TempDir() - t.Chdir(dir) - index := newSonarLineIndex() - - for _, fileName := range []string{ - "../escape.ts", - "../../../../etc/passwd", - "nested/../../escape.ts", - } { - t.Run("rejects "+fileName, func(t *testing.T) { - _, ok := index.resolveSourcePath(fileName) - assert.False(t, ok, "path outside the working directory must be rejected") - }) - } - - t.Run("rejects an absolute path", func(t *testing.T) { - absolute := filepath.Join(dir, "abs.ts") - assert.NoError(t, os.WriteFile(absolute, []byte("abc\n"), 0o600)) - // Absolute inputs are refused even when they resolve inside baseDir. - _, ok := index.resolveSourcePath(absolute) - assert.False(t, ok) - }) - - t.Run("accepts a plain relative path inside the base directory", func(t *testing.T) { - name := writeSourceFile(t, dir, "inside.ts", "abc\n") - resolved, ok := index.resolveSourcePath(name) - assert.True(t, ok) - assert.True(t, strings.HasSuffix(resolved, "inside.ts")) - }) -} - -// TestParseSonarTextRangeCustomerRegression reproduces the reported failure end -// to end: a node whose Column plus Length runs past the end of the line it -// points at previously produced endColumn 17 on a 12 character line, which -// SonarQube rejects with "17 is not a valid line offset for pointer". -func TestParseSonarTextRangeCustomerRegression(t *testing.T) { - dir := t.TempDir() - t.Chdir(dir) - - const componentLine = "@Component({" // 12 characters, line 10 below - source := "import { Component, inject, OnInit } from '@angular/core'\n" + - "import { DomSanitizer } from '@angular/platform-browser'\n" + - "import jwtDecode from 'jwt-decode'\n" + - "import { TranslateModule } from '@ngx-translate/core'\n" + - "import { MatCardModule } from '@angular/material/card'\n" + - "\n\n\n\n" + // lines 6 to 9 - componentLine + "\n" + // line 10 - " selector: 'app-last-login-ip',\n" - - fileName := writeSourceFile(t, - dir, - "cxone-sq-integration/juice-shop-master/frontend/src/app/last-login-ip/last-login-ip.component.ts", - source, - ) - - index := newSonarLineIndex() - node := &wrappers.ScanResultNode{ - FileName: fileName, - Line: 10, - Column: 12, - Length: 6, - } - - textRange := parseSonarTextRange(node, index) - - assert.NotNil(t, textRange) - assert.Equal(t, uint(10), textRange.StartLine) - assert.Equal(t, uint(11), textRange.StartColumn) - assert.Equal(t, uint(12), textRange.EndColumn, "endColumn must be clamped to the 12 character line, not 17") - assertValidSonarRange(t, textRange, 11, uint(len(componentLine))) -} - -func TestParseSonarTextRangeFallsBackToLineLevel(t *testing.T) { - dir := t.TempDir() - t.Chdir(dir) - index := newSonarLineIndex() - - t.Run("unreadable file keeps the line and emits no columns", func(t *testing.T) { - node := &wrappers.ScanResultNode{FileName: "absent.ts", Line: 7, Column: 12, Length: 6} - textRange := parseSonarTextRange(node, index) - assert.NotNil(t, textRange) - assert.Equal(t, uint(7), textRange.StartLine) - assert.Zero(t, textRange.StartColumn, "columns are omitempty, so zero drops them from the report") - assert.Zero(t, textRange.EndColumn) - }) - - t.Run("empty line emits no columns", func(t *testing.T) { - fileName := writeSourceFile(t, dir, "blank.ts", "\nabc\n") - node := &wrappers.ScanResultNode{FileName: fileName, Line: 1, Column: 3, Length: 4} - textRange := parseSonarTextRange(node, index) - assert.NotNil(t, textRange) - assert.Equal(t, uint(1), textRange.StartLine) - assert.Zero(t, textRange.StartColumn) - assert.Zero(t, textRange.EndColumn) - }) -} - -// TestParseSonarTextRangeOmitsRangeForMissingLine covers the minified asset case -// found on real scan data: the engine reported line 803 of a file that has only -// 82 lines, because its coordinates are against a transformed view of the -// source. Emitting that line at all aborts the SonarQube analysis, so the whole -// textRange must be dropped and the issue reported at file level. -func TestParseSonarTextRangeOmitsRangeForMissingLine(t *testing.T) { - dir := t.TempDir() - t.Chdir(dir) - - fileName := writeSourceFile(t, dir, "assets/private/dat.gui.min.js", "var a=1\nvar b=2\n") - index := newSonarLineIndex() - - node := &wrappers.ScanResultNode{FileName: fileName, Line: 803, Column: 5, Length: 4} - textRange := parseSonarTextRange(node, index) - - assert.Nil(t, textRange, "textRange must be omitted when the line does not exist in the file") -} - -// TestParseSonarAllLocationsAreValid walks a result with a primary and several -// secondary nodes, all deliberately overflowing, and asserts every emitted -// location satisfies SonarQube's invariant. A single violation anywhere in the -// report aborts the whole import, so secondary locations matter as much as the -// primary one. -func TestParseSonarAllLocationsAreValid(t *testing.T) { - dir := t.TempDir() - t.Chdir(dir) - - const line = "@Component({" // 12 characters - fileName := writeSourceFile(t, dir, "app/widget.component.ts", line+"\n"+line+"\n") - - results := &wrappers.ScanResultsCollection{ - Results: []*wrappers.ScanResult{ - { - Type: params.SastType, - ScanResultData: wrappers.ScanResultData{ - QueryName: "Angular_Client_Stored_DOM_XSS", - Nodes: []*wrappers.ScanResultNode{ - {FileName: fileName, Line: 1, Column: 12, Length: 6}, - {FileName: fileName, Line: 2, Column: 40, Length: 9}, - {FileName: fileName, Line: 1, Column: 1, Length: 0}, - {FileName: "missing.ts", Line: 3, Column: 5, Length: 4}, - // Line beyond end of a readable file: must be dropped. - {FileName: fileName, Line: 803, Column: 5, Length: 4}, - }, - }, - }, - }, - } - - issues, _ := parseSonar(results) - - assert.Len(t, issues, 1) - // Five nodes: one primary plus four secondaries, of which the node on line - // 803 is dropped because SonarQube requires textRange on secondary - // locations and no valid range exists for it. - assert.Len(t, issues[0].SecondaryLocations, 3) - - all := append([]wrappers.SonarLocation{issues[0].PrimaryLocation}, issues[0].SecondaryLocations...) - for i := range all { - location := all[i] - t.Run("location "+string(rune('A'+i)), func(t *testing.T) { - if location.FilePath != fileName { - // A file that is not on disk cannot be verified, so the engine - // line is kept and only the columns are dropped. - assert.NotNil(t, location.TextRange) - assert.NotZero(t, location.TextRange.StartLine) - assert.Zero(t, location.TextRange.StartColumn) - assert.Zero(t, location.TextRange.EndColumn) - return - } - assertValidSonarRange(t, location.TextRange, 2, uint(len(line))) - }) - } - - // No secondary location may carry a nil textRange: SonarQube rejects the - // whole report with "missing mandatory field 'textRange' in a secondary - // location of the issue". - for _, location := range issues[0].SecondaryLocations { - assert.NotNil(t, location.TextRange, "secondary locations must always carry a textRange") - } -} diff --git a/internal/commands/result_test.go b/internal/commands/result_test.go index ce82ad0ed..660074995 100644 --- a/internal/commands/result_test.go +++ b/internal/commands/result_test.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "os" + "path/filepath" "reflect" "regexp" "strings" @@ -19,6 +20,7 @@ import ( "github.com/checkmarx/ast-cli/internal/wrappers" "github.com/checkmarx/ast-cli/internal/wrappers/mock" "github.com/pkg/errors" + asserts "github.com/stretchr/testify/assert" "golang.org/x/text/cases" "golang.org/x/text/language" "gotest.tools/assert" @@ -1938,3 +1940,320 @@ func (m *customScanSummaryMockWrapper) GetScanSummaryByScanID(scanID string) (*w TotalCount: 1, }, nil, nil } + +// writeSourceFile creates a file with the given content inside dir and returns its path relative to dir. +func writeSourceFile(t *testing.T, dir, name, content string) string { + t.Helper() + path := filepath.Join(dir, name) + asserts.NoError(t, os.MkdirAll(filepath.Dir(path), 0o750)) + asserts.NoError(t, os.WriteFile(path, []byte(content), 0o600)) + return filepath.ToSlash(name) +} + +// assertValidSonarRange asserts a textRange satisfies the bounds SonarScanner enforces on import. +func assertValidSonarRange(t *testing.T, tr *wrappers.SonarTextRange, totalLines, lineLength uint) { + t.Helper() + if tr == nil { + return // omitted entirely, so the issue is file level and always valid + } + asserts.NotZero(t, tr.StartLine, "startLine must be set when textRange is present") + asserts.LessOrEqual(t, tr.StartLine, totalLines, "startLine must exist in the file") + + if tr.StartColumn == 0 && tr.EndColumn == 0 { + return // line level range, always accepted + } + asserts.LessOrEqual(t, tr.StartColumn, lineLength, "startColumn must not exceed line length") + asserts.LessOrEqual(t, tr.EndColumn, lineLength, "endColumn must not exceed line length") + asserts.Less(t, tr.StartColumn, tr.EndColumn, "range must move forward") +} + +func TestClampSonarColumns(t *testing.T) { + tests := []struct { + name string + startColumn uint + length uint + lineLength uint + wantStart uint + wantEnd uint + wantEmit bool + }{ + { + // Reported case: a 12 character line with Column=12, Length=6 produced endColumn 17. + name: "overflowing end column is clamped to line length", + startColumn: 11, length: 6, lineLength: 12, + wantStart: 11, wantEnd: 12, wantEmit: true, + }, + { + name: "valid range is preserved unchanged", + startColumn: 4, length: 5, lineLength: 40, + wantStart: 4, wantEnd: 9, wantEmit: true, + }, + { + name: "range ending exactly at line length is valid", + startColumn: 0, length: 12, lineLength: 12, + wantStart: 0, wantEnd: 12, wantEmit: true, + }, + { + name: "zero length node falls back to the whole line", + startColumn: 4, length: 0, lineLength: 40, + wantStart: 0, wantEnd: 40, wantEmit: true, + }, + { + name: "start beyond end of line falls back to the whole line", + startColumn: 50, length: 3, lineLength: 12, + wantStart: 0, wantEnd: 12, wantEmit: true, + }, + { + name: "empty line yields no column range", + startColumn: 0, length: 3, lineLength: 0, + wantStart: 0, wantEnd: 0, wantEmit: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + start, end, emit := clampSonarColumns(tt.startColumn, tt.length, tt.lineLength) + asserts.Equal(t, tt.wantEmit, emit) + asserts.Equal(t, tt.wantStart, start) + asserts.Equal(t, tt.wantEnd, end) + if emit { + asserts.LessOrEqual(t, end, tt.lineLength) + asserts.Less(t, start, end) + } + }) + } +} + +func TestSonarLineIndexLineLength(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + + lfFile := writeSourceFile(t, dir, "lf.ts", "one\nthree3\n") + crlfFile := writeSourceFile(t, dir, "crlf.ts", "one\r\nthree3\r\n") + bomFile := writeSourceFile(t, dir, "bom.ts", string(byteOrderMarkRune)+"abc\n") + utf8File := writeSourceFile(t, dir, "utf8.ts", "héllo→\n") + emptyLineFile := writeSourceFile(t, dir, "nested/dir/empty.ts", "\nabc\n") + + index := newSonarLineIndex() + + t.Run("counts characters on an LF file", func(t *testing.T) { + length, status := index.resolveLine(lfFile, 2) + asserts.Equal(t, lineStatusOK, status) + asserts.Equal(t, uint(6), length) + }) + + t.Run("CRLF terminator is not counted", func(t *testing.T) { + length, status := index.resolveLine(crlfFile, 2) + asserts.Equal(t, lineStatusOK, status) + asserts.Equal(t, uint(6), length) + }) + + t.Run("leading BOM is not counted", func(t *testing.T) { + length, status := index.resolveLine(bomFile, 1) + asserts.Equal(t, lineStatusOK, status) + asserts.Equal(t, uint(3), length) + }) + + t.Run("multi byte characters count as one character each", func(t *testing.T) { + length, status := index.resolveLine(utf8File, 1) + asserts.Equal(t, lineStatusOK, status) + // "héllo→" is 6 characters but 9 bytes. + asserts.Equal(t, uint(6), length) + }) + + t.Run("empty line reports zero length", func(t *testing.T) { + length, status := index.resolveLine(emptyLineFile, 1) + asserts.Equal(t, lineStatusOK, status) + asserts.Equal(t, uint(0), length) + }) + + t.Run("leading slash in the report path is tolerated", func(t *testing.T) { + length, status := index.resolveLine("/"+lfFile, 1) + asserts.Equal(t, lineStatusOK, status) + asserts.Equal(t, uint(3), length) + }) + + t.Run("line past end of a readable file is reported missing", func(t *testing.T) { + _, status := index.resolveLine(lfFile, 99) + asserts.Equal(t, lineStatusLineMissing, status) + }) + + t.Run("line zero of a readable file is reported missing", func(t *testing.T) { + _, status := index.resolveLine(lfFile, 0) + asserts.Equal(t, lineStatusLineMissing, status) + }) + + t.Run("missing file is reported unknown, not missing line", func(t *testing.T) { + _, status := index.resolveLine("does/not/exist.ts", 1) + asserts.Equal(t, lineStatusFileUnknown, status) + }) + + t.Run("empty file name is reported unknown", func(t *testing.T) { + _, status := index.resolveLine("", 1) + asserts.Equal(t, lineStatusFileUnknown, status) + }) +} + +func TestSonarLineIndexRejectsPathsOutsideBaseDir(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + index := newSonarLineIndex() + + for _, fileName := range []string{ + "../escape.ts", + "../../../../etc/passwd", + "nested/../../escape.ts", + } { + t.Run("rejects "+fileName, func(t *testing.T) { + _, ok := index.resolveSourcePath(fileName) + asserts.False(t, ok, "path outside the working directory must be rejected") + }) + } + + t.Run("rejects an absolute path", func(t *testing.T) { + absolute := filepath.Join(dir, "abs.ts") + asserts.NoError(t, os.WriteFile(absolute, []byte("abc\n"), 0o600)) + // Absolute inputs are refused even when they resolve inside baseDir. + _, ok := index.resolveSourcePath(absolute) + asserts.False(t, ok) + }) + + t.Run("accepts a plain relative path inside the base directory", func(t *testing.T) { + name := writeSourceFile(t, dir, "inside.ts", "abc\n") + resolved, ok := index.resolveSourcePath(name) + asserts.True(t, ok) + asserts.True(t, strings.HasSuffix(resolved, "inside.ts")) + }) +} + +// TestParseSonarTextRangeCustomerRegression reproduces the reported "17 is not a valid line offset" failure end to end. +func TestParseSonarTextRangeCustomerRegression(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + + const componentLine = "@Component({" // 12 characters, line 10 below + source := "import { Component, inject, OnInit } from '@angular/core'\n" + + "import { DomSanitizer } from '@angular/platform-browser'\n" + + "import jwtDecode from 'jwt-decode'\n" + + "import { TranslateModule } from '@ngx-translate/core'\n" + + "import { MatCardModule } from '@angular/material/card'\n" + + "\n\n\n\n" + // lines 6 to 9 + componentLine + "\n" + // line 10 + " selector: 'app-last-login-ip',\n" + + fileName := writeSourceFile(t, + dir, + "cxone-sq-integration/juice-shop-master/frontend/src/app/last-login-ip/last-login-ip.component.ts", + source, + ) + + index := newSonarLineIndex() + node := &wrappers.ScanResultNode{ + FileName: fileName, + Line: 10, + Column: 12, + Length: 6, + } + + textRange := parseSonarTextRange(node, index) + + asserts.NotNil(t, textRange) + asserts.Equal(t, uint(10), textRange.StartLine) + asserts.Equal(t, uint(11), textRange.StartColumn) + asserts.Equal(t, uint(12), textRange.EndColumn, "endColumn must be clamped to the 12 character line, not 17") + assertValidSonarRange(t, textRange, 11, uint(len(componentLine))) +} + +func TestParseSonarTextRangeFallsBackToLineLevel(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + index := newSonarLineIndex() + + t.Run("unreadable file keeps the line and emits no columns", func(t *testing.T) { + node := &wrappers.ScanResultNode{FileName: "absent.ts", Line: 7, Column: 12, Length: 6} + textRange := parseSonarTextRange(node, index) + asserts.NotNil(t, textRange) + asserts.Equal(t, uint(7), textRange.StartLine) + asserts.Zero(t, textRange.StartColumn, "columns are omitempty, so zero drops them from the report") + asserts.Zero(t, textRange.EndColumn) + }) + + t.Run("empty line emits no columns", func(t *testing.T) { + fileName := writeSourceFile(t, dir, "blank.ts", "\nabc\n") + node := &wrappers.ScanResultNode{FileName: fileName, Line: 1, Column: 3, Length: 4} + textRange := parseSonarTextRange(node, index) + asserts.NotNil(t, textRange) + asserts.Equal(t, uint(1), textRange.StartLine) + asserts.Zero(t, textRange.StartColumn) + asserts.Zero(t, textRange.EndColumn) + }) +} + +// TestParseSonarTextRangeOmitsRangeForMissingLine covers the minified-asset case where the engine reports a line past the end of the file. +func TestParseSonarTextRangeOmitsRangeForMissingLine(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + + fileName := writeSourceFile(t, dir, "assets/private/dat.gui.min.js", "var a=1\nvar b=2\n") + index := newSonarLineIndex() + + node := &wrappers.ScanResultNode{FileName: fileName, Line: 803, Column: 5, Length: 4} + textRange := parseSonarTextRange(node, index) + + asserts.Nil(t, textRange, "textRange must be omitted when the line does not exist in the file") +} + +// TestParseSonarAllLocationsAreValid asserts every location of a result with overflowing nodes satisfies SonarQube's invariant. +func TestParseSonarAllLocationsAreValid(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + + const line = "@Component({" // 12 characters + fileName := writeSourceFile(t, dir, "app/widget.component.ts", line+"\n"+line+"\n") + + results := &wrappers.ScanResultsCollection{ + Results: []*wrappers.ScanResult{ + { + Type: params.SastType, + ScanResultData: wrappers.ScanResultData{ + QueryName: "Angular_Client_Stored_DOM_XSS", + Nodes: []*wrappers.ScanResultNode{ + {FileName: fileName, Line: 1, Column: 12, Length: 6}, + {FileName: fileName, Line: 2, Column: 40, Length: 9}, + {FileName: fileName, Line: 1, Column: 1, Length: 0}, + {FileName: "missing.ts", Line: 3, Column: 5, Length: 4}, + // Line beyond end of a readable file: must be dropped. + {FileName: fileName, Line: 803, Column: 5, Length: 4}, + }, + }, + }, + }, + } + + issues, _ := parseSonar(results) + + asserts.Len(t, issues, 1) + // The node on line 803 is dropped: it has no valid textRange, which is mandatory on secondary locations. + asserts.Len(t, issues[0].SecondaryLocations, 3) + + all := append([]wrappers.SonarLocation{issues[0].PrimaryLocation}, issues[0].SecondaryLocations...) + for i := range all { + location := all[i] + t.Run("location "+string(rune('A'+i)), func(t *testing.T) { + if location.FilePath != fileName { + // A file that is not on disk keeps its line but drops the columns. + asserts.NotNil(t, location.TextRange) + asserts.NotZero(t, location.TextRange.StartLine) + asserts.Zero(t, location.TextRange.StartColumn) + asserts.Zero(t, location.TextRange.EndColumn) + return + } + assertValidSonarRange(t, location.TextRange, 2, uint(len(line))) + }) + } + + // SonarQube rejects the whole report if any secondary location has a nil textRange. + for _, location := range issues[0].SecondaryLocations { + asserts.NotNil(t, location.TextRange, "secondary locations must always carry a textRange") + } +} From fe9d6ce976706a83cad13c48feb28db730a8c50f Mon Sep 17 00:00:00 2001 From: Harshjeet Patil Date: Tue, 25 Aug 2026 15:14:12 +0530 Subject: [PATCH 3/4] changes --- .checkmarx/checkmarxIgnoredTempList.json | 7 ------- .gitignore | 5 ++++- internal/commands/config.yaml | 0 internal/commands/result.go | 12 ++++++------ internal/wrappers/results-sonar.go | 5 +---- 5 files changed, 11 insertions(+), 18 deletions(-) delete mode 100644 .checkmarx/checkmarxIgnoredTempList.json delete mode 100644 internal/commands/config.yaml diff --git a/.checkmarx/checkmarxIgnoredTempList.json b/.checkmarx/checkmarxIgnoredTempList.json deleted file mode 100644 index a42259d4c..000000000 --- a/.checkmarx/checkmarxIgnoredTempList.json +++ /dev/null @@ -1,7 +0,0 @@ -[ - { - "FileName": "result.go", - "Line": 2630, - "RuleID": 3007 - } -] \ No newline at end of file diff --git a/.gitignore b/.gitignore index f77686f22..5480fdb94 100644 --- a/.gitignore +++ b/.gitignore @@ -66,4 +66,7 @@ override.tf.json vendor/* # Build artifacts and temporary directories -internal/commands/data/manifests/obj/ \ No newline at end of file +internal/commands/data/manifests/obj/ + +# Local realtime-engine ignore list (per-checkout runtime state, not app code) +/.checkmarx/ \ No newline at end of file diff --git a/internal/commands/config.yaml b/internal/commands/config.yaml deleted file mode 100644 index e69de29bb..000000000 diff --git a/internal/commands/result.go b/internal/commands/result.go index b715cd9ef..7714d90e1 100644 --- a/internal/commands/result.go +++ b/internal/commands/result.go @@ -2525,6 +2525,10 @@ type sonarLineIndex struct { func newSonarLineIndex() *sonarLineIndex { baseDir, err := os.Getwd() + if err == nil { + // Resolved once here so resolveSourcePath can compare against it directly on every call. + baseDir, err = filepath.EvalSymlinks(baseDir) + } if err != nil { baseDir = "" } @@ -2533,7 +2537,7 @@ func newSonarLineIndex() *sonarLineIndex { // resolveLine reports whether the given 1-based line of fileName exists and, when it does, how many characters it holds. func (index *sonarLineIndex) resolveLine(fileName string, line uint) (length uint, status lineStatus) { - if index == nil || fileName == "" { + if fileName == "" { return 0, lineStatusFileUnknown } @@ -2566,16 +2570,12 @@ func (index *sonarLineIndex) resolveSourcePath(fileName string) (path string, ok cleaned := filepath.Join(index.baseDir, relative) // EvalSymlinks confines containment to the real path, not a lexical one. - realBase, err := filepath.EvalSymlinks(index.baseDir) - if err != nil { - return "", false - } realPath, err := filepath.EvalSymlinks(cleaned) if err != nil { return "", false } - inside, err := filepath.Rel(realBase, realPath) + inside, err := filepath.Rel(index.baseDir, realPath) if err != nil || inside == parentDir || strings.HasPrefix(inside, parentDir+string(os.PathSeparator)) { return "", false } diff --git a/internal/wrappers/results-sonar.go b/internal/wrappers/results-sonar.go index afc86d5a5..a8f18392b 100644 --- a/internal/wrappers/results-sonar.go +++ b/internal/wrappers/results-sonar.go @@ -29,10 +29,7 @@ type SonarIssues struct { type SonarLocation struct { Message string `json:"message,omitempty"` FilePath string `json:"filePath,omitempty"` - // TextRange is omitted when the location cannot be expressed validly, for - // example when the engine reports a line that does not exist in the file on - // disk. SonarQube treats a location without a textRange as file level, - // whereas an out of range line aborts the whole analysis. + // TextRange is omitted when the engine reports a line that does not exist in the file on disk. TextRange *SonarTextRange `json:"textRange,omitempty"` } From 0eb60c42341bf2ae4d9a147e5e274c782a7b843d Mon Sep 17 00:00:00 2001 From: Harshjeet Patil Date: Tue, 25 Aug 2026 15:58:44 +0530 Subject: [PATCH 4/4] restored file --- .gitignore | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index 5480fdb94..f77686f22 100644 --- a/.gitignore +++ b/.gitignore @@ -66,7 +66,4 @@ override.tf.json vendor/* # Build artifacts and temporary directories -internal/commands/data/manifests/obj/ - -# Local realtime-engine ignore list (per-checkout runtime state, not app code) -/.checkmarx/ \ No newline at end of file +internal/commands/data/manifests/obj/ \ No newline at end of file