Skip to content

fix: Substring symbol names cross-tainting #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
martinnaj merged 1 commit into from
Aug 7, 2026
Merged

fix: Substring symbol names cross-tainting #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [0.25.4] - 2026-08-07

### Fixed
- Intra-file taint propagation now matches symbol names as whole identifiers instead of raw substrings. The dependency/usage checks used `strings.Contains(body, name)`, so a symbol whose name is a **substring** of another was falsely linked — e.g. removing the unused `chooseAction` tainted the surviving, used `chooseActionByIndex` (its body "contains" the string `chooseAction`), which then propagated to a spec and flagged `gdc-dashboards-e2e` for a dead-code deletion. All six propagation sites (`astdiff.go` intra-file graph, plus the seed/importer/usage propagation in `analyzer.go`) now require the name to appear flanked by non-identifier characters (`[A-Za-z0-9_$]`) via a shared `containsIdentifier` helper; non-identifier tokens like the `*` wildcard keep the substring behaviour. Strictly more precise (it only drops matches that were substrings inside a larger identifier — never a real usage), so no false negatives.

## [0.25.3] - 2026-08-07

### Fixed
Expand Down Expand Up @@ -408,6 +413,7 @@ Together these keep genuine import-time changes flagged while eliminating the la
- Multi-stage Docker build
- Automated vendor upgrade workflow

