fix(release): sign the manifest against the base the package is zipped with

script/command-executor.ts

@@ -215,6 +215,11 @@ function appRename(command: cli.IAppRenameCommand): Promise<void> {
   });
 }
 
+/** Must match the base packageFileFromPath zips with, or the signature covers keys the package lacks. */
+export function signatureManifestBase(filePath: string): string {
+  return path.dirname(filePath);
+}
+
 export function resolvePrivateKey(value: string): string {
   return value.trimStart().startsWith("-----BEGIN")
     ? value                              // inline PEM content
@@ -1266,7 +1271,7 @@ export const release = (command: cli.IReleaseCommand): Promise<void> => {
           map.set(path.basename(filePath), fileHash);
           return new hashUtils.PackageManifest(map).computePackageHash();
         })
-      : hashUtils.generatePackageHashFromDirectory(filePath, path.join(filePath, ".."));
+      : hashUtils.generatePackageHashFromDirectory(filePath, signatureManifestBase(filePath));
     return hashPromise.then((packageHash: string) => createRS256JWT(privateKey, packageHash));
   };
 

test/cli.ts

@@ -2,13 +2,15 @@
 // Licensed under the MIT License.
 
 import * as assert from "assert";
+import * as crypto from "crypto";
 import * as fs from "fs";
 import * as sinon from "sinon";
 import Q = require("q");
 import * as path from "path";
 import * as codePush from "../script/types";
 import * as cli from "../script/types/cli";
 import * as cmdexec from "../script/command-executor";
+import * as hashUtils from "../script/hash-utils";
 import * as os from "os";
 import moment = require("moment");
 
@@ -223,6 +225,10 @@ export class SdkStub {
     return Q(<void>null);
   }
 
+  public isAuthenticated(): Q.Promise<boolean> {
+    return Q(true);
+  }
+
   public release(): Q.Promise<string> {
     return Q("Successfully released");
   }
@@ -1798,6 +1804,49 @@ describe("CLI", () => {
       .done();
   });
 
+  it("signs a directory release against the base the package is zipped with", (done: Mocha.Done): void => {
+    // The call site, not the helper: releasing "." used to sign keys the zip did not contain, and still
+    // reported success.
+    const { privateKey } = crypto.generateKeyPairSync("rsa", {
+      modulusLength: 2048,
+      privateKeyEncoding: { type: "pkcs8", format: "pem" },
+      publicKeyEncoding: { type: "spki", format: "pem" },
+    });
+
+    const bases: string[] = [];
+    sandbox.stub(<any>hashUtils, "generatePackageHashFromDirectory").callsFake((dir: string, base: string) => {
+      bases.push(base);
+      return Q("deadbeef");
+    });
+
+    const dir = fs.mkdtempSync(path.join(os.tmpdir(), "dpctl-sign-"));
+    fs.writeFileSync(path.join(dir, "main.jsbundle"), "bundle contents");
+    const previousCwd = process.cwd();
+    process.chdir(dir);
+
+    const finish = (error?: any): void => {
+      process.chdir(previousCwd);
+      fs.rmSync(dir, { recursive: true, force: true });
+      if (error) return done(error);
+      assert.deepStrictEqual(bases, [path.dirname(".")], "must key the manifest the way the package is zipped");
+      done();
+    };
+
+    cmdexec
+      .execute(<any>{
+        type: cli.CommandType.release,
+        appName: "a",
+        deploymentName: "Staging",
+        description: "",
+        mandatory: false,
+        rollout: null,
+        appStoreVersion: "1.0.0",
+        package: ".",
+        privateKey,
+      })
+      .done(() => finish(), finish);
+  });
+
   function releaseHelperFunction(command: cli.IReleaseCommand, done: Mocha.Done, expectedError: string): void {
     cmdexec.execute(command).done(
       (): void => {

test/hash-utils.ts

@@ -5,6 +5,7 @@ import * as assert from "assert";
 import * as crypto from "crypto";
 import * as fs from "fs";
 import * as hashUtils from "../script/hash-utils";
+import * as cmdexec from "../script/command-executor";
 var { mkdirp } = require("mkdirp");
 import * as os from "os";
 import * as path from "path";
@@ -205,4 +206,74 @@ describe("Hashing utility", () => {
         });
     });
   });
