typescript-go icon indicating copy to clipboard operation
typescript-go copied to clipboard

Fix empty wildcard match on exports

Open lauckhart opened this issue 2 weeks ago • 2 comments

This fixes a bug whereby a wildcard package.json export such as:

./foo/*/index.js

when matched against an import such as:

import "./foo/index.js";

would crash:

panic: runtime error: slice bounds out of range [50:49]

goroutine 31853 [running]:
github.com/microsoft/typescript-go/internal/modulespecifiers.tryGetModuleNameFromExportsOrImports(0x140001a8908, {0x1096fdd18, 0x14017902078}, {0x1400021c100, 0x3a}, {0x14000298310, 0x23}, {0x14000010fc0, 0xc}, {{0x58?, ...}, ...}, ...)
        github.com/microsoft/typescript-go/internal/modulespecifiers/specifiers.go:1215 +0xfe8
github.com/microsoft/typescript-go/internal/modulespecifiers.tryGetModuleNameFromPackageJsonImports-range1(...)
        github.com/microsoft/typescript-go/internal/modulespecifiers/specifiers.go:1000
github.com/microsoft/typescript-go/internal/modulespecifiers.tryGetModuleNameFromPackageJsonImports.(*OrderedMap[...]).Entries.func1(...)
        github.com/microsoft/typescript-go/internal/collections/ordered_map.go:175
github.com/microsoft/typescript-go/internal/modulespecifiers.tryGetModuleNameFromPackageJsonImports({0x1400021c100, 0x3a}, {0x140002dafc0?, 0x23?}, 0x140001a8908, {0x1096fdd18, 0x14017902078}, 0x63, 0x0)
        github.com/microsoft/typescript-go/internal/modulespecifiers/specifiers.go:990 +0x3e8
github.com/microsoft/typescript-go/internal/modulespecifiers.getLocalModuleSpecifier({0x1400021c100, 0x3a}, {0x0, {0x140002dafc0, 0x3d}, {0x140002dafc0, 0x2f}}, 0x140001a8908, {0x1096fdd18, 0x14017902078}, ...)
        github.com/microsoft/typescript-go/internal/modulespecifiers/specifiers.go:496 +0x8c4
...

lauckhart avatar Dec 07 '25 16:12 lauckhart

@microsoft-github-policy-service agree

lauckhart avatar Dec 07 '25 16:12 lauckhart

Is there a test you can make for this?

The fix seems obviously right, however. The original code looked more like:

            case MatchingMode.Pattern:
                const starPos = pathOrPattern.indexOf("*");
                const leadingSlice = pathOrPattern.slice(0, starPos);
                const trailingSlice = pathOrPattern.slice(starPos + 1);
                if (canTryTsExtension && startsWith(targetFilePath, leadingSlice, ignoreCase) && endsWith(targetFilePath, trailingSlice, ignoreCase)) {
                    const starReplacement = targetFilePath.slice(leadingSlice.length, targetFilePath.length - trailingSlice.length);
                    return { moduleFileToTry: replaceFirstStar(packageName, starReplacement) };
                }

Which we could technically replicate?

jakebailey avatar Dec 08 '25 23:12 jakebailey

@jakebailey I did my best to add coverage in specifiers_test.go

Interestingly the original code looks like it may be subtly incorrect as well. It won't crash due to JS's more lenient slice() but it does have the false positive for an overlapping prefix/suffix which could (maybe?) result in a match against an unintended file.

lauckhart avatar Dec 09 '25 21:12 lauckhart

Interestingly the original code looks like it may be subtly incorrect as well. It won't crash due to JS's more lenient slice() but it does have the false positive for an overlapping prefix/suffix which could (maybe?) result in a match against an unintended file.

Hm, I assumed not, given it splits the string before checking?

jakebailey avatar Dec 09 '25 22:12 jakebailey

Hm, I assumed not, given it splits the string before checking?

The problem is the independent tests against targetFilePath:

startsWith(targetFilePath, leadingSlice, ignoreCase) && endsWith(targetFilePath, trailingSlice, ignoreCase)

in my example above this ends up as something like:

startsWith("foo/index.js", "foo/") && endsWith("foo/index.js", "/index.js")

So "foo/*/index.js" unintentionally matches "foo/index.js". Not sure if there's a non-contrived scenario where the output could cause problems though... In our case the output references a module name containing "//" so is innocuous.

lauckhart avatar Dec 09 '25 22:12 lauckhart