From 59f2f19871114b94986fd63871bf6f5b219157b6 Mon Sep 17 00:00:00 2001 From: achebe Date: Mon, 31 Aug 2026 19:36:53 -0700 Subject: [PATCH] fix: stop sending the API key across redirects (sable-2s6p) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit requests' SessionRedirectMixin.rebuild_auth strips Authorization on a host change and leaves arbitrary custom headers intact; axios/follow-redirects does the same. So `x-api-key` rides a 302 to whatever host it points at. Nothing in this CLI needs to follow a redirect, so nothing does any more. PRE-EXISTING, not introduced by #220. The retry loop added there raises exposure from one transmission to as many as five, which is why it matters more today than it did last week, but it is not the cause. The action's curl paths were never affected — verified, no -L or --location on any of the eight curl invocations in github-action/action.yml. Every authenticated call site, not just the poll path: 14 in Node behind a shared `apiClient` (axios instance, maxRedirects: 0), 13 in Python behind api_get/api_post (allow_redirects forced False, so no call site can opt back in). Deliberately left alone: update-checker's npm registry call and the Slack/Discord webhook posts, none of which carry the key. WHAT THE SECURITY REVIEW CAUGHT, and it would have shipped a broken CLI: API/API_BASE end in "/" and 11 call sites concatenated "/static/...", building https://rafter.so/api//static/scan. Production answers that with a 308 to the single-slash form. It worked only because the client followed the redirect — so refusing redirects turned every core command into a hard failure. Verified against the live API: the double-slash URL 308s, the single-slash one reaches the endpoint. Both runtimes now build URLs through apiUrl()/api_url(), and a test in each fails on any `${API}/` or `{API_BASE}/` construction. Every other test mocks the transport, which is why nothing caught this. Also from that review: - The message told users to point --rafter-url at the final URL. That flag does not exist in the CLI — it is a GitHub Action input. Removed the instruction rather than shipping advice nobody can follow. - A redirect Location is attacker-controlled if the endpoint is. Header values cannot carry CR/LF but ESC is legal, so the raw value could rewrite the user's terminal. Both runtimes strip non-printables and cap at 200 chars, asserted with an ANSI sequence in the fixture. - The source-scanning guards only caught the most literal bypass. They now also match the .request() form and flag any second axios.create() / requests.Session() built outside the api utils. - The Node test shim made axios and apiClient the same mock, so a regression to bare axios would still have passed. create() now returns a distinct object and the tests watch that instance; mutation-tested by reverting one call site to bare axios, which the guard catches. A refused redirect now explains itself instead of surfacing a bare 302. --- node/src/commands/backend/get.ts | 1 - node/src/commands/backend/run.ts | 14 +- node/src/commands/backend/scan-status.ts | 8 +- node/src/commands/backend/usage.ts | 5 +- node/src/commands/issues/from-scan.ts | 5 +- node/src/commands/mcp/server.ts | 11 +- node/src/commands/notify.ts | 5 +- node/src/commands/sites/create.ts | 5 +- node/src/commands/sites/get.ts | 5 +- node/src/commands/sites/list.ts | 5 +- node/src/commands/sites/scan.ts | 5 +- node/src/utils/api.ts | 45 ++++++ node/tests/api-no-redirect.test.ts | 125 +++++++++++++++++ node/tests/mcp-sites.test.ts | 33 ++++- node/tests/plus-scan-approval.test.ts | 33 ++++- node/tests/scan-poll-transient-500.test.ts | 33 ++++- node/tests/scan-remote.test.ts | 33 ++++- node/tests/sites-cli.test.ts | 33 ++++- python/rafter_cli/commands/backend.py | 15 +- .../rafter_cli/commands/issues/issues_app.py | 6 +- python/rafter_cli/commands/mcp_server.py | 10 +- python/rafter_cli/commands/notify.py | 6 +- python/rafter_cli/commands/sites.py | 10 +- python/rafter_cli/utils/api.py | 67 +++++++++ python/tests/test_api_no_redirect.py | 130 ++++++++++++++++++ python/tests/test_api_scope.py | 16 +-- python/tests/test_plus_scan_approval.py | 4 +- python/tests/test_scan_poll_transient_500.py | 36 ++--- python/tests/test_scan_remote.py | 70 +++++----- python/tests/test_sites.py | 46 +++---- 30 files changed, 665 insertions(+), 155 deletions(-) create mode 100644 node/tests/api-no-redirect.test.ts create mode 100644 python/tests/test_api_no_redirect.py diff --git a/node/src/commands/backend/get.ts b/node/src/commands/backend/get.ts index 66f47469..40471d1a 100644 --- a/node/src/commands/backend/get.ts +++ b/node/src/commands/backend/get.ts @@ -1,5 +1,4 @@ import { Command } from "commander"; -import axios from "axios"; import { API, resolveKey, diff --git a/node/src/commands/backend/run.ts b/node/src/commands/backend/run.ts index 6fb73162..db46df9f 100644 --- a/node/src/commands/backend/run.ts +++ b/node/src/commands/backend/run.ts @@ -1,5 +1,4 @@ import { Command } from "commander"; -import axios from "axios"; import ora from "ora"; import { detectRepo } from "../../utils/git.js"; import { @@ -8,8 +7,9 @@ import { EXIT_GENERAL_ERROR, EXIT_QUOTA_EXHAUSTED, EXIT_CONFIRMATION_REQUIRED, - handle403 -} from "../../utils/api.js"; + handle403, + apiClient, + apiUrl} from "../../utils/api.js"; import { ConfigManager } from "../../core/config-manager.js"; import { loadPolicy } from "../../core/policy-loader.js"; import { askYesNo } from "../../utils/prompt.js"; @@ -133,8 +133,8 @@ export async function runRemoteScan(opts: RunOpts): Promise { if (!opts.quiet) { const spinner = ora("Submitting scan").start(); try { - const { data } = await axios.post( - `${API}/static/scan`, + const { data } = await apiClient.post( + apiUrl("static/scan"), body, { headers: { "x-api-key": key } } ); @@ -161,8 +161,8 @@ export async function runRemoteScan(opts: RunOpts): Promise { } } else { try { - const { data } = await axios.post( - `${API}/static/scan`, + const { data } = await apiClient.post( + apiUrl("static/scan"), body, { headers: { "x-api-key": key } } ); diff --git a/node/src/commands/backend/scan-status.ts b/node/src/commands/backend/scan-status.ts index 2aa1bb85..2d7090ac 100644 --- a/node/src/commands/backend/scan-status.ts +++ b/node/src/commands/backend/scan-status.ts @@ -1,12 +1,12 @@ -import axios from "axios"; import ora from "ora"; import { API, API_TIMEOUT_SHORT_MS, writePayload, EXIT_GENERAL_ERROR, - EXIT_SCAN_NOT_FOUND -} from "../../utils/api.js"; + EXIT_SCAN_NOT_FOUND, + apiClient, + apiUrl} from "../../utils/api.js"; import { fmt as output } from "../../utils/formatter.js"; /** @@ -175,7 +175,7 @@ async function pollUntilReadable( ): Promise { for (;;) { try { - const res = await axios.get(`${API}/static/scan`, { + const res = await apiClient.get(apiUrl("static/scan"), { params: { scan_id, format: fmt }, headers, // Without this a hung server stalls inside a single request, and the diff --git a/node/src/commands/backend/usage.ts b/node/src/commands/backend/usage.ts index 5a1c2c0e..3c18412a 100644 --- a/node/src/commands/backend/usage.ts +++ b/node/src/commands/backend/usage.ts @@ -1,6 +1,5 @@ import { Command } from "commander"; -import axios from "axios"; -import { API, resolveKey, EXIT_GENERAL_ERROR } from "../../utils/api.js"; +import { API, resolveKey, EXIT_GENERAL_ERROR, apiClient, apiUrl} from "../../utils/api.js"; export function createUsageCommand(): Command { return new Command("usage") @@ -8,7 +7,7 @@ export function createUsageCommand(): Command { .action(async (opts) => { const key = resolveKey(opts.apiKey); try { - const { data } = await axios.get(`${API}/static/usage`, { headers: { "x-api-key": key } }); + const { data } = await apiClient.get(apiUrl("static/usage"), { headers: { "x-api-key": key } }); console.log(JSON.stringify(data, null, 2)); } catch (e: any) { if (e.response?.data) { diff --git a/node/src/commands/issues/from-scan.ts b/node/src/commands/issues/from-scan.ts index 88d01575..38852bcb 100644 --- a/node/src/commands/issues/from-scan.ts +++ b/node/src/commands/issues/from-scan.ts @@ -7,8 +7,7 @@ */ import { Command } from "commander"; import fs from "fs"; -import axios from "axios"; -import { API, resolveKey, EXIT_GENERAL_ERROR } from "../../utils/api.js"; +import { API, resolveKey, EXIT_GENERAL_ERROR, apiClient, apiUrl} from "../../utils/api.js"; import { detectRepo } from "../../utils/git.js"; import { fmt } from "../../utils/formatter.js"; import { createIssue, listOpenIssues } from "./github-client.js"; @@ -170,7 +169,7 @@ async function draftsFromBackendScan( apiKey?: string ): Promise { const key = resolveKey(apiKey); - const { data } = await axios.get(`${API}/static/scan`, { + const { data } = await apiClient.get(apiUrl("static/scan"), { params: { scan_id: scanId, format: "json" }, headers: { "x-api-key": key }, }); diff --git a/node/src/commands/mcp/server.ts b/node/src/commands/mcp/server.ts index 821f0534..48ffd243 100644 --- a/node/src/commands/mcp/server.ts +++ b/node/src/commands/mcp/server.ts @@ -15,9 +15,8 @@ import { AuditLogger } from "../../core/audit-logger.js"; import { ConfigManager, redactConfigSecrets, isSecretConfigKey, maskSecretValue } from "../../core/config-manager.js"; import { listDocs, resolveDocSelector, fetchDoc } from "../../core/docs-loader.js"; import { writeSuppression } from "../../core/suppression-writer.js"; -import { apiUrl } from "../../utils/api.js"; +import { apiUrl, apiClient} from "../../utils/api.js"; import { describeSitesError, resolveMcpApiKey } from "../sites/errors.js"; -import axios from "axios"; import { createRequire } from "module"; const _require = createRequire(import.meta.url); @@ -361,7 +360,7 @@ export function createServer(): Server { const key = resolveMcpApiKey(); if (!key) return errorResult("No API key configured. Set RAFTER_API_KEY or run 'rafter agent config set backend.apiKey '."); try { - const { data } = await axios.post(apiUrl("static/sites"), { url }, { headers: { "x-api-key": key } }); + const { data } = await apiClient.post(apiUrl("static/sites"), { url }, { headers: { "x-api-key": key } }); return textResult(data); } catch (e: any) { return errorResult(describeSitesError(e).message); @@ -378,7 +377,7 @@ export function createServer(): Server { const body: Record = projectId ? { projectId } : { url }; if (Array.isArray(args?.sections)) body.sections = (args!.sections as unknown[]).map((s) => String(s)); try { - const { data } = await axios.post(apiUrl("static/sites/scan"), body, { headers: { "x-api-key": key } }); + const { data } = await apiClient.post(apiUrl("static/sites/scan"), body, { headers: { "x-api-key": key } }); return textResult(data); } catch (e: any) { return errorResult(describeSitesError(e).message); @@ -393,7 +392,7 @@ export function createServer(): Server { if (args?.offset !== undefined) params.offset = String(args.offset); if (args?.include_archived) params.include_archived = "true"; try { - const { data } = await axios.get(apiUrl("static/sites"), { params, headers: { "x-api-key": key } }); + const { data } = await apiClient.get(apiUrl("static/sites"), { params, headers: { "x-api-key": key } }); return textResult(data); } catch (e: any) { return errorResult(describeSitesError(e).message); @@ -406,7 +405,7 @@ export function createServer(): Server { const key = resolveMcpApiKey(); if (!key) return errorResult("No API key configured. Set RAFTER_API_KEY or run 'rafter agent config set backend.apiKey '."); try { - const { data } = await axios.get(apiUrl(`static/sites/${encodeURIComponent(id)}`), { headers: { "x-api-key": key } }); + const { data } = await apiClient.get(apiUrl(`static/sites/${encodeURIComponent(id)}`), { headers: { "x-api-key": key } }); return textResult(data); } catch (e: any) { return errorResult(describeSitesError(e).message); diff --git a/node/src/commands/notify.ts b/node/src/commands/notify.ts index 459f6597..782632eb 100644 --- a/node/src/commands/notify.ts +++ b/node/src/commands/notify.ts @@ -1,6 +1,5 @@ import { Command } from "commander"; -import axios from "axios"; -import { API, resolveKey, EXIT_GENERAL_ERROR, EXIT_SCAN_NOT_FOUND } from "../utils/api.js"; +import { API, resolveKey, EXIT_GENERAL_ERROR, EXIT_SCAN_NOT_FOUND, apiClient, apiUrl} from "../utils/api.js"; import { validateWebhookUrl } from "../core/audit-logger.js"; import { ConfigManager } from "../core/config-manager.js"; import { fmt, isAgentMode } from "../utils/formatter.js"; @@ -221,7 +220,7 @@ export function createNotifyCommand(): Command { if (scanId) { const key = resolveKey(opts?.apiKey as string | undefined); try { - const { data } = await axios.get(`${API}/static/scan`, { + const { data } = await apiClient.get(apiUrl("static/scan"), { params: { scan_id: scanId, format: "json" }, headers: { "x-api-key": key }, }); diff --git a/node/src/commands/sites/create.ts b/node/src/commands/sites/create.ts index dc35d4c8..d8b36f38 100644 --- a/node/src/commands/sites/create.ts +++ b/node/src/commands/sites/create.ts @@ -1,6 +1,5 @@ import { Command } from "commander"; -import axios from "axios"; -import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR } from "../../utils/api.js"; +import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR, apiClient} from "../../utils/api.js"; import { describeSitesError, rejectUnsupportedFormat } from "./errors.js"; export interface SitesCreateOpts { @@ -14,7 +13,7 @@ export async function runSitesCreate(url: string, opts: SitesCreateOpts): Promis if (rejectUnsupportedFormat(opts.format)) return EXIT_GENERAL_ERROR; const key = resolveKey(opts.apiKey); try { - const { data } = await axios.post( + const { data } = await apiClient.post( apiUrl("static/sites"), { url }, { headers: { "x-api-key": key } } diff --git a/node/src/commands/sites/get.ts b/node/src/commands/sites/get.ts index 1a163417..e002f209 100644 --- a/node/src/commands/sites/get.ts +++ b/node/src/commands/sites/get.ts @@ -1,6 +1,5 @@ import { Command } from "commander"; -import axios from "axios"; -import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR } from "../../utils/api.js"; +import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR, apiClient} from "../../utils/api.js"; import { describeSitesError, rejectUnsupportedFormat } from "./errors.js"; export interface SitesGetOpts { @@ -14,7 +13,7 @@ export async function runSitesGet(id: string, opts: SitesGetOpts): Promise { if (opts.includeArchived) params.include_archived = "true"; try { - const { data } = await axios.get( + const { data } = await apiClient.get( apiUrl("static/sites"), { params, headers: { "x-api-key": key } } ); diff --git a/node/src/commands/sites/scan.ts b/node/src/commands/sites/scan.ts index b584585b..b57e8c55 100644 --- a/node/src/commands/sites/scan.ts +++ b/node/src/commands/sites/scan.ts @@ -1,6 +1,5 @@ import { Command } from "commander"; -import axios from "axios"; -import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR } from "../../utils/api.js"; +import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR, apiClient} from "../../utils/api.js"; import { describeSitesError, rejectUnsupportedFormat } from "./errors.js"; const VALID_SECTIONS = new Set(["flight", "security", "dns"]); @@ -42,7 +41,7 @@ export async function runSitesScan(projectIdOrUrl: string, opts: SitesScanOpts): } try { - const { data } = await axios.post( + const { data } = await apiClient.post( apiUrl("static/sites/scan"), body, { headers: { "x-api-key": key } } diff --git a/node/src/utils/api.ts b/node/src/utils/api.ts index 740431bd..a95a6b48 100644 --- a/node/src/utils/api.ts +++ b/node/src/utils/api.ts @@ -1,7 +1,52 @@ +import axios from "axios"; import { ConfigManager } from "../core/config-manager.js"; export const API = "https://rafter.so/api/"; +/** + * sable-2s6p — the HTTP client for every authenticated Rafter API call. + * + * `maxRedirects: 0` is the point of it. axios (via follow-redirects) replays + * request headers on a redirect, and unlike `Authorization` the custom + * `x-api-key` header is not stripped when the host changes. Since the API base + * is user-settable (`--rafter-url`, self-hosted installs), a 302 from a + * misconfigured or hostile endpoint would walk the caller's API key to another + * host. Nothing in this CLI needs to follow a redirect, so none of them do. + * + * Use this for anything that sends `x-api-key`. Plain `axios` is fine for + * user-supplied webhooks and other unauthenticated calls. + */ +export const apiClient = axios.create({ + maxRedirects: 0, +}); + +/** + * A redirect target is attacker-controlled if the endpoint is. Header values + * cannot contain CR/LF, but ESC is a legal byte, so an unsanitized Location can + * emit ANSI sequences that rewrite the user's terminal. Strip anything + * non-printable and cap the length. + */ +function safeForTerminal(value: unknown): string { + if (typeof value !== "string") return ""; + // eslint-disable-next-line no-control-regex + const printable = value.replace(/[\u0000-\u001f\u007f-\u009f]/g, ""); + return printable.length > 200 ? `${printable.slice(0, 200)}…` : printable; +} + +// A refused redirect otherwise surfaces as a bare "Request failed with status +// code 302", which tells the user nothing about why. Name the cause. +apiClient.interceptors.response.use(undefined, (error: any) => { + const status = error?.response?.status; + if (status >= 300 && status < 400) { + const target = safeForTerminal(error?.response?.headers?.location) || "another host"; + error.message = + `The Rafter API redirected to ${target}, and Rafter does not follow redirects ` + + `on authenticated requests — your API key would be sent to the redirect target. ` + + `If you are pointing Rafter at a self-hosted instance, use its final URL.`; + } + return Promise.reject(error); +}); + /** Join API with a path segment without producing a double slash, regardless of leading/trailing slashes on either side. */ export function apiUrl(path: string): string { return `${API.replace(/\/+$/, "")}/${path.replace(/^\/+/, "")}`; diff --git a/node/tests/api-no-redirect.test.ts b/node/tests/api-no-redirect.test.ts new file mode 100644 index 00000000..f26d2ccd --- /dev/null +++ b/node/tests/api-no-redirect.test.ts @@ -0,0 +1,125 @@ +import { describe, it, expect } from "vitest"; +import { readFileSync, readdirSync, statSync } from "node:fs"; +import { join } from "node:path"; + +import { apiClient } from "../src/utils/api.js"; + +/** + * sable-2s6p — a custom `x-api-key` header is NOT stripped across a cross-host + * redirect the way `Authorization` is. axios replays it verbatim, and the API + * base is user-settable (`--rafter-url`, self-hosted installs), so a 302 from a + * misconfigured or hostile endpoint walks the caller's API key to another host. + * + * Nothing in this CLI needs to follow a redirect. These tests pin that, and — + * more importantly — pin that no NEW authenticated call site can reintroduce + * the hole by reaching for bare `axios`. + */ + +describe("apiClient (sable-2s6p)", () => { + it("refuses to follow redirects", () => { + expect(apiClient.defaults.maxRedirects).toBe(0); + }); + + it("explains why, instead of surfacing a bare 302", async () => { + // Drive the interceptor directly: it is the thing that turns an opaque + // status code into something a customer can act on. + const handlers = (apiClient.interceptors.response as any).handlers.filter(Boolean); + expect(handlers.length).toBeGreaterThan(0); + const onRejected = handlers[handlers.length - 1].rejected; + + const err: any = { + response: { + status: 302, + // ANSI escape included on purpose: an attacker-controlled Location must + // not be able to rewrite the user's terminal. + headers: { location: "https://evil.example/collect\u001b[31m" }, + }, + message: "Request failed with status code 302", + }; + + await expect(onRejected(err)).rejects.toBeDefined(); + expect(err.message).toContain("evil.example"); + expect(err.message).toContain("does not follow redirects"); + expect(err.message).not.toContain("\u001b"); + expect(err.message).toContain("self-hosted"); + }); +}); + +/** Every .ts file under a directory, recursively. */ +function sourceFiles(dir: string): string[] { + const out: string[] = []; + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) out.push(...sourceFiles(full)); + else if (entry.endsWith(".ts")) out.push(full); + } + return out; +} + +describe("no authenticated call bypasses apiClient (sable-2s6p)", () => { + it("creates no second axios instance outside the api utils", () => { + const offenders: string[] = []; + for (const file of sourceFiles("src")) { + if (file.endsWith("utils/api.ts")) continue; + const text = readFileSync(file, "utf8"); + if (/\baxios\.create\(/.test(text)) offenders.push(file); + } + expect( + offenders, + `A second axios instance can be created without maxRedirects: 0. Use ` + + `apiClient from src/utils/api.ts:\n${offenders.join("\n")}` + ).toEqual([]); + }); + + it("has no bare axios verb call that sends x-api-key", () => { + const offenders: string[] = []; + + for (const file of sourceFiles("src")) { + const text = readFileSync(file, "utf8"); + const lines = text.split("\n"); + lines.forEach((line, i) => { + if (!/\baxios(\.(get|post|put|delete|patch|request))?\(/.test(line)) return; + // Look at the call and the few lines after it — the header object is + // usually on a following line. + const window = lines.slice(i, i + 6).join("\n"); + if (window.includes("x-api-key")) { + offenders.push(`${file}:${i + 1}`); + } + }); + } + + expect( + offenders, + `These calls send the API key through bare axios, which follows redirects ` + + `across hosts. Use apiClient from src/utils/api.ts instead:\n${offenders.join("\n")}` + ).toEqual([]); + }); +}); + +describe("API URL construction (sable-2s6p)", () => { + it("builds no double slash after the scheme", () => { + // Not cosmetic. `API` ends in "/", and concatenating "/static/..." produced + // https://rafter.so/api//static/scan, which production answers with a 308. + // That worked only because the client followed redirects — so refusing + // them would have broken every core command. Caught by security review, + // not by any test, because every other test mocks the transport. + const offenders: string[] = []; + for (const file of sourceFiles("src")) { + const text = readFileSync(file, "utf8"); + text.split("\n").forEach((line, i) => { + if (/\$\{API\}\//.test(line)) offenders.push(`${file}:${i + 1}`); + }); + } + expect( + offenders, + `These build a double-slash URL. Use apiUrl() instead:\n${offenders.join("\n")}` + ).toEqual([]); + }); + + it("apiUrl joins cleanly regardless of slashes", async () => { + const { apiUrl, API } = await import("../src/utils/api.js"); + expect(apiUrl("static/scan")).toBe("https://rafter.so/api/static/scan"); + expect(apiUrl("/static/scan")).toBe("https://rafter.so/api/static/scan"); + expect(API.endsWith("/")).toBe(true); // the trap this guards against + }); +}); diff --git a/node/tests/mcp-sites.test.ts b/node/tests/mcp-sites.test.ts index 412359ac..5228d59d 100644 --- a/node/tests/mcp-sites.test.ts +++ b/node/tests/mcp-sites.test.ts @@ -9,7 +9,35 @@ import type { Server } from "@modelcontextprotocol/sdk/server/index.js"; * through an in-memory MCP client/server pair, with axios mocked. */ -vi.mock("axios"); +vi.mock("axios", () => { + // sable-2s6p — the code calls `apiClient`, an axios instance created with + // maxRedirects: 0. `create` must return something, and it returns the same + // object as the default export so `mockedAxios.get` still refers to the + // function under test. + const instance: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + defaults: { maxRedirects: 0 }, + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + }; + // The default export gets its OWN mocks, distinct from the instance's. If + // production code regresses to bare `axios.get`, the assertions below — which + // watch the instance — stop seeing calls, and the test fails. A shim where + // both are the same object would silently accept that regression. + const bare: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + create: () => instance, + }; + return { default: bare }; +}); vi.mock("../src/core/config-manager.js", async (importOriginal) => ({ ...(await importOriginal()), @@ -29,7 +57,8 @@ vi.mock("@modelcontextprotocol/sdk/server/stdio.js", () => ({ import axios from "axios"; import { createServer } from "../src/commands/mcp/server.js"; -const mockedAxios = vi.mocked(axios, true); +// The code calls `apiClient` — the instance `create()` returns. +const mockedAxios = vi.mocked((axios as any).create(), true); let client: Client; let server: Server; diff --git a/node/tests/plus-scan-approval.test.ts b/node/tests/plus-scan-approval.test.ts index ccb4721c..cff2d34f 100644 --- a/node/tests/plus-scan-approval.test.ts +++ b/node/tests/plus-scan-approval.test.ts @@ -40,7 +40,35 @@ vi.mock("../src/utils/prompt.js", () => ({ askYesNo: vi.fn(async () => state.promptAnswer), })); -vi.mock("axios"); +vi.mock("axios", () => { + // sable-2s6p — the code calls `apiClient`, an axios instance created with + // maxRedirects: 0. `create` must return something, and it returns the same + // object as the default export so `mockedAxios.get` still refers to the + // function under test. + const instance: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + defaults: { maxRedirects: 0 }, + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + }; + // The default export gets its OWN mocks, distinct from the instance's. If + // production code regresses to bare `axios.get`, the assertions below — which + // watch the instance — stop seeing calls, and the test fails. A shim where + // both are the same object would silently accept that regression. + const bare: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + create: () => instance, + }; + return { default: bare }; +}); vi.mock("ora", () => ({ default: () => ({ start: vi.fn().mockReturnThis(), @@ -58,7 +86,8 @@ import { } from "../src/commands/backend/run.js"; import { EXIT_CONFIRMATION_REQUIRED } from "../src/utils/api.js"; -const mockedAxios = vi.mocked(axios, true); +// The code calls `apiClient` — the instance `create()` returns. +const mockedAxios = vi.mocked((axios as any).create(), true); function resetState() { state.globalFlag = undefined; diff --git a/node/tests/scan-poll-transient-500.test.ts b/node/tests/scan-poll-transient-500.test.ts index b4bf1ba3..aade35f3 100644 --- a/node/tests/scan-poll-transient-500.test.ts +++ b/node/tests/scan-poll-transient-500.test.ts @@ -10,7 +10,35 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; * still fail, and the failure message is one a customer can act on. */ -vi.mock("axios"); +vi.mock("axios", () => { + // sable-2s6p — the code calls `apiClient`, an axios instance created with + // maxRedirects: 0. `create` must return something, and it returns the same + // object as the default export so `mockedAxios.get` still refers to the + // function under test. + const instance: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + defaults: { maxRedirects: 0 }, + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + }; + // The default export gets its OWN mocks, distinct from the instance's. If + // production code regresses to bare `axios.get`, the assertions below — which + // watch the instance — stop seeing calls, and the test fails. A shim where + // both are the same object would silently accept that regression. + const bare: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + create: () => instance, + }; + return { default: bare }; +}); vi.mock("ora", () => ({ default: () => ({ start: vi.fn().mockReturnThis(), @@ -32,7 +60,8 @@ import { } from "../src/commands/backend/scan-status.js"; import { EXIT_SUCCESS, EXIT_GENERAL_ERROR, EXIT_SCAN_NOT_FOUND } from "../src/utils/api.js"; -const mockedAxios = vi.mocked(axios, true); +// The code calls `apiClient` — the instance `create()` returns. +const mockedAxios = vi.mocked((axios as any).create(), true); /** The verbatim body the customer saw. */ const OBJECT_NOT_FOUND = { diff --git a/node/tests/scan-remote.test.ts b/node/tests/scan-remote.test.ts index c2eefbac..7d620a1c 100644 --- a/node/tests/scan-remote.test.ts +++ b/node/tests/scan-remote.test.ts @@ -12,7 +12,35 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; // ── Mocks ────────────────────────────────────────────────────────────── -vi.mock("axios"); +vi.mock("axios", () => { + // sable-2s6p — the code calls `apiClient`, an axios instance created with + // maxRedirects: 0. `create` must return something, and it returns the same + // object as the default export so `mockedAxios.get` still refers to the + // function under test. + const instance: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + defaults: { maxRedirects: 0 }, + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + }; + // The default export gets its OWN mocks, distinct from the instance's. If + // production code regresses to bare `axios.get`, the assertions below — which + // watch the instance — stop seeing calls, and the test fails. A shim where + // both are the same object would silently accept that regression. + const bare: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + create: () => instance, + }; + return { default: bare }; +}); vi.mock("ora", () => ({ default: () => ({ start: vi.fn().mockReturnThis(), @@ -26,7 +54,8 @@ import axios from "axios"; import { handleScanStatus } from "../src/commands/backend/scan-status.js"; import { EXIT_SUCCESS, EXIT_GENERAL_ERROR, EXIT_SCAN_NOT_FOUND } from "../src/utils/api.js"; -const mockedAxios = vi.mocked(axios, true); +// The code calls `apiClient` — the instance `create()` returns. +const mockedAxios = vi.mocked((axios as any).create(), true); // ── handleScanStatus ─────────────────────────────────────────────────── diff --git a/node/tests/sites-cli.test.ts b/node/tests/sites-cli.test.ts index 398bdece..05c26406 100644 --- a/node/tests/sites-cli.test.ts +++ b/node/tests/sites-cli.test.ts @@ -5,7 +5,35 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; * security monitoring). All tests mock axios so no network calls are made. */ -vi.mock("axios"); +vi.mock("axios", () => { + // sable-2s6p — the code calls `apiClient`, an axios instance created with + // maxRedirects: 0. `create` must return something, and it returns the same + // object as the default export so `mockedAxios.get` still refers to the + // function under test. + const instance: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + defaults: { maxRedirects: 0 }, + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + }; + // The default export gets its OWN mocks, distinct from the instance's. If + // production code regresses to bare `axios.get`, the assertions below — which + // watch the instance — stop seeing calls, and the test fails. A shim where + // both are the same object would silently accept that regression. + const bare: any = { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + interceptors: { response: { use: vi.fn() }, request: { use: vi.fn() } }, + create: () => instance, + }; + return { default: bare }; +}); import axios from "axios"; import { runSitesCreate } from "../src/commands/sites/create.js"; @@ -14,7 +42,8 @@ import { runSitesList } from "../src/commands/sites/list.js"; import { runSitesGet } from "../src/commands/sites/get.js"; import { EXIT_SUCCESS, EXIT_GENERAL_ERROR, EXIT_INSUFFICIENT_SCOPE, EXIT_SCAN_NOT_FOUND, EXIT_QUOTA_EXHAUSTED } from "../src/utils/api.js"; -const mockedAxios = vi.mocked(axios, true); +// The code calls `apiClient` — the instance `create()` returns. +const mockedAxios = vi.mocked((axios as any).create(), true); const opts = { apiKey: "test-key", format: "json", quiet: true }; beforeEach(() => { diff --git a/python/rafter_cli/commands/backend.py b/python/rafter_cli/commands/backend.py index 5e7314b1..90ead2f4 100644 --- a/python/rafter_cli/commands/backend.py +++ b/python/rafter_cli/commands/backend.py @@ -9,6 +9,9 @@ import typer from ..utils.api import ( + api_url, + api_get, + api_post, API_BASE, API_TIMEOUT, API_TIMEOUT_SHORT, @@ -254,8 +257,8 @@ def _poll_until_readable( """ while True: try: - resp = requests.get( - f"{API_BASE}/static/scan", + resp = api_get( + api_url("static/scan"), headers=headers, params={"scan_id": scan_id, "format": fmt}, timeout=API_TIMEOUT_SHORT, @@ -422,8 +425,8 @@ def _do_remote_scan( body["provider"] = resolved_provider body["repo_url"] = resolved_repo_url - resp = requests.post( - f"{API_BASE}/static/scan", + resp = api_post( + api_url("static/scan"), headers=headers, json=body, timeout=API_TIMEOUT, @@ -508,8 +511,8 @@ def usage( """Check quota and usage.""" key = resolve_key(api_key) headers = {"x-api-key": key} - resp = requests.get( - f"{API_BASE}/static/usage", headers=headers, timeout=API_TIMEOUT_SHORT + resp = api_get( + api_url("static/usage"), headers=headers, timeout=API_TIMEOUT_SHORT ) if resp.status_code != 200: print(f"Error: {resp.text}", file=sys.stderr) diff --git a/python/rafter_cli/commands/issues/issues_app.py b/python/rafter_cli/commands/issues/issues_app.py index 638a9177..ec198c47 100644 --- a/python/rafter_cli/commands/issues/issues_app.py +++ b/python/rafter_cli/commands/issues/issues_app.py @@ -14,7 +14,7 @@ import requests import typer -from ...utils.api import API_BASE, EXIT_GENERAL_ERROR, EXIT_SUCCESS, resolve_key +from ...utils.api import api_url, API_BASE, api_get, EXIT_GENERAL_ERROR, EXIT_SUCCESS, resolve_key from ...utils.formatter import fmt, print_stderr from ...utils.git import detect_repo from .dedup import find_duplicates @@ -213,8 +213,8 @@ def from_text( def _drafts_from_backend(scan_id: str, api_key: str | None) -> list[IssueDraft]: key = resolve_key(api_key) - resp = requests.get( - f"{API_BASE}/static/scan", + resp = api_get( + api_url("static/scan"), headers={"x-api-key": key}, params={"scan_id": scan_id, "format": "json"}, timeout=(10, 60), diff --git a/python/rafter_cli/commands/mcp_server.py b/python/rafter_cli/commands/mcp_server.py index 0982f4c1..9ba7fb35 100644 --- a/python/rafter_cli/commands/mcp_server.py +++ b/python/rafter_cli/commands/mcp_server.py @@ -22,7 +22,7 @@ from ..scanners.betterleaks import BetterleaksScanner from ..scanners.regex_scanner import RegexScanner, ScanResult from ..scanners.union import union_scan_results -from ..utils.api import API_TIMEOUT +from ..utils.api import API_TIMEOUT, api_get, api_post from .sites import SITES_API_BASE, describe_sites_error, resolve_mcp_api_key mcp_app = typer.Typer( @@ -218,7 +218,7 @@ def _require_mcp_api_key() -> str: def handle_sites_create(url: str) -> dict: """Register a URL as a Rafter Site and kick off its first scan.""" key = _require_mcp_api_key() - resp = requests.post(SITES_API_BASE, headers={"x-api-key": key}, json={"url": url}, timeout=API_TIMEOUT) + resp = api_post(SITES_API_BASE, headers={"x-api-key": key}, json={"url": url}, timeout=API_TIMEOUT) if resp.status_code != 200: message, _ = describe_sites_error(resp) raise RuntimeError(message) @@ -241,7 +241,7 @@ def handle_sites_scan( if sections: body["sections"] = list(sections) - resp = requests.post(f"{SITES_API_BASE}/scan", headers={"x-api-key": key}, json=body, timeout=API_TIMEOUT) + resp = api_post(f"{SITES_API_BASE}/scan", headers={"x-api-key": key}, json=body, timeout=API_TIMEOUT) if resp.status_code != 200: message, _ = describe_sites_error(resp) raise RuntimeError(message) @@ -263,7 +263,7 @@ def handle_sites_list( if include_archived: params["include_archived"] = "true" - resp = requests.get(SITES_API_BASE, headers={"x-api-key": key}, params=params, timeout=API_TIMEOUT) + resp = api_get(SITES_API_BASE, headers={"x-api-key": key}, params=params, timeout=API_TIMEOUT) if resp.status_code != 200: message, _ = describe_sites_error(resp) raise RuntimeError(message) @@ -275,7 +275,7 @@ def handle_sites_get(id: str) -> dict: from urllib.parse import quote key = _require_mcp_api_key() - resp = requests.get(f"{SITES_API_BASE}/{quote(id, safe='')}", headers={"x-api-key": key}, timeout=API_TIMEOUT) + resp = api_get(f"{SITES_API_BASE}/{quote(id, safe='')}", headers={"x-api-key": key}, timeout=API_TIMEOUT) if resp.status_code != 200: message, _ = describe_sites_error(resp) raise RuntimeError(message) diff --git a/python/rafter_cli/commands/notify.py b/python/rafter_cli/commands/notify.py index 6e7e684e..e760279b 100644 --- a/python/rafter_cli/commands/notify.py +++ b/python/rafter_cli/commands/notify.py @@ -10,6 +10,8 @@ import typer from ..utils.api import ( + api_url, + api_get, API_BASE, API_TIMEOUT_SHORT, EXIT_GENERAL_ERROR, @@ -309,8 +311,8 @@ def _fetch_scan(scan_id: str, api_key: str) -> dict: import requests headers = {"x-api-key": api_key} - resp = requests.get( - f"{API_BASE}/static/scan", + resp = api_get( + api_url("static/scan"), headers=headers, params={"scan_id": scan_id, "format": "json"}, timeout=API_TIMEOUT_SHORT, diff --git a/python/rafter_cli/commands/sites.py b/python/rafter_cli/commands/sites.py index 5ecef8d4..f36e90b3 100644 --- a/python/rafter_cli/commands/sites.py +++ b/python/rafter_cli/commands/sites.py @@ -18,6 +18,8 @@ import typer from ..utils.api import ( + api_get, + api_post, API_BASE, API_TIMEOUT, EXIT_GENERAL_ERROR, @@ -135,7 +137,7 @@ def sites_create( if reject_unsupported_format(fmt): raise typer.Exit(code=EXIT_GENERAL_ERROR) key = resolve_key(api_key) - resp = requests.post( + resp = api_post( SITES_API_BASE, headers={"x-api-key": key}, json={"url": url}, @@ -174,7 +176,7 @@ def sites_scan( body["sections"] = section_list key = resolve_key(api_key) - resp = requests.post( + resp = api_post( f"{SITES_API_BASE}/scan", headers={"x-api-key": key}, json=body, @@ -209,7 +211,7 @@ def sites_list( params["include_archived"] = "true" key = resolve_key(api_key) - resp = requests.get( + resp = api_get( SITES_API_BASE, headers={"x-api-key": key}, params=params, @@ -234,7 +236,7 @@ def sites_get( raise typer.Exit(code=EXIT_GENERAL_ERROR) key = resolve_key(api_key) site_id = quote(id, safe="") - resp = requests.get( + resp = api_get( f"{SITES_API_BASE}/{site_id}", headers={"x-api-key": key}, timeout=API_TIMEOUT, diff --git a/python/rafter_cli/utils/api.py b/python/rafter_cli/utils/api.py index 10d7ce7d..ee4ccb36 100644 --- a/python/rafter_cli/utils/api.py +++ b/python/rafter_cli/utils/api.py @@ -5,6 +5,7 @@ import os import sys +import requests import typer from dotenv import load_dotenv @@ -62,6 +63,72 @@ def handle_scope_error(resp: "requests.Response") -> bool: API_TIMEOUT_SHORT = (10, 30) +def _safe_for_terminal(value: "str | None") -> str: + """Strip non-printable bytes and cap length before echoing untrusted text. + + A redirect target is attacker-controlled if the endpoint is. Header values + cannot contain CR/LF, but ESC is a legal byte, so an unsanitized Location + can emit ANSI sequences that rewrite the user's terminal. + """ + if not isinstance(value, str): + return "" + printable = "".join(c for c in value if c.isprintable()) + return printable[:200] + "\u2026" if len(printable) > 200 else printable + + +def api_url(path: str) -> str: + """Join API_BASE with a path without producing a double slash. + + Mirrors Node's ``apiUrl()``. This is not cosmetic: API_BASE ends in "/" and + call sites used to concatenate "/static/...", producing + ``https://rafter.so/api//static/scan``, which production answers with a 308 + to the single-slash form. That worked only because the client followed the + redirect — so sable-2s6p's fix would have broken every core command. + """ + return f"{API_BASE.rstrip('/')}/{path.lstrip('/')}" + + +def api_request(method: str, url: str, **kwargs) -> "requests.Response": + """sable-2s6p — the HTTP entry point for every authenticated Rafter API call. + + ``allow_redirects=False`` is the point of it. ``requests`` replays headers on + a redirect and, unlike ``Authorization``, a custom ``x-api-key`` header is + NOT stripped when the host changes (``SessionRedirectMixin.rebuild_auth`` + only handles ``Authorization``). Since the API base is user-settable + (``--rafter-url``, self-hosted installs), a 302 from a misconfigured or + hostile endpoint would walk the caller's API key to another host. + + Nothing in this CLI needs to follow a redirect, so none of them do. A + redirect now arrives at the caller as a plain 3xx response, which every + caller already treats as a non-200 error. + + Use this for anything that sends ``x-api-key``. Plain ``requests`` is fine + for user-supplied webhooks and other unauthenticated calls. + """ + kwargs["allow_redirects"] = False + resp = requests.request(method, url, **kwargs) + if 300 <= resp.status_code < 400: + # Otherwise this surfaces as a bare non-200 with an empty body, which + # tells the user nothing about why. + target = _safe_for_terminal(resp.headers.get("location")) or "another host" + print( + f"The Rafter API redirected to {target}, and Rafter does not follow " + "redirects on authenticated requests — your API key would be sent to " + "the redirect target. If you are pointing Rafter at a self-hosted " + "instance, use its final URL.", + file=sys.stderr, + ) + return resp + + +def api_get(url: str, **kwargs) -> "requests.Response": + return api_request("GET", url, **kwargs) + + +def api_post(url: str, **kwargs) -> "requests.Response": + return api_request("POST", url, **kwargs) + + def resolve_key(cli_opt: str | None) -> str: """Resolve API key: --api-key flag > RAFTER_API_KEY env > global config.""" if cli_opt: diff --git a/python/tests/test_api_no_redirect.py b/python/tests/test_api_no_redirect.py new file mode 100644 index 00000000..f8c7a3fe --- /dev/null +++ b/python/tests/test_api_no_redirect.py @@ -0,0 +1,130 @@ +"""sable-2s6p — authenticated Rafter calls must not follow redirects. + +``requests.sessions.SessionRedirectMixin.rebuild_auth`` strips ``Authorization`` +on a host change and leaves arbitrary custom headers intact, so ``x-api-key`` +rides a 302 to whatever host it points at. The API base is user-settable +(``--rafter-url``, self-hosted installs), which makes that a real exfiltration +path rather than a theoretical one. + +Mirrors node/tests/api-no-redirect.test.ts. +""" +from __future__ import annotations + +import pathlib +import re +from unittest.mock import MagicMock, patch + +from rafter_cli.utils.api import api_get, api_post, api_request + +REPO_PY = pathlib.Path(__file__).resolve().parents[1] / "rafter_cli" + + +def _resp(status_code: int = 200, headers=None) -> MagicMock: + resp = MagicMock() + resp.status_code = status_code + resp.headers = headers or {} + return resp + + +class TestRedirectsRefused: + def test_api_request_forces_allow_redirects_false(self): + with patch("rafter_cli.utils.api.requests.request") as req: + req.return_value = _resp() + api_request("GET", "https://rafter.so/api/static/scan") + + assert req.call_args.kwargs["allow_redirects"] is False + + def test_callers_cannot_re_enable_redirects(self): + # Even an explicit allow_redirects=True is overridden — the point is + # that no call site can opt back into leaking the key. + with patch("rafter_cli.utils.api.requests.request") as req: + req.return_value = _resp() + api_request("GET", "https://rafter.so/x", allow_redirects=True) + + assert req.call_args.kwargs["allow_redirects"] is False + + def test_api_get_and_api_post_use_the_right_verbs(self): + with patch("rafter_cli.utils.api.requests.request") as req: + req.return_value = _resp() + api_get("https://rafter.so/x") + api_post("https://rafter.so/y") + + assert [c.args[0] for c in req.call_args_list] == ["GET", "POST"] + assert all(c.kwargs["allow_redirects"] is False for c in req.call_args_list) + + def test_a_refused_redirect_explains_itself(self, capsys): + with patch("rafter_cli.utils.api.requests.request") as req: + # ANSI escape included on purpose: an attacker-controlled Location + # must not be able to rewrite the user's terminal. + req.return_value = _resp( + 302, {"location": "https://evil.example/collect\x1b[31m"} + ) + api_get("https://rafter.so/api/static/scan") + + err = capsys.readouterr().err + assert "evil.example" in err + assert "does not follow redirects" in err + assert "self-hosted" in err + assert "\x1b" not in err + + +class TestNoCallSiteBypassesTheHelper: + """A new authenticated call site must not be able to reintroduce the hole.""" + + def test_no_bare_requests_call_sends_the_api_key(self): + offenders = [] + call = re.compile(r"\brequests\.(get|post|put|delete|patch|request)\(") + + for path in REPO_PY.rglob("*.py"): + if path.name == "api.py" and path.parent.name == "utils": + continue # the helper itself is where requests is allowed + lines = path.read_text().splitlines() + for i, line in enumerate(lines): + if not call.search(line): + continue + window = "\n".join(lines[i : i + 6]) + if "x-api-key" in window or "headers=headers" in window: + offenders.append(f"{path}:{i + 1}") + + assert offenders == [], ( + "These calls send the API key through bare requests, which replays it " + "across a cross-host redirect. Use api_get/api_post from " + f"rafter_cli.utils.api instead:\n" + "\n".join(offenders) + ) + + +class TestNoSecondSession: + def test_no_module_builds_its_own_requests_session(self): + offenders = [ + str(p) + for p in REPO_PY.rglob("*.py") + if not (p.name == "api.py" and p.parent.name == "utils") + and "requests.Session(" in p.read_text() + ] + assert offenders == [], ( + "A bare Session follows redirects by default. Use api_get/api_post " + "from rafter_cli.utils.api:\n" + "\n".join(offenders) + ) + + +class TestApiUrlConstruction: + def test_no_module_builds_a_double_slash_url(self): + # Not cosmetic. API_BASE ends in "/", and f"{API_BASE}/static/..." + # produced https://rafter.so/api//static/scan, which production answers + # with a 308. That worked only because the client followed redirects. + offenders = [] + for path in REPO_PY.rglob("*.py"): + for i, line in enumerate(path.read_text().splitlines()): + if "{API_BASE}/" in line: + offenders.append(f"{path}:{i + 1}") + assert offenders == [], ( + "These build a double-slash URL. Use api_url() instead:\n" + + "\n".join(offenders) + ) + + def test_api_url_joins_cleanly(self): + from rafter_cli.utils.api import API_BASE, api_url + + assert api_url("static/scan") == "https://rafter.so/api/static/scan" + assert api_url("/static/scan") == "https://rafter.so/api/static/scan" + assert API_BASE.endswith("/") # the trap this guards against diff --git a/python/tests/test_api_scope.py b/python/tests/test_api_scope.py index 6089745d..8e143543 100644 --- a/python/tests/test_api_scope.py +++ b/python/tests/test_api_scope.py @@ -101,7 +101,7 @@ def test_no_collisions(self): class TestRemoteScan403: """Verify _do_remote_scan properly handles 403 scope errors.""" - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("org/repo", "main", "github", "https://github.com/org/repo")) def test_scope_403_raises_exit_with_code_4(self, _mock_repo, mock_post, capsys): mock_post.return_value = _mock_response( @@ -124,7 +124,7 @@ def test_scope_403_raises_exit_with_code_4(self, _mock_repo, mock_post, capsys): assert "read access" in err assert "https://rfrr.co/account" in err - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("org/repo", "main", "github", "https://github.com/org/repo")) def test_generic_403_raises_exit_with_code_4(self, _mock_repo, mock_post, capsys): mock_post.return_value = _mock_response(403, "forbidden") @@ -141,7 +141,7 @@ def test_generic_403_raises_exit_with_code_4(self, _mock_repo, mock_post, capsys ) assert exc_info.value.exit_code == EXIT_INSUFFICIENT_SCOPE - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("org/repo", "main", "github", "https://github.com/org/repo")) def test_429_still_raises_quota_exhausted(self, _mock_repo, mock_post, capsys): mock_post.return_value = _mock_response(429, "quota exhausted") @@ -158,7 +158,7 @@ def test_429_still_raises_quota_exhausted(self, _mock_repo, mock_post, capsys): ) assert exc_info.value.exit_code == EXIT_QUOTA_EXHAUSTED - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("org/repo", "main", "github", "https://github.com/org/repo")) def test_200_succeeds(self, _mock_repo, mock_post): mock_post.return_value = _mock_response(200, "") @@ -184,7 +184,7 @@ class TestReadOnlyEndpoints: The server returns 200 for valid keys of either scope on GET endpoints. These tests verify the CLI doesn't accidentally scope-check GET calls.""" - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_get_scan_200_with_read_key(self, mock_get): """GET /api/static/scan works fine — no scope check needed.""" mock_get.return_value = _mock_response(200, "") @@ -198,7 +198,7 @@ def test_get_scan_200_with_read_key(self, mock_get): result = _handle_scan_status_interactive("abc", {"x-api-key": "read_key"}, "json", True) assert result == 0 - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_get_usage_200_with_read_key(self, mock_get, capsys): """GET /api/static/usage works with read-only key.""" mock_get.return_value = _mock_response(200, "") @@ -217,7 +217,7 @@ def test_get_usage_200_with_read_key(self, mock_get, capsys): class TestBackwardCompatibility: """Ensure existing 401 and other error paths are unaffected.""" - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("org/repo", "main", "github", "https://github.com/org/repo")) def test_401_raises_general_error(self, _mock_repo, mock_post, capsys): mock_post.return_value = _mock_response(401, "invalid api key") @@ -235,7 +235,7 @@ def test_401_raises_general_error(self, _mock_repo, mock_post, capsys): # 401 should NOT hit scope handler, falls through to general error assert exc_info.value.exit_code == EXIT_GENERAL_ERROR - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("org/repo", "main", "github", "https://github.com/org/repo")) def test_500_raises_general_error(self, _mock_repo, mock_post, capsys): mock_post.return_value = _mock_response(500, "internal server error") diff --git a/python/tests/test_plus_scan_approval.py b/python/tests/test_plus_scan_approval.py index 1046aeae..809dd58c 100644 --- a/python/tests/test_plus_scan_approval.py +++ b/python/tests/test_plus_scan_approval.py @@ -149,7 +149,7 @@ def test_refuses_exit_5_when_prompt_answered_no(self, monkeypatch): class TestGateIntegration: - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") def test_refuses_gated_plus_without_calling_backend(self, mock_post, monkeypatch): monkeypatch.delenv("RAFTER_CONFIRM", raising=False) with patch( @@ -168,7 +168,7 @@ def test_refuses_gated_plus_without_calling_backend(self, mock_post, monkeypatch assert exc.value.exit_code == EXIT_CONFIRMATION_REQUIRED mock_post.assert_not_called() - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch( "rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", None), diff --git a/python/tests/test_scan_poll_transient_500.py b/python/tests/test_scan_poll_transient_500.py index 3d7299a5..659466f9 100644 --- a/python/tests/test_scan_poll_transient_500.py +++ b/python/tests/test_scan_poll_transient_500.py @@ -74,7 +74,7 @@ def test_backoff_is_exponential_not_flat(self): assert [backoff_seconds(n) for n in (1, 2, 3, 4)] == [2, 4, 8, 16] def test_actually_sleeps_the_backoff_schedule(self, sleeps): - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [_processing()] + [ _server_500() for _ in range(MAX_TRANSIENT_POLL_FAILURES) ] @@ -93,7 +93,7 @@ def test_caps_total_failures_so_a_flapping_server_cannot_loop_forever(self): for _ in range(MAX_TOTAL_TRANSIENT_POLL_FAILURES + 5): flapping += [_server_500(), _processing()] - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = flapping with pytest.raises(typer.Exit) as exc: _handle_scan_status_interactive("s1", HEADERS, "md", quiet=True) @@ -102,7 +102,7 @@ def test_caps_total_failures_so_a_flapping_server_cannot_loop_forever(self): def test_consecutive_counter_resets_on_a_successful_poll(self, sleeps): # Two blips far apart must NOT add up to a give-up. - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [ _processing(), _server_500(), @@ -123,7 +123,7 @@ def test_survives_a_nested_json_error_object(self): # methods on that object raised an AttributeError no caller catches, # surfacing as a traceback and defeating the retry entirely. nested = _resp(500, text=json.dumps({"error": {"message": "nested"}})) - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [_processing(), nested, _completed()] code = _handle_scan_status_interactive("s1", HEADERS, "md", quiet=True) @@ -132,7 +132,7 @@ def test_survives_a_nested_json_error_object(self): def test_truncates_a_very_long_server_error(self, capsys): huge = "x" * 5000 big = _resp(500, text=json.dumps({"error": huge})) - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [_processing()] + [ big for _ in range(MAX_TRANSIENT_POLL_FAILURES) ] @@ -148,7 +148,7 @@ def test_reports_the_real_attempt_count_not_the_consecutive_cap(self, capsys): for _ in range(MAX_TOTAL_TRANSIENT_POLL_FAILURES + 5): flapping += [_server_500(), _processing()] - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = flapping with pytest.raises(typer.Exit): _handle_scan_status_interactive("s1", HEADERS, "md", quiet=True) @@ -158,7 +158,7 @@ def test_reports_the_real_attempt_count_not_the_consecutive_cap(self, capsys): assert f"after {MAX_TOTAL_TRANSIENT_POLL_FAILURES} attempts" in err def test_unreachable_api_is_not_blamed_on_the_report(self, capsys): - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [_processing()] + [ requests.ConnectionError("no route to host") for _ in range(MAX_TRANSIENT_POLL_FAILURES) @@ -174,7 +174,7 @@ def test_first_poll_retries_a_transient_500(self): # The give-up message tells the user to run `rafter get `, which # re-enters at the first poll. If that path did not retry, the remedy # we recommend would be defeated by one bad read. - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [_server_500(), _completed()] code = _handle_scan_status_interactive("s1", HEADERS, "md", quiet=True) @@ -184,14 +184,14 @@ def test_first_poll_retries_a_transient_500(self): def test_any_2xx_counts_as_success(self): # Node's axios accepts any 2xx; Python must not diverge. accepted = _resp(202, json_body={"status": "completed", "markdown": "# Done"}) - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [accepted] code = _handle_scan_status_interactive("s1", HEADERS, "md", quiet=True) assert code == EXIT_SUCCESS def test_retry_notice_goes_to_stderr(self, capsys): - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [_processing(), _server_500(), _completed()] _handle_scan_status_interactive("s1", HEADERS, "md", quiet=False) @@ -200,7 +200,7 @@ def test_retry_notice_goes_to_stderr(self, capsys): assert "retrying in 2s" in err def test_rides_out_a_single_500_and_completes(self, capsys): - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [_processing(), _server_500(), _completed()] code = _handle_scan_status_interactive("s1", HEADERS, "md", quiet=True) @@ -208,7 +208,7 @@ def test_rides_out_a_single_500_and_completes(self, capsys): assert get.call_count == 3 def test_rides_out_several_consecutive_500s(self): - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [ _processing(), _server_500(), @@ -222,14 +222,14 @@ def test_rides_out_several_consecutive_500s(self): assert get.call_count == 5 def test_midpoll_404_is_lag_not_a_missing_scan(self): - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [_processing(), _resp(404, text="{}"), _completed()] code = _handle_scan_status_interactive("s1", HEADERS, "md", quiet=True) assert code == EXIT_SUCCESS def test_first_poll_404_still_reports_not_found(self): - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [_resp(404, text="{}")] with pytest.raises(typer.Exit) as exc: _handle_scan_status_interactive("nope", HEADERS, "md", quiet=True) @@ -238,7 +238,7 @@ def test_first_poll_404_still_reports_not_found(self): assert get.call_count == 1 def test_gives_up_when_report_never_becomes_readable(self, capsys): - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [_processing()] + [ _server_500() for _ in range(MAX_TRANSIENT_POLL_FAILURES) ] @@ -253,7 +253,7 @@ def test_gives_up_when_report_never_becomes_readable(self, capsys): assert "Object not found" in err def test_does_not_retry_a_non_transient_error(self): - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [ _processing(), _resp(403, text=json.dumps({"error": "Invalid API key"})), @@ -265,7 +265,7 @@ def test_does_not_retry_a_non_transient_error(self): assert get.call_count == 2 def test_retries_transport_errors(self): - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [ _processing(), requests.ConnectionError("ECONNRESET"), @@ -279,7 +279,7 @@ def test_a_500_body_is_never_mistaken_for_a_report(self): """The pre-fix bug: the loop called .json() on the 500 body, got no status, fell out of the loop and wrote the error payload out as if it were results.""" - with patch("rafter_cli.commands.backend.requests.get") as get: + with patch("rafter_cli.commands.backend.api_get") as get: get.side_effect = [_processing()] + [ _server_500() for _ in range(MAX_TRANSIENT_POLL_FAILURES) ] diff --git a/python/tests/test_scan_remote.py b/python/tests/test_scan_remote.py index 1c5091fb..4acf0198 100644 --- a/python/tests/test_scan_remote.py +++ b/python/tests/test_scan_remote.py @@ -44,7 +44,7 @@ def _mock_response(status_code: int, text: str = "", json_body=None) -> MagicMoc class TestDoRemoteScan: """Unit tests for the core remote scan trigger function.""" - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_success_skip_interactive(self, _mock_repo, mock_post): """200 with skip_interactive returns without polling.""" @@ -60,7 +60,7 @@ def test_success_skip_interactive(self, _mock_repo, mock_post): quiet=True, ) - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_posts_correct_body(self, _mock_repo, mock_post): """Verify POST body contains repository_name, branch_name, scan_mode.""" @@ -83,7 +83,7 @@ def test_posts_correct_body(self, _mock_repo, mock_post): assert body["scan_mode"] == "fast" assert "github_token" not in body - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_github_body_omits_provider_and_repo_url(self, _mock_repo, mock_post): """A github remote produces a body with NO provider/repo_url (byte-identical to today).""" @@ -110,7 +110,7 @@ def test_github_body_omits_provider_and_repo_url(self, _mock_repo, mock_post): assert "provider" not in body assert "repo_url" not in body - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_explicit_provider_github_still_omits(self, _mock_repo, mock_post): """An explicit --provider github still omits provider/repo_url.""" @@ -132,7 +132,7 @@ def test_explicit_provider_github_still_omits(self, _mock_repo, mock_post): assert "provider" not in body assert "repo_url" not in body - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_flag_provider_gitlab_sends_provider_and_repo_url(self, _mock_repo, mock_post): """--provider gitlab + --repo-url are sent for a non-github remote.""" @@ -154,7 +154,7 @@ def test_flag_provider_gitlab_sends_provider_and_repo_url(self, _mock_repo, mock assert body["provider"] == "gitlab" assert body["repo_url"] == "https://gitlab.com/group/project" - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_flag_provider_bitbucket_sends_pair(self, _mock_repo, mock_post): """--provider bitbucket + --repo-url are sent together.""" @@ -176,7 +176,7 @@ def test_flag_provider_bitbucket_sends_pair(self, _mock_repo, mock_post): assert body["provider"] == "bitbucket" assert body["repo_url"] == "https://bitbucket.org/team/repo" - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("group/project", "main", None, None)) def test_provider_without_any_repo_url_is_omitted(self, _mock_repo, mock_post): """A resolved provider with no repo_url anywhere can't send the pair; stays backward-compatible.""" @@ -200,7 +200,7 @@ def test_provider_without_any_repo_url_is_omitted(self, _mock_repo, mock_post): assert "provider" not in body assert "repo_url" not in body - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch( "rafter_cli.commands.backend.detect_repo", return_value=("group/project", "main", "gitlab", "https://gitlab.com/group/project"), @@ -224,7 +224,7 @@ def test_inferred_gitlab_provider_flows_into_body(self, _mock_repo, mock_post): assert body["provider"] == "gitlab" assert body["repo_url"] == "https://gitlab.com/group/project" - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch( "rafter_cli.commands.backend.detect_repo", return_value=("group/project", "main", "gitlab", "https://gitlab.com/group/project"), @@ -249,7 +249,7 @@ def test_explicit_flag_overrides_inferred_provider(self, _mock_repo, mock_post): assert body["provider"] == "bitbucket" assert body["repo_url"] == "https://bitbucket.org/group/project" - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_includes_github_token(self, _mock_repo, mock_post): """GitHub token is included in POST body when provided.""" @@ -268,7 +268,7 @@ def test_includes_github_token(self, _mock_repo, mock_post): _, kwargs = mock_post.call_args assert kwargs["json"]["github_token"] == "ghp_test123" - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_plus_mode(self, _mock_repo, mock_post): """scan_mode=plus is sent when mode='plus'.""" @@ -287,7 +287,7 @@ def test_plus_mode(self, _mock_repo, mock_post): _, kwargs = mock_post.call_args assert kwargs["json"]["scan_mode"] == "plus" - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_prints_scan_id_when_not_quiet(self, _mock_repo, mock_post, capsys): """Scan ID is printed to stderr when not quiet.""" @@ -305,7 +305,7 @@ def test_prints_scan_id_when_not_quiet(self, _mock_repo, mock_post, capsys): err = capsys.readouterr().err assert "s-xyz" in err - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_auto_detect_message_when_not_explicit(self, _mock_repo, mock_post, capsys): """Auto-detection message prints when repo/branch not explicitly provided.""" @@ -323,7 +323,7 @@ def test_auto_detect_message_when_not_explicit(self, _mock_repo, mock_post, caps err = capsys.readouterr().err assert "auto-detected" in err.lower() - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_429_raises_quota_exhausted(self, _mock_repo, mock_post): """HTTP 429 → exit code 3 (quota exhausted).""" @@ -340,7 +340,7 @@ def test_429_raises_quota_exhausted(self, _mock_repo, mock_post): ) assert exc_info.value.exit_code == EXIT_QUOTA_EXHAUSTED - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_403_scope_raises_insufficient_scope(self, _mock_repo, mock_post, capsys): """HTTP 403 with scope keyword → exit code 4.""" @@ -361,7 +361,7 @@ def test_403_scope_raises_insufficient_scope(self, _mock_repo, mock_post, capsys ) assert exc_info.value.exit_code == EXIT_INSUFFICIENT_SCOPE - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_403_quota_raises_quota_exhausted(self, _mock_repo, mock_post, capsys): """HTTP 403 with scan_mode body → exit code 3 (quota).""" @@ -380,7 +380,7 @@ def test_403_quota_raises_quota_exhausted(self, _mock_repo, mock_post, capsys): ) assert exc_info.value.exit_code == EXIT_QUOTA_EXHAUSTED - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_401_raises_general_error(self, _mock_repo, mock_post): """HTTP 401 → exit code 1 (general error).""" @@ -397,7 +397,7 @@ def test_401_raises_general_error(self, _mock_repo, mock_post): ) assert exc_info.value.exit_code == EXIT_GENERAL_ERROR - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_500_raises_general_error(self, _mock_repo, mock_post): """HTTP 500 → exit code 1 (general error).""" @@ -431,7 +431,7 @@ def test_detect_repo_failure_raises_general_error(self, mock_detect): assert exc_info.value.exit_code == EXIT_GENERAL_ERROR @patch("rafter_cli.commands.backend._handle_scan_status_interactive") - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_calls_status_handler_when_not_skip_interactive( self, _mock_repo, mock_post, mock_status @@ -457,7 +457,7 @@ def test_calls_status_handler_when_not_skip_interactive( ) @patch("rafter_cli.commands.backend._handle_scan_status_interactive") - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_skip_interactive_does_not_call_status_handler( self, _mock_repo, mock_post, mock_status @@ -476,7 +476,7 @@ def test_skip_interactive_does_not_call_status_handler( mock_status.assert_not_called() - @patch("rafter_cli.commands.backend.requests.post") + @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_sends_api_key_header(self, _mock_repo, mock_post): """x-api-key header is set correctly.""" @@ -501,7 +501,7 @@ def test_sends_api_key_header(self, _mock_repo, mock_post): class TestHandleScanStatusInteractive: """Unit tests for the polling/status handler.""" - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_completed_immediately(self, mock_get): """Scan already completed on first poll → return success.""" mock_get.return_value = _mock_response( @@ -514,7 +514,7 @@ def test_completed_immediately(self, mock_get): assert result == EXIT_SUCCESS assert mock_get.call_count == 1 - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_completed_outputs_markdown(self, mock_get, capsys): """Completed scan outputs markdown to stdout.""" mock_get.return_value = _mock_response( @@ -525,7 +525,7 @@ def test_completed_outputs_markdown(self, mock_get, capsys): out = capsys.readouterr().out assert "# Results" in out - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_completed_outputs_json(self, mock_get, capsys): """Completed scan outputs JSON to stdout.""" response_data = {"status": "completed", "findings": []} @@ -535,7 +535,7 @@ def test_completed_outputs_json(self, mock_get, capsys): out = capsys.readouterr().out assert json.loads(out) == response_data - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_404_raises_exit_scan_not_found(self, mock_get): """HTTP 404 → exit code 2 (scan not found).""" mock_get.return_value = _mock_response(404, "not found") @@ -544,7 +544,7 @@ def test_404_raises_exit_scan_not_found(self, mock_get): _handle_scan_status_interactive("bad-id", {"x-api-key": "k"}, "md", True) assert exc_info.value.exit_code == EXIT_SCAN_NOT_FOUND - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_non_200_raises_general_error(self, mock_get): """Non-200, non-404 → exit code 1 (general error).""" mock_get.return_value = _mock_response(500, "server error") @@ -553,7 +553,7 @@ def test_non_200_raises_general_error(self, mock_get): _handle_scan_status_interactive("s1", {"x-api-key": "k"}, "md", True) assert exc_info.value.exit_code == EXIT_GENERAL_ERROR - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_failed_status_raises_general_error(self, mock_get): """Status 'failed' → exit code 1.""" mock_get.return_value = _mock_response( @@ -565,7 +565,7 @@ def test_failed_status_raises_general_error(self, mock_get): assert exc_info.value.exit_code == EXIT_GENERAL_ERROR @patch("rafter_cli.commands.backend.time.sleep") - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_polls_queued_then_completed(self, mock_get, mock_sleep): """Queued → poll → completed.""" mock_get.side_effect = [ @@ -581,7 +581,7 @@ def test_polls_queued_then_completed(self, mock_get, mock_sleep): mock_sleep.assert_called_with(10) @patch("rafter_cli.commands.backend.time.sleep") - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_polls_pending_then_completed(self, mock_get, mock_sleep): """Pending → poll → completed.""" mock_get.side_effect = [ @@ -596,7 +596,7 @@ def test_polls_pending_then_completed(self, mock_get, mock_sleep): assert mock_get.call_count == 2 @patch("rafter_cli.commands.backend.time.sleep") - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_polls_processing_then_failed(self, mock_get, mock_sleep): """Processing → poll → failed.""" mock_get.side_effect = [ @@ -609,7 +609,7 @@ def test_polls_processing_then_failed(self, mock_get, mock_sleep): assert exc_info.value.exit_code == EXIT_GENERAL_ERROR @patch("rafter_cli.commands.backend.time.sleep") - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_multiple_polls_before_completion(self, mock_get, mock_sleep): """Multiple polls before scan completes.""" mock_get.side_effect = [ @@ -626,7 +626,7 @@ def test_multiple_polls_before_completion(self, mock_get, mock_sleep): assert mock_get.call_count == 4 assert mock_sleep.call_count == 3 - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_waiting_message_in_non_quiet_mode(self, mock_get, capsys): """Status messages print to stderr in non-quiet mode.""" mock_get.return_value = _mock_response( @@ -637,7 +637,7 @@ def test_waiting_message_in_non_quiet_mode(self, mock_get, capsys): err = capsys.readouterr().err assert "completed" in err.lower() - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_quiet_mode_suppresses_stderr(self, mock_get, capsys): """Quiet mode suppresses status messages on stderr.""" mock_get.return_value = _mock_response( @@ -649,7 +649,7 @@ def test_quiet_mode_suppresses_stderr(self, mock_get, capsys): # Should NOT print "Scan completed!" in quiet mode assert "completed" not in err.lower() - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_passes_format_param_to_api(self, mock_get): """format param is passed to the API.""" mock_get.return_value = _mock_response( @@ -660,7 +660,7 @@ def test_passes_format_param_to_api(self, mock_get): _, kwargs = mock_get.call_args assert kwargs["params"]["format"] == "json" - @patch("rafter_cli.commands.backend.requests.get") + @patch("rafter_cli.commands.backend.api_get") def test_passes_scan_id_param_to_api(self, mock_get): """scan_id param is passed to the API.""" mock_get.return_value = _mock_response( diff --git a/python/tests/test_sites.py b/python/tests/test_sites.py index a6cd0b1d..bff355d0 100644 --- a/python/tests/test_sites.py +++ b/python/tests/test_sites.py @@ -41,7 +41,7 @@ def _mock_response(status_code: int, json_body: dict | None = None) -> MagicMock class TestSitesCreateCli: - @patch("rafter_cli.commands.sites.requests.post") + @patch("rafter_cli.commands.sites.api_post") def test_success(self, mock_post): mock_post.return_value = _mock_response( 200, {"site": {"id": "p1"}, "run": {"id": "r1"}, "created": True} @@ -56,7 +56,7 @@ def test_success(self, mock_post): assert kwargs["json"] == {"url": "https://example.com"} assert kwargs["headers"] == {"x-api-key": "test-key"} - @patch("rafter_cli.commands.sites.requests.post") + @patch("rafter_cli.commands.sites.api_post") def test_401(self, mock_post): mock_post.return_value = _mock_response(401, {"error": "bad key"}) result = runner.invoke( @@ -64,7 +64,7 @@ def test_401(self, mock_post): ) assert result.exit_code == EXIT_GENERAL_ERROR - @patch("rafter_cli.commands.sites.requests.post") + @patch("rafter_cli.commands.sites.api_post") def test_403_wrong_scope(self, mock_post): mock_post.return_value = _mock_response( 403, {"error": "insufficient scope: requires read-and-scan"} @@ -74,7 +74,7 @@ def test_403_wrong_scope(self, mock_post): ) assert result.exit_code == EXIT_INSUFFICIENT_SCOPE - @patch("rafter_cli.commands.sites.requests.post") + @patch("rafter_cli.commands.sites.api_post") def test_429(self, mock_post): mock_post.return_value = _mock_response(429, {"error": "Rate limit exceeded"}) result = runner.invoke( @@ -95,7 +95,7 @@ def test_rejects_format_md(self): class TestSitesScanCli: - @patch("rafter_cli.commands.sites.requests.post") + @patch("rafter_cli.commands.sites.api_post") def test_sends_project_id_for_bare_id(self, mock_post): mock_post.return_value = _mock_response(200, {"run": {"id": "r1"}}) result = runner.invoke(sites_app, ["scan", "proj-123", "-k", "test-key"]) @@ -103,7 +103,7 @@ def test_sends_project_id_for_bare_id(self, mock_post): _, kwargs = mock_post.call_args assert kwargs["json"] == {"projectId": "proj-123"} - @patch("rafter_cli.commands.sites.requests.post") + @patch("rafter_cli.commands.sites.api_post") def test_sends_url_for_url(self, mock_post): mock_post.return_value = _mock_response(200, {"run": {"id": "r1"}}) result = runner.invoke( @@ -113,7 +113,7 @@ def test_sends_url_for_url(self, mock_post): _, kwargs = mock_post.call_args assert kwargs["json"] == {"url": "https://example.com"} - @patch("rafter_cli.commands.sites.requests.post") + @patch("rafter_cli.commands.sites.api_post") def test_includes_sections(self, mock_post): mock_post.return_value = _mock_response(200, {"run": {"id": "r1"}}) result = runner.invoke( @@ -124,7 +124,7 @@ def test_includes_sections(self, mock_post): _, kwargs = mock_post.call_args assert kwargs["json"] == {"projectId": "proj-123", "sections": ["security", "dns"]} - @patch("rafter_cli.commands.sites.requests.post") + @patch("rafter_cli.commands.sites.api_post") def test_rejects_invalid_section(self, mock_post): result = runner.invoke( sites_app, @@ -133,13 +133,13 @@ def test_rejects_invalid_section(self, mock_post): assert result.exit_code == EXIT_GENERAL_ERROR mock_post.assert_not_called() - @patch("rafter_cli.commands.sites.requests.post") + @patch("rafter_cli.commands.sites.api_post") def test_404_not_owned(self, mock_post): mock_post.return_value = _mock_response(404, {"error": "not found"}) result = runner.invoke(sites_app, ["scan", "proj-123", "-k", "test-key"]) assert result.exit_code == EXIT_SCAN_NOT_FOUND - @patch("rafter_cli.commands.sites.requests.post") + @patch("rafter_cli.commands.sites.api_post") def test_403_run_limit(self, mock_post): mock_post.return_value = _mock_response(403, {"error": "run limit reached"}) result = runner.invoke(sites_app, ["scan", "proj-123", "-k", "test-key"]) @@ -150,7 +150,7 @@ def test_403_run_limit(self, mock_post): class TestSitesListCli: - @patch("rafter_cli.commands.sites.requests.get") + @patch("rafter_cli.commands.sites.api_get") def test_passes_pagination_params(self, mock_get): mock_get.return_value = _mock_response( 200, {"sites": [], "limit": 10, "offset": 5, "has_more": False} @@ -173,7 +173,7 @@ def test_passes_pagination_params(self, mock_get): "include_archived": "true", } - @patch("rafter_cli.commands.sites.requests.get") + @patch("rafter_cli.commands.sites.api_get") def test_401(self, mock_get): mock_get.return_value = _mock_response(401, {"error": "invalid key"}) result = runner.invoke(sites_app, ["list", "-k", "test-key"]) @@ -184,7 +184,7 @@ def test_401(self, mock_get): class TestSitesGetCli: - @patch("rafter_cli.commands.sites.requests.get") + @patch("rafter_cli.commands.sites.api_get") def test_success(self, mock_get): mock_get.return_value = _mock_response( 200, @@ -199,13 +199,13 @@ def test_success(self, mock_get): args, _ = mock_get.call_args assert args[0].endswith("/static/sites/p1") - @patch("rafter_cli.commands.sites.requests.get") + @patch("rafter_cli.commands.sites.api_get") def test_404(self, mock_get): mock_get.return_value = _mock_response(404, {"error": "not found"}) result = runner.invoke(sites_app, ["get", "nonexistent", "-k", "test-key"]) assert result.exit_code == EXIT_SCAN_NOT_FOUND - @patch("rafter_cli.commands.sites.requests.get") + @patch("rafter_cli.commands.sites.api_get") def test_429(self, mock_get): mock_get.return_value = _mock_response( 429, {"error": "Rate limit exceeded", "retryAfter": 30} @@ -218,7 +218,7 @@ def test_429(self, mock_get): class TestMcpSitesCreate: - @patch("rafter_cli.commands.mcp_server.requests.post") + @patch("rafter_cli.commands.mcp_server.api_post") @patch("rafter_cli.commands.mcp_server.resolve_mcp_api_key", return_value="test-key") def test_success(self, _mock_key, mock_post): mock_post.return_value = _mock_response( @@ -229,7 +229,7 @@ def test_success(self, _mock_key, mock_post): _, kwargs = mock_post.call_args assert kwargs["json"] == {"url": "https://example.com"} - @patch("rafter_cli.commands.mcp_server.requests.post") + @patch("rafter_cli.commands.mcp_server.api_post") @patch("rafter_cli.commands.mcp_server.resolve_mcp_api_key", return_value="test-key") def test_401_raises(self, _mock_key, mock_post): mock_post.return_value = _mock_response(401, {"error": "invalid key"}) @@ -243,7 +243,7 @@ def test_missing_key_raises_without_crashing(self, _mock_key): class TestMcpSitesScan: - @patch("rafter_cli.commands.mcp_server.requests.post") + @patch("rafter_cli.commands.mcp_server.api_post") @patch("rafter_cli.commands.mcp_server.resolve_mcp_api_key", return_value="test-key") def test_by_project_id(self, _mock_key, mock_post): mock_post.return_value = _mock_response(200, {"run": {"id": "r1"}}) @@ -262,7 +262,7 @@ def test_rejects_both_project_id_and_url(self): with pytest.raises(RuntimeError, match="not both"): handle_sites_scan(project_id="proj-1", url="https://example.com") - @patch("rafter_cli.commands.mcp_server.requests.post") + @patch("rafter_cli.commands.mcp_server.api_post") @patch("rafter_cli.commands.mcp_server.resolve_mcp_api_key", return_value="test-key") def test_404_not_owned(self, _mock_key, mock_post): mock_post.return_value = _mock_response(404, {"error": "not found"}) @@ -271,7 +271,7 @@ def test_404_not_owned(self, _mock_key, mock_post): class TestMcpSitesList: - @patch("rafter_cli.commands.mcp_server.requests.get") + @patch("rafter_cli.commands.mcp_server.api_get") @patch("rafter_cli.commands.mcp_server.resolve_mcp_api_key", return_value="test-key") def test_success_with_params(self, _mock_key, mock_get): mock_get.return_value = _mock_response( @@ -281,7 +281,7 @@ def test_success_with_params(self, _mock_key, mock_get): _, kwargs = mock_get.call_args assert kwargs["params"] == {"limit": "5", "include_archived": "true"} - @patch("rafter_cli.commands.mcp_server.requests.get") + @patch("rafter_cli.commands.mcp_server.api_get") @patch("rafter_cli.commands.mcp_server.resolve_mcp_api_key", return_value="test-key") def test_429_raises(self, _mock_key, mock_get): mock_get.return_value = _mock_response(429, {"error": "Rate limit exceeded"}) @@ -290,7 +290,7 @@ def test_429_raises(self, _mock_key, mock_get): class TestMcpSitesGet: - @patch("rafter_cli.commands.mcp_server.requests.get") + @patch("rafter_cli.commands.mcp_server.api_get") @patch("rafter_cli.commands.mcp_server.resolve_mcp_api_key", return_value="test-key") def test_success(self, _mock_key, mock_get): mock_get.return_value = _mock_response( @@ -304,7 +304,7 @@ def test_success(self, _mock_key, mock_get): result = handle_sites_get("p1") assert result["site"]["id"] == "p1" - @patch("rafter_cli.commands.mcp_server.requests.get") + @patch("rafter_cli.commands.mcp_server.api_get") @patch("rafter_cli.commands.mcp_server.resolve_mcp_api_key", return_value="test-key") def test_404_raises(self, _mock_key, mock_get): mock_get.return_value = _mock_response(404, {"error": "not found"})