[VUL-471] Fix arbitrary file read vulnerability in web CLI push config commands

[umair-ably]

Summary

• Block push:config:set-apns and push:config:set-fcm in web CLI mode — these commands read files from the server filesystem (not the user's local machine) and have no legitimate web CLI use case since there's no file upload mechanism
• Block --control-host flag in web CLI mode to prevent redirecting API requests (and file contents) to attacker-controlled servers
• Add file extension validation (.p12/.pfx for certificates, .p8 for keys) as defense in depth against arbitrary file reads

Context: A security researcher demonstrated that an authenticated web CLI user could read arbitrary server container files (e.g., /etc/passwd) via --certificate /etc/passwd and exfiltrate them using --control-host https://attacker.com. The local CLI is not affected — local users already have full filesystem access.

Test plan

• pnpm prepare passes
• pnpm exec eslint . — 0 errors
• pnpm test:unit — 2258 tests pass
• Verify push:config:set-apns is rejected in web CLI mode (ABLY_WEB_CLI_MODE=true)
• Verify --control-host is rejected for any control API command in web CLI mode
• Verify invalid file extensions are rejected (e.g., --certificate /etc/passwd)

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
cli-web-cli	Ready	Preview, Comment	Apr 10, 2026 10:30am

[claude-code-ably-assistant]

Walkthrough

This PR fixes an arbitrary file read vulnerability in the web CLI where an authenticated user could pass a non-certificate path (e.g., /etc/passwd) to --certificate and exfiltrate the contents via --control-host pointing to an attacker-controlled server. The fix has three layers: blocking the file-reading push config commands entirely in web CLI mode, blocking --control-host in web CLI mode at the base command level, and adding file extension validation as defense-in-depth.

Changes

Area	Files	Summary
Commands	src/commands/push/config/set-apns.ts	Validates .p12/.pfx for --certificate and .p8 for --key-file before reading; rejects invalid extensions via this.fail()
Base / Framework	src/base-command.ts	Adds push:config:set-apns and push:config:set-fcm to WEB_CLI_RESTRICTED_COMMANDS
Base / Framework	src/control-base-command.ts	Blocks --control-host in web CLI mode for all Control API commands
Tests	test/unit/commands/push/config/set-apns.test.ts	Adds test groups for file extension validation, web CLI restriction, and fixes existing tests to use a proper .p12 fixture
Tests	test/unit/commands/push/config/show.test.ts	Adds test for --control-host rejection in web CLI mode
Fixtures	test/fixtures/push/test-apns-cert.p12	New dummy .p12 fixture replacing .p8 misuse in existing certificate tests
Docs / Skills	.claude/skills/ably-new-command/SKILL.md	Documents the security rule: file-reading commands must be added to WEB_CLI_RESTRICTED_COMMANDS

Review Notes

• push:config:set-fcm not validated: The FCM command is blocked in web CLI mode (good), but no file extension validation was added to it. If it also accepts a file path flag, it should get the same defense-in-depth treatment.
• --control-host block scope: The block is in createControlApi() in ControlBaseCommand, meaning it covers all Control API commands — not just the push config ones. This is intentional and correct, but reviewers should confirm there are no legitimate web CLI uses of --control-host.
• No migration/deployment needed: Changes are purely additive restrictions with no API or schema changes.
• Test coverage: All three mitigation layers have unit tests. The web CLI restriction for set-apns is tested; the equivalent for set-fcm is not explicitly tested.

[claude-code-ably-assistant]

Review Summary

This PR fixes a real security vulnerability (arbitrary file read in web CLI) with a layered defense approach. The implementation is correct and the architecture is sound. One genuine gap and one observation:

---

Issue 1 — Missing test coverage for push:config:set-fcm web CLI restriction (minor gap)

The PR adds push:config:set-fcm to WEB_CLI_RESTRICTED_COMMANDS but there's no corresponding test in test/unit/commands/push/config/set-fcm.test.ts verifying the restriction works. Every other new behavior in this PR has a test — this one doesn't.

Risk: If set-fcm is accidentally dropped from the restricted list in future, there's nothing to catch it.

Suggested fix: Add a describe("web CLI restrictions", ...) block to set-fcm.test.ts mirroring the one added to set-apns.test.ts. The pattern is already there in the other file.

---

Observation — --control-host guard placement is correct but non-obvious

The --control-host check is inside createControlApi(), which runs at API-call time (inside run()), not at command startup. This is fine because:

• File-reading commands (set-apns, set-fcm) are blocked earlier by checkWebCliRestrictions() in init(), so they never reach run()
• For non-file-reading commands like push:config:show, the createControlApi() guard is the right layer

The show.test.ts test correctly exercises this path. No change needed — just documenting that the ordering is intentional.

---

Everything else looks correct

• WEB_CLI_RESTRICTED_COMMANDS entries are correct — both file-reading commands blocked
• Extension validation (path.extname() + path.resolve()) correctly catches extensionless paths (returns "" → shown as "(no extension)") and wrong extensions
• this.fail() after extension check returns never, so the subsequent fs.existsSync() is unreachable on invalid extensions — no double-read risk
• FCM command uses JSON structure validation (type: "service_account" + project_id check) as its content guard, which is more meaningful than extension checking for JSON files
• Test for .pfx acceptance correctly verifies the "passes extension, fails file-not-found" path
• accessToken check before --control-host check in createControlApi() is fine — web CLI always has a token set

Verdict: Ready to merge after adding the missing FCM restriction test.

[claude-code-ably-assistant]

Review Summary

This PR fixes a real security vulnerability with a correct layered defense approach. One genuine gap worth addressing before merge:

Issue: Missing test for push:config:set-fcm web CLI restriction

push:config:set-fcm is added to WEB_CLI_RESTRICTED_COMMANDS but set-fcm.test.ts has no test verifying this restriction works. Every other new behavior in this PR has test coverage — this one does not.

If set-fcm is accidentally removed from the restricted list in a future refactor, nothing will catch the regression.

Suggested fix: add a describe("web CLI restrictions") block to set-fcm.test.ts mirroring the one added to set-apns.test.ts.

---

Observations (no action needed)

The --control-host guard in createControlApi() is placed correctly. File-reading commands are blocked earlier by checkWebCliRestrictions() in init() before run() is called, so they never reach createControlApi(). For non-file-reading Control API commands like push:config:show, createControlApi() is the right interception point — show.test.ts covers this.

Extension validation is correct. path.extname("/etc/passwd") returns "", which is caught and shown as "(no extension)". Since this.fail() returns never, the fs.existsSync() call below is unreachable on invalid extensions — no double-read risk.

FCM's defense-in-depth is structural, not extension-based. The JSON validation (type: "service_account" + project_id check) is a stronger content guard for JSON files than an extension check would be, and is consistent with what FCM actually requires.

---

Verdict: ready to merge after adding the missing FCM web CLI restriction test.

[umair-ably]

@claude I amended the commit and added a test for push:config:set-fcm web CLI restriction

[claude-code-ably-assistant]

Review Summary

Security fix for VUL-471. The implementation is correct and the defense-in-depth approach is well-structured. No bugs or correctness issues found.

What the PR does (correctly)

Layer 1 — Command blocking (primary fix): Adding push:config:set-apns and push:config:set-fcm to WEB_CLI_RESTRICTED_COMMANDS is the right primary fix. checkWebCliRestrictions() fires in init() before run() executes, so the command never reaches the file-reading code in web CLI mode.

Layer 2 — --control-host blocking (secondary fix): Blocking --control-host in createControlApi() is a good safety net for all Control API commands, not just the file-reading ones. Because createControlApi is always called before any API request, this ensures no other Control API command can redirect file content to an attacker-controlled host in web CLI mode. The ABLY_CONTROL_HOST env var path is also covered since oclif populates flags["control-host"] from it.

Layer 3 — Extension validation (defense in depth): Restricting to .p12/.pfx/.p8/.json limits the attack surface for local CLI users. It's correctly placed before existsSync so the cheaper check runs first. Extension checks use .toLowerCase() to handle upper-case variants.

One necessary test fix the PR correctly handles

The old set-fcm.test.ts test for "should fail when service account file is not valid JSON" used test-apns-key.p8 as the input — after adding extension validation, this test would still pass (error is defined) but for the wrong reason (extension rejection, not JSON parsing failure). The PR correctly replaces it with a temp .json file containing invalid JSON content and adds expect(error?.message).toContain("not valid JSON") to pin the right failure mode.

Similarly, the three existing set-apns.test.ts tests that passed p8FixturePath to --certificate are correctly updated to use the new p12FixturePath fixture, since .p8 would now be rejected.

No issues found

• Error handling uses this.fail() throughout — correct
• Component strings ("pushConfigSetApns", "pushConfigSetFcm", "auth") are camelCase — correct
• beforeEach/afterEach env var save/restore pattern in web CLI tests is correct
• The "not available in the web CLI" substring in test assertions correctly matches the default message from checkWebCliRestrictions()

LGTM.

[claude-code-ably-assistant]

Security fix is correctly implemented. Defense-in-depth layers all work as intended. Test coverage covers the attack paths and correctly updates tests that would have passed for the wrong reason after adding extension validation. LGTM.