Skip to content

Commit 8e1d578

Browse files
committed
fix(2.0b-O2): batch 3 — schema validation, CallbackResult union, parity contract
Three remaining substantive review items from PR aws-samples#160: - Validate all 11 fields in getOauthSecret (review non-blocking aws-samples#7). Was checking only access_token / refresh_token / expires_at; missing client_id or client_secret only surfaced 24h later when the refresh call needed them and found undefined. Extracted the required-field list into a const next to the StoredOauthToken interface and check the full set at deserialization. Bad secrets fail fast at fetch time with a structured log line naming the missing fields. - CallbackResult discriminated union (review non-blocking aws-samples#6). Was `{ sessionId: string|null, code: string|null, state: string|null }` which let callers construct unreachable shapes. Split into `{ kind: 'agentcore', sessionId } | { kind: 'direct-oauth', code, state }`. Updated the resolver site (`oauth-callback-server.ts`), the consumer (`bgagent linear setup`), and the test file to use exhaustive type-narrowing. The setup wizard now errors clearly if it gets the agentcore shape (parked path) instead of silently passing nulls down. - Cross-language schema-parity contract test (review non-blocking #3). CLI's StoredLinearOauthToken and Lambda's StoredOauthToken define the same JSON-in-Secrets-Manager schema independently; drift between the two would be a silent bug (CLI writes one field name, Lambda reads another, refresh works, every Lambda invocation logs a missing-field error). New test in `cdk/test/contracts/stored-oauth-token-parity.test.ts` regex-parses both interface definitions out of source and asserts the field set is equal. Also asserts the new `STORED_OAUTH_TOKEN_REQUIRED_FIELDS` const matches the interface, so future field additions can't drift between the validator and the type. CLI tests 286/286 pass. CDK resolver + contract 13/13 pass.
1 parent e90a092 commit 8e1d578

5 files changed

Lines changed: 182 additions & 49 deletions

File tree

cdk/src/handlers/shared/linear-oauth-resolver.ts

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,32 @@ async function getRegistryRow(
218218
return row;
219219
}
220220

