-
Notifications
You must be signed in to change notification settings - Fork 14
Add token refresh endpoints to agent server #2018
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| import { tmpdir } from "node:os"; | ||
| import { describe, expect, it } from "vitest"; | ||
| import { isLoopbackAddress, spawnAndCollect } from "./gh-exec"; | ||
|
|
||
| describe("isLoopbackAddress", () => { | ||
| it.each([ | ||
| ["127.0.0.1", true], | ||
| ["127.0.0.2", true], | ||
| ["127.1.2.3", true], | ||
| ["::1", true], | ||
| ["::ffff:127.0.0.1", true], | ||
| ["::ffff:127.1.2.3", true], | ||
| ["localhost", true], | ||
| ["10.0.0.1", false], | ||
| ["192.168.1.1", false], | ||
| ["::ffff:10.0.0.1", false], | ||
| ["8.8.8.8", false], | ||
| ["128.0.0.1", false], | ||
| ["", false], | ||
| [undefined, false], | ||
| ])("address %s -> %s", (input, expected) => { | ||
| expect(isLoopbackAddress(input)).toBe(expected); | ||
| }); | ||
| }); | ||
|
|
||
| // Test the spawn/collect plumbing via spawnAndCollect. runGh is a thin wrapper | ||
| // that pins the binary to "gh" — verified by inspection, not exercised here so | ||
| // tests don't depend on a `gh` install. | ||
| describe("spawnAndCollect", () => { | ||
| it("captures stdout, stderr, and a zero exit code", async () => { | ||
| const result = await spawnAndCollect( | ||
| process.execPath, | ||
| ["-e", "process.stdout.write('hi'); process.stderr.write('err');"], | ||
| { cwd: tmpdir(), timeoutMs: 5_000 }, | ||
| ); | ||
|
|
||
| expect(result.stdout).toBe("hi"); | ||
| expect(result.stderr).toBe("err"); | ||
| expect(result.exitCode).toBe(0); | ||
| expect(result.timedOut).toBe(false); | ||
| }); | ||
|
|
||
| it("surfaces non-zero exit codes without throwing", async () => { | ||
| const result = await spawnAndCollect( | ||
| process.execPath, | ||
| ["-e", "process.exit(2)"], | ||
| { cwd: tmpdir(), timeoutMs: 5_000 }, | ||
| ); | ||
|
|
||
| expect(result.exitCode).toBe(2); | ||
| expect(result.timedOut).toBe(false); | ||
| }); | ||
|
|
||
| it("rejects when the binary is missing", async () => { | ||
| await expect( | ||
| spawnAndCollect( | ||
| "/nonexistent/binary/that/cannot/possibly/exist", | ||
| ["--version"], | ||
| { cwd: tmpdir(), timeoutMs: 5_000 }, | ||
| ), | ||
| ).rejects.toThrow(); | ||
| }); | ||
|
|
||
| it("kills the process on timeout and reports timedOut: true", async () => { | ||
| const start = Date.now(); | ||
| const result = await spawnAndCollect( | ||
| process.execPath, | ||
| ["-e", "setTimeout(() => process.exit(0), 60_000);"], | ||
| { cwd: tmpdir(), timeoutMs: 200 }, | ||
| ); | ||
|
|
||
| expect(result.timedOut).toBe(true); | ||
| // Should be killed well before the 60s sleep would have completed. | ||
| expect(Date.now() - start).toBeLessThan(10_000); | ||
| }); | ||
|
|
||
| it("respects the cwd option", async () => { | ||
| const expected = tmpdir(); | ||
| const result = await spawnAndCollect( | ||
| process.execPath, | ||
| ["-e", "process.stdout.write(process.cwd());"], | ||
| { cwd: expected, timeoutMs: 5_000 }, | ||
| ); | ||
|
|
||
| // Resolve through realpath-style equivalence: tmpdir on macOS may be a | ||
| // symlink (e.g. /var/folders -> /private/var/folders). | ||
| expect([expected, `/private${expected}`]).toContain(result.stdout); | ||
| expect(result.exitCode).toBe(0); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| import { spawn } from "node:child_process"; | ||
| import type { Logger } from "../utils/logger"; | ||
|
|
||
| export interface GhExecOptions { | ||
| cwd: string; | ||
| timeoutMs: number; | ||
| logger?: Logger; | ||
| } | ||
|
|
||
| export interface GhExecResult { | ||
| stdout: string; | ||
| stderr: string; | ||
| exitCode: number | null; | ||
| signal: NodeJS.Signals | null; | ||
| timedOut: boolean; | ||
| } | ||
|
|
||
| const KILL_GRACE_MS = 2_000; | ||
|
|
||
| export function runGh( | ||
| args: string[], | ||
| options: GhExecOptions, | ||
| ): Promise<GhExecResult> { | ||
| // Binary is pinned here. Callers (incl. the HTTP route) cannot override it — | ||
| // the body schema strips unknowns and runGh has no binary parameter. | ||
| return spawnAndCollect("gh", args, options); | ||
| } | ||
|
|
||
| export async function spawnAndCollect( | ||
| binary: string, | ||
| args: string[], | ||
| options: GhExecOptions, | ||
| ): Promise<GhExecResult> { | ||
| const logger = options.logger; | ||
|
|
||
| logger?.debug("Running command", { | ||
| binary, | ||
| args, | ||
| cwd: options.cwd, | ||
| timeoutMs: options.timeoutMs, | ||
| }); | ||
|
|
||
| return new Promise<GhExecResult>((resolve, reject) => { | ||
| const child = spawn(binary, args, { | ||
| cwd: options.cwd, | ||
| env: process.env, | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }); | ||
|
|
||
| let stdout = ""; | ||
| let stderr = ""; | ||
| let timedOut = false; | ||
| let killTimer: NodeJS.Timeout | null = null; | ||
|
|
||
| const timeout = setTimeout(() => { | ||
| timedOut = true; | ||
| logger?.warn("Command timed out, sending SIGTERM", { | ||
| pid: child.pid, | ||
| timeoutMs: options.timeoutMs, | ||
| }); | ||
| child.kill("SIGTERM"); | ||
| killTimer = setTimeout(() => { | ||
| if (!child.killed) { | ||
| logger?.warn("Command did not exit, sending SIGKILL", { | ||
| pid: child.pid, | ||
| }); | ||
| child.kill("SIGKILL"); | ||
| } | ||
| }, KILL_GRACE_MS); | ||
| }, options.timeoutMs); | ||
|
|
||
| child.stdout?.setEncoding("utf8"); | ||
| child.stderr?.setEncoding("utf8"); | ||
| child.stdout?.on("data", (chunk: string) => { | ||
| stdout += chunk; | ||
| }); | ||
| child.stderr?.on("data", (chunk: string) => { | ||
| stderr += chunk; | ||
| }); | ||
|
|
||
| child.on("error", (err) => { | ||
| clearTimeout(timeout); | ||
| if (killTimer) clearTimeout(killTimer); | ||
| reject(err); | ||
| }); | ||
|
|
||
| child.on("close", (code, signal) => { | ||
| clearTimeout(timeout); | ||
| if (killTimer) clearTimeout(killTimer); | ||
| resolve({ | ||
| stdout, | ||
| stderr, | ||
| exitCode: code, | ||
| signal, | ||
| timedOut, | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| export function isLoopbackAddress(address: string | undefined): boolean { | ||
|
Comment on lines
+96
to
+101
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
IPv4 reserves the entire Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/agent/src/server/gh-exec.ts
Line: 88-100
Comment:
**`isLoopbackAddress` only recognises a four-value fixed set, not the full loopback range**
IPv4 reserves the entire `127.0.0.0/8` subnet as loopback, and IPv4-mapped loopback in IPv6 covers `::ffff:127.0.0.0/104`. The set currently contains only `127.0.0.1` and `::ffff:127.0.0.1`, so a connection arriving on any other address in those ranges (e.g. `127.0.0.2`, `::ffff:127.1.2.3`) is silently rejected with 403. A safer check would be a proper prefix test for `127.` and `::ffff:127.`.
How can I resolve this? If you propose a fix, please make it concise. |
||
| if (!address) return false; | ||
| if (address === "::1" || address === "localhost") return true; | ||
| // IPv4 reserves the full 127.0.0.0/8 range as loopback, and IPv4-mapped | ||
| // IPv6 covers ::ffff:127.0.0.0/104 — match by prefix rather than equality. | ||
| if (address.startsWith("127.")) return true; | ||
| if (address.startsWith("::ffff:127.")) return true; | ||
| return false; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ghexecutes anyghsubcommand, not just token-refresh operationsThe
argsarray is unconstrained, so any local process can invokegh secret set,gh auth logout,gh repo delete, or other destructive subcommands with the agent's inherited credentials. The loopback restriction limits exposure to co-located processes, but it's broader than the stated use case requires. Consider restrictingargs[0]to an allowlist of safe subcommands (e.g.auth) or documenting why fullghpass-through is intentionally required.Prompt To Fix With AI