+  // ---------------------------------------------------------------------------
+  // The signed hash must be keyed the same way the zip is, or the signature covers keys the package does
+  // not contain. path.join(dir, "..") and path.dirname(dir) differ only for "." and "./", which is
+  // exactly what `dpctl release MyApp . 1.0.0` passes.
+  // ---------------------------------------------------------------------------
+
+  describe("signed manifest base path", () => {
+    let workDir: string;
+    let previousCwd: string;
+
+    beforeEach(() => {
+      workDir = fs.mkdtempSync(path.join(os.tmpdir(), "dpctl-hash-base-"));
+      fs.writeFileSync(path.join(workDir, "main.jsbundle"), "bundle contents");
+      previousCwd = process.cwd();
+      process.chdir(workDir);
+    });
+
+    afterEach(() => {
+      process.chdir(previousCwd);
+      fs.rmSync(workDir, { recursive: true, force: true });
+    });
+
+    it('keys the manifest the way the zip does when the release path is "."', (done) => {
+      hashUtils.generatePackageManifestFromDirectory(".", path.dirname(".")).then((manifest: PackageManifest) => {
+        const keys = Array.from(manifest.toMap().keys());
+        assert.deepStrictEqual(keys, ["main.jsbundle"], `got ${JSON.stringify(keys)}`);
+        done();
+      }, done);
+    });
+
+    it("the CLI keys the signed manifest against the base the SDK zips with", () => {
+      assert.strictEqual(cmdexec.signatureManifestBase("."), path.dirname("."));
+      assert.strictEqual(cmdexec.signatureManifestBase("."), ".");
+      assert.strictEqual(cmdexec.signatureManifestBase("/tmp/build"), "/tmp");
+      assert.notStrictEqual(cmdexec.signatureManifestBase("."), path.join(".", ".."));
+    });
+
+    it('path.join(dir, "..") is NOT path.dirname(dir) for "."', () => {
+      // The test above only bites while these disagree.
+      assert.strictEqual(path.dirname("."), ".");
+      assert.strictEqual(path.join(".", ".."), "..");
+      assert.notStrictEqual(path.join(".", ".."), path.dirname("."));
+    });
+
+    it('signs a hash that matches the zip when the release path is "."', (done) => {
+      // The base the CLI actually passes, not a copy of it.
+      const signed = hashUtils.generatePackageHashFromDirectory(".", cmdexec.signatureManifestBase("."));
+      const onDevice = hashUtils.generatePackageManifestFromDirectory(".", path.dirname(".")).then((manifest: PackageManifest) =>
+        manifest.computePackageHash()
+      );
+
+      q.all([signed, onDevice]).then((hashes: string[]) => {
+        assert.strictEqual(hashes[0], hashes[1], "the signed hash must match what the zip implies");
+        done();
+      }, done);
+    });
+
+    it('would not have matched with the old path.join(dir, "..") base', (done) => {
+      // Keeps the fix load-bearing rather than cosmetic.
+      const buggy = hashUtils.generatePackageHashFromDirectory(".", path.join(".", ".."));
+      const onDevice = hashUtils.generatePackageManifestFromDirectory(".", path.dirname(".")).then((manifest: PackageManifest) =>
+        manifest.computePackageHash()
+      );
+
+      q.all([buggy, onDevice]).then((hashes: string[]) => {
+        assert.notStrictEqual(hashes[0], hashes[1], "the old base produced a hash that verified nowhere");
+        done();
+      }, done);
+    });
+  });
 });