Skip to content

Commit dbb48ec

Browse files
RhysSullivanRhysSullivan
authored
Fix WorkOS Vault credential owner partitioning (#950)
* Fix WorkOS Vault credential owner partitioning The vault provider filed every credential's metadata under the acting caller's partition (ownerOf(binding)) — so an org member creating a workspace connection filed it under their OWN user partition. Org-shared credentials then resolved only for whoever pasted them; every other member got connection_value_missing, despite the key being stored fine. Partition by the owner embedded in the item id instead (connection|oauth|oauth-client:<owner>:…). Three regression tests cover it: a workspace connection created by one member resolves for another (and for a subjectless automation), a personal connection stays private, and oauth/oauth-client ids partition by their embedded owner. The bug dates to the v1.5 provider rework (the v1->v2 migration itself filed org metadata correctly); 106 rows mis-filed live since. A one-off cloud migration (db:repartition-vault:prod, --dry-run supported) re-files them org-ward — only the metadata pointer moves, the Vault object is untouched. * Format changeset
1 parent f485e4a commit dbb48ec

5 files changed

Lines changed: 326 additions & 7 deletions

File tree

‎.changeset/vault-org-connection-partition.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"executor": patch
3+
---
4+
5+
**Fix: workspace connections were resolvable only by whoever created them**
6+
7+
The WorkOS Vault credential provider filed a credential's metadata under the _acting user's_ private partition instead of the credential's own owner. Org-shared connections (and OAuth tokens, and OAuth client secrets) created by one member therefore resolved only for that member — every other member of the workspace hit `connection_value_missing` ("no resolvable credential value") even though the key was saved correctly. The provider now partitions by the owner embedded in the credential's item id (`connection:org:…` → org-shared, `connection:user:…` → private), so a key pasted by one member works for the whole workspace. Pre-existing mis-filed metadata is repaired by a one-off cloud migration (`db:repartition-vault:prod`); the stored secret value itself was never affected.

‎apps/cloud/package.json‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
"db:migrate:prod": "op run --env-file=.env.production -- bun --bun ../../node_modules/.bun/node_modules/drizzle-kit/bin.cjs migrate",
1616
"db:migrate-auth:prod": "op run --env-file=.env.production -- bun run scripts/migrate-auth-configs.ts",
1717
"db:migrate-auth:dev": "op run --env-file=.env.op -- bun run scripts/migrate-auth-configs.ts",
18+
"db:repartition-vault:prod": "op run --env-file=.env.production -- bun run scripts/repartition-vault-metadata.ts",
19+
"db:repartition-vault:dev": "op run --env-file=.env.op -- bun run scripts/repartition-vault-metadata.ts",
1820
"db:migrate:dev": "op run --env-file=.env.op -- bun --bun ../../node_modules/.bun/node_modules/drizzle-kit/bin.cjs migrate",
1921
"build": "node scripts/build.mjs",
2022
"preview": "vite preview",

‎apps/cloud/scripts/repartition-vault-metadata.ts‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
/* oxlint-disable executor/no-try-catch-or-throw -- boundary: out-of-band migration script over a raw postgres connection */
2+
// ---------------------------------------------------------------------------
3+
// One-off data migration: re-file mis-partitioned WorkOS Vault metadata rows.
4+
//
5+
// A bug in the v1.5 vault provider (`ownerOf(binding)`) filed every credential
6+
// created by a bound user — including ORG-shared connections — under that
7+
// user's private partition. Org-shared credentials then resolved only for
8+
// whoever pasted them; every other org member got `connection_value_missing`.
9+
//
10+
// The fix (secret-store.ts: `ownerForItemId`) files by the owner embedded in
11+
// the item id. This script repairs the rows already written wrong: an item id
12+
// of `connection:org:…` / `oauth:org:…` / `oauth-client:org:…` whose metadata
13+
// row sits at owner='user' is moved to owner='org', subject=''. The Vault
14+
// object itself is untouched (flat context) — only the metadata pointer moves.
15+
//
16+
// bun run db:repartition-vault:prod # op run --env-file=.env.production
17+
// bun run db:repartition-vault:dev # against the local PGlite dev db
18+
//
19+
// Idempotent — already-correct rows are skipped. Pass --dry-run to print the
20+
// plan without writing.
21+
// ---------------------------------------------------------------------------
22+
23+
import postgres from "postgres";
24+
25+
const VAULT_PLUGIN_ID = "workosVault";
26+
const METADATA_COLLECTION = "metadata";
27+
// Item-id prefixes whose second colon-segment is the owning partition.
28+
const OWNER_SCOPED_PREFIXES = ["connection", "oauth", "oauth-client"];
29+
30+
const connectionString = process.env.DATABASE_URL;
31+
if (!connectionString) {
32+
console.error("DATABASE_URL is not set");
33+
process.exit(1);
34+
}
35+
const dryRun = process.argv.includes("--dry-run");
36+
37+
// Direct (non-Hyperdrive) connection — PlanetScale requires TLS.
38+
const sql = postgres(connectionString, { max: 1, prepare: false, ssl: "require" });
39+
40+
type Row = {
41+
row_id: string;
42+
tenant: string;
43+
owner: string;
44+
subject: string;
45+
key: string;
46+
};
47+
48+
const embeddedOwner = (key: string): "org" | "user" | null => {
49+
const [prefix, owner] = key.split(":");
50+
if (!OWNER_SCOPED_PREFIXES.includes(prefix ?? "")) return null;
51+
return owner === "org" || owner === "user" ? owner : null;
52+
};
53+
54+
try {
55+
const rows = await sql<Row[]>`
56+
SELECT row_id, tenant, owner, subject, key
57+
FROM plugin_storage
58+
WHERE plugin_id = ${VAULT_PLUGIN_ID} AND collection = ${METADATA_COLLECTION}
59+
`;
60+
61+
// A row is mis-filed when its stored partition disagrees with the owner
62+
// embedded in its item id. In practice only org credentials stuck in a user
63+
// partition, but compute it generally and symmetrically.
64+
const misfiled = rows.filter((row) => {
65+
const want = embeddedOwner(row.key);
66+
if (want === null) return false;
67+
const wantSubject = want === "org" ? "" : row.subject;
68+
return row.owner !== want || row.subject !== wantSubject;
69+
});
70+
71+
console.log(`${rows.length} vault metadata row(s), ${misfiled.length} mis-partitioned`);
72+
const byPrefix = new Map<string, number>();
73+
for (const row of misfiled) {
74+
const prefix = row.key.split(":")[0] ?? "?";
75+
byPrefix.set(prefix, (byPrefix.get(prefix) ?? 0) + 1);
76+
}
77+
for (const [prefix, n] of byPrefix) console.log(` ${prefix}: ${n}`);
78+
79+
if (dryRun) {
80+
for (const row of misfiled) {
81+
console.log(
82+
` would move ${row.owner}/${row.subject || "''"} → ${embeddedOwner(row.key)} : ${row.key}`,
83+
);
84+
}
85+
} else if (misfiled.length > 0) {
86+
let moved = 0;
87+
await sql.begin(async (tx) => {
88+
for (const row of misfiled) {
89+
const want = embeddedOwner(row.key)!;
90+
const wantSubject = want === "org" ? "" : row.subject;
91+
// Re-file in place: copy the data into the correct partition (no-op if a
92+
// post-fix write already created it), then drop the mis-filed row.
93+
await tx`
94+
INSERT INTO plugin_storage
95+
(row_id, tenant, owner, subject, plugin_id, collection, key, data, created_at, updated_at)
96+
SELECT row_id || '-repart', tenant, ${want}, ${wantSubject},
97+
plugin_id, collection, key, data, created_at, now()
98+
FROM plugin_storage WHERE row_id = ${row.row_id}
99+
ON CONFLICT (tenant, owner, subject, plugin_id, collection, key) DO NOTHING
100+
`;
101+
await tx`DELETE FROM plugin_storage WHERE row_id = ${row.row_id}`;
102+
moved += 1;
103+
}
104+
});
105+
console.log(`re-filed ${moved} row(s)`);
106+
}
107+
} finally {
108+
await sql.end();
109+
}

‎packages/plugins/workos-vault/src/sdk/secret-store.test.ts‎

Lines changed: 180 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -359,10 +359,8 @@ describe("WorkOS Vault credential provider", () => {
359359
}),
360360
);
361361

