feat(git-node): do not trust user input metadata

Author: aduh95

components/git/land.js

@@ -202,12 +202,6 @@ async function main(state, argv, cli, dir) {
     }
     session = new LandingSession(cli, req, dir, argv);
     const metadata = await getMetadata(session.argv, argv.skipRefs, cli);
-    if (argv.backport) {
-      const split = metadata.metadata.split('\n')[0];
-      if (split === 'PR-URL: ') {
-        cli.error('Commit message is missing PR-URL');
-      }
-    }
     return session.start(metadata);
   } else if (state === APPLY) {
     return session.apply();

lib/landing_session.js

@@ -286,6 +286,12 @@ export default class LandingSession extends Session {
       cli.warn('Not yet ready to amend, run `git node land --abort`');
       return;
     }
+
+    const BACKPORT_RE = /^BACKPORT-PR-URL\s*:\s*(\S+)$/i;
+    const PR_RE = /^PR-URL\s*:\s*(\S+)$/i;
+    const REVIEW_RE = /^Reviewed-By\s*:\s*(\S+)$/i;
+    const CVE_RE = /^CVE-ID\s*:\s*(\S+)$/i;
+
     this.startAmending();
 
     const rev = this.getCurrentRev();
@@ -303,7 +309,12 @@ export default class LandingSession extends Session {
     }).trim().length !== 0;
 
     const metadata = this.metadata.trim().split('\n');
-    const amended = original.split('\n');
+    // Filtering out metadata that we will add ourselves:
+    const isFiltered = line =>
+      (!PR_RE.test(line) || this.backport) &&
+      !BACKPORT_RE.test(line) &&
+      !REVIEW_RE.test(line);
+    const amended = original.split('\n').filter(isFiltered);
 
     // If the original commit message already contains trailers (such as
     // "Co-authored-by"), we simply add our own metadata after those. Otherwise,
@@ -313,35 +324,29 @@ export default class LandingSession extends Session {
       amended.push('');
     }
 
-    const BACKPORT_RE = /BACKPORT-PR-URL\s*:\s*(\S+)/i;
-    const PR_RE = /PR-URL\s*:\s*(\S+)/i;
-    const REVIEW_RE = /Reviewed-By\s*:\s*(\S+)/i;
-    const CVE_RE = /CVE-ID\s*:\s*(\S+)/i;
-
     let containCVETrailer = false;
     for (const line of metadata) {
-      if (line.length !== 0 && original.includes(line)) {
-        if (line.match(CVE_RE)) {
-          containCVETrailer = true;
-        }
+      if (line.length !== 0 && original.includes(line) && !isFiltered(line)) {
+        containCVETrailer ||= CVE_RE.test(line);
         if (originalHasTrailers) {
           cli.warn(`Found ${line}, skipping..`);
+          continue;
         } else {
           cli.error('Git found no trailers in the original commit message, ' +
                     `but '${line}' is present and should be a trailer.`);
           process.exit(1);  // make it work with git rebase -x
         }
-      } else {
-        if (line.match(BACKPORT_RE)) {
-          let prIndex = amended.findIndex(datum => datum.match(PR_RE));
-          if (prIndex === -1) {
-            prIndex = amended.findIndex(datum => datum.match(REVIEW_RE)) - 1;
-          }
-          amended.splice(prIndex + 1, 0, line);
-        } else {
-          amended.push(line);
+      }
+      if (BACKPORT_RE.test(line)) {
+        const prIndex =
+          (amended.findIndex(datum => PR_RE.test(datum)) + 1) ||
+          amended.findIndex(datum => REVIEW_RE.test(datum));
+        if (prIndex !== -1) {
+          amended.splice(prIndex, 0, line);
+          continue;
         }
       }
+      amended.push(line);
     }
 
     if (!containCVETrailer && this.includeCVE) {