Merge pull request #407 from ably/fix/web-cli-push-payload-file-read

umair-ably · web-flow · commit 58476a849f81 · 2026-06-05T14:57:55.000+01:00
[VUL-506] fix(push): stop web CLI from reading server-side files via payload args
diff --git a/.claude/skills/ably-new-command/SKILL.md b/.claude/skills/ably-new-command/SKILL.md
@@ -429,7 +429,10 @@ If the new command shouldn't be available in the web CLI, add it to the appropri
 - `WEB_CLI_ANONYMOUS_RESTRICTED_COMMANDS` — for commands that expose account/app data
 - `INTERACTIVE_UNSUITABLE_COMMANDS` — for commands that don't work in interactive mode
 
-**Security rule:** Any command that reads or uploads files from the filesystem (e.g., certificate uploads, key file imports) **must** be added to `WEB_CLI_RESTRICTED_COMMANDS`. The web CLI runs on a server — file-reading commands would read from the server's filesystem, not the user's local machine, which could expose server contents. There is no file upload mechanism in the web CLI to transfer local files.
+**Security rule:** The web CLI runs on a shared server — any command that reads files from the filesystem reads the *server's* files, not the user's local machine, which can expose server contents. There is no file-upload mechanism in the web CLI to transfer local files. So for any command that reads files, choose one of:
+
+1. **Block the command** — add it to `WEB_CLI_RESTRICTED_COMMANDS` when the *entire* command is meaningless without local files (e.g. `push config set-fcm`/`set-apns` exist only to upload a credentials file).
+2. **Disable just the file read** — when the command is still useful with inline input (e.g. `push publish`/`batch-publish`/`devices save` accept JSON inline), load the input through the `this.resolveJsonInput(input, label, flags, component)` base-command helper. It reads `@file`/path inputs from disk in local mode but, in web CLI mode, treats the input as literal data and rejects `@file` references — so the command keeps working without a server-side file-read sink. **Never** call `fs.readFileSync` directly on flag/arg values; always go through `resolveJsonInput` so the web-CLI restriction is inherited rather than re-implemented (and re-missed) per command.
 
 ## Step 6: Validate
 
diff --git a/src/base-command.ts b/src/base-command.ts
@@ -38,6 +38,8 @@ import {
 } from "./utils/long-running.js";
 import isTestMode from "./utils/test-mode.js";
 import isWebCliMode from "./utils/web-mode.js";
+import * as fs from "node:fs";
+import * as path from "node:path";
 
 // List of commands not allowed in web CLI mode - EXPORTED
 export const WEB_CLI_RESTRICTED_COMMANDS = [
@@ -1709,6 +1711,75 @@ export abstract class AblyBaseCommand extends InteractiveBaseCommand {
     }
   }
 
+  /**
+   * Resolve a JSON input that may be either a literal JSON string or a
+   * reference to a local file. Supports the documented `@path` prefix, bare
+   * path prefixes (`/`, `./`, `../`), and bare paths that happen to exist on
+   * disk. Returns the raw JSON string for the caller to parse.
+   *
+   * SECURITY: in web CLI mode the filesystem belongs to the shared
+   * terminal-server container, not the user's machine — reading from it
+   * would disclose server-side files. So in web CLI mode this NEVER touches the
+   * filesystem: the input is always treated as a literal JSON string, and any
+   * file reference (`@file` or a path prefix) is rejected with a clear error.
+   * Any command that accepts a JSON-or-file input MUST go through this helper so
+   * the restriction is inherited rather than re-implemented per command.
+   */
+  protected resolveJsonInput(
+    input: string,
+    inputLabel: string,
+    flags: BaseFlags,
+    component: string,
+  ): string {
+    // Web CLI: the filesystem is server-side. Never read it; treat the input as
+    // literal data. Reject anything that looks like a file reference explicitly
+    // so users get a clear message instead of a confusing JSON parse error.
+    // Detection is prefix-based only — we must not probe `existsSync` here, as
+    // that would itself touch the server's filesystem.
+    if (this.isWebCliMode) {
+      const looksLikeFileReference =
+        input.startsWith("@") ||
+        input.startsWith("/") ||
+        input.startsWith("./") ||
+        input.startsWith("../");
+      if (looksLikeFileReference) {
+        this.fail(
+          `Loading ${inputLabel} from a file is not supported in the web CLI. Pass the JSON inline instead.`,
+          flags,
+          component,
+        );
+      }
+      return input;
+    }
+
+    // Local CLI: load from a file when the input is a file reference.
+    if (input.startsWith("@")) {
+      const filePath = path.resolve(input.slice(1));
+      if (!fs.existsSync(filePath)) {
+        this.fail(`File not found: ${filePath}`, flags, component);
+      }
+      return fs.readFileSync(filePath, "utf8");
+    }
+
+    if (
+      input.startsWith("/") ||
+      input.startsWith("./") ||
+      input.startsWith("../")
+    ) {
+      const filePath = path.resolve(input);
+      if (!fs.existsSync(filePath)) {
+        this.fail(`File not found: ${filePath}`, flags, component);
+      }
+      return fs.readFileSync(filePath, "utf8");
+    }
+
+    if (fs.existsSync(path.resolve(input))) {
+      return fs.readFileSync(path.resolve(input), "utf8");
+    }
+
+    return input;
+  }
+
   /**
    * Parse a flag value as a JSON object. Rejects arrays and primitives.
    */
