feat(workflow-executor): add OAuth credential store + deposit endpoint (PRD-367 PR1) by hercemer42 · Pull Request #1619 · ForestAdmin/agent-nodejs

hercemer42 · 2026-06-02T16:53:54Z

What

Executor-side persistence + intake for OAuth-protected MCP credentials (PRD-367, PR1 of the story):

ai_mcp_oauth_credentials table via a new Umzug migration (002) + McpOAuthCredentialsStore
CredentialEncryption: HKDF (read lazily from FOREST_EXECUTOR_ENCRYPTION_KEY, fixed context label) + AES-256-GCM, fail-closed
POST/DELETE /mcp-oauth-credentials on the existing koaJwt-authed HTTP server (user_id from the token), encrypts + upserts; returns a typed executor_encryption_key_missing (503) when the key is unset

Notes

Additive and dormant — nothing reads the table or calls the endpoint yet, so it merges with no production impact.
The encryption key is read lazily; an executor without OAuth in use boots unaffected.
enc_key_version is persisted per row for the deferred key-rotation work; decrypt is single-generation for now.

Tests

Unit — CredentialEncryption: round-trip, AES-GCM tamper/cross-key fail-closed, lazy/missing-key.
Store (sqlite) — upsert/get/delete, UNIQUE(user_id, mcp_server_id), nullable public-client fields, isolation, migration.
Route — JWT auth, user_id-from-token (never body), encrypt-before-persist, key-missing 503, body validation.

Refs PRD-367

🤖 Generated with Claude Code

Note

Add OAuth credential store and deposit/delete endpoints to workflow-executor

Adds McpOAuthCredentialsStore backed by a new ai_mcp_oauth_credentials table (keyed by user_id + mcp_server_id), with Umzug migration run at server startup.
Adds CredentialEncryption using AES-256-GCM with HKDF-derived keys from the FOREST_EXECUTOR_ENCRYPTION_KEY env var to encrypt refresh tokens and client secrets at rest.
Exposes POST /mcp-oauth-credentials to validate, encrypt, and upsert credentials bound to the JWT user, and DELETE /mcp-oauth-credentials/:mcpServerId to remove them.
Risk: if FOREST_EXECUTOR_ENCRYPTION_KEY is unset, deposit requests return 503 with executor_encryption_key_missing; the env var must be configured before deploying.

📊 Macroscope summarized b20ff7c. 59 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

linear-code · 2026-06-02T16:53:58Z

PRD-367

qltysh · 2026-06-02T16:55:17Z

1 new issue

Tool	Category	Rule	Count
qlty	Structure	Function with many returns (count = 5): handleTrigger	1

qltysh · 2026-06-02T16:59:51Z

Coverage Impact

⬇️ Merging this pull request will decrease total coverage on main by 0.02%.

Modified Files with Diff Coverage (7)

File	% Diff	Uncovered Line #s
packages/workflow-executor/src/build-workflow-executor.ts	100.0%
packages/workflow-executor/src/http/executor-http-server.ts	97.9%	127
packages/workflow-executor/src/errors.ts	100.0%
packages/workflow-executor/src/index.ts	100.0%
...workflow-executor/src/http/mcp-oauth-credentials-validators.ts	100.0%
...es/workflow-executor/src/stores/mcp-oauth-credentials-store.ts	85.2%	119, 131-134, 195
packages/workflow-executor/src/crypto/credential-encryption.ts	100.0%
Total	95.9%

🤖 Increase coverage with AI coding...

In the `feat/prd-367-pr1-executor-oauth-credentials` branch, add test coverage for this new code:

- `packages/workflow-executor/src/http/executor-http-server.ts` -- Line 127
- `packages/workflow-executor/src/stores/mcp-oauth-credentials-store.ts` -- Lines 119, 131-134, and 195

🚦 See full report on Qlty Cloud »

🛟 Help

Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.
Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.
File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)
- Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

Add the ai_mcp_oauth_credentials table (002 migration) + store, an HKDF/AES-GCM credential encryption helper read lazily from FOREST_EXECUTOR_ENCRYPTION_KEY, and a koaJwt-authed POST/DELETE /mcp-oauth-credentials endpoint. Additive and dormant: nothing reads the table or calls the endpoint yet. Refs PRD-367 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Pre-empt the security-review question raised in code review: the empty salt is deliberate (domain separation comes from the fixed HKDF info label, input is a single high-entropy secret).

Cover the buildDatabaseExecutor wiring of the credentials store and encryption into the HTTP server (and its absence in-memory), and pin the DELETE success contract to 204 with no body, replacing the todo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

