fix(installsource): reject npm and GitHub source names that resolve as paths, tags or tarballs

Package segments made only of dots, leading dots, a leading underscore on
unscoped names and .tgz/.tar/.tar.gz names are no longer accepted by the
installsource and registry checks. parseGitHubRepoSource refuses dot-leading
owners and dot-only repos such as the one TrimSuffix(".git") leaves from
"...git".

Refs #11353

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
This commit is contained in:
drakeo338andClaude Sonnet 5.5 committed 2026-10-06 15:07:01 +00:00
1 parent 8c196cc72e
commit 79cf8d34a1
10 files changed
+99 -13

No files matched your search

@@ -1211,6 +1211,7 @@ func TestParseGitHubRepoSourceAcceptsCanonicalRepositoryPaths(t *testing.T) {
}{
{"https://github.com/o/r", githubRepoSource{Owner: "o", Repo: "r"}},
{"https://github.com/o/r.git/", githubRepoSource{Owner: "o", Repo: "r"}},
{"https://github.com/o/.github", githubRepoSource{Owner: "o", Repo: ".github"}},
{"https://github.com/o/r/tree/main", githubRepoSource{Owner: "o", Repo: "r", Branch: "main"}},
{"https://github.com/o/r/tree/main/plugins/demo", githubRepoSource{Owner: "o", Repo: "r", Branch: "main", Path: "plugins/demo"}},
}
@@ -1226,6 +1227,11 @@ func TestParseGitHubRepoSourceAcceptsCanonicalRepositoryPaths(t *testing.T) {
func TestParseGitHubRepoSourceRejectsPagesAndUnsafePaths(t *testing.T) {
for _, source := range []string{
"https://github.com/o/...git",
"https://github.com/o/..git",
"https://github.com/o/.git",
"https://github.com/./r",
"https://github.com/.o/r",
"https://github.com/o/r/issues/1",
"https://github.com/o/r/blob/main/reasonix-plugin.json",
"https://github.com/o/r/pull/1",
+24 -5
View File
@@ -11,11 +11,30 @@ import (
"reasonix/internal/contract/config"
)
// packageNameRe matches valid npm package-name segments. Pinned by [a-z0-9._-]
// — exactly what npm allows. The leading character may be a digit (scoped
// packages like @5/test are rare but valid).
// packageNameRe is a permissive character filter for npm name segments. It is
// not npm's full rule set (it admits uppercase); the stricter checks that keep
// a name from resolving as a path, tag or tarball live in isPackageSegment and
// isPackageName. The leading character may be a digit (@5/test is valid).
var packageNameRe = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`)
// isPackageSegment reports whether s is an acceptable npm name segment. npm
// treats a name starting with "." as a local path, so such segments (including
// "." and "..") are rejected.
func isPackageSegment(s string) bool {
return packageNameRe.MatchString(s) && !strings.HasPrefix(s, ".")
}
// isPackageName reports whether s is an unscoped package name. Beyond the
// segment rules, npm reads a leading "_" as a tag and a .tgz/.tar/.tar.gz
// suffix as a local tarball, so those are rejected.
func isPackageName(s string) bool {
if !isPackageSegment(s) || strings.HasPrefix(s, "_") {
return false
}
l := strings.ToLower(s)
return !strings.HasSuffix(l, ".tgz") && !strings.HasSuffix(l, ".tar") && !strings.HasSuffix(l, ".tar.gz")
}
func isURL(s string) bool {
u, err := url.Parse(s)
return err == nil && (u.Scheme == "http" || u.Scheme == "https") && u.Host != ""
@@ -88,9 +107,9 @@ func SplitPackageSpec(s string) (name, version string, ok bool) {
}
if strings.HasPrefix(name, "@") {
parts := strings.Split(name, "/")
ok = len(parts) == 2 && packageNameRe.MatchString(parts[0][1:]) && packageNameRe.MatchString(parts[1])
ok = len(parts) == 2 && isPackageSegment(parts[0][1:]) && isPackageSegment(parts[1])
} else {
ok = packageNameRe.MatchString(name)
ok = isPackageName(name)
}
return name, version, ok
}
+28 -1
View File
@@ -1,6 +1,10 @@
package installsource
import "testing"
import (
"encoding/json"
"os"
"testing"
)
func TestLooksLikePackageVersionPin(t *testing.T) {
accept := []string{
@@ -116,3 +120,26 @@ func TestPackageMCPActionPinnedVersion(t *testing.T) {
}
}
}
// package_specs.json is shared with the registry publish validation tests in
// workers/crash-report so the Go and TS sides cannot drift.
func TestLooksLikePackageSharedCases(t *testing.T) {
raw, err := os.ReadFile("testdata/package_specs.json")
if err != nil {
t.Fatal(err)
}
var cases struct{ Accept, Reject []string }
if err := json.Unmarshal(raw, &cases); err != nil {
t.Fatal(err)
}
for _, s := range cases.Reject {
if LooksLikePackage(s) {
t.Errorf("LooksLikePackage(%q) = true, want false", s)
}
}
for _, s := range cases.Accept {
if !LooksLikePackage(s) {
t.Errorf("LooksLikePackage(%q) = false, want true", s)
}
}
}
+4 -2
View File
@@ -178,11 +178,13 @@ func parseGitHubRepoSource(source string) (githubRepoSource, bool) {
}
parts = append(parts, part)
}
if len(parts) < 2 || !packageNameRe.MatchString(parts[0]) {
if len(parts) < 2 || !isPackageSegment(parts[0]) {
return githubRepoSource{}, false
}
repo := strings.TrimSuffix(parts[1], ".git")
if !packageNameRe.MatchString(repo) {
// Not isPackageSegment: GitHub allows repos with a leading dot (".github"),
// so only the dot-only names ("." and "..", e.g. from "...git") are refused.
if !packageNameRe.MatchString(repo) || strings.Trim(repo, ".") == "" {
return githubRepoSource{}, false
}
out := githubRepoSource{Owner: parts[0], Repo: repo}
@@ -0,0 +1,9 @@
{
"accept": ["a.b", "@a/b.c", "@a.b/c", "@a/b..", "@a/b..c", "@_a/b", "@a/_b", "a.tgz.b", "@a/x.tgz", "a-tar"],
"reject": [
"@a/..", "@../b", "@./b", "@a/.", "@../..", "@a/[email protected]",
"@a/.b", "@a/..b", "@.a/b",
"a.tgz", "A.TGZ", "a.tar", "a.TAR", "a.tar.gz", "a.Tar.Gz", "[email protected]",
"_a"
]
}
+1 -1
View File
@@ -40,7 +40,7 @@ var (
var slugSegment = regexp.MustCompile(`^[A-Za-z0-9._-]{1,64}$`)
func slugPart(s string) bool { return slugSegment.MatchString(s) && s != "." && s != ".." }
func slugPart(s string) bool { return slugSegment.MatchString(s) && !strings.HasPrefix(s, ".") }
// Package is one listed capability as the registry describes it. Every field is
// the publisher's claim; none of it decides what gets installed.
+1 -1
View File
@@ -148,7 +148,7 @@ func TestClientSortsFailuresByCause(t *testing.T) {
}
func TestSplitSlugRefusesPathTricks(t *testing.T) {
for _, s := range []string{"", "a", "a/b/c", "../b", "a/..", "a/b?x=1", "a/%2e%2e", `a\b/c`} {
for _, s := range []string{"", "a", "a/b/c", "../b", "a/..", ".b/c", "a/.b", "a/..b", "a/b?x=1", "a/%2e%2e", `a\b/c`} {
if _, _, err := SplitSlug(s); !errors.Is(err, ErrBadSlug) {
t.Errorf("SplitSlug(%q) accepted", s)
}
+1 -1
View File
@@ -273,7 +273,7 @@ func TestNPMPackageAcceptsOnlyPackageNames(t *testing.T) {
"C:x": false, "@scope/../x": false, "": false, strings.Repeat("a", 65): false,
"@playwright/[email protected]": true, "[email protected]": true, "[email protected]": true,
"name@latest": false, "name@^1.2.3": false, "name@": false, "a@b@c": false,
"-y": false, "--help": false, "[email protected]": false, "@a/..": false, "@a/[email protected]": false,
"-y": false, "--help": false, "[email protected]": false, "@a/..": false, "@a/[email protected]": false, "@a/.b": false, "x.tgz": false, "_a": false, "a.b": true,
} {
if npmPackage(s) != want {
t.Errorf("npmPackage(%q) = %v", s, !want)
@@ -1,3 +1,4 @@
import { readFileSync } from "node:fs";
import { describe, expect, it } from "vitest";
import { ListQuerySchema, PublishSchema } from "./validation";
@@ -76,6 +77,19 @@ describe("PublishSchema source", () => {
}
});
// Shared with internal/ext/installsource/names_test.go so both sides agree.
it("matches the shared package spec cases", () => {
const cases = JSON.parse(
readFileSync(new URL("../../../../../internal/ext/installsource/testdata/package_specs.json", import.meta.url), "utf8"),
) as { accept: string[]; reject: string[] };
for (const source of cases.reject) {
expect(parse({ kind: "mcp", source }).success, source).toBe(false);
}
for (const source of cases.accept) {
expect(parse({ kind: "mcp", source }).success, source).toBe(true);
}
});
it("still refuses a pinned package for kind=skill", () => {
expect(parse({ kind: "skill", source: "@scope/[email protected]" }).success).toBe(false);
});
@@ -20,6 +20,15 @@ const httpUrl = z.string().trim().url().max(500);
// (isURL || git: shorthand || looksLikePackage); a bare local path is refused
// because it resolves on the publisher's machine, never the installer's.
const pkgSegment = /^[a-zA-Z0-9._-]+$/;
// Permissive character filter, not npm's full rule set (it admits uppercase).
// isPackageSegment/isPackageName add the checks that keep a name from resolving
// as a path, tag or tarball. Keep in sync with internal/ext/installsource/names.go.
function isPackageSegment(segment: string): boolean {
return pkgSegment.test(segment) && !segment.startsWith(".");
}
function isPackageName(name: string): boolean {
return isPackageSegment(name) && !name.startsWith("_") && !/\.(tgz|tar|tar\.gz)$/i.test(name);
}
const unsafeSourceCharacter = /[\s\u0000-\u001f\u007f-\u009f]/u;
function hasUnsafeSourceCharacters(source: string): boolean {
@@ -43,9 +52,9 @@ function looksLikePackage(source: string): boolean {
}
if (name.startsWith("@")) {
const parts = name.split("/");
return parts.length === 2 && pkgSegment.test(parts[0].slice(1)) && pkgSegment.test(parts[1]);
return parts.length === 2 && isPackageSegment(parts[0].slice(1)) && isPackageSegment(parts[1]);
}
return pkgSegment.test(name);
return isPackageName(name);
}
function isHttpUrl(source: string): boolean {