fix(privacy): skip comments when scanning for tracking SDKs and ATT

internal/privacy/scanner.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
+	"sort"
 	"strings"
 )
 
@@ -93,6 +94,9 @@ var requiredReasonAPIs = []RequiredReasonAPI{
 	},
 }
 
+// attPattern matches an App Tracking Transparency implementation.
+var attPattern = regexp.MustCompile(`(?i)(ATTrackingManager|requestTrackingAuthorization|AppTrackingTransparency|expo-tracking-transparency)`)
+
 // Known tracking/advertising SDKs
 var trackingSDKPatterns = []struct {
 	Pattern *regexp.Regexp
@@ -138,6 +142,7 @@ func Scan(projectPath string) (*ScanResult, error) {
 	// 2. Scan code for Required Reason API usage
 	detectedAPIs := make(map[string][]FileHit)
 	trackingSDKsFound := make(map[string]bool)
+	trackingSDKHits := make(map[string]FileHit)
 	hasATT := false
 
 	skipDirs := map[string]bool{
@@ -165,17 +170,32 @@ func Scan(projectPath string) (*ScanResult, error) {
 			return nil
 		}
 
-		fullContent := strings.Join(lines, "\n")
+		// Tracking SDKs and the ATT call are matched line by line, skipping comments.
+		// A commented-out reference is not an integration: counting one as a tracking
+		// SDK produces a CRITICAL for an app that does not track, and counting one as
+		// an ATT implementation hides a real CRITICAL from an app that does.
+		for lineNum, line := range lines {
+			if isCommentLine(line) {
+				continue
+			}
 
-		// Check for ATT implementation
-		if regexp.MustCompile(`(?i)(ATTrackingManager|requestTrackingAuthorization|AppTrackingTransparency|expo-tracking-transparency)`).MatchString(fullContent) {
-			hasATT = true
-		}
+			if attPattern.MatchString(line) {
+				hasATT = true
+			}
 
-		// Check for tracking SDKs
-		for _, sdk := range trackingSDKPatterns {
-			if sdk.Pattern.MatchString(fullContent) {
+			for _, sdk := range trackingSDKPatterns {
+				if !sdk.Pattern.MatchString(line) {
+					continue
+				}
 				trackingSDKsFound[sdk.Name] = true
+				if _, seen := trackingSDKHits[sdk.Name]; !seen {
+					trackingSDKHits[sdk.Name] = FileHit{
+						File: relPath,
+						Line: lineNum + 1,
+						Code: strings.TrimSpace(line),
+						API:  sdk.Name,
+					}
+				}
 			}
 		}
 
@@ -185,8 +205,7 @@ func Scan(projectPath string) (*ScanResult, error) {
 				continue
 			}
 			for lineNum, line := range lines {
-				trimmed := strings.TrimSpace(line)
-				if strings.HasPrefix(trimmed, "//") || strings.HasPrefix(trimmed, "/*") || strings.HasPrefix(trimmed, "*") {
+				if isCommentLine(line) {
 					continue
 				}
 				for _, p := range api.Patterns {
@@ -242,16 +261,26 @@ func Scan(projectPath string) (*ScanResult, error) {
 	for sdk := range trackingSDKsFound {
 		result.TrackingSDKs = append(result.TrackingSDKs, sdk)
 	}
+	// Map iteration is randomised, so sort for a stable report.
+	sort.Strings(result.TrackingSDKs)
 
 	if len(trackingSDKsFound) > 0 && !hasATT {
 		sdkList := strings.Join(result.TrackingSDKs, ", ")
-		result.Findings = append(result.Findings, Finding{
+		finding := Finding{
 			Severity:  "CRITICAL",
 			Guideline: "5.1.2",
 			Title:     "Tracking SDKs detected without ATT implementation",
 			Detail:    "Found: " + sdkList + ". App Tracking Transparency prompt is required before any tracking.",
 			Fix:       "Import AppTrackingTransparency and call requestTrackingAuthorization() before initializing any tracking SDK.",
-		})
+		}
+		// Cite where the first SDK was matched, so the finding can be checked
+		// against the source the way the Required Reason findings already can be.
+		if hit, ok := earliestHit(trackingSDKHits); ok {
+			finding.File = hit.File
+			finding.Line = hit.Line
+			finding.Detail += " First seen at " + hit.File + ":" + fmt.Sprint(hit.Line) + "."
+		}
+		result.Findings = append(result.Findings, finding)
 	}
 
 	// 5. Check if privacy manifest declares tracking but no tracking SDKs found
@@ -335,6 +364,31 @@ func parsePrivacyManifest(content string) []string {
 	return apis
 }
 
+// earliestHit returns the hit that comes first in the project, ordered by file
+// then line. Ranging over the map directly would pick an arbitrary SDK, so the
+// cited location — and "first seen" — would change between runs.
+func earliestHit(hits map[string]FileHit) (FileHit, bool) {
+	var earliest FileHit
+	found := false
+	for _, hit := range hits {
+		if !found || hit.File < earliest.File || (hit.File == earliest.File && hit.Line < earliest.Line) {
+			earliest = hit
+			found = true
+		}
+	}
+	return earliest, found
+}
+
+// isCommentLine reports whether a source line is a single-line or block comment.
+// It is deliberately conservative: it only skips lines that begin with a comment
+// marker, so a trailing comment on a line of real code is still scanned.
+func isCommentLine(line string) bool {
+	trimmed := strings.TrimSpace(line)
+	return strings.HasPrefix(trimmed, "//") ||
+		strings.HasPrefix(trimmed, "/*") ||
+		strings.HasPrefix(trimmed, "*")
+}
+
 func detectLang(path string) string {
 	ext := strings.ToLower(filepath.Ext(path))
 	switch ext {

internal/privacy/scanner_test.go

@@ -1,8 +1,11 @@
 package privacy
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
+	"slices"
+	"strings"
 	"testing"
 )
 
@@ -39,3 +42,142 @@ func TestRequiredReasonDetectsRealCallSites(t *testing.T) {
 		}
 	}
 }
+
+// Tracking SDKs and the ATT call used to be matched against the whole file with
+// MatchString(fullContent), which reads commented-out code as if it were live. The
+// Required Reason scan in the same walk already skipped comments; these two did not.
+func TestTrackingScanIgnoresComments(t *testing.T) {
+	dir := t.TempDir()
+	writeFile(t, dir, "Notes.swift", strings.Join([]string{
+		"// We deliberately do not use mixpanel here.",
+		"/* AppsFlyer was removed in v2.0 */",
+		" * and so was applovin",
+		"func body() { render() }",
+	}, "\n")+"\n")
+
+	res, err := Scan(dir)
+	if err != nil {
+		t.Fatalf("Scan: %v", err)
+	}
+	if len(res.TrackingSDKs) != 0 {
+		t.Errorf("commented-out references counted as tracking SDKs: %v", res.TrackingSDKs)
+	}
+	for _, f := range res.Findings {
+		if f.Guideline == "5.1.2" {
+			t.Errorf("commented-out references produced a §5.1.2 finding: %+v", f)
+		}
+	}
+}
+
+// The inverse, and the more dangerous direction: an ATT call that only exists in a
+// comment used to suppress the CRITICAL for an app that really does track.
+func TestCommentedOutATTDoesNotSuppressFinding(t *testing.T) {
+	dir := t.TempDir()
+	writeFile(t, dir, "Tracking.swift", strings.Join([]string{
+		"import AppsFlyerLib",
+		"// TODO: call ATTrackingManager.requestTrackingAuthorization before this ships",
+		"func start() { AppsFlyerLib.shared().start() }",
+	}, "\n")+"\n")
+
+	res, err := Scan(dir)
+	if err != nil {
+		t.Fatalf("Scan: %v", err)
+	}
+
+	var found bool
+	for _, f := range res.Findings {
+		if f.Guideline == "5.1.2" {
+			found = true
+		}
+	}
+	if !found {
+		t.Errorf("a commented-out ATT call suppressed the §5.1.2 finding; findings=%+v", res.Findings)
+	}
+}
+
+// A trailing comment must not hide the code in front of it.
+func TestTrackingScanStillReadsCodeWithTrailingComment(t *testing.T) {
+	dir := t.TempDir()
+	writeFile(t, dir, "A.swift", "import AppsFlyerLib // analytics\n")
+
+	res, err := Scan(dir)
+	if err != nil {
+		t.Fatalf("Scan: %v", err)
+	}
+	if len(res.TrackingSDKs) == 0 {
+		t.Error("real SDK on a line with a trailing comment was not detected")
+	}
+}
+
+// The §5.1.2 finding is the only CRITICAL that carried no location, which is exactly
+// what makes a false positive hard to disprove. Finding already has File and Line.
+func TestTrackingFindingCitesFileAndLine(t *testing.T) {
+	dir := t.TempDir()
+	writeFile(t, dir, "Analytics.swift", strings.Join([]string{
+		"import Foundation",
+		"import AppsFlyerLib",
+		"func boot() {}",
+	}, "\n")+"\n")
+
+	res, err := Scan(dir)
+	if err != nil {
+		t.Fatalf("Scan: %v", err)
+	}
+
+	for _, f := range res.Findings {
+		if f.Guideline != "5.1.2" {
+			continue
+		}
+		if f.File != "Analytics.swift" {
+			t.Errorf("File = %q, want %q", f.File, "Analytics.swift")
+		}
+		if f.Line != 2 {
+			t.Errorf("Line = %d, want 2", f.Line)
+		}
+		return
+	}
+	t.Fatalf("no §5.1.2 finding produced; findings=%+v", res.Findings)
+}
+
+// TrackingSDKs is built by ranging over a map, and Go randomises map iteration, so
+// both the reported SDK list and the cited location used to change between runs on
+// a project using more than one SDK. The list is sorted, and the citation is the
+// genuinely earliest hit by file then line rather than whichever key came out first.
+func TestTrackingFindingIsDeterministic(t *testing.T) {
+	dir := t.TempDir()
+	writeFile(t, dir, "A.swift", "import AppsFlyerLib\n")
+	writeFile(t, dir, "B.swift", "import Mixpanel\n")
+	writeFile(t, dir, "C.swift", "import AppLovinSDK\n")
+
+	var first string
+	for i := 0; i < 25; i++ {
+		res, err := Scan(dir)
+		if err != nil {
+			t.Fatalf("Scan: %v", err)
+		}
+
+		if want := []string{"AppLovin", "AppsFlyer", "Mixpanel"}; !slices.Equal(res.TrackingSDKs, want) {
+			t.Fatalf("TrackingSDKs = %v, want %v (sorted)", res.TrackingSDKs, want)
+		}
+
+		var got string
+		for _, f := range res.Findings {
+			if f.Guideline == "5.1.2" {
+				got = fmt.Sprintf("%s:%d|%s", f.File, f.Line, f.Detail)
+			}
+		}
+		if got == "" {
+			t.Fatalf("no §5.1.2 finding; findings=%+v", res.Findings)
+		}
+		if i == 0 {
+			first = got
+			if !strings.HasPrefix(got, "A.swift:1|") {
+				t.Errorf("citation = %q, want the earliest hit A.swift:1", got)
+			}
+			continue
+		}
+		if got != first {
+			t.Fatalf("finding changed between runs:\n  run 0: %s\n  run %d: %s", first, i, got)
+		}
+	}
+}