hercemer42 · 2026-06-11T10:27:11Z

+
+    const body = (ctx.request.body ?? {}) as DepositCredentialsBody;
+
+    if (!body.mcpServerId || !body.refreshToken) {


Claude Fable 5 — [Violates conventions] The deposit body is a frontend HTTP body, and this package's CLAUDE.md (Boundary validation) requires those to be zod-validated with .strict() — as the sibling /trigger route does via pending-data-validators.ts. Here DepositCredentialsBody is a plain interface plus a truthy presence check, so: non-string values pass the guard (a numeric refreshToken reaches encryption.encrypt), an unparseable clientSecretExpiresAt persists as an Invalid Date instead of being rejected, over-length values for the bounded columns (2048/64/2048) aren't caught at the boundary, and unknown keys are silently accepted. Recommend a .strict() zod schema mirroring pending-data-validators.ts, returning 400 on parse failure. Not flagged by the linter — note for the team: this convention isn't lintable today.

Claude Fable 5 — Agreed and fixed in 564bb77. Added http/mcp-oauth-credentials-validators.ts with a .strict() zod schema mirroring the pending-data-validators.ts sibling: required mcpServerId/refreshToken, column-bound lengths (255/2048/64) on the VARCHAR-backed fields, and a parseable-date check on clientSecretExpiresAt; the handler now safeParses and returns 400 with the issue summary. Two deliberate scope choices: refreshToken/clientSecret stay unbounded (BLOB columns, and the body-size question was already adjudicated on the earlier thread — bodyParser caps the payload), and .strict() upgraded the 'userId in body' behavior from ignore-and-override to outright 400, with the test updated to assert the stricter contract.

The deposit body is a frontend HTTP body, which the package's boundary- validation convention requires to be zod-validated with .strict() like the /trigger pending-data payloads. Rejecting unknown keys also makes the token-only user identity explicit, and the column-bound fields now fail with a 400 at the boundary instead of a dialect-dependent insert error. Adds the missing DELETE 400 coverage for tokens without a numeric id. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

hercemer42 · 2026-06-11T12:56:27Z

+    const bearerUserId = this.getBearerUserId(ctx);

-    if (!Number.isFinite(bearerUserId)) {
+    if (bearerUserId === null) {


Why not just if (!bearerUserId) - if for whatever strange reason it's undefined or 0 it's also a problem ?

Claude Opus 4.8 — Good call on 0. Handled centrally in the shared requireBearerUserId helper rather than !bearerUserId at each site: it now rejects non-integers, 0, and negatives (!Number.isInteger(userId) || userId <= 0) since a Forest user id is always a positive integer. Call sites keep the explicit === null sentinel. 4ca006e.

hercemer42 · 2026-06-11T13:01:10Z

+  ): Promise<void> {
+    const userId = this.getBearerUserId(ctx);
+
+    if (userId === null) {


Likewise, if (!userId) ?

Claude Opus 4.8 — Same fix as the trigger guard: the 0/negative/non-integer check lives once in requireBearerUserId, so all three handlers share one validity rule. 4ca006e.

hercemer42 · 2026-06-11T13:02:44Z

+      ctx.body = { error: 'Missing or invalid user id in token' };
+
+      return;
+    }


Minor duplication with handletrigger, could move this to a function - debatable if worth it for the size

Claude Opus 4.8 — Agreed, extracted. The three handlers now call requireBearerUserId(ctx), which writes the 400 and returns null (callers just if (id === null) return). Removed the repeated block from handleTrigger, handleDepositCredentials, and handleDeleteCredentials. 4ca006e.

hercemer42 · 2026-06-11T13:07:53Z

+
+    try {
+      const refreshToken = encryption.encrypt(body.refreshToken);
+      const clientSecret = body.clientSecret ? encryption.encrypt(body.clientSecret) : null;


Why the ternary ? Won't it just return a null value when trying to encrypt a null value ?

Claude Opus 4.8 — Pushing back: the ternary is necessary. encrypt(plaintext: string) has no null pass-through — it calls cipher.update(plaintext, 'utf8'), so encrypt(undefined) throws rather than returning null. clientSecret is optional (public / PKCE clients have none), so we encrypt only when present and store null otherwise. The logic moved into buildMcpOAuthCredentialInput per your other comment, but the guard itself is unchanged and intentional.

hercemer42 · 2026-06-11T13:27:51Z

+// rejects unknown keys outright (including any attempt to smuggle a user id; the JWT is the only
+// identity source). Length bounds mirror the column limits so oversized input fails here with a
+// 400 instead of a dialect-dependent insert error. The refresh token and client secret are
+// unbounded: they land in BLOB columns and the body parser already caps the payload size.


Shorten comment if possible

Claude Opus 4.8 — Shortened from 5 lines to 3, keeping the three rationales (.strict()/unknown-keys, lengths-mirror-columns, BLOB-fields-unbounded). 4ca006e.

hercemer42 · 2026-06-11T13:47:52Z

+      .string()
+      .refine(value => !Number.isNaN(Date.parse(value)), { message: 'must be a parseable date' })
+      .optional(),
+    tokenEndpoint: z.string().max(2048).optional(),


tokenEndpoint should be required, better to give the front a specific error if it's not provided

Claude Opus 4.8 — Done. tokenEndpoint is now required in the schema (z.string().min(1).max(2048)), so a missing one returns a specific 400 at deposit rather than failing once PR2 reads the row. Matched with NOT NULL on the column + non-null store types (your two comments below). Left clientId / clientSecret / authMethod / scopes optional per our discussion — tokenEndpoint is the only one that's unrecoverable when absent. Added a route test (400 when tokenEndpoint missing) and a mapper unit test. 4ca006e.

hercemer42 · 2026-06-11T13:49:20Z

+  client_id: string | null;
+  client_secret_enc: Buffer | null;
+  client_secret_expires_at: string | Date | null;
+  token_endpoint: string | null;


not nullable

Claude Opus 4.8 — Done: token_endpoint: string in CredentialRow and tokenEndpoint: string in McpOAuthCredentialInput; toCredential no longer ?? nulls it. 4ca006e.

hercemer42 · 2026-06-11T13:51:26Z

+              },
+              tokenEndpoint: {
+                type: DataTypes.STRING(2048),
+                allowNull: true,


not nullable

Claude Opus 4.8 — Done: allowNull: false on the column in migration 002. Since PR1 is unmerged and 002 has never run anywhere, I edited the column in place rather than stacking a follow-up migration — no real rows to migrate. Added a store test asserting the DB rejects a null token_endpoint, and fixed the UNIQUE-constraint test's direct INSERT to include token_endpoint (otherwise it would have started passing on the NOT NULL violation instead of the UNIQUE one — a silent false pass). 4ca006e.

…oint Apply review feedback on PR1: - require tokenEndpoint (zod schema + NOT NULL column): the refresh grant has nowhere to go without it, so a missing one 400s at deposit rather than failing once the runtime read path lands - extract the body-to-record mapping into buildMcpOAuthCredentialInput and rename mcp-oauth-credentials-validators.ts to mcp-oauth-credentials.ts; the mapping is credential-domain logic, not transport - dedupe the bearer-user-id guard into requireBearerUserId, which also rejects non-positive / non-integer ids before they reach the store - shorten the new HKDF/boundary comments Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

hercemer42 · 2026-06-11T14:52:13Z

+// Translates a validated deposit body into the at-rest record: encrypts the refresh token (and
+// client secret when present) and maps optional fields to their nullable columns. Lives outside the
+// HTTP layer because it is credential-domain logic, not transport. encrypt() throws
+// ExecutorEncryptionKeyMissingError when the key is unset; the caller maps that to a 503.


Tighten this comment, we don't need to explain why we moved it. The fact that it is moved is enough.

hercemer42 commented Jun 2, 2026

View reviewed changes

Comment thread packages/workflow-executor/src/stores/mcp-oauth-credentials-store.ts

hercemer42 commented Jun 2, 2026

View reviewed changes

Comment thread packages/workflow-executor/src/crypto/credential-encryption.ts

hercemer42 commented Jun 2, 2026

View reviewed changes

Comment thread packages/workflow-executor/src/http/executor-http-server.ts Outdated

hercemer42 commented Jun 2, 2026

View reviewed changes

Comment thread packages/workflow-executor/src/http/executor-http-server.ts

hercemer42 and others added 2 commits June 11, 2026 11:57

docs(workflow-executor): note intentional empty HKDF salt

b20ff7c

Pre-empt the security-review question raised in code review: the empty salt is deliberate (domain separation comes from the fixed HKDF info label, input is a single high-entropy secret).

hercemer42 force-pushed the feat/prd-367-pr1-executor-oauth-credentials branch from 2d516a3 to b20ff7c Compare June 11, 2026 10:00

hercemer42 changed the base branch from feat/prd-214-server-step-mapper to main June 11, 2026 10:00