feat(cli): Basic CLI commands (Spec 008-cli)

[HamedMP]

Summary

• Implements basic CLI commands to complete Milestone 1
• Adds @finna/cli package with Commander.js
• Adds config module to @finna/core for configuration management

Commands Added

• finna gateway run/status - Gateway server management
• finna config show/get/set/unset - Configuration management
• finna channels list/status - Channel listing and connectivity
• finna pairing list/approve/reject - DM pairing flow

Test plan

• CLI builds without TypeScript errors
• Lint passes
• finna --help shows all commands
• finna config set/get/show works correctly
• Gateway status checks connection
• Channels list shows configured channels

[coderabbitai]

Warning

Rate limit exceeded

@HamedMP has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 28 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

In general, to fix prototype-polluting assignments arising from user-controlled property names, either (1) use data structures that are not prototype-based (like Map), or (2) ensure that untrusted keys cannot be special prototype-related names (__proto__, constructor, prototype), by rejecting or transforming them before using them as object keys.

For this specific code, the minimal, non-breaking fix is to validate each part (and the final lastKey) before using it as a property name. If a path segment is one of the dangerous keys, we should reject the path (throwing an error) instead of creating/using that property. This keeps the existing behavior for all legitimate keys and avoids changing the shape or type of config. Concretely:

• Add a small helper function in this file (e.g., isUnsafeKey) that checks if a key is '__proto__', 'constructor', or 'prototype'.
• In setConfigValue, inside the for (const part of parts) loop, before accessing current[part] or assigning to it, check isUnsafeKey(part) and throw an error if it’s unsafe.
• After computing lastKey, validate it in the same way before using it in delete current[lastKey] or current[lastKey] = value.
  This requires no new imports; the helper can be a simple local function.

In general, to fix this kind of issue you must prevent user‑controlled strings from being used as raw property keys on ordinary objects in a way that could touch the prototype chain. Typical strategies are: (1) using a safe key‑value structure like Map instead of plain objects, or (2) rejecting or transforming dangerous keys such as __proto__, constructor, and prototype before using them as property names.

For this codebase, the minimal, non‑breaking change is to validate each path segment (part) and the final key (lastKey) before using them as object property names. If any segment is one of the dangerous names, we should reject the path with a clear error rather than silently modifying Object.prototype. This preserves the existing data model (plain objects backed by JSON) and behavior for all legitimate keys, while blocking only clearly unsafe ones.

Concretely in packages/core/src/config/index.ts:

• Add a small helper function, e.g. isUnsafeConfigKey(key: string): boolean, that returns true for keys __proto__, constructor, and prototype.
• Use this helper:
  • In getConfigValue, before indexing with each part, return undefined if an unsafe key is encountered.
  • In setConfigValue, after computing parts and lastKey, check each segment in parts and lastKey; if any is unsafe, throw an Error('Invalid config path: unsafe key segment').
• Keep using plain objects and the existing file I/O; no new imports are needed.

This ensures that none of the property reads or writes in this file can ever target Object.prototype, eliminating the prototype pollution vector without altering legitimate configuration behavior.

In general, to fix prototype-polluting assignments, you must ensure that untrusted strings are never used directly as property names on objects that inherit from Object.prototype. This can be done either by (1) switching to a data structure that does not share Object.prototype (like Map or Object.create(null)), or (2) validating and rejecting dangerous property names such as __proto__, constructor, and prototype.

For this code, the least invasive fix that preserves existing behavior is to keep using plain objects but validate every path segment (part) and the final lastKey before using them as property names. If any segment is __proto__, constructor, or prototype, we should reject the path by throwing an error rather than creating or assigning into that property. This directly prevents writes to these special keys and eliminates the prototype pollution vector while keeping the config shape and JSON serialization intact.

Concretely, in packages/core/src/config/index.ts:

• Add a small helper (or inline checks) to validate keys.
• In the loop over parts in setConfigValue, before accessing current[part], check that part is not one of the forbidden keys. If it is, throw an error like Invalid config path segment.
• After computing lastKey, validate it similarly before using it in delete current[lastKey] or current[lastKey] = value.
• Optionally, apply the same validation in getConfigValue so reads cannot access those properties either; this keeps behavior consistent and avoids ever traversing through polluted prototypes.

No new imports are required; this can be implemented with simple string comparisons.

[claude]

Code Review

I found 3 issues that need attention:

---

1. Missing Test Coverage (CLAUDE.md Violation)

Location: Multiple files across the PR

This PR adds 777 lines of new production code across 7 files without any test coverage:

• apps/cli/src/commands/channels.ts (133 lines)
• apps/cli/src/commands/config.ts (128 lines)
• apps/cli/src/commands/gateway.ts (97 lines)
• apps/cli/src/commands/pairing.ts (150 lines)
• apps/cli/src/utils/client.ts (81 lines)
• apps/cli/src/utils/output.ts (91 lines)
• packages/core/src/config/index.ts (97 lines)

This violates the CLAUDE.md Testing guidelines:

Philosophy: Test-Driven Development

• Write tests before or alongside features - not as an afterthought
• Tests verify implementation matches specs
• No PR should regress existing functionality

Test Guidelines

1. Colocate tests: foo.ts → foo.test.ts

Recommendation: Add colocated test files (.test.ts) for the new modules, especially:

• Config path parsing and type conversion in config/index.ts
• WebSocket RPC client in utils/client.ts
• Gateway process spawning in commands/gateway.ts

---

2. Documentation Uses pnpm Instead of bun (CLAUDE.md Violation)

Location: docs/cli/overview.md:13-14

The documentation shows pnpm commands for development, but CLAUDE.md mandates using bun:

Development (use bun)

bun run --filter cli dev

Runtime

• Bun is the primary runtime for all development and execution
• Use bun commands instead of node or npx

Current code:

pnpm install
pnpm --filter cli build

Suggested fix:

pnpm install
bun run --filter cli build

---

3. Prototype Pollution Vulnerability in setConfigValue (Security)

Location: packages/core/src/config/index.ts:69-93

The setConfigValue function lacks validation against dangerous keys (__proto__, constructor, prototype), allowing prototype pollution via CLI:

finna config set __proto__.polluted true

This is inconsistent with the prototype pollution protection added to the file storage driver in this same PR (file.ts:1670-1676).

Recommended fix: Add the same FORBIDDEN_KEYS validation used in the file storage driver:

const FORBIDDEN_KEYS = ["__proto__", "constructor", "prototype"];

export async function setConfigValue(path: string, value: unknown): Promise<void> {
  const config = await getConfig();
  const parts = path.split(".");
  
  // Validate all parts against forbidden keys
  for (const part of parts) {
    if (FORBIDDEN_KEYS.includes(part)) {
      throw new Error(`Forbidden config key: ` + part);
    }
  }
  
  const lastKey = parts.pop();
  if (\!lastKey) throw new Error("Invalid config path");

  let current: Record<string, unknown> = config;
  for (const part of parts) {
    if (\!Object.prototype.hasOwnProperty.call(current, part) || typeof current[part] \!== "object") {
      current[part] = {};
    }
    current = current[part] as Record<string, unknown>;
  }

  current[lastKey] = value;
  await saveConfig(config);
  clearConfigCache();
}