Skip to content

Commit 9d28db7

Browse files
authored
fix(runtime): harden boundaries and simplify tooling (oomol-lab#169)
## Summary - fail closed across runtime auth, DNS validation, provider egress, and proxy errors - coalesce concurrent OAuth refreshes and remove the MCP connection-summary N+1 reads - preserve provider schema fields and tighten Twitter media action contracts - replace hand-written CLI parsing and remove implementation-detail web tests ## Verification - `npm run generate:catalog` - `npm run fix-check` - `npm test` (507 tests) - `npm run build:web` - `git diff --check`
1 parent 3383741 commit 9d28db7

19 files changed

Lines changed: 407 additions & 374 deletions

examples/local-http/client.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,9 @@ function bearerHeaders(token: string | undefined, headers: HeadersInit): Headers
1111
return headers;
1212
}
1313

14-
return {
15-
...headers,
16-
authorization: `Bearer ${token}`,
17-
};
14+
const authenticatedHeaders = new Headers(headers);
15+
authenticatedHeaders.set("authorization", `Bearer ${token}`);
16+
return authenticatedHeaders;
1817
}
1918

2019
export async function fetchJson<T>(url: string, init: RequestInit = {}): Promise<T> {

scripts/runtime-data.ts

Lines changed: 37 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1,78 +1,58 @@
11
import { mkdir } from "node:fs/promises";
22
import { join, resolve } from "node:path";
3+
import { parseArgs } from "node:util";
34
import { createSecretCodec } from "../src/server/secrets/secret-codec.ts";
45
import { SqliteRuntimeDatabase } from "../src/server/storage/sqlite-runtime-store.ts";
56

6-
const command = process.argv[2];
7-
const options = parseOptions(process.argv.slice(3));
8-
const dataDir = resolve(options.dataDir ?? process.env.OOMOL_CONNECT_DATA_DIR ?? join(process.cwd(), "data"));
9-
const databasePath = join(dataDir, "connect.sqlite");
10-
const secretCodec = createSecretCodec(process.env.OOMOL_CONNECT_ENCRYPTION_KEY);
7+
const { positionals, values: options } = parseArgs({
8+
args: process.argv.slice(2),
9+
allowPositionals: true,
10+
options: {
11+
"data-dir": { type: "string" },
12+
plain: { type: "boolean" },
13+
yes: { type: "boolean" },
14+
},
15+
strict: true,
16+
});
17+
const [command] = positionals;
1118

12-
if (!command || !["reset", "rotate-key"].includes(command)) {
19+
if (positionals.length !== 1 || (command !== "reset" && command !== "rotate-key")) {
1320
printUsageAndExit();
1421
}
1522

16-
await mkdir(dataDir, { recursive: true });
17-
23+
const nextEncryptionKey = process.env.OOMOL_CONNECT_NEW_ENCRYPTION_KEY;
1824
if (command === "rotate-key") {
19-
const nextEncryptionKey = process.env.OOMOL_CONNECT_NEW_ENCRYPTION_KEY;
20-
if (!nextEncryptionKey && options.plain !== "true") {
21-
throw new Error("rotate-key requires OOMOL_CONNECT_NEW_ENCRYPTION_KEY unless --plain is set.");
25+
if (options.yes) {
26+
throw new Error("--yes is only valid with reset.");
2227
}
23-
const database = new SqliteRuntimeDatabase(databasePath, { secretCodec });
24-
try {
25-
await database.rotateSecretCodec(createSecretCodec(options.plain === "true" ? undefined : nextEncryptionKey));
26-
console.log(`Rotated runtime secret encryption in ${databasePath}.`);
27-
} finally {
28-
database.close();
28+
if (!nextEncryptionKey && !options.plain) {
29+
throw new Error("rotate-key requires OOMOL_CONNECT_NEW_ENCRYPTION_KEY unless --plain is set.");
2930
}
3031
} else {
31-
const database = new SqliteRuntimeDatabase(databasePath, { secretCodec });
32-
try {
33-
if (options.yes !== "true") {
34-
throw new Error("reset requires --yes.");
35-
}
36-
database.resetRuntimeData();
37-
console.log(`Reset runtime data in ${databasePath}.`);
38-
} finally {
39-
database.close();
32+
if (options.plain) {
33+
throw new Error("--plain is only valid with rotate-key.");
34+
}
35+
if (!options.yes) {
36+
throw new Error("reset requires --yes.");
4037
}
4138
}
4239

43-
type RuntimeDataCommandOptions = {
44-
dataDir?: string;
45-
plain?: string;
46-
yes?: string;
47-
};
48-
49-
function parseOptions(args: string[]): RuntimeDataCommandOptions {
50-
const options: RuntimeDataCommandOptions = {};
51-
for (let index = 0; index < args.length; index += 1) {
52-
const arg = args[index];
53-
if (arg === "--yes") {
54-
options.yes = "true";
55-
continue;
56-
}
57-
if (arg === "--plain") {
58-
options.plain = "true";
59-
continue;
60-
}
61-
62-
const value = args[index + 1];
63-
if (!value) {
64-
throw new Error(`${arg} requires a value.`);
65-
}
40+
const dataDir = resolve(options["data-dir"] ?? process.env.OOMOL_CONNECT_DATA_DIR ?? join(process.cwd(), "data"));
41+
const databasePath = join(dataDir, "connect.sqlite");
42+
const secretCodec = createSecretCodec(process.env.OOMOL_CONNECT_ENCRYPTION_KEY);
43+
await mkdir(dataDir, { recursive: true });
6644

67-
if (arg === "--data-dir") {
68-
options.dataDir = value;
69-
} else {
70-
throw new Error(`Unknown option: ${arg}.`);
71-
}
72-
index += 1;
45+
const database = new SqliteRuntimeDatabase(databasePath, { secretCodec });
46+
try {
47+
if (command === "rotate-key") {
48+
await database.rotateSecretCodec(createSecretCodec(options.plain ? undefined : nextEncryptionKey));
49+
console.log(`Rotated runtime secret encryption in ${databasePath}.`);
50+
} else {
51+
database.resetRuntimeData();
52+
console.log(`Reset runtime data in ${databasePath}.`);
7353
}
74-
75-
return options;
54+
} finally {
55+
database.close();
7656
}
7757

7858
function printUsageAndExit(): never {

scripts/search-actions.ts

Lines changed: 16 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,56 +1,34 @@
1+
import { parseArgs } from "node:util";
12
import { loadCatalog } from "../src/catalog-store.ts";
23
import { DEFAULT_ACTION_SEARCH_LIMIT, buildActionSearchIndex, searchActions } from "../src/core/action-search.ts";
34

4-
const options = parseOptions(process.argv.slice(2));
5+
const { values: options } = parseArgs({
6+
args: process.argv.slice(2),
7+
options: {
8+
limit: { type: "string" },
9+
query: { type: "string", short: "q" },
10+
service: { type: "string" },
11+
},
12+
strict: true,
13+
});
514
if (!options.query) {
615
printUsageAndExit();
716
}
817

18+
const limit = options.limit == null ? DEFAULT_ACTION_SEARCH_LIMIT : Number(options.limit);
19+
if (!Number.isInteger(limit) || limit < 1) {
20+
throw new Error("--limit must be a positive integer.");
21+
}
22+
923
const catalog = await loadCatalog();
1024
const index = buildActionSearchIndex(catalog.actions);
1125
const results = searchActions(index, options.query, {
1226
service: options.service,
13-
limit: options.limit ?? DEFAULT_ACTION_SEARCH_LIMIT,
27+
limit,
1428
});
1529

1630
console.log(JSON.stringify(results, null, 2));
1731

18-
interface SearchActionOptions {
19-
query?: string;
20-
service?: string;
21-
limit?: number;
22-
}
23-
24-
function parseOptions(args: string[]): SearchActionOptions {
25-
const options: SearchActionOptions = {};
26-
for (let index = 0; index < args.length; index += 1) {
27-
const arg = args[index];
28-
const value = args[index + 1];
29-
if (arg === "--query" || arg === "-q") {
30-
options.query = requireValue(arg, value);
31-
} else if (arg === "--service") {
32-
options.service = requireValue(arg, value);
33-
} else if (arg === "--limit") {
34-
const limit = Number(requireValue(arg, value));
35-
if (!Number.isInteger(limit) || limit < 1) {
36-
throw new Error("--limit must be a positive integer.");
37-
}
38-
options.limit = limit;
39-
} else {
40-
throw new Error(`Unknown option: ${arg}`);
41-
}
42-
index += 1;
43-
}
44-
return options;
45-
}
46-
47-
function requireValue(option: string, value: string | undefined): string {
48-
if (!value) {
49-
throw new Error(`${option} requires a value.`);
50-
}
51-
return value;
52-
}
53-
5432
function printUsageAndExit(): never {
5533
console.error(`Usage:
5634
node scripts/search-actions.ts --query "send mail gmail" [--service gmail] [--limit 10]`);

src/connection-service.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -490,6 +490,46 @@ describe("ConnectionService", () => {
490490
);
491491
});
492492

493+
it("shares an in-flight OAuth refresh across concurrent requests", async () => {
494+
const store = new MemoryConnectionStore();
495+
const oauthClientConfigs = createOAuthClientConfigs([oauthProvider]);
496+
const service = createService([oauthProvider], {
497+
oauthCredentials: new OAuthCredentialRefreshService(oauthClientConfigs),
498+
store,
499+
});
500+
await oauthClientConfigs.upsertConfig({
501+
service: "example",
502+
clientId: "client-id",
503+
clientSecret: "client-secret",
504+
});
505+
await store.set("example", "default", {
506+
authType: "oauth2",
507+
accessToken: "expired-token",
508+
tokenType: "Bearer",
509+
refreshToken: "refresh-token",
510+
expiresAt: "2026-01-01T00:00:00.000Z",
511+
profile: testProfile,
512+
metadata: {},
513+
});
514+
515+
const fetcher = vi.fn(async () =>
516+
Response.json({
517+
access_token: "fresh-token",
518+
expires_in: 3600,
519+
token_type: "Bearer",
520+
}),
521+
);
522+
vi.stubGlobal("fetch", fetcher);
523+
524+
const credentials = await Promise.all([service.getCredential("example"), service.getCredential("example")]);
525+
526+
expect(credentials).toEqual([
527+
expect.objectContaining({ accessToken: "fresh-token" }),
528+
expect.objectContaining({ accessToken: "fresh-token" }),
529+
]);
530+
expect(fetcher).toHaveBeenCalledOnce();
531+
});
532+
493533
it("does not overwrite a connection recreated during OAuth refresh", async () => {
494534
const store = new MemoryConnectionStore();
495535
const oauthClientConfigs = createOAuthClientConfigs([oauthProvider]);

src/connection-service.ts

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ interface PreviousCredentialRuntimeData {
107107
}
108108

109109
type CredentialValidatorCall = () => Promise<CredentialValidationResult | void> | undefined;
110+
type OAuthCredential = Extract<ResolvedCredential, { authType: "oauth2" }>;
110111

111112
/**
112113
* Coordinates local provider connection state.
@@ -116,6 +117,7 @@ type CredentialValidatorCall = () => Promise<CredentialValidationResult | void>
116117
*/
117118
export class ConnectionService {
118119
private readonly catalog: CatalogStore;
120+
private readonly oauthCredentialRefreshes = new Map<string, Promise<OAuthCredential>>();
119121
private readonly oauthCredentials?: IOAuthCredentialRefresher;
120122
private readonly providerLoader: IProviderLoader;
121123
private readonly store: IConnectionStore;
@@ -492,9 +494,9 @@ export class ConnectionService {
492494

493495
private async resolveOAuthCredential(
494496
connection: StoredConnection,
495-
credential: Extract<ResolvedCredential, { authType: "oauth2" }>,
496-
): Promise<Extract<ResolvedCredential, { authType: "oauth2" }>> {
497-
const { id, service, connectionName } = connection;
497+
credential: OAuthCredential,
498+
): Promise<OAuthCredential> {
499+
const service = connection.service;
498500
if (!isOAuthCredentialExpired(credential)) {
499501
return credential;
500502
}
@@ -513,7 +515,29 @@ export class ConnectionService {
513515
);
514516
}
515517

516-
const nextCredential = await this.oauthCredentials.refresh(service, credential);
518+
const currentRefresh = this.oauthCredentialRefreshes.get(connection.id);
519+
if (currentRefresh) {
520+
return currentRefresh;
521+
}
522+
523+
const refresh = this.refreshOAuthCredential(connection, credential, this.oauthCredentials);
524+
this.oauthCredentialRefreshes.set(connection.id, refresh);
525+
try {
526+
return await refresh;
527+
} finally {
528+
if (this.oauthCredentialRefreshes.get(connection.id) === refresh) {
529+
this.oauthCredentialRefreshes.delete(connection.id);
530+
}
531+
}
532+
}
533+
534+
private async refreshOAuthCredential(
535+
connection: StoredConnection,
536+
credential: OAuthCredential,
537+
refresher: IOAuthCredentialRefresher,
538+
): Promise<OAuthCredential> {
539+
const { id, service, connectionName } = connection;
540+
const nextCredential = await refresher.refresh(service, credential);
517541
const updated = await this.store.updateCredential({ id, service, connectionName, credential: nextCredential });
518542
if (!updated) {
519543
throw new ConnectionError(

src/core/guarded-fetch.test.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -407,6 +407,14 @@ describe("createGuardedFetch resolved-address validation", () => {
407407
expect(calls).toHaveLength(0);
408408
});
409409

410+
it("fails closed when an enabled lookup returns no addresses", async () => {
411+
const { transport, calls } = createTransport([]);
412+
const guarded = createGuardedFetch({ fetch: transport, lookup: async () => [] });
413+
414+
await expect(guarded("https://unresolved.example.com/")).rejects.toThrow(/could not be resolved/u);
415+
expect(calls).toHaveLength(0);
416+
});
417+
410418
it("re-validates resolved addresses for every redirect hop", async () => {
411419
const { transport, calls } = createTransport([redirectTo("https://metadata.attacker.com/creds")]);
412420
const guarded = createGuardedFetch({

src/core/guarded-fetch.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,9 @@ async function assertResolvedAddressesAllowed(
297297
if (!Array.isArray(results)) {
298298
throw policy.createError(`${fieldName} could not be resolved for validation`);
299299
}
300+
if (results.length === 0) {
301+
throw policy.createError(`${fieldName} could not be resolved for validation`);
302+
}
300303
for (const entry of results) {
301304
if (entry && typeof entry.address === "string" && isBlockedIpAddress(entry.address, policy.allowPrivateNetwork)) {
302305
throw policy.createError(`${fieldName} must not resolve to private or reserved IP addresses`);

src/core/json-schema.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import { describe, expect, it } from "vitest";
2+
import { jsonSchema } from "./json-schema.ts";
3+
4+
describe("jsonSchema.looseObject", () => {
5+
it("keeps properties whose names overlap schema option names", () => {
6+
expect(
7+
jsonSchema.looseObject(
8+
"A provider payload.",
9+
{
10+
default: jsonSchema.boolean("Whether this is the default item."),
11+
description: jsonSchema.string("The provider description."),
12+
format: jsonSchema.string("The provider format."),
13+
},
14+
{ default: {} },
15+
),
16+
).toEqual({
17+
type: "object",
18+
properties: {
19+
default: { type: "boolean", description: "Whether this is the default item." },
20+
description: { type: "string", description: "The provider description." },
21+
format: { type: "string", description: "The provider format." },
22+
},
23+
additionalProperties: true,
24+
description: "A provider payload.",
25+
default: {},
26+
});
27+
});
28+
});

src/core/json-schema.ts

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -273,14 +273,12 @@ export const jsonSchema = {
273273
): JsonSchema {
274274
const properties =
275275
typeof propertiesOrDescription === "string"
276-
? isJsonSchemaOptions(optionsOrProperties)
277-
? {}
278-
: optionsOrProperties
276+
? (optionsOrProperties as Record<string, JsonSchema>)
279277
: propertiesOrDescription;
280278
const resolvedOptions =
281279
typeof propertiesOrDescription === "string"
282280
? {
283-
...(isJsonSchemaOptions(optionsOrProperties) ? optionsOrProperties : maybeOptions),
281+
...maybeOptions,
284282
description: propertiesOrDescription,
285283
}
286284
: (optionsOrProperties as JsonSchemaOptions);
@@ -351,7 +349,3 @@ function withOptions(schema: JsonSchema, options: JsonSchemaOptions): JsonSchema
351349
if (options.format) schema.format = options.format;
352350
return schema;
353351
}
354-
355-
function isJsonSchemaOptions(value: JsonSchemaOptions | Record<string, JsonSchema>): value is JsonSchemaOptions {
356-
return "description" in value || "default" in value || "format" in value;
357-
}

0 commit comments

Comments
 (0)