DEV Community

Kas
Kas

Posted on

Code Review: GitHub Copilot CLI Extension -- 11 Issues Found and Fixed

#ai #codereview

Code Review: GitHub Copilot CLI Extension -- 11 Issues Found

Summary

This review covers the GitHub Copilot CLI extension codebase, focusing on the agent integration layer and API client modules. 11 issues were identified ranging from critical security vulnerabilities to lower-priority documentation gaps. All issues include before/after code examples.

Reviewed files: src/api/client.ts, src/commands/process.ts, src/config/loader.ts, src/utils/logger.ts, src/handlers/request.ts
Language: TypeScript (Node.js)

Issue 1: API Key Exposed in Log Output -- Severity: CRITICAL

Problem: The configuration object including the raw API key is serialised and written to the application log on startup. Any user with log access can extract the credential.

Before:

const config = {
  apiKey: "sk-copilot-abc123def456",
  endpoint: "https://api.github.com/copilot"
};
logger.info(`Starting with config: ${JSON.stringify(config)}`);
Enter fullscreen mode Exit fullscreen mode

After:

const config = {
  apiKey: process.env.GITHUB_COPILOT_API_KEY,
  endpoint: process.env.COPILOT_ENDPOINT ?? "https://api.github.com/copilot"
};
if (!config.apiKey) throw new Error("GITHUB_COPILOT_API_KEY env var not set");
logger.info(`Starting -- endpoint: ${config.endpoint}, key present: ${!!config.apiKey}`);
Enter fullscreen mode Exit fullscreen mode

Impact: Any log aggregation pipeline (Datadog, Splunk, CloudWatch) would ingest and potentially expose this key. Credential rotation is costly and disruptive.

Issue 2: No Input Validation on User Prompt -- Severity: HIGH

Problem: User-supplied prompt strings are passed directly to the API without length checking or sanitisation. Extremely long inputs trigger API 400 errors; control characters corrupt log output.

Before:

async function processPrompt(userInput: string) {
  return await callCopilotAPI(userInput);
}
Enter fullscreen mode Exit fullscreen mode

After:

const MAX_PROMPT_LENGTH = 4096;

async function processPrompt(userInput: string): Promise<string> {
  if (!userInput || userInput.trim().length === 0) {
    throw new Error("Prompt cannot be empty");
  }
  if (userInput.length > MAX_PROMPT_LENGTH) {
    throw new Error(`Prompt too long: ${userInput.length} chars (max ${MAX_PROMPT_LENGTH})`);
  }
  const sanitized = userInput.replace(/[--]/g, "");
  return await callCopilotAPI(sanitized);
}
Enter fullscreen mode Exit fullscreen mode

Issue 3: Missing Rate-Limit Handling -- Severity: HIGH

Problem: When the API returns HTTP 429, the code throws immediately instead of honouring the Retry-After header. This causes cascading failures under moderate load.

Before:

async function callAPI(prompt: string) {
  const response = await fetch(ENDPOINT, {
    method: "POST",
    body: JSON.stringify({ prompt }),
    headers: { Authorization: `Bearer ${API_KEY}` }
  });
  return response.json();
}
Enter fullscreen mode Exit fullscreen mode

After:

async function callAPI(prompt: string, retries = 3): Promise<unknown> {
  for (let attempt = 1; attempt <= retries; attempt++) {
    const response = await fetch(ENDPOINT, {
      method: "POST",
      body: JSON.stringify({ prompt }),
      headers: { Authorization: `Bearer ${API_KEY}` }
    });
    if (response.status === 429) {
      const wait = parseInt(response.headers.get("Retry-After") ?? "5", 10) * 1000;
      await new Promise(r => setTimeout(r, wait * attempt));
      continue;
    }
    if (!response.ok) throw new Error(`API error ${response.status}`);
    return response.json();
  }
  throw new Error(`API call failed after ${retries} retries`);
}
Enter fullscreen mode Exit fullscreen mode

Issue 4: Hardcoded Configuration Values -- Severity: MEDIUM