diff --git a/src/commands/push/batch-publish.ts b/src/commands/push/batch-publish.ts
@@ -1,6 +1,4 @@
 import { Args } from "@oclif/core";
-import * as fs from "node:fs";
-import * as path from "node:path";
 
 import { AblyBaseCommand } from "../../base-command.js";
 import { CommandError } from "../../errors/command-error.js";
@@ -108,26 +106,13 @@ export default class PushBatchPublish extends AblyBaseCommand {
         jsonString = await this.readStdin();
       } else if (payloadArg === "-") {
         jsonString = await this.readStdin();
-      } else if (payloadArg.startsWith("@")) {
-        const filePath = path.resolve(payloadArg.slice(1));
-        if (!fs.existsSync(filePath)) {
-          this.fail(`File not found: ${filePath}`, flags, "pushBatchPublish");
-        }
-        jsonString = fs.readFileSync(filePath, "utf8");
-      } else if (
-        payloadArg.startsWith("/") ||
-        payloadArg.startsWith("./") ||
-        payloadArg.startsWith("../")
-      ) {
-        const filePath = path.resolve(payloadArg);
-        if (!fs.existsSync(filePath)) {
-          this.fail(`File not found: ${filePath}`, flags, "pushBatchPublish");
-        }
-        jsonString = fs.readFileSync(filePath, "utf8");
-      } else if (fs.existsSync(path.resolve(payloadArg))) {
-        jsonString = fs.readFileSync(path.resolve(payloadArg), "utf8");
       } else {
-        jsonString = payloadArg;
+        jsonString = this.resolveJsonInput(
+          payloadArg,
+          "the batch payload",
+          flags,
+          "pushBatchPublish",
+        );
       }
 
       let batchPayload: unknown[];
diff --git a/src/commands/push/devices/save.ts b/src/commands/push/devices/save.ts
@@ -1,6 +1,4 @@
 import { Flags } from "@oclif/core";
-import * as fs from "node:fs";
-import * as path from "node:path";
 
 import { AblyBaseCommand } from "../../../base-command.js";
 import { productApiFlags } from "../../../flags.js";
