Skip to content

Commit 5a84e6f

Browse files
authored
Rollup of security fixes (#1285)
1 parent d01ce4a commit 5a84e6f

221 files changed

Lines changed: 1199 additions & 374 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.changeset/social-cows-yell.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@definitelytyped/header-parser": patch
3+
---
4+
5+
Reject non-ranges in package dependencies

packages/dtslint/src/lint.ts

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -112,23 +112,35 @@ function getEslintOptions(
112112
],
113113
};
114114

115+
// Always disable cascading .eslintrc.* discovery. Contributor-authored config files in
116+
// types/<pkg>/ would otherwise be loaded by ESLint 8's legacy eslintrc engine, which
117+
// resolves `extends` and `parser` (including in `overrides[]`) via
118+
// createRequire(configFilePath).resolve(value) and require()s the result. Since dtslint has
119+
// no file-extension allowlist, a contributor could ship a `.cjs` payload alongside
120+
// `.eslintrc.json` and obtain arbitrary code execution in the lint process.
121+
const baseOverrideConfig = {
122+
plugins: ["@definitelytyped", "@typescript-eslint", "jsdoc"],
123+
parser: "@typescript-eslint/parser",
124+
parserOptions: {
125+
project: true,
126+
warnOnUnsupportedTypeScriptVersion: false,
127+
},
128+
...overrideConfig,
129+
};
130+
115131
if (expectOnly) {
116132
return {
117133
useEslintrc: false,
118-
overrideConfig: {
119-
plugins: ["@definitelytyped", "@typescript-eslint", "jsdoc"],
120-
parser: "@typescript-eslint/parser",
121-
parserOptions: {
122-
project: true,
123-
warnOnUnsupportedTypeScriptVersion: false,
124-
},
125-
...overrideConfig,
126-
},
134+
overrideConfig: baseOverrideConfig,
127135
};
128136
}
129137

130138
return {
131-
overrideConfig,
139+
useEslintrc: false,
140+
overrideConfig: {
141+
...baseOverrideConfig,
142+
extends: ["plugin:@definitelytyped/all"],
143+
},
132144
};
133145
}
134146

packages/header-parser/src/index.ts

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,13 @@ export function validatePackageJson(
9898
`${typesDirectoryName}'s package.json has bad "devDependencies": must include \`"@types/${typesDirectoryName}": "workspace:."\``,
9999
);
100100
}
101+
// dependency version ranges
102+
for (const depsKey of ["dependencies", "peerDependencies", "devDependencies"] as const) {
103+
const deps = packageJson[depsKey];
104+
if (deps && typeof deps === "object" && !Array.isArray(deps)) {
105+
errors.push(...checkDependencyVersions(typesDirectoryName, depsKey, deps as Record<string, unknown>));
106+
}
107+
}
101108
// typesVersions
102109
if (needsTypesVersions) {
103110
assert.strictEqual(
@@ -475,10 +482,6 @@ export function checkPackageJsonDependencies(
475482
Please make a pull request to microsoft/DefinitelyTyped-tools adding it to \`packages/definitions-parser/allowedPackageJsonDependencies.txt\`.`;
476483
errors.push(`In ${path}: ${msg}`);
477484
}
478-
const version = (dependencies as { [key: string]: unknown })[dependencyName];
479-
if (typeof version !== "string") {
480-
errors.push(`In ${path}: Dependency version for ${dependencyName} should be a string.`);
481-
}
482485
}
483486
if (devDependencySelfName) {
484487
const selfDependency = (dependencies as { [key: string]: string | undefined })[devDependencySelfName];
@@ -492,3 +495,36 @@ Please make a pull request to microsoft/DefinitelyTyped-tools adding it to \`pac
492495
}
493496
return errors;
494497
}
498+
499+
function checkDependencyVersions(
500+
typesDirectoryName: string,
501+
depsKey: "dependencies" | "peerDependencies" | "devDependencies",
502+
dependencies: Record<string, unknown>,
503+
): string[] {
504+
const errors: string[] = [];
505+
for (const dependencyName of Object.keys(dependencies)) {
506+
const version = dependencies[dependencyName];
507+
if (typeof version !== "string") {
508+
errors.push(
509+
`${typesDirectoryName}'s package.json has bad "${depsKey}": version for ${dependencyName} should be a string.`,
510+
);
511+
} else if (version !== "workspace:." && !isValidRegistrySpec(version)) {
512+
errors.push(
513+
`${typesDirectoryName}'s package.json has bad "${depsKey}": version for ${dependencyName} (${JSON.stringify(
514+
version,
515+
)}) must be a valid semver range, dist-tag, or "workspace:.".`,
516+
);
517+
}
518+
}
519+
return errors;
520+
}
521+
522+
// A registry dependency spec must be a valid semver range/version, or a dist-tag matching
523+
// this strict allowlist.
524+
const distTagRegex = /^[A-Za-z][A-Za-z0-9_-]*$/;
525+
function isValidRegistrySpec(spec: string): boolean {
526+
const trimmed = spec.trim();
527+
if (trimmed === "") return false;
528+
if (semver.validRange(trimmed) !== null) return true;
529+
return distTagRegex.test(trimmed);
530+
}