Problem: Timeout, max token count, and model ID are hardcoded as inline literals. Changing them requires a code release rather than a configuration update.

Before:

const response = await fetch(ENDPOINT, {
  signal: AbortSignal.timeout(30000),
  body: JSON.stringify({ prompt, max_tokens: 2048, model: "copilot-gpt-4" })
});
Enter fullscreen mode Exit fullscreen mode

After:

const TIMEOUT_MS = parseInt(process.env.COPILOT_TIMEOUT_MS ?? "30000", 10);
const MAX_TOKENS = parseInt(process.env.COPILOT_MAX_TOKENS  ?? "2048",  10);
const MODEL_ID   = process.env.COPILOT_MODEL_ID ?? "copilot-gpt-4";

const response = await fetch(ENDPOINT, {
  signal: AbortSignal.timeout(TIMEOUT_MS),
  body: JSON.stringify({ prompt, max_tokens: MAX_TOKENS, model: MODEL_ID })
});
Enter fullscreen mode Exit fullscreen mode

Issue 5: Unhandled Promise Rejections -- Severity: HIGH

Problem: The top-level request handler is async but has no try/catch. Unhandled rejections crash the Node.js process in production (Node 15+).

Before:

async function handleRequest(req: Request) {
  const result = await processPrompt(req.body.prompt);
  return { status: "ok", data: result };
}
Enter fullscreen mode Exit fullscreen mode

After:

async function handleRequest(req: Request): Promise<Response> {
  try {
    const result = await processPrompt(req.body.prompt);
    return { status: "ok", data: result };
  } catch (err) {
    logger.error("Request failed", { error: err, requestId: req.id });
    const message = err instanceof Error ? err.message : "Unexpected error";
    return { status: "error", message };
  }
}
Enter fullscreen mode Exit fullscreen mode

Issue 6: Missing TypeScript Type Annotations -- Severity: MEDIUM

Problem: Several core functions use implicit any types, defeating TypeScript's safety guarantees.

Before:

function buildPayload(messages, options) {
  return {
    messages: messages,
    max_tokens: options.maxTokens || 1000,
    temperature: options.temp
  };
}
Enter fullscreen mode Exit fullscreen mode

After:

interface Message { role: "user" | "assistant"; content: string; }
interface PayloadOptions { maxTokens?: number; temperature?: number; }
interface CopilotPayload { messages: Message[]; max_tokens: number; temperature: number; }

function buildPayload(messages: Message[], options: PayloadOptions): CopilotPayload {
  return {
    messages,
    max_tokens: options.maxTokens ?? 1000,
    temperature: options.temperature ?? 0.7
  };
}
Enter fullscreen mode Exit fullscreen mode

Issue 7: No Retry on Network Failure -- Severity: MEDIUM

Problem: Transient network errors throw immediately. A single flaky DNS or TCP timeout fails the entire request with no recovery.

Before:

async function fetchCompletion(prompt: string) {
  const resp = await axios.post(ENDPOINT, { prompt });
  return resp.data.choices[0].text;
}
Enter fullscreen mode Exit fullscreen mode

After:

async function fetchCompletion(prompt: string, maxRetries = 3): Promise<string> {
  let lastErr: Error | null = null;
  for (let i = 0; i < maxRetries; i++) {
    try {
      const resp = await axios.post(ENDPOINT, { prompt }, { timeout: TIMEOUT_MS });
      return resp.data.choices[0].text;
    } catch (err) {
      lastErr = err as Error;
      if (axios.isAxiosError(err) && err.response?.status && err.response.status < 500) break;
      await new Promise(r => setTimeout(r, 2 ** i * 500));
    }
  }
  throw lastErr ?? new Error("fetchCompletion: unknown failure");
}
Enter fullscreen mode Exit fullscreen mode

Issue 8: User Prompt Written to Log -- Severity: CRITICAL

Problem: The full user prompt is written to the debug log before processing. In regulated industries this is a data retention violation. It also inflates log storage costs.

Before:

logger.debug("Processing request", { userId, prompt: userPrompt, apiKey: config.apiKey });
Enter fullscreen mode Exit fullscreen mode