@@ -202,15 +200,12 @@ export default class PushDevicesSave extends AblyBaseCommand {
     data: string,
     flags: Record<string, unknown>,
   ): Record<string, unknown> {
-    let jsonString = data;
-
-    if (data.startsWith("@")) {
-      const filePath = path.resolve(data.slice(1));
-      if (!fs.existsSync(filePath)) {
-        this.fail(`File not found: ${filePath}`, flags, "pushDeviceSave");
-      }
-      jsonString = fs.readFileSync(filePath, "utf8");
-    }
+    const jsonString = this.resolveJsonInput(
+      data,
+      "--data",
+      flags,
+      "pushDeviceSave",
+    );
 
     let parsed: unknown;
     try {
diff --git a/src/commands/push/publish.ts b/src/commands/push/publish.ts
@@ -1,6 +1,4 @@
 import { Flags } from "@oclif/core";
-import * as fs from "node:fs";
-import * as path from "node:path";
 
 import { AblyBaseCommand } from "../../base-command.js";
 import { forceFlag, productApiFlags } from "../../flags.js";
@@ -148,28 +146,12 @@ export default class PushPublish extends AblyBaseCommand {
       // Build notification payload
       let payload: Record<string, unknown>;
       if (flags.payload) {
-        let jsonString: string;
-        if (flags.payload.startsWith("@")) {
-          const filePath = path.resolve(flags.payload.slice(1));
-          if (!fs.existsSync(filePath)) {
-            this.fail(`File not found: ${filePath}`, flags, "pushPublish");
-          }
-          jsonString = fs.readFileSync(filePath, "utf8");
-        } else if (
-          flags.payload.startsWith("/") ||
-          flags.payload.startsWith("./") ||
-          flags.payload.startsWith("../")
-        ) {
-          const filePath = path.resolve(flags.payload);
-          if (!fs.existsSync(filePath)) {
-            this.fail(`File not found: ${filePath}`, flags, "pushPublish");
-          }
-          jsonString = fs.readFileSync(filePath, "utf8");
-        } else if (fs.existsSync(path.resolve(flags.payload))) {
-          jsonString = fs.readFileSync(path.resolve(flags.payload), "utf8");
-        } else {
-          jsonString = flags.payload;
-        }
+        const jsonString = this.resolveJsonInput(
+          flags.payload,
+          "--payload",
+          flags,
+          "pushPublish",
+        );
         payload = this.parseJsonObjectFlag(jsonString, "--payload", flags);
       } else {
         const notification: Record<string, unknown> = {};
diff --git a/test/unit/commands/push/batch-publish.test.ts b/test/unit/commands/push/batch-publish.test.ts
@@ -1,5 +1,8 @@
-import { describe, it, expect, beforeEach } from "vitest";
+import { describe, it, expect, beforeEach, afterEach } from "vitest";
 import { runCommand } from "@oclif/test";
+import * as fs from "node:fs";
+import * as os from "node:os";
+import * as path from "node:path";
 import { getMockAblyRest } from "../../../helpers/mock-ably-rest.js";
 import {
   standardHelpTests,
@@ -483,4 +486,76 @@ describe("push:batch-publish command", () => {
       expect(error?.message).toContain("40300");
     });
   });
+
+  // In web CLI mode the batch payload must never be read from the server's
+  // filesystem. The file-loading shortcut is local-CLI only.
+  describe("web CLI file-read restriction", () => {
+    let originalWebCliMode: string | undefined;
+    let secretFile: string;
+
+    beforeEach(() => {
+      originalWebCliMode = process.env.ABLY_WEB_CLI_MODE;
+      secretFile = path.join(os.tmpdir(), `vul506-batch-${process.pid}.json`);
+      fs.writeFileSync(
+        secretFile,
+        '[{"recipient":{"deviceId":"dev-1"},"payload":{"notification":{"title":"SECRET_FROM_FILE"}}}]',
+      );
+    });
+
+    afterEach(() => {
+      if (originalWebCliMode === undefined) {
+        delete process.env.ABLY_WEB_CLI_MODE;
+      } else {
+        process.env.ABLY_WEB_CLI_MODE = originalWebCliMode;
+      }
+      if (fs.existsSync(secretFile)) fs.rmSync(secretFile);
+    });
+
+    it("reads a local file payload when NOT in web CLI mode", async () => {
+      const mock = getMockAblyRest();
+      mock.request.mockResolvedValue({ statusCode: 200, items: [] });
+
+      const { stderr } = await runCommand(
+        ["push:batch-publish", secretFile, "--force"],
+        import.meta.url,
+      );
+
+      expect(stderr).toContain("published");
+      expect(mock.request).toHaveBeenCalledWith(
+        "post",
+        "/push/batch/publish",
+        2,
+        null,
+        expect.any(Array),
+      );
+    });
+
+    it("rejects a server-side file path in web CLI mode without reading it", async () => {
+      process.env.ABLY_WEB_CLI_MODE = "true";
+      const mock = getMockAblyRest();
+
+      const { error } = await runCommand(
+        ["push:batch-publish", secretFile, "--force"],
+        import.meta.url,
+      );
+
+      // A path-like payload is rejected with a clear message and the file
+      // contents are never read or published.
+      expect(error).toBeDefined();
+      expect(error?.message).toContain("not supported in the web CLI");
+      expect(mock.request).not.toHaveBeenCalled();
+    });
+
+    it("rejects @file payload references in web CLI mode", async () => {
+      process.env.ABLY_WEB_CLI_MODE = "true";
+
+      const { error } = await runCommand(
+        ["push:batch-publish", `@${secretFile}`, "--force"],
+        import.meta.url,
+      );
+
+      expect(error).toBeDefined();
+      expect(error?.message).toContain("not supported in the web CLI");
+    });
+  });
 });
diff --git a/test/unit/commands/push/devices/save.test.ts b/test/unit/commands/push/devices/save.test.ts
@@ -1,5 +1,8 @@
-import { describe, it, expect, beforeEach } from "vitest";
+import { describe, it, expect, beforeEach, afterEach } from "vitest";
 import { runCommand } from "@oclif/test";
+import * as fs from "node:fs";
+import * as os from "node:os";
+import * as path from "node:path";
 import { getMockAblyRest } from "../../../../helpers/mock-ably-rest.js";
 import {
   standardHelpTests,
@@ -307,4 +310,91 @@ describe("push:devices:save command", () => {
       expect(error?.message).toContain("must be a JSON object");
     });
   });