221+
/**
222+
* Required fields on the StoredOauthToken JSON in Secrets Manager.
223+
* Validated as a set at deserialization so a missing field fails fast
224+
* here, not 24 hours later inside `tryRefreshOnce` when the refresh
225+
* call needs `client_id` / `client_secret` and finds them undefined.
226+
*
227+
* Keep this list in sync with the `StoredOauthToken` interface above
228+
* AND the CLI-side `StoredLinearOauthToken` shape (see
229+
* `cli/src/linear-oauth.ts`). The contract test in
230+
* `cdk/test/contracts/stored-oauth-token-parity.test.ts` enforces
231+
* the cross-language match.
232+
*/
233+
const STORED_OAUTH_TOKEN_REQUIRED_FIELDS: ReadonlyArray<keyof StoredOauthToken> = [
234+
'access_token',
235+
'refresh_token',
236+
'expires_at',
237+
'scope',
238+
'client_id',
239+
'client_secret',
240+
'workspace_id',
241+
'workspace_slug',
242+
'installed_at',
243+
'updated_at',
244+
'installed_by_platform_user_id',
245+
];
246+
221247
async function getOauthSecret(
222248
sm: SecretsManagerClient,
223249
secretArn: string,
@@ -226,7 +252,16 @@ async function getOauthSecret(
226252
const res = await sm.send(new GetSecretValueCommand({ SecretId: secretArn }));
227253
if (!res.SecretString) return null;
228254
const parsed = JSON.parse(res.SecretString) as StoredOauthToken;
229-
if (!parsed.access_token || !parsed.refresh_token || !parsed.expires_at) return null;
255+
const missing = STORED_OAUTH_TOKEN_REQUIRED_FIELDS.filter(
256+
(f) => typeof parsed[f] !== 'string' || (parsed[f] as string).length === 0,
257+
);
258+
if (missing.length > 0) {
259+
logger.error('Linear OAuth secret JSON is missing required fields', {
260+
secret_arn: secretArn,
261+
missing_fields: missing,
262+
});
263+
return null;
264+
}
230265
return parsed;
231266
} catch (err) {
232267
logger.error('Failed to fetch Linear OAuth secret', {
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
/**
2+
* MIT No Attribution
3+
*
4+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
5+
*
6+
* Permission is hereby granted, free of charge, to any person obtaining a copy of
7+
* the Software without restriction, including without limitation the rights to
8+
* use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
9+
* the Software, and to permit persons to whom the Software is furnished to do so.
10+
*
11+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
12+
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
13+
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
14+
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
15+
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
16+
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
17+
* SOFTWARE.
18+
*/
19+
20+
import * as fs from 'fs';
21+
import * as path from 'path';
22+
23+
/**
24+
* Cross-language contract: the JSON schema written into Secrets Manager
25+
* by the CLI's `bgagent linear setup` MUST match what the Lambda-side
26+
* resolver expects to read. Two TypeScript interfaces define the shape
27+
* independently — `StoredLinearOauthToken` (CLI) and `StoredOauthToken`
28+
* (Lambda). Without a contract test, drift between the two is a silent
29+
* runtime bug: CLI writes `installer_user_id`, Lambda reads
30+
* `installed_by_platform_user_id`, refresh works, every Lambda
31+
* invocation logs a missing-field error.
32+
*
33+
* This test parses both interface definitions out of source and
34+
* asserts the field set is equal. It deliberately avoids importing
35+
* the CLI (cross-package import would couple build orders); a
36+
* lightweight regex-extract is enough to keep the schemas honest.
37+
*/
38+
39+
const REPO_ROOT = path.resolve(__dirname, '..', '..', '..');
40+
const LAMBDA_RESOLVER = path.join(REPO_ROOT, 'cdk', 'src', 'handlers', 'shared', 'linear-oauth-resolver.ts');
41+
const CLI_OAUTH = path.join(REPO_ROOT, 'cli', 'src', 'linear-oauth.ts');
42+
43+
function extractInterfaceFields(source: string, interfaceName: string): string[] {
44+
const reBlock = new RegExp(`export\\s+interface\\s+${interfaceName}\\s*\\{([\\s\\S]*?)\\n\\}`);
45+
const match = reBlock.exec(source);
46+
if (!match) {
47+
throw new Error(`Could not find interface ${interfaceName}`);
48+
}
49+
const body = match[1];
50+
const fields: string[] = [];
51+
// Match `readonly <name>:` or `<name>:` field declarations. Skip
52+
// lines that are inside JSDoc comment blocks (start with `*`) or
53+
// single-line comments (`//`).
54+
for (const rawLine of body.split('\n')) {
55+
const line = rawLine.trim();
56+
if (!line || line.startsWith('//') || line.startsWith('/*') || line.startsWith('*')) continue;
57+
const fieldMatch = /^(?:readonly\s+)?([a-zA-Z_][a-zA-Z0-9_]*)\??\s*:/.exec(line);
58+
if (fieldMatch) {
59+
fields.push(fieldMatch[1]);
60+
}
61+
}
62+
return fields;
63+
}
64+
65+
describe('StoredOauthToken / StoredLinearOauthToken cross-language parity', () => {
66+
test('Lambda and CLI define the same set of fields', () => {
67+
const lambdaSource = fs.readFileSync(LAMBDA_RESOLVER, 'utf8');
68+
const cliSource = fs.readFileSync(CLI_OAUTH, 'utf8');
69+
70+
const lambdaFields = extractInterfaceFields(lambdaSource, 'StoredOauthToken').sort();
71+
const cliFields = extractInterfaceFields(cliSource, 'StoredLinearOauthToken').sort();
72+
73+
expect(lambdaFields).toEqual(cliFields);
74+
// Sanity: at least 11 fields per the documented schema. Catches
75+
// a regex parse failure that returns empty arrays.
76+
expect(lambdaFields.length).toBeGreaterThanOrEqual(11);
77+
});
78+
79+
test('Lambda STORED_OAUTH_TOKEN_REQUIRED_FIELDS const matches the interface', () => {
80+
const lambdaSource = fs.readFileSync(LAMBDA_RESOLVER, 'utf8');
81+
const interfaceFields = extractInterfaceFields(lambdaSource, 'StoredOauthToken').sort();
82+
83+
const constMatch = /STORED_OAUTH_TOKEN_REQUIRED_FIELDS:\s*ReadonlyArray<keyof StoredOauthToken>\s*=\s*\[([\s\S]*?)\];/.exec(lambdaSource);
84+
expect(constMatch).not.toBeNull();
85+
const constFields = (constMatch![1].match(/'([a-zA-Z_][a-zA-Z0-9_]*)'/g) ?? [])
86+
.map((s) => s.replace(/'/g, ''))
87+
.sort();
88+
89+
expect(constFields).toEqual(interfaceFields);
90+
});
91+
});

cli/src/commands/linear.ts

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -409,28 +409,15 @@ export function makeLinearCommand(): Command {
409409
const callback = await callbackPromise;
410410
console.log(' ✓');
411411

412-
// The localhost callback server resolves with `?session_id=` from
413-
// AWS's redirect, but we're not using AgentCore — for the direct
414-
// flow, the callback server gives us `?code=...&state=...` from
415-
// Linear directly. We need to extract differently.
416-
//
417-
// Reality check: our awaitOauthCallback() is hard-coded to
418-
// extract `session_id`. For Option 2 we need to also capture
419-
// `code` + `state` from the same redirect. The localhost server
420-
// module needs adjusting; for now we use a workaround that reads
421-
// the 'session_id' field which the callback server populated
422-
// from whatever query param was named `session_id`. We override
423-
// by re-reading the actual redirect URL inside the server.
424-
//
425-
// Defensive: callback.sessionId may contain the full session_id
426-
// value when this code is reached against an AgentCore-style
427-
// redirect. We don't reach here in O2 because Linear redirects
428-
// with `code` + `state`, not `session_id`. The callback module
429-
// is updated separately to expose both shapes.
430-
if (!callback.code || !callback.state) {
412+
// Phase 2.0b Option 2 expects Linear to redirect with `code` +
413+
// `state`. If we got the AgentCore session_id shape, the user
414+
// likely configured an `actor=app` flow against an AgentCore
415+
// Identity provider — that path is parked, error out clearly.
416+
if (callback.kind !== 'direct-oauth') {
431417
throw new CliError(
432-
'Localhost callback did not surface code/state. This indicates the callback '
433-
+ 'server module is in legacy AgentCore-only mode; rebuild the CLI.',
418+
'Localhost callback returned an AgentCore session_id, not a direct OAuth code. '
419+
+ 'Phase 2.0b Option 2 only supports the direct redirect — verify Linear\'s '
420+
+ 'redirect URI is set to http://localhost:8080/oauth/callback and re-run.',
434421
);
435422
}
436423
if (callback.state !== state) {

cli/src/oauth-callback-server.ts

Lines changed: 36 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -49,26 +49,34 @@ const FAILURE_HTML = `<!doctype html>
4949
<style>body{font-family:system-ui,sans-serif;max-width:480px;margin:8em auto;text-align:center;color:#222}h1{color:#c00}p{color:#666}</style></head>
5050
<body><h1>✗ Authorization not captured</h1><p>The callback URL did not include a session_id. Re-run <code>bgagent linear setup</code> and try again.</p></body></html>`;
5151

52-
export interface CallbackResult {
53-
/**
54-
* Value of the `session_id` query param if present (AgentCore-style
55-
* redirect). Null in direct-OAuth flows where Linear redirects with
56-
* `code` + `state` instead.
57-
*/
58-
readonly sessionId: string | null;
59-
/**
60-
* OAuth `code` from a direct-Linear redirect (Phase 2.0b Option 2).
61-
* Null in AgentCore-style flows where AWS performs the code-to-token
62-
* exchange itself.
63-
*/
64-
readonly code: string | null;
65-
/**
66-
* OAuth `state` from a direct-Linear redirect — caller MUST verify
67-
* against the value passed into `buildAuthorizationUrl` to prevent
68-
* CSRF. Null in AgentCore-style flows.
69-
*/
70-
readonly state: string | null;
71-
}
52+
/**
53+
* Discriminated union over the two redirect shapes Linear may send to
54+
* the localhost callback. Was previously an all-nullable struct
55+
* `{ sessionId: string | null, code: string | null, state: string | null }`
56+
* which let callers (and tests) construct nonsense values like
57+
* "all three null" or "sessionId AND code+state set" — neither is
58+
* actually reachable in production. Splitting into two cases makes
59+
* downstream pattern-matching exhaustive and the "impossible state"
60+
* unrepresentable.
61+
*
62+
* - `agentcore`: legacy AgentCore Identity USER_FEDERATION redirect.
63+
* AWS handles the code-for-token exchange itself; we receive only
64+
* the session_id we use to poll for the resulting token. Parked
65+
* path; kept for the eventual 2.0c resume.
66+
* - `direct-oauth`: Phase 2.0b Option 2. Linear redirects directly to
67+
* localhost with `code` + `state`. Caller MUST verify `state` against
68+
* the value passed into `buildAuthorizationUrl` to prevent CSRF.
69+
*/
70+
export type CallbackResult =
71+
| {
72+
readonly kind: 'agentcore';
73+
readonly sessionId: string;
74+
}
75+
| {
76+
readonly kind: 'direct-oauth';
77+
readonly code: string;
78+
readonly state: string;
79+
};
7280

7381
export interface CallbackServerOptions {
7482
/**
@@ -179,8 +187,15 @@ export async function awaitOauthCallback(
179187
}
180188
res.statusCode = 200;
181189
res.setHeader('content-type', 'text/html; charset=utf-8');
190+
// Build the discriminated union value here so callers don't
191+
// see the all-nullable shape. Direct-OAuth path takes
192+
// precedence: if Linear sent both session_id AND code+state
193+
// (shouldn't happen, but defensively…) we treat it as direct.
194+
const result: CallbackResult = code && state
195+
? { kind: 'direct-oauth', code, state }
196+
: { kind: 'agentcore', sessionId: sessionId! };
182197
res.once('finish', () => {
183-
settle(() => resolve({ sessionId, code, state }));
198+
settle(() => resolve(result));
184199
});
185200
res.end(SUCCESS_HTML);
186201
},

cli/test/oauth-callback-server.test.ts

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ describe('awaitOauthCallback', () => {
5050
// Jest's default test isolation per file. If a developer has another
5151
// bgagent setup running locally, expect EADDRINUSE.
5252

53-
test('captures session_id from the first valid request and resolves', async () => {
53+
test('captures session_id from the first valid request and resolves with kind=agentcore', async () => {
5454
// Fire the server + the request in parallel; the server resolves once it
5555
// sees the request, then closes.
5656
const expectedSessionId = 'urn:ietf:params:oauth:request_uri:test-uuid';
@@ -60,12 +60,15 @@ describe('awaitOauthCallback', () => {
6060
const requestPromise = localGet(`/oauth/callback?session_id=${encodeURIComponent(expectedSessionId)}`);
6161

6262
const [callbackResult, response] = await Promise.all([callbackPromise, requestPromise]);
63-
expect(callbackResult.sessionId).toBe(expectedSessionId);
63+
expect(callbackResult.kind).toBe('agentcore');
64+
if (callbackResult.kind === 'agentcore') {
65+
expect(callbackResult.sessionId).toBe(expectedSessionId);
66+
}
6467
expect(response.status).toBe(200);
6568
expect(response.body).toContain('Linear authorized');
6669
});
6770

68-
test('captures code+state from a direct Linear OAuth redirect', async () => {
71+
test('captures code+state from a direct Linear OAuth redirect with kind=direct-oauth', async () => {
6972
// Phase 2.0b Option 2 path: Linear redirects with `code` + `state`
7073
// (no AgentCore proxy in the middle).
7174
const callbackPromise = awaitOauthCallback({ timeoutMs: 5_000 });
@@ -75,9 +78,11 @@ describe('awaitOauthCallback', () => {
7578
);
7679

7780
const [callbackResult, response] = await Promise.all([callbackPromise, requestPromise]);
78-
expect(callbackResult.code).toBe('lin_authcode_abc');
79-
expect(callbackResult.state).toBe('stateuuid');
80-
expect(callbackResult.sessionId).toBeNull();
81+
expect(callbackResult.kind).toBe('direct-oauth');
82+
if (callbackResult.kind === 'direct-oauth') {
83+
expect(callbackResult.code).toBe('lin_authcode_abc');
84+
expect(callbackResult.state).toBe('stateuuid');
85+
}
8186
expect(response.status).toBe(200);
8287
});
8388

0 commit comments

Comments
 (0)