From f04b441fd2d034a74fd42da4cbccf11316322eee Mon Sep 17 00:00:00 2001 From: Maher Date: Sun, 20 Sep 2026 00:08:24 +0200 Subject: [PATCH 1/2] fix(privacy): skip comments when scanning for tracking SDKs and ATT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tracking SDK and ATT checks matched against the whole file with MatchString(fullContent), so commented-out code counted as live. The Required Reason scan in the same walk already skipped comment lines. Both directions were wrong. A file containing only `// we do not use mixpanel` reported Mixpanel as a tracking SDK and produced a CRITICAL §5.1.2 for an app that does not track. More seriously, a commented-out `ATTrackingManager.requestTrackingAuthorization` set hasATT and hid the CRITICAL from an app that really does track. Match line by line instead, reusing the same comment rule, which is equivalent for these patterns because Go's `.` does not cross newlines. Only lines that begin with a comment marker are skipped, so a trailing comment does not hide the code in front of it. Also record where each SDK was first seen. §5.1.2 was the only CRITICAL that cited no location, which is what makes a false positive hard to check against the source; Finding already had File and Line. The ATT regexp is hoisted out of the walk rather than recompiled per file. --- internal/privacy/scanner.go | 60 ++++++++++++++++---- internal/privacy/scanner_test.go | 97 ++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 12 deletions(-) diff --git a/internal/privacy/scanner.go b/internal/privacy/scanner.go index 0fc7744..eb6603f 100644 --- a/internal/privacy/scanner.go +++ b/internal/privacy/scanner.go @@ -93,6 +93,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 +141,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 +169,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 +204,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 { @@ -245,13 +263,21 @@ func Scan(projectPath string) (*ScanResult, error) { 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 := trackingSDKHits[result.TrackingSDKs[0]]; 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 +361,16 @@ func parsePrivacyManifest(content string) []string { return apis } +// 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 { diff --git a/internal/privacy/scanner_test.go b/internal/privacy/scanner_test.go index 1335846..0267410 100644 --- a/internal/privacy/scanner_test.go +++ b/internal/privacy/scanner_test.go @@ -3,6 +3,7 @@ package privacy import ( "os" "path/filepath" + "strings" "testing" ) @@ -39,3 +40,99 @@ 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) +} From 2db79cb56d5697beacc6786a3805fc85512015b5 Mon Sep 17 00:00:00 2001 From: Maher Date: Sun, 20 Sep 2026 00:37:36 +0200 Subject: [PATCH 2/2] fix(privacy): make the tracking finding deterministic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch: `result.TrackingSDKs` is filled by ranging over a map, and Go randomises map iteration, so indexing `[0]` picked an arbitrary SDK. The cited file and line therefore changed between runs on any project using more than one tracking SDK — 3 distinct outputs over 40 scans of the same three-SDK fixture. Sort the SDK list, which also settles the `Found:` ordering that was already unstable before this branch, and cite the genuinely earliest hit by file then line so "first seen" means what it says. --- internal/privacy/scanner.go | 20 +++++++++++++- internal/privacy/scanner_test.go | 45 ++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/internal/privacy/scanner.go b/internal/privacy/scanner.go index eb6603f..5641212 100644 --- a/internal/privacy/scanner.go +++ b/internal/privacy/scanner.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "regexp" + "sort" "strings" ) @@ -260,6 +261,8 @@ 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, ", ") @@ -272,7 +275,7 @@ func Scan(projectPath string) (*ScanResult, error) { } // 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 := trackingSDKHits[result.TrackingSDKs[0]]; ok { + 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) + "." @@ -361,6 +364,21 @@ 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. diff --git a/internal/privacy/scanner_test.go b/internal/privacy/scanner_test.go index 0267410..1d0948b 100644 --- a/internal/privacy/scanner_test.go +++ b/internal/privacy/scanner_test.go @@ -1,8 +1,10 @@ package privacy import ( + "fmt" "os" "path/filepath" + "slices" "strings" "testing" ) @@ -136,3 +138,46 @@ func TestTrackingFindingCitesFileAndLine(t *testing.T) { } 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) + } + } +}