+
+  // In web CLI mode --data must never be read from the server's filesystem.
+  // The @file shortcut is local-CLI only.
+  describe("web CLI file-read restriction", () => {
+    let originalWebCliMode: string | undefined;
+    let secretFile: string;
+
+    beforeEach(() => {
+      originalWebCliMode = process.env.ABLY_WEB_CLI_MODE;
+      secretFile = path.join(os.tmpdir(), `vul506-device-${process.pid}.json`);
+      fs.writeFileSync(
+        secretFile,
+        '{"id":"device-2","platform":"android","formFactor":"tablet","push":{"recipient":{"transportType":"fcm","registrationToken":"tok"}}}',
+      );
+    });
+
+    afterEach(() => {
+      if (originalWebCliMode === undefined) {
+        delete process.env.ABLY_WEB_CLI_MODE;
+      } else {
+        process.env.ABLY_WEB_CLI_MODE = originalWebCliMode;
+      }
+      if (fs.existsSync(secretFile)) fs.rmSync(secretFile);
+    });
+
+    it("reads a local --data @file when NOT in web CLI mode", async () => {
+      const mock = getMockAblyRest();
+      mock.push.admin.deviceRegistrations.save.mockResolvedValue({
+        id: "device-2",
+      });
+
+      const { stderr } = await runCommand(
+        ["push:devices:save", "--data", `@${secretFile}`],
+        import.meta.url,
+      );
+
+      expect(stderr).toContain("Device registration saved");
+      expect(mock.push.admin.deviceRegistrations.save).toHaveBeenCalledWith(
+        expect.objectContaining({ id: "device-2", platform: "android" }),
+      );
+    });
+
+    it("reads a local --data path input when NOT in web CLI mode", async () => {
+      const mock = getMockAblyRest();
+      mock.push.admin.deviceRegistrations.save.mockResolvedValue({
+        id: "device-2",
+      });
+
+      const { stderr } = await runCommand(
+        ["push:devices:save", "--data", secretFile],
+        import.meta.url,
+      );
+
+      expect(stderr).toContain("Device registration saved");
+      expect(mock.push.admin.deviceRegistrations.save).toHaveBeenCalledWith(
+        expect.objectContaining({ id: "device-2", platform: "android" }),
+      );
+    });
+
+    it("rejects --data @file references in web CLI mode", async () => {
+      process.env.ABLY_WEB_CLI_MODE = "true";
+      const mock = getMockAblyRest();
+
+      const { error } = await runCommand(
+        ["push:devices:save", "--data", `@${secretFile}`],
+        import.meta.url,
+      );
+
+      expect(error).toBeDefined();
+      expect(error?.message).toContain("not supported in the web CLI");
+      expect(mock.push.admin.deviceRegistrations.save).not.toHaveBeenCalled();
+    });
+
+    it("rejects a --data path input in web CLI mode without reading it", async () => {
+      process.env.ABLY_WEB_CLI_MODE = "true";
+      const mock = getMockAblyRest();
+
+      const { error } = await runCommand(
+        ["push:devices:save", "--data", secretFile],
+        import.meta.url,
+      );
+
+      expect(error).toBeDefined();
+      expect(error?.message).toContain("not supported in the web CLI");
+      expect(mock.push.admin.deviceRegistrations.save).not.toHaveBeenCalled();
+    });
+  });
 });
diff --git a/test/unit/commands/push/publish.test.ts b/test/unit/commands/push/publish.test.ts