After:

logger.debug("Processing request", {
  userId,
  promptLength: userPrompt.length,
  promptPreview: userPrompt.slice(0, 20) + (userPrompt.length > 20 ? "..." : "")
  // Never log: full prompt, API keys, or PII
});
Enter fullscreen mode Exit fullscreen mode

Issue 9: Zero Unit Test Coverage -- Severity: MEDIUM

Problem: No test files exist. The package.json test script returns echo "no tests". Any refactor risks silent regressions.

Recommended fix -- add Jest and write critical-path tests:

// processPrompt.test.ts
import { processPrompt } from "../src/commands/process";

describe("processPrompt", () => {
  it("throws on empty prompt", async () => {
    await expect(processPrompt("")).rejects.toThrow("Prompt cannot be empty");
  });

  it("throws on oversized prompt", async () => {
    await expect(processPrompt("x".repeat(5000))).rejects.toThrow("Prompt too long");
  });

  it("strips control characters before sending", async () => {
    const spy = jest.fn().mockResolvedValue("ok");
    await processPrompt("helloworld", spy);
    expect(spy).toHaveBeenCalledWith("helloworld");
  });
});
Enter fullscreen mode Exit fullscreen mode

Issue 10: No JSDoc on Public API Functions -- Severity: LOW

Problem: Public functions exported from src/utils/ have no documentation. Consumers must read the implementation to understand arguments and return values.

Before:

export function truncate(text, maxLen) {
  return text.length > maxLen ? text.slice(0, maxLen) + "..." : text;
}
Enter fullscreen mode Exit fullscreen mode

After:

/**
 * Truncates a string to a maximum character length.
 * @param text   - The input string.
 * @param maxLen - Maximum characters to keep. Default 200.
 * @returns      The original string, or a truncated version ending with "..."
 */
export function truncate(text: string, maxLen = 200): string {
  return text.length > maxLen ? text.slice(0, maxLen) + "..." : text;
}
Enter fullscreen mode Exit fullscreen mode

Issue 11: Outdated Dependencies with Known CVEs -- Severity: HIGH

Problem: npm audit reports 3 high-severity vulnerabilities in pinned dependencies:

  • axios@0.26.0 -- SSRF vulnerability (CVE-2023-45857)
  • node-fetch@2.6.7 -- Header injection (CVE-2022-0235)
  • semver@5.7.1 -- ReDoS (CVE-2022-25883)

Fix -- update package.json and run npm audit fix:

{
  "dependencies": {
    "axios": "^1.6.0",
    "node-fetch": "^3.3.2"
  },
  "devDependencies": {
    "semver": "^7.5.4"
  }
}
Enter fullscreen mode Exit fullscreen mode

Then: npm install && npm audit to confirm zero high-severity findings.

Priority Matrix

Issue Severity Effort Impact
1 -- API key in logs CRITICAL Low Credential exposure
8 -- Prompt in logs CRITICAL Low Data / compliance breach
2 -- Input validation HIGH Low Crashes, injection
3 -- Rate limit handling HIGH Medium Production stability
5 -- Unhandled rejections HIGH Low Process crashes
11 -- CVE dependencies HIGH Low Known exploits
7 -- No network retry MEDIUM Medium Reliability
4 -- Hardcoded config MEDIUM Low Ops agility
6 -- Type annotations MEDIUM Medium Maintainability
9 -- Missing tests MEDIUM High Regression risk
10 -- Missing JSDoc LOW Medium Developer experience

Recommendations

  1. Fix Issues 1 and 8 immediately -- credential and prompt logging is a zero-cost fix with catastrophic downside if left in production.
  2. Issues 2, 5, and 11 can be resolved in a single PR in under one day.
  3. Issue 3 (rate limiting) should be addressed before the next traffic spike.
  4. Issue 9 (tests) is the highest-effort item but should be treated as ongoing -- start with the happy path and error cases for processPrompt.
  5. Adopt eslint-plugin-security to catch Issues 1 and 8 at lint time before they reach review.

Top comments (0)