mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-22 12:50:50 +02:00
fix(browse): CLI always sends explicit pair scopes via shared DEFAULT_PAIR_SCOPES
The effective pairing default lived in two places: the CLI omitted scopes unless --restrict was passed, and the server filled in its own literal. handlePairAgent now always sends an explicit scopes list and both sides reference one exported constant, DEFAULT_PAIR_SCOPES, so the default cannot silently drift again (pinned by a server-auth source tripwire). Three input traps closed in the same surface: - Bare --restrict (or --restrict swallowing the next flag) parsed as "no restriction" and silently granted FULL access, the opposite of the user's intent. validatePairAgentFlags rejects it pre-server, before any consent gate, so an arg error never boots a daemon. - A scopes list could smuggle the control scope past the explicit flag: --restrict "read,control" minted a control-scoped session with no --control. /pair now 400s on control in a scopes list without the control flag, and the CLI points the user at --control. - Option typos validated only at exchange time: createSetupKey stored any scope string and any rateLimit, so /pair returned 200 with a poisoned setup key whose failure surfaced to the REMOTE agent at /connect as a misleading "Invalid request body". Shared validation now runs in both creators and throws typed InvalidScopeError; /pair and /token 400 with the message, naming the bad scope or negative rateLimit. Also `opts.rateLimit || 10` became `?? 10` so the documented "0 = unlimited" survives the /pair path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
f28bfd0158
commit
44e0b6f7be
@@ -194,6 +194,76 @@ describe('pair-agent flow end-to-end (HTTP only, no ngrok)', () => {
|
||||
expect(Array.isArray(scopes)).toBe(true);
|
||||
});
|
||||
|
||||
// ─── Pair scope contract: defaults, explicit lists, typo naming ───────
|
||||
|
||||
test('default /connect scopes are exactly read,write,admin,meta', async () => {
|
||||
const pairResp = await fetch(`${daemon.baseUrl}/pair`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` },
|
||||
body: JSON.stringify({ clientId: 'default-scopes' }),
|
||||
});
|
||||
const { setup_key, scopes: pairScopes } = await pairResp.json() as any;
|
||||
expect(pairScopes).toEqual(['read', 'write', 'admin', 'meta']);
|
||||
const connectResp = await fetch(`${daemon.baseUrl}/connect`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({ setup_key }),
|
||||
});
|
||||
const { scopes } = await connectResp.json() as any;
|
||||
expect(scopes).toEqual(['read', 'write', 'admin', 'meta']);
|
||||
});
|
||||
|
||||
test('explicit scopes are honored end-to-end (the --restrict wire contract)', async () => {
|
||||
const pairResp = await fetch(`${daemon.baseUrl}/pair`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` },
|
||||
body: JSON.stringify({ clientId: 'restricted-scopes', scopes: ['read'] }),
|
||||
});
|
||||
const { setup_key } = await pairResp.json() as any;
|
||||
const connectResp = await fetch(`${daemon.baseUrl}/connect`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({ setup_key }),
|
||||
});
|
||||
const { scopes } = await connectResp.json() as any;
|
||||
expect(scopes).toEqual(['read']);
|
||||
});
|
||||
|
||||
test('POST /pair with a scope typo fails fast, naming the scope', async () => {
|
||||
// Regression: pre-fix this returned 200 with a poisoned setup key whose
|
||||
// failure surfaced at /connect as a misleading "Invalid request body".
|
||||
const resp = await fetch(`${daemon.baseUrl}/pair`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` },
|
||||
body: JSON.stringify({ clientId: 'typo-agent', scopes: ['raed'] }),
|
||||
});
|
||||
expect(resp.status).toBe(400);
|
||||
const body = await resp.json() as any;
|
||||
expect(body.error).toContain('Invalid scope: raed');
|
||||
});
|
||||
|
||||
test('POST /token with a scope typo names the scope too', async () => {
|
||||
const resp = await fetch(`${daemon.baseUrl}/token`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` },
|
||||
body: JSON.stringify({ clientId: 'typo-token', scopes: ['wirte'] }),
|
||||
});
|
||||
expect(resp.status).toBe(400);
|
||||
const body = await resp.json() as any;
|
||||
expect(body.error).toContain('Invalid scope: wirte');
|
||||
});
|
||||
|
||||
test('control cannot ride in through a /pair scopes list without the control flag', async () => {
|
||||
const resp = await fetch(`${daemon.baseUrl}/pair`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` },
|
||||
body: JSON.stringify({ clientId: 'sneaky', scopes: ['read', 'control'] }),
|
||||
});
|
||||
expect(resp.status).toBe(400);
|
||||
const body = await resp.json() as any;
|
||||
expect(body.error).toContain('control');
|
||||
});
|
||||
|
||||
// ─── Revocation e2e: revoke-all + the /agents verification surface ────
|
||||
|
||||
test('DELETE /token revokes session AND setup keys; agent leaves /agents; token 401s; re-connect fails', async () => {
|
||||
|
||||
@@ -409,3 +409,32 @@ describe('Server auth security', () => {
|
||||
expect(routeSrc).toContain('SameSite=Strict');
|
||||
});
|
||||
});
|
||||
|
||||
describe('Pair scope defaults and revocation surface', () => {
|
||||
// Regression: the CLI only sent scopes when --restrict was passed, so the
|
||||
// effective pairing default lived in two places (CLI omission + server
|
||||
// fallback) and could silently drift. Both sides must reference the shared
|
||||
// DEFAULT_PAIR_SCOPES constant, and the CLI must send scopes
|
||||
// unconditionally (the old conditional-spread shape is banned).
|
||||
test('/pair default and CLI pairing body share DEFAULT_PAIR_SCOPES', () => {
|
||||
const pairBlock = sliceBetween(SERVER_SRC, "url.pathname === '/pair'", "url.pathname === '/tunnel/start'");
|
||||
expect(pairBlock).toContain('DEFAULT_PAIR_SCOPES');
|
||||
const cliBlock = sliceBetween(CLI_SRC, 'async function handlePairAgent', 'Determine the URL to use');
|
||||
expect(cliBlock).toContain('DEFAULT_PAIR_SCOPES');
|
||||
expect(cliBlock).not.toContain('...(restrict ? { scopes');
|
||||
});
|
||||
|
||||
// control is the only scope behind an explicit flag; a scopes list must
|
||||
// not be able to smuggle it into a pairing grant.
|
||||
test('/pair rejects control inside a scopes list without the control flag', () => {
|
||||
const pairBlock = sliceBetween(SERVER_SRC, "url.pathname === '/pair'", "url.pathname === '/tunnel/start'");
|
||||
expect(pairBlock).toContain("pairBody.scopes.includes('control')");
|
||||
});
|
||||
|
||||
// CLI-encoded clientIds (spaces, UTF-8) must round-trip through the revoke
|
||||
// route; slicing the raw pathname 404s on every encoded name.
|
||||
test('DELETE /token decodes the clientId path segment', () => {
|
||||
const revokeBlock = sliceBetween(SERVER_SRC, "url.pathname.startsWith('/token/')", "url.pathname === '/agents'");
|
||||
expect(revokeBlock).toContain('decodeURIComponent');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -6,6 +6,7 @@ import {
|
||||
revokeToken, rotateRoot, listTokens, recordCommand,
|
||||
serializeRegistry, restoreRegistry, checkConnectRateLimit,
|
||||
SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN, SCOPE_CONTROL, SCOPE_META,
|
||||
DEFAULT_PAIR_SCOPES, InvalidScopeError,
|
||||
__resetRegistry,
|
||||
} from '../src/token-registry';
|
||||
|
||||
@@ -347,6 +348,35 @@ describe('token-registry', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('pair defaults and option validation', () => {
|
||||
it('DEFAULT_PAIR_SCOPES is exactly read,write,admin,meta (b73f3644: the ceremony is the trust boundary)', () => {
|
||||
expect([...DEFAULT_PAIR_SCOPES]).toEqual(['read', 'write', 'admin', 'meta']);
|
||||
});
|
||||
|
||||
// Regression: only createToken validated options, so a scope typo minted
|
||||
// a poisoned setup key at /pair and surfaced to the REMOTE agent at
|
||||
// /connect as a misleading "Invalid request body".
|
||||
it('createSetupKey rejects an unknown scope with InvalidScopeError naming it', () => {
|
||||
expect(() => createSetupKey({ scopes: ['raed' as never] }))
|
||||
.toThrow(InvalidScopeError);
|
||||
expect(() => createSetupKey({ scopes: ['raed' as never] }))
|
||||
.toThrow('Invalid scope: raed');
|
||||
});
|
||||
|
||||
it('createSetupKey rejects a negative rateLimit', () => {
|
||||
expect(() => createSetupKey({ rateLimit: -5 })).toThrow(InvalidScopeError);
|
||||
});
|
||||
|
||||
// Regression: `opts.rateLimit || 10` coerced the documented "0 = unlimited"
|
||||
// into 10 on the /pair path while /token honored it.
|
||||
it('createSetupKey preserves rateLimit 0 (unlimited)', () => {
|
||||
const setup = createSetupKey({ rateLimit: 0 });
|
||||
expect(setup.rateLimit).toBe(0);
|
||||
const session = exchangeSetupKey(setup.token)!;
|
||||
expect(session.rateLimit).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('rotateRoot', () => {
|
||||
it('generates new root and invalidates all tokens', () => {
|
||||
const oldRoot = getRootToken();
|
||||
|
||||
@@ -97,6 +97,50 @@ describe('tunnel subcommand parsing', () => {
|
||||
}, 60_000);
|
||||
});
|
||||
|
||||
// pair-agent scope-flag misuse shares this file's subprocess harness: like
|
||||
// tunnel, the validation must run pre-server, so "no daemon spawned" is the
|
||||
// load-bearing assertion.
|
||||
describe('pair-agent scope-flag validation (pre-server)', () => {
|
||||
test('bare --restrict → exit 1 usage error, NO daemon spawned (was: silent FULL grant)', async () => {
|
||||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-restrict-bare-'));
|
||||
const stateFile = path.join(tmpDir, 'browse.json');
|
||||
try {
|
||||
const result = await runCli(['pair-agent', '--restrict'], baseEnv(stateFile));
|
||||
expect(result.code).toBe(1);
|
||||
expect(result.stderr).toContain('--restrict needs a scope list');
|
||||
expect(fs.existsSync(stateFile)).toBe(false);
|
||||
} finally {
|
||||
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||
}
|
||||
}, 30_000);
|
||||
|
||||
test('--restrict swallowing the next flag → exit 1, NO daemon spawned', async () => {
|
||||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-restrict-flag-'));
|
||||
const stateFile = path.join(tmpDir, 'browse.json');
|
||||
try {
|
||||
const result = await runCli(['pair-agent', '--restrict', '--client', 'bob'], baseEnv(stateFile));
|
||||
expect(result.code).toBe(1);
|
||||
expect(result.stderr).toContain('--restrict needs a scope list');
|
||||
expect(fs.existsSync(stateFile)).toBe(false);
|
||||
} finally {
|
||||
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||
}
|
||||
}, 30_000);
|
||||
|
||||
test('--restrict "read,control" → exit 1 pointing at --control, NO daemon spawned', async () => {
|
||||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-restrict-ctl-'));
|
||||
const stateFile = path.join(tmpDir, 'browse.json');
|
||||
try {
|
||||
const result = await runCli(['pair-agent', '--restrict', 'read,control'], baseEnv(stateFile));
|
||||
expect(result.code).toBe(1);
|
||||
expect(result.stderr).toContain('Re-run with --control');
|
||||
expect(fs.existsSync(stateFile)).toBe(false);
|
||||
} finally {
|
||||
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||
}
|
||||
}, 30_000);
|
||||
});
|
||||
|
||||
describe('tunnel against no daemon (#2254 — never boot one)', () => {
|
||||
test('revoke → exit 0, "No daemon running", nothing spawned', async () => {
|
||||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-tunnel-dead-'));
|
||||
|
||||
Reference in New Issue
Block a user