fix: extra file should include strategy path (#1187)

* fix: extra file path relativity

* fix: extra files at root of project

* fix: cross-platform file path joining

always use posix to prevent issues when dos paths are used in urls

* feat: add tilde file pathing

plus more detailed test

* fix: prevent leading pathing chars in extra file paths

* refactor: do not use `path` module

reject relative pathing altogether

* refactor: drop support for `~/` extra files

Co-authored-by: Jeff Ching <chingor@google.com>

src/strategies/base.ts

@@ -285,13 +285,11 @@ export abstract class BaseStrategy implements Strategy {
 
   private extraFileUpdates(version: Version): Update[] {
     const genericUpdater = new Generic({version});
-    return this.extraFiles.map(path => {
-      return {
-        path,
-        createIfMissing: false,
-        updater: genericUpdater,
-      };
-    });
+    return this.extraFiles.map(path => ({
+      path: this.addPath(path),
+      createIfMissing: false,
+      updater: genericUpdater,
+    }));
   }
 
   protected changelogEmpty(changelogEntry: string): boolean {
@@ -450,16 +448,27 @@ export abstract class BaseStrategy implements Strategy {
     return Version.parse('1.0.0');
   }
 
+  /**
+   * Adds a given file path to the strategy path.
+   * @param {string} file Desired file path.
+   * @returns {string} The file relative to the strategy.
+   * @throws {Error} If the file path contains relative pathing characters, i.e. ../, ~/
+   */
   protected addPath(file: string) {
-    if (this.path === ROOT_PROJECT_PATH) {
-      return file;
+    // There is no strategy path to join, the strategy is at the root, or the
+    // file is at the root (denoted by a leading slash or tilde)
+    if (!this.path || this.path === ROOT_PROJECT_PATH || file.startsWith('/')) {
+      file = file.replace(/^\/+/, '');
+    }
+    // Otherwise, the file is relative to the strategy path
+    else {
+      file = `${this.path.replace(/\/+$/, '')}/${file}`;
     }
-    file = file.replace(/^[/\\]/, '');
-    if (this.path === undefined) {
-      return file;
-    } else {
-      const path = this.path.replace(/[/\\]$/, '');
-      return `${path}/${file}`;
+    // Ensure the file path does not escape the workspace
+    if (/((^|\/)\.{1,2}|^~|^\/*)+\//.test(file)) {
+      throw new Error(`illegal pathing characters in path: ${file}`);
     }
+    // Strip any trailing slashes and return
+    return file.replace(/\/+$/, '');
   }
 }

test/manifest.ts

@@ -1563,6 +1563,71 @@ describe('Manifest', () => {
       );
     });
 
+    it('should handle extra files', async () => {
+      mockReleases(github, []);
+      mockCommits(github, [
+        {
+          sha: 'aaaaaa',
+          message: 'fix: a bugfix',
+          files: ['foo', 'pkg.properties'],
+        },
+        {
+          sha: 'bbbbbb',
+          message: 'fix: b bugfix',
+          files: ['pkg/b/foo'],
+        },
+        {
+          sha: 'cccccc',
+          message: 'fix: c bugfix',
+          files: ['pkg/c/foo'],
+        },
+      ]);
+      const manifest = new Manifest(
+        github,
+        'main',
+        {
+          '.': {
+            releaseType: 'simple',
+            component: 'a',
+            extraFiles: ['root.properties'],
+          },
+          'pkg/b': {
+            releaseType: 'simple',
+            component: 'b',
+            extraFiles: ['pkg.properties', 'src/version', '/bbb.properties'],
+            skipGithubRelease: true,
+          },
+          'pkg/c': {
+            releaseType: 'simple',
+            component: 'c',
+            extraFiles: ['/pkg/pkg-c.properties', 'ccc.properties'],
+            skipGithubRelease: true,
+          },
+        },
+        {
+          '.': Version.parse('1.1.0'),
+          'pkg/b': Version.parse('1.0.0'),
+          'pkg/c': Version.parse('1.0.1'),
+        }
+      );
+      const pullRequests = await manifest.buildPullRequests();
+      expect(pullRequests).lengthOf(1);
+      expect(pullRequests[0].updates).to.be.an('array');
+      expect(pullRequests[0].updates.map(update => update.path))
+        .to.include.members([
+          'root.properties',
+          'bbb.properties',
+          'pkg/pkg-c.properties',
+          'pkg/b/pkg.properties',
+          'pkg/b/src/version',
+          'pkg/c/ccc.properties',
+        ])
+        .but.not.include.oneOf([
+          'pkg/b/bbb.properties', // should be at root
+          'pkg/c/pkg-c.properties', // should be up one level
+        ]);
+    });
+
     describe('with plugins', () => {
       beforeEach(() => {
         mockReleases(github, [

test/strategies/base.ts

@@ -69,6 +69,62 @@ describe('Strategy', () => {
       expect(pullRequest?.version?.toString()).to.eql('2.3.4');
       snapshot(dateSafe(pullRequest!.body.toString()));
     });
+    it('updates extra files', async () => {
+      const strategy = new TestStrategy({
+        targetBranch: 'main',
+        github,
+        component: 'google-cloud-automl',
+        extraFiles: ['0', 'foo/1.~csv', 'foo/2.bak', 'foo/baz/bar/', '/3.java'],
+      });
+      const pullRequest = await strategy.buildReleasePullRequest(
+        [{sha: 'aaa', message: 'fix: a bugfix'}],
+        undefined
+      );
+      expect(pullRequest).to.exist;
+      expect(pullRequest?.updates).to.be.an('array');
+      expect(pullRequest?.updates.map(update => update.path))
+        .to.include.members([
+          '0',
+          '3.java',
+          'foo/1.~csv',
+          'foo/2.bak',
+          'foo/baz/bar',
+        ])
+        .and.not.include('foo/baz/bar/', 'expected file but got directory');
+    });
+    it('rejects relative extra files', async () => {
+      const extraFiles = [
+        './bar',
+        './../../../etc/hosts',
+        '../../../../etc/hosts',
+        '~/./5',
+        '~/.ssh/config',
+        '~/../../.././level/../../../up',
+        '/../../../opt',
+        'foo/bar/../baz',
+        'foo/baz/../../../../../etc/hostname',
+      ];
+      for (const file of extraFiles) {
+        try {
+          const strategy = new TestStrategy({
+            targetBranch: 'main',
+            github,
+            component: 'google-cloud-automl',
+            extraFiles: [file],
+          });
+          await strategy.buildReleasePullRequest(
+            [{sha: 'aaa', message: 'fix: a bugfix'}],
+            undefined
+          );
+          expect.fail(`expected [addPath] to reject path: ${file}`);
+        } catch (err) {
+          expect(err).to.be.instanceof(Error);
+          expect(err.message).to.have.string(
+            'illegal pathing characters in path'
+          );
+        }
+      }
+    });
   });
   describe('buildRelease', () => {
     it('builds a release tag', async () => {