362-
it.effect("files metadata under the executor owner binding", () =>
362+
it.effect("an id without an embedded owner falls back to the caller binding", () =>
363363
Effect.gen(function* () {
364-
// A bound subject writes the user partition; the provider still keys
365-
// solely by the opaque id, so resolution is unchanged.
366364
const userBinding: OwnerBinding = {
367365
tenant: Tenant.make("tenant-a"),
368366
subject: Subject.make("subject-a"),
@@ -374,3 +372,182 @@ describe("WorkOS Vault credential provider", () => {
374372
}),
375373
);
376374
});
375+
376+
// ---------------------------------------------------------------------------
377+
// Owner-partition regression: a credential's metadata must be filed under the
378+
// CREDENTIAL's owner (embedded in the item id), not the acting caller's
379+
// binding. Org-shared credentials filed under the creator's user partition are
380+
// resolvable only by whoever pasted them — every other org member gets
381+
// `connection_value_missing`. This fake models the real cross-owner visibility
382+
// (`org` ∪ the caller's own `user` partition) the flat fake above does not.
383+
// ---------------------------------------------------------------------------
384+
385+
const makePartitionedBacking = () => {
386+
type Row = {
387+
readonly owner: Owner;
388+
readonly subject: string;
389+
readonly collection: string;
390+
readonly key: string;
391+
readonly data: unknown;
392+
};
393+
const rows = new Map<string, Row>();
394+
const partKey = (owner: string, subject: string, collection: string, key: string) =>
395+
`${owner}${subject}${collection}${key}`;
396+
const subjectFor = (owner: Owner, binding: OwnerBinding) =>
397+
owner === "org" ? "" : String(binding.subject ?? "");
398+
const toEntry = (row: Row): PluginStorageEntry => ({
399+
id: `${row.collection} ${row.key}`,
400+
owner: row.owner,
401+
pluginId: "workosVault",
402+
collection: row.collection,
403+
key: row.key,
404+
data: row.data,
405+
createdAt: new Date(0),
406+
updatedAt: new Date(0),
407+
});
408+
409+
const depsFor = (binding: OwnerBinding): StorageDeps => {
410+
// Mirrors `ownerVisibilityCondition`: a pure-org caller sees only org rows;
411+
// a bound subject sees its own user rows plus org rows (user first).
412+
const visibleOwners: readonly Owner[] =
413+
binding.subject == null ? [Owner.make("org")] : [Owner.make("user"), Owner.make("org")];
414+
const findVisible = (collection: string, key: string): Row | null => {
415+
for (const owner of visibleOwners) {
416+
const row = rows.get(partKey(owner, subjectFor(owner, binding), collection, key));
417+
if (row) return row;
418+
}
419+
return null;
420+
};
421+
const pluginStorage = {
422+
collection: () => expect.unreachable("collection() not used by the metadata store"),
423+
get: (input: { collection: string; key: string }) =>
424+
Effect.sync(() => {
425+
const row = findVisible(input.collection, input.key);
426+
return row ? (toEntry(row) as never) : null;
427+
}),
428+
getForOwner: (input: { collection: string; key: string; owner: Owner }) =>
429+
Effect.sync(() => {
430+
const row = rows.get(
431+
partKey(input.owner, subjectFor(input.owner, binding), input.collection, input.key),
432+
);
433+
return row ? (toEntry(row) as never) : null;
434+
}),
435+
list: (input: { collection: string }) =>
436+
Effect.sync(
437+
() =>
438+
[...rows.values()]
439+
.filter((row) => row.collection === input.collection)
440+
.map(toEntry) as never,
441+
),
442+
put: (input: { collection: string; key: string; owner: Owner; data: unknown }) =>
443+
Effect.sync(() => {
444+
const subject = subjectFor(input.owner, binding);
445+
const row: Row = {
446+
owner: input.owner,
447+
subject,
448+
collection: input.collection,
449+
key: input.key,
450+
data: input.data,
451+
};
452+
rows.set(partKey(input.owner, subject, input.collection, input.key), row);
453+
return toEntry(row) as never;
454+
}),
455+
remove: (input: { collection: string; key: string; owner: Owner }) =>
456+
Effect.sync(() => {
457+
rows.delete(
458+
partKey(input.owner, subjectFor(input.owner, binding), input.collection, input.key),
459+
);
460+
}),
461+
};
462+
return {
463+
owner: binding,
464+
// oxlint-disable-next-line executor/no-double-cast -- test boundary: blobs unused
465+
blobs: undefined as never,
466+
// oxlint-disable-next-line executor/no-double-cast -- test boundary: partition-aware fake
467+
pluginStorage: pluginStorage as never,
468+
};
469+
};
470+
return { depsFor };
471+
};
472+
473+
const userBinding = (subject: string): OwnerBinding => ({
474+
tenant: Tenant.make("tenant-a"),
475+
subject: Subject.make(subject),
476+
});
477+
478+
describe("WorkOS Vault — credential owner partitioning", () => {
479+
it.effect("a workspace connection created by one member resolves for another", () =>
480+
Effect.gen(function* () {
481+
const backing = makePartitionedBacking();
482+
const client = makeFakeClient(); // shared vault — the value object is org-wide
483+
const creator = makeWorkOSVaultCredentialProvider({
484+
client,
485+
store: makeWorkosVaultStore(backing.depsFor(userBinding("subject-a"))),
486+
});
487+
const other = makeWorkOSVaultCredentialProvider({
488+
client,
489+
store: makeWorkosVaultStore(backing.depsFor(userBinding("subject-b"))),
490+
});
491+
492+
// user A pastes the org connection's API key
493+
yield* creator.set!(id("connection:org:exa_search_api:workspaceexa:token"), "exa_secret");
494+
495+
// user B (same org, different subject) resolves the same org connection
496+
expect(yield* other.get(id("connection:org:exa_search_api:workspaceexa:token"))).toBe(
497+
"exa_secret",
498+
);
499+
// …and an automation context with no subject resolves it too
500+
const automation = makeWorkOSVaultCredentialProvider({
501+
client,
502+
store: makeWorkosVaultStore(
503+
backing.depsFor({ tenant: Tenant.make("tenant-a"), subject: null }),
504+
),
505+
});
506+
expect(yield* automation.get(id("connection:org:exa_search_api:workspaceexa:token"))).toBe(
507+
"exa_secret",
508+
);
509+
}),
510+
);
511+
512+
it.effect("a personal connection stays private to its owner", () =>
513+
Effect.gen(function* () {
514+
const backing = makePartitionedBacking();
515+
const client = makeFakeClient();
516+
const userA = makeWorkOSVaultCredentialProvider({
517+
client,
518+
store: makeWorkosVaultStore(backing.depsFor(userBinding("subject-a"))),
519+
});
520+
const userB = makeWorkOSVaultCredentialProvider({
521+
client,
522+
store: makeWorkosVaultStore(backing.depsFor(userBinding("subject-b"))),
523+
});
524+
525+
yield* userA.set!(id("connection:user:notion:personal:token"), "private_secret");
526+
527+
// user A resolves their own; user B cannot see the metadata → no value
528+
expect(yield* userA.get(id("connection:user:notion:personal:token"))).toBe("private_secret");
529+
expect(yield* userB.get(id("connection:user:notion:personal:token"))).toBeNull();
530+
}),
531+
);
532+
533+
it.effect("oauth and oauth-client ids are partitioned by their embedded owner", () =>
534+
Effect.gen(function* () {
535+
const backing = makePartitionedBacking();
536+
const client = makeFakeClient();
537+
const creator = makeWorkOSVaultCredentialProvider({
538+
client,
539+
store: makeWorkosVaultStore(backing.depsFor(userBinding("subject-a"))),
540+
});
541+
const other = makeWorkOSVaultCredentialProvider({
542+
client,
543+
store: makeWorkosVaultStore(backing.depsFor(userBinding("subject-b"))),
544+
});
545+
546+
yield* creator.set!(id("oauth:org:slack:workspace"), "access_tok");
547+
yield* creator.set!(id("oauth-client:org:my_app:secret"), "client_sec");
548+
549+
expect(yield* other.get(id("oauth:org:slack:workspace"))).toBe("access_tok");
550+
expect(yield* other.get(id("oauth-client:org:my_app:secret"))).toBe("client_sec");
551+
}),
552+
);
553+
});

‎packages/plugins/workos-vault/src/sdk/secret-store.ts‎

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,31 @@ type WorkosVaultMetadataData = typeof WorkosVaultMetadataData.Type;
9191

9292
/** Map the executor's (tenant, subject?) binding onto the storage `Owner`
9393
* literal: a bound subject writes the user's own partition, otherwise the
94-
* org-shared one. */
94+
* org-shared one. Fallback only — prefer `ownerForItemId`. */
9595
const ownerOf = (binding: OwnerBinding): Owner =>
9696
binding.subject == null ? Owner.make("org") : Owner.make("user");
9797

98+
// Item ids whose SECOND colon-segment is the owning partition:
99+
// connection:<owner>:<integration>:<name>:<variable>
100+
// oauth:<owner>:<integration>:<name>[:refresh]
101+
// oauth-client:<owner>:<slug>:secret
102+
const OWNER_SCOPED_PREFIXES: ReadonlySet<string> = new Set(["connection", "oauth", "oauth-client"]);
103+
104+
/** The partition a credential's metadata belongs to: the CREDENTIAL's owner
105+
* (embedded in the item id), NOT the acting caller's binding. An org member
106+
* creating a workspace connection must file org-shared metadata so every
107+
* other member can resolve it — filing under the creator's user partition
108+
* (the old `ownerOf(binding)` behavior) made shared credentials resolvable
109+
* only by whoever pasted them. Ids without an embedded owner fall back to the
110+
* caller binding. */
111+
const ownerForItemId = (id: string, binding: OwnerBinding): Owner => {
112+
const [prefix, owner] = id.split(":");
113+
if (OWNER_SCOPED_PREFIXES.has(prefix ?? "") && (owner === "org" || owner === "user")) {
114+
return Owner.make(owner);
115+
}
116+
return ownerOf(binding);
117+
};
118+
98119
// ---------------------------------------------------------------------------
99120
// WorkosVaultStore — typed metadata-store the plugin uses internally.
100121
//
@@ -113,7 +134,6 @@ export interface WorkosVaultStore {
113134

114135
export const makeWorkosVaultStore = (deps: StorageDeps): WorkosVaultStore => {
115136
const { pluginStorage } = deps;
116-
const owner = ownerOf(deps.owner);
117137

118138
const find = (id: string): Effect.Effect<MetadataRow | null, StorageFailure> =>
119139
pluginStorage
@@ -129,7 +149,7 @@ export const makeWorkosVaultStore = (deps: StorageDeps): WorkosVaultStore => {
129149
upsert: (row: MetadataRow) =>
130150
pluginStorage
131151
.put({
132-
owner,
152+
owner: ownerForItemId(row.id, deps.owner),
133153
collection: METADATA_COLLECTION,
134154
key: row.id,
135155
data: metadataData(row),
@@ -139,7 +159,11 @@ export const makeWorkosVaultStore = (deps: StorageDeps): WorkosVaultStore => {
139159
Effect.gen(function* () {
140160
const existing = yield* find(id);
141161
if (!existing) return false;
142-
yield* pluginStorage.remove({ owner, collection: METADATA_COLLECTION, key: id });
162+
yield* pluginStorage.remove({
163+
owner: ownerForItemId(id, deps.owner),
164+
collection: METADATA_COLLECTION,
165+
key: id,
166+
});
143167
return true;
144168
}),
145169
list: () =>

0 commit comments

Comments
 (0)