[0.25.4]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.25.3...v0.25.4
[0.25.3]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.25.2...v0.25.3
[0.25.2]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.25.1...v0.25.2
[0.25.1]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.25.0...v0.25.1
Expand Down
2 changes: 1 addition & 1 deletion VERSION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0.25.3
0.25.4
10 changes: 5 additions & 5 deletions internal/analyzer/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ func AnalyzeLibraryPackage(projectFolder string, entrypoints []Entrypoint, merge
}
bodyText := tsparse.ExtractTextForLines(sourceText, lineMap, sym.StartLine, sym.EndLine)
for tName := range names {
if strings.Contains(bodyText, tName) {
if containsIdentifier(bodyText, tName) {
names[sym.Name] = true
changed = true
log.Debugf(" %s: %s tainted via intra-file dep on %s (seed propagation)", stem, sym.Name, tName)
Expand Down Expand Up @@ -834,7 +834,7 @@ func AnalyzeLibraryPackage(projectFolder string, entrypoints []Entrypoint, merge
}
bodyText := tsparse.ExtractTextForLines(sourceText, lineMap, sym.StartLine, sym.EndLine)
for tName := range taintedSet {
if strings.Contains(bodyText, tName) {
if containsIdentifier(bodyText, tName) {
taintedSet[sym.Name] = true
newlyTainted = append(newlyTainted, sym.Name)
changed = true
Expand Down Expand Up @@ -1021,7 +1021,7 @@ func findTaintedSymbolsByUsage(analysis *tsparse.FileAnalysis, taintedNames []st
for _, sym := range analysis.Symbols {
bodyText := tsparse.ExtractTextForLines(sourceText, lineMap, sym.StartLine, sym.EndLine)
for tName := range taintSet {
if strings.Contains(bodyText, tName) {
if containsIdentifier(bodyText, tName) {
result = append(result, sym.Name)
break
}
Expand Down Expand Up @@ -1558,7 +1558,7 @@ func FindAffectedFiles(globPattern string, filterPattern string, upstreamTaint m
}
bodyText := tsparse.ExtractTextForLines(sourceText, lineMap, sym.StartLine, sym.EndLine)
for tName := range names {
if strings.Contains(bodyText, tName) {
if containsIdentifier(bodyText, tName) {
names[sym.Name] = true
changed = true
log.Debugf(" %s: %s tainted via intra-file dep on %s (seed propagation)", stem, sym.Name, tName)
Expand Down Expand Up @@ -1673,7 +1673,7 @@ func FindAffectedFiles(globPattern string, filterPattern string, upstreamTaint m
}
bodyText := tsparse.ExtractTextForLines(sourceText, lineMap, sym.StartLine, sym.EndLine)
for tName := range taintedSet {
if strings.Contains(bodyText, tName) {
if containsIdentifier(bodyText, tName) {
taintedSet[sym.Name] = true
newlyTainted = append(newlyTainted, sym.Name)
changed = true
Expand Down
47 changes: 46 additions & 1 deletion internal/analyzer/astdiff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ func findAffectedSymbolsByASTDiff(oldAnalysis *tsparse.FileAnalysis, newAnalysis
bodyText := tsparse.ExtractTextForLines(newText, newLineMap, sym.StartLine, sym.EndLine)
deps := make(map[string]bool)
for _, other := range newAnalysis.Symbols {
if other.Name != sym.Name && strings.Contains(bodyText, other.Name) {
if other.Name != sym.Name && containsIdentifier(bodyText, other.Name) {
deps[other.Name] = true
}
}
Expand Down Expand Up @@ -564,6 +564,51 @@ func repointedImportBindings(oldA, newA *tsparse.FileAnalysis, includeTypes bool
return repointed
}

// containsIdentifier reports whether name occurs in text as a whole identifier
// token — flanked by non-identifier characters — rather than as a substring of a
// larger identifier. This prevents false taint links like `chooseAction` matching
// inside `chooseActionByIndex`. Names that are not plain identifiers (e.g. the "*"
// wildcard or "*:ns" namespace markers) fall back to a raw substring test.
func containsIdentifier(text, name string) bool {
if name == "" {
return false
}
if !isPlainIdentifier(name) {
return strings.Contains(text, name)
}
for from := 0; ; {
i := strings.Index(text[from:], name)
if i < 0 {
return false
}
start := from + i
end := start + len(name)
beforeOK := start == 0 || !isIdentByte(text[start-1])
afterOK := end == len(text) || !isIdentByte(text[end])
Comment on lines +576 to +587

@coderabbitai coderabbitai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n 'containsIdentifier|isPlainIdentifier|isIdentByte' internal/analyzer --glob '*.go'
rg -nP '\\u[0-9A-Fa-f]{4}|[^\x00-\x7F]' internal/analyzer --glob '*_test.go' || true

Repository: gooddata/gooddata-goodchanges

Length of output: 1317

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- target implementation ---'
sed -n '130,185p;540,620p' internal/analyzer/astdiff.go

printf '%s\n' '--- parser and identifier-related code ---'
rg -n 'Parse|parser|Identifier|identifier|bodyText|containsIdentifier' internal/analyzer go.mod go.sum --glob '*.go' --glob 'go.mod' --glob 'go.sum' | head -240

printf '%s\n' '--- relevant tests ---'
fd -i '.*_test\.go$' internal/analyzer -x sh -c 'rg -n -C 3 "containsIdentifier|unicode|escaped|identifier|Rename|rename" "$1"' sh {} || true

Repository: gooddata/gooddata-goodchanges

Length of output: 7240

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- repository files and module dependencies ---'
git ls-files | rg '(^|/)(tsparse|parser|.*_test\.go$)|go\.mod$|go\.sum$' | head -240
sed -n '1,180p' go.mod

printf '%s\n' '--- symbol extraction and identifier handling ---'
sed -n '220,300p' internal/analyzer/astdiff.go
rg -n -C 4 'type Symbol|struct .*Symbol|Symbols =|Name:' --glob '*.go' .

printf '%s\n' '--- parser package API references ---'
rg -n -C 3 'package tsparse|func .*Parse|func .*Extract|IsIdentifier|IdentifierName|Unicode|escaped' --glob '*.go' .

Repository: gooddata/gooddata-goodchanges

Length of output: 25678

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- tsparse analysis construction ---'
sed -n '1,115p;300,435p' internal/tsparse/tsparse.go

printf '%s\n' '--- vendor scanner and source-file APIs ---'
fd -i '.*\.go$' _vendor/typescript-go | head -80
rg -n -C 3 'func .*Scan|type Scanner|Scanner|Token.*Identifier|Identifier.*Text|Unescape|UnicodeEscape|Text\(\)' _vendor/typescript-go --glob '*.go' | head -320

printf '%s\n' '--- analyzer tests and test helpers ---'
git ls-files internal/analyzer | sort
rg -n -C 5 'FindAffected|affected|repointed|intra-file|taint|astdiff' internal/analyzer --glob '*_test.go' || true

Repository: gooddata/gooddata-goodchanges

Length of output: 7547

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- module layout and submodules ---'
git submodule status || true
git ls-tree HEAD _vendor/typescript-go
cat .gitmodules 2>/dev/null || true
find . -maxdepth 3 -type d -name '*typescript*' -o -name '_vendor'

printf '%s\n' '--- scanner usage in tracked source ---'
rg -n -C 5 'scanner\.' internal --glob '*.go'
sed -n '1,110p' internal/tsparse/tsparse.go
sed -n '400,440p' internal/tsparse/tsparse.go

printf '%s\n' '--- tests in the analyzer package ---'
git ls-files internal/analyzer | sort
git ls-files '*_test.go' | wc -l

Repository: gooddata/gooddata-goodchanges

Length of output: 8719

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
def is_ident_byte(b):
    return b in (ord('_'), ord('$')) or 65 <= b <= 90 or 97 <= b <= 122 or 48 <= b <= 57

def is_plain_identifier(s):
    return bool(s) and all(is_ident_byte(b) for b in s.encode())

def contains_identifier(text, name):
    if not name:
        return False
    if not is_plain_identifier(name):
        return name in text
    start = 0
    while True:
        i = text.find(name, start)
        if i < 0:
            return False
        end = i + len(name)
        before_ok = i == 0 or not is_ident_byte(text.encode()[i - 1])
        after_ok = end == len(text) or not is_ident_byte(text.encode()[end])
        if before_ok and after_ok:
            return True
        start = i + 1

cases = [
    ("const fooé = 1", "foo"),
    (r"const foo\u0062ar = 1", "foo"),
    ("const é = 1", "é"),
    ("const éx = 1", "é"),
]
for text, name in cases:
    print(repr(text), repr(name), contains_identifier(text, name))
PY

printf '%s\n' '--- exact call sites and absence of tests ---'
rg -n -C 3 'containsIdentifier\(' internal/analyzer --glob '*.go'
git ls-files '*_test.go' | sort

Repository: gooddata/gooddata-goodchanges

Length of output: 4155

Use parser-aware identifier boundaries.

containsIdentifier can match part of a Unicode or escaped identifier, such as foo in foo\u0062ar or fooé. Non-ASCII names use raw substring matching and can match inside éx. This creates false taint links. Add regression tests and use TypeScript tokenization or an equivalent grammar-aware scanner.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/analyzer/astdiff.go` around lines 576 - 587, Update
containsIdentifier in internal/analyzer/astdiff.go to use TypeScript
tokenization or an equivalent grammar-aware scanner for identifier matching,
preventing matches inside Unicode or escaped identifiers such as foo\u0062ar,
fooé, or éx. Preserve plain substring behavior for non-identifiers only, and add
regression tests covering these boundary cases.

if beforeOK && afterOK {
return true
}
from = start + 1
}
}

func isPlainIdentifier(s string) bool {
if s == "" {
return false
}
for i := 0; i < len(s); i++ {
if !isIdentByte(s[i]) {
return false
}
}
return true
}

func isIdentByte(b byte) bool {
return b == '_' || b == '$' ||
(b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || (b >= '0' && b <= '9')
}

// bareImportSources returns the set of module specifiers imported purely for
// their side effects (`import "./x"` — no bindings). These execute at import time.
func bareImportSources(a *tsparse.FileAnalysis) map[string]bool {
Expand Down
Loading