packages/header-parser/test/index.test.ts

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,75 @@ describe("validatePackageJson", () => {
9999
it("works with old-version packages", () => {
100100
expect(Array.isArray(validatePackageJson("hapi", { ...pkgJson, version: "16.6.9999" }, []))).toBeFalsy();
101101
});
102+
it("requires dependency versions to be valid semver ranges, dist-tags, or 'workspace:.'", () => {
103+
expect(
104+
validatePackageJson(
105+
"hapi",
106+
{ ...pkgJson, dependencies: { ...(pkgJson.dependencies as object), joi: "not a range" } },
107+
[],
108+
),
109+
).toEqual([
110+
`hapi's package.json has bad "dependencies": version for joi ("not a range") must be a valid semver range, dist-tag, or "workspace:.".`,
111+
]);
112+
});
113+
it.each([
114+
["file:./local.tgz"],
115+
["./local.tgz"],
116+
["local.tgz"],
117+
["foo.tar.gz"],
118+
["git+https://example.com/x.git"],
119+
["git+ssh://[email protected]:x/y.git"],
120+
["[email protected]:x/y.git"],
121+
["https://example.com/x.tgz"],
122+
["http://example.com/x.tgz"],
123+
["user/repo"],
124+
["user/repo#branch"],
125+
["npm:other@^1"],
126+
["~/local"],
127+
["../local"],
128+
])("rejects non-registry dependency spec %p", (bad) => {
129+
const result = validatePackageJson(
130+
"hapi",
131+
{ ...pkgJson, dependencies: { ...(pkgJson.dependencies as object), joi: bad } },
132+
[],
133+
);
134+
expect(Array.isArray(result)).toBe(true);
135+
expect(result as string[]).toContainEqual(
136+
`hapi's package.json has bad "dependencies": version for joi (${JSON.stringify(
137+
bad,
138+
)}) must be a valid semver range, dist-tag, or "workspace:.".`,
139+
);
140+
});
141+
it.each([["latest"], ["next"], ["beta"], ["rc"], ["canary"], ["experimental"], ["nightly"]])(
142+
"allows dist-tag %p as a dependency version",
143+
(tag) => {
144+
expect(
145+
Array.isArray(
146+
validatePackageJson(
147+
"hapi",
148+
{ ...pkgJson, dependencies: { ...(pkgJson.dependencies as object), joi: tag } },
149+
[],
150+
),
151+
),
152+
).toBeFalsy();
153+
},
154+
);
155+
it("allows 'workspace:.' as a dependency version", () => {
156+
expect(
157+
Array.isArray(
158+
validatePackageJson(
159+
"hapi",
160+
{ ...pkgJson, dependencies: { ...(pkgJson.dependencies as object), joi: "workspace:." } },
161+
[],
162+
),
163+
),
164+
).toBeFalsy();
165+
});
166+
it("requires dependency versions to be strings", () => {
167+
expect(validatePackageJson("hapi", { ...pkgJson, peerDependencies: { foo: 5 } }, [])).toEqual([
168+
`hapi's package.json has bad "peerDependencies": version for foo should be a string.`,
169+
]);
170+
});
102171
});
103172

104173
describe("makeTypesVersionsForPackageJson", () => {

packages/mergebot/src/_tests/discussions.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,4 +25,18 @@ describe(extractNPMReference, () => {
2525
test.concurrent.each(eventActions)("(%s, %s) is %s", async (title, result) => {
2626
expect(extractNPMReference({ title })).toEqual(result);
2727
});
28+
29+
const invalid = [
30+
"[Pkg: foo] inject", // space disallowed
31+
"[node @attacker] hi", // space + invalid char
32+
"[FOO] uppercase not allowed in npm names",
33+
"[../etc/passwd] traversal",
34+
"[]", // empty
35+
"[ leading-space]",
36+
"[trailing-space ]",
37+
"[has\nnewline]",
38+
];
39+
test.concurrent.each(invalid)("rejects invalid title %p", async (title) => {
40+
expect(extractNPMReference({ title })).toBeUndefined();
41+
});
2842
});

packages/mergebot/src/_tests/fixtures/38979/_response.json

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,9 @@
3838
"state": "OPEN",
3939
"headRefOid": "222334139e52fc16369464cfb5dc95c82f71192f",
4040
"changedFiles": 72,
41-
"commitIds": {},
41+
"commitIds": {
42+
"totalCount": 0
43+
},
4244
"timelineItems": {
4345
"nodes": [
4446
{
@@ -835,7 +837,8 @@
835837
"__typename": "PullRequestReview"
836838
}
837839
],
838-
"__typename": "PullRequestReviewConnection"
840+
"__typename": "PullRequestReviewConnection",
841+
"totalCount": 21
839842
},
840843
"commits": {
841844
"totalCount": 24,
@@ -2485,7 +2488,8 @@
24852488
}
24862489
],
24872490
"__typename": "ProjectV2ItemConnection"
2488-
}
2491+
},
2492+
"baseRefOid": "master"
24892493
},
24902494
"__typename": "Repository"
24912495
}

packages/mergebot/src/_tests/fixtures/38979/derived.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
"isFirstContribution": false,
1212
"tooManyFiles": false,
1313
"hugeChange": false,
14+
"tooManyCommits": false,
15+
"tooManyReviews": false,
1416
"popularityLevel": "Critical",
1517
"pkgInfo": [
1618
{

packages/mergebot/src/_tests/fixtures/43136/_response.json

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,9 @@
3737
"state": "OPEN",
3838
"headRefOid": "e6863537248bbfee8f0ef8c636bb00c25cf40b96",
3939
"changedFiles": 2,
40-
"commitIds": {},
40+
"commitIds": {
41+
"totalCount": 0
42+
},
4143
"timelineItems": {
4244
"nodes": [
4345
{
@@ -129,7 +131,8 @@
129131
}
130132
}
131133
],
132-
"__typename": "PullRequestReviewConnection"
134+
"__typename": "PullRequestReviewConnection",
135+
"totalCount": 2
133136
},
134137
"commits": {
135138
"totalCount": 2,
@@ -326,7 +329,8 @@
326329
}
327330
],
328331
"__typename": "ProjectV2ItemConnection"
329-
}
332+
},
333+
"baseRefOid": "master"
330334
},
331335
"__typename": "Repository"
332336
}

packages/mergebot/src/_tests/fixtures/43136/derived.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
"isFirstContribution": true,
1212
"tooManyFiles": false,
1313
"hugeChange": false,
14+
"tooManyCommits": false,
15+
"tooManyReviews": false,
1416
"popularityLevel": "Critical",
1517
"pkgInfo": [
1618
{

packages/mergebot/src/_tests/fixtures/43144/_response.json

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,9 @@
3333
"state": "OPEN",
3434
"headRefOid": "f1f5c4bb0ae553f56766882f6458d2e22baa87c7",
3535
"changedFiles": 2,
36-
"commitIds": {},
36+
"commitIds": {
37+
"totalCount": 0
38+
},
3739
"timelineItems": {
3840
"nodes": [
3941
{
@@ -96,7 +98,8 @@
9698
}
9799
}
98100
],
99-
"__typename": "PullRequestReviewConnection"
101+
"__typename": "PullRequestReviewConnection",
102+
"totalCount": 1
100103
},
101104
"commits": {
102105
"totalCount": 3,
@@ -267,7 +270,8 @@
267270
}
268271
],
269272
"__typename": "ProjectV2ItemConnection"
270-
}
273+
},
274+
"baseRefOid": "master"
271275
},
272276
"__typename": "Repository"
273277
}

0 commit comments

Comments
 (0)