Skip to content
Open
Changes from all commits
Commits
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
55 changes: 53 additions & 2 deletions internal/detectors/guards_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,35 @@ func TestNoDirectSPDXExpressionUse(t *testing.T) {
}
}

// guardFiles are the files whose job is to forbid a module, so they are the
// files that have to spell it. Exempting them is not a loophole -- naming a
// module in a rule that bans it is the opposite of reaching for it.
//
// An explicit set of canonical paths, for two reasons learned the hard way.
// Matching a name instead exempted every guards_test.go under internal/, so a
// second guard file anywhere could name a forbidden module unnoticed. And
// exempting only this one file made the guards flag each other the moment a
// second one existed: internal/output grew its own presentation-layer guard,
// which must name packageurl-go to forbid it, and this rule reported it.
//
// Adding a guard therefore costs one line here, on purpose. That is the point:
// a new exemption should be a deliberate edit somebody reviews, not a pattern
// that silently widens.
var guardFiles = map[string]struct{}{
filepath.Clean(filepath.Join(internalRoot, "detectors", "guards_test.go")): {},
filepath.Clean(filepath.Join(internalRoot, "output", "registry_lookup_guard_test.go")): {},
}

// isGuardFile reports whether a path is one of the guard files above.
//
// By path. Exempting anything named guards_test.go was the earlier bug: it
// meant a second guard file in any package could name a forbidden module and
// no rule would report it.
func isGuardFile(path string) bool {
_, ok := guardFiles[filepath.Clean(path)]
return ok
}

// filesNamingModule returns every Go file under internal/ -- test files
// included -- whose text names the module path. Tests count because a test
// reaching a library directly proves the hazard is still reachable, and a test
Expand All @@ -127,13 +156,15 @@ func TestNoDirectSPDXExpressionUse(t *testing.T) {
// the right direction to fail.
func filesNamingModule(t *testing.T, module string) []string {
t.Helper()
self := filepath.Clean(filepath.Join(internalRoot, "detectors", "guards_test.go"))
var offenders []string
err := filepath.Walk(internalRoot, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() || !strings.HasSuffix(path, ".go") || filepath.Clean(path) == self {
if info.IsDir() || !strings.HasSuffix(path, ".go") {
return nil
}
if isGuardFile(path) {
return nil
}
Comment on lines +167 to 169

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Scope each guard-file exemption to its forbidden module

The unconditional isGuardFile return exempts each listed file from every filesNamingModule check, not just from the module its own guard must name. For example, registry_lookup_guard_test.go legitimately names the official package-URL module, but it can now directly import github.com/github/go-spdx or the deprecated Anchore package-URL module without either repository guard reporting it, despite the rule covering all files under internal/, including tests. Track exemptions by both path and module so each guard only suppresses its intentional self-reference.

AGENTS.md reference: AGENTS.md:L122-L123

Useful? React with 👍 / 👎.

body, err := os.ReadFile(path)
Expand Down Expand Up @@ -279,3 +310,23 @@ func TestDetectionResultsCarryingGraphsAreAttributed(t *testing.T) {
"wrap the result in detectors.Attributed: %v", offenders)
}
}

// The exemption is a set of paths, and a file is not exempt for being named
// like a guard. Both halves have been wrong here before: matching the basename
// hid a forbidden import in a second guards_test.go, and exempting only one
// file made two guards report each other.
func TestGuardExemptionIsByPathNotByName(t *testing.T) {
for path := range guardFiles {
if _, err := os.Stat(path); err != nil {
t.Errorf("guard file %q does not exist; a dead exemption is a rule nobody is applying", path)
}
if !isGuardFile(path) {
t.Errorf("guard file %q is not recognized by its own predicate", path)
}
}
// A file named like a guard, in a package that has none, must not be
// exempt -- whether or not it exists today.
if impostor := filepath.Join(internalRoot, "sbom", "guards_test.go"); isGuardFile(impostor) {
t.Errorf("%q is exempt for being named guards_test.go rather than for being a guard", impostor)
}
}
Loading