Validate package names for discussion-trigger

Author: jakebailey

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

@@ -25,4 +25,18 @@ describe(extractNPMReference, () => {
   test.concurrent.each(eventActions)("(%s, %s) is %s", async (title, result) => {
     expect(extractNPMReference({ title })).toEqual(result);
   });
+
+  const invalid = [
+    "[Pkg: foo] inject", // space disallowed
+    "[node @attacker] hi", // space + invalid char
+    "[FOO] uppercase not allowed in npm names",
+    "[../etc/passwd] traversal",
+    "[]", // empty
+    "[ leading-space]",
+    "[trailing-space ]",
+    "[has\nnewline]",
+  ];
+  test.concurrent.each(invalid)("rejects invalid title %p", async (title) => {
+    expect(extractNPMReference({ title })).toBeUndefined();
+  });
 });

packages/mergebot/src/discussions.ts

@@ -1,11 +1,18 @@
 export const canHandleRequest = (event: string, action: string) =>
   event === "discussion" && (action === "created" || action === "edited");
 
+// npm package name grammar (post-2017): optional `@scope/`, lowercase, limited punctuation.
+// Used to validate the substring extracted from an untrusted discussion title before it is fed
+// into bot comments or used to create repository labels via GraphQL.
+const npmPackageNameRegex = /^(?:@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/;
+
 export function extractNPMReference(discussion: { title: string }) {
   const title = discussion.title;
   if (title.includes("[") && title.includes("]")) {
-    const full = title.split("[")[1]!.split("]")[0];
-    return full!.replace("@types/", "");
+    const full = title.split("[")[1]!.split("]")[0]!;
+    const name = full.replace("@types/", "");
+    if (!npmPackageNameRegex.test(name)) return undefined;
+    return name;
   }
   return undefined;
 }

packages/mergebot/src/functions/discussions-trigger.ts

@@ -55,16 +55,16 @@ const couldNotFindMessage = txt`
 `;
 
 const errorsGettingOwners = (str: string) => txt`
-  |Hi, we could not find [${str}] in DefinitelyTyped, is there possibly a typo? 
+  |Hi, we could not find [\`${str}\`] in DefinitelyTyped, is there possibly a typo? 
 `;
 
 const couldNotFindOwners = (str: string) => txt`
-  |Hi, we had an issue getting the owners for [${str}] - first check if you have a typeo, otherwise please raise an issue on 
+  |Hi, we had an issue getting the owners for [\`${str}\`] - first check if you have a typeo, otherwise please raise an issue on 
   |microsoft/DefinitelyTyped-tools if the module exists on DT but this bot could not find information for it.
 `;
 
 const gotAReferenceMessage = (module: string, owners: string[]) => txt`
-  |Thanks for the discussion about "${module}", some useful links for everyone:
+  |Thanks for the discussion about "\`${module}\`", some useful links for everyone:
   | 
   | - [npm](https://www.npmjs.com/package/${module})
   | - [DT](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/${module})
@@ -87,8 +87,11 @@ async function pingAuthorsAndSetUpDiscussion(discussion: Discussion) {
     } else {
       const message = gotAReferenceMessage(aboutNPMRef, owners);
       await updateOrCreateMainComment(discussion, message);
+      // Only create a label once we've confirmed the package actually exists on DT --
+      // otherwise an unprivileged user could make typescript-bot create arbitrarily-named
+      // repository labels by editing the discussion title.
+      await addLabel(discussion, "Pkg: " + aboutNPMRef, `Discussions related to ${aboutNPMRef}`);
     }
-    await addLabel(discussion, "Pkg: " + aboutNPMRef, `Discussions related to ${aboutNPMRef}`);
   }
   return { status: 200, body: "OK" };
 }