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 bb1b281. (Automatic summaries will resume when PR exits draft mode or review begins).}

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>

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

PRD-367

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

2 new issues

Tool	Category	Rule	Count
qlty	Structure	Function with high complexity (count = 17): constructor	1
qlty	Structure	Function with many returns (count = 4): handleTrigger	1

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

Coverage Impact

Unable to calculate total coverage change because base branch coverage was not found.

Modified Files with Diff Coverage (6)

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

🤖 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` -- Lines 135 and 326-329
- `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.

hercemer42 · 2026-06-02T17:00:29Z

+    }
+  }
+
+  async get(userId: number, mcpServerId: string): Promise<StoredMcpOAuthCredential | null> {


claude-opus-4-8 — [Should fix] get/upsert/delete let raw Sequelize errors propagate, unlike the sibling DatabaseStore which routes every method through callPort → RunStorePortError. Consider matching that pattern (or noting it is intentional) so infra failures surface as typed errors instead of reaching the HTTP layer as generic 500s.

claude-opus-4-8 — Pushing back; leaving this deliberate. RunStorePortError is semantically the RunStore port error (its userMessage is about step state), so reusing it here would be wrong, and adding a bespoke port-error class is scope this dormant PR1 does not need. Unexpected DB failures surface correctly as 500 at the HTTP error middleware. Happy to revisit with a typed error contract when the runtime read path lands.

hercemer42 · 2026-06-02T17:00:30Z

+
+    // hkdfSync returns an ArrayBuffer; wrap it as a concrete Uint8Array<ArrayBuffer> so it
+    // satisfies the crypto CipherKey type (Buffer's generic ArrayBufferLike backing does not).
+    return new Uint8Array(hkdfSync(HKDF_DIGEST, secret, new Uint8Array(0), HKDF_INFO, KEY_BYTES));


claude-opus-4-8 — [Preferential] HKDF uses an empty salt. This is acceptable (the fixed info label provides domain separation and the input is a high-entropy secret), but a one-line comment noting the empty salt is intentional would preempt the security-review question.

claude-opus-4-8 — Agreed. Added a comment noting the empty salt is intentional (domain separation comes from the fixed info label; the input is a single high-entropy secret). Pending in the next push.

hercemer42 · 2026-06-02T17:00:32Z

+    }
+
+    try {
+      const refreshToken = encryption.encrypt(body.refreshToken);


claude-opus-4-8 — [Preferential] No explicit upper bound on refreshToken/clientSecret length before encryption. bodyParser caps the JSON body (~1mb) and the columns are BLOB, so it is bounded in practice; an explicit length guard would be defensive on this authenticated external surface.

claude-opus-4-8 — Pushing back; low probability and already bounded: bodyParser caps the JSON body (~1mb) and these are BLOB columns. An explicit length guard is validation surface the spec does not call for, so declining for now.

hercemer42 · 2026-06-02T17:00:33Z

+    }
+
+    await store.delete(userId, ctx.params.mcpServerId);
+    ctx.status = 204;


claude-opus-4-8 — [Preferential] DELETE returns 204 no-body while POST returns 200 { stored: true }. 204 is correct REST, but since the frontend will consume this, confirm it expects a no-body delete rather than a JSON envelope (the success-body convention is currently an it.todo in the route test).

claude-opus-4-8 — Intentional: 204 No Content is correct REST for a delete. The frontend consumer is a later PR and will be written against this endpoint, so there is no existing contract to break; the success-body shape stays an it.todo until that PR pins it.

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 commented Jun 2, 2026

View reviewed changes

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

2d516a3

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).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(workflow-executor): add OAuth credential store + deposit endpoint (PRD-367 PR1)#1619

feat(workflow-executor): add OAuth credential store + deposit endpoint (PRD-367 PR1)#1619
hercemer42 wants to merge 2 commits into
feat/prd-214-server-step-mapperfrom
feat/prd-367-pr1-executor-oauth-credentials

hercemer42 commented Jun 2, 2026 •

edited by macroscopeapp Bot

Loading

Uh oh!

linear-code Bot commented Jun 2, 2026

Uh oh!

qltysh Bot commented Jun 2, 2026

Uh oh!

qltysh Bot commented Jun 2, 2026

Uh oh!

hercemer42 Jun 2, 2026

Uh oh!

hercemer42 Jun 2, 2026

Uh oh!

hercemer42 Jun 2, 2026

Uh oh!

hercemer42 Jun 2, 2026

Uh oh!

hercemer42 Jun 2, 2026

Uh oh!

hercemer42 Jun 2, 2026

Uh oh!

hercemer42 Jun 2, 2026

Uh oh!

hercemer42 Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

hercemer42 commented Jun 2, 2026 • edited by macroscopeapp Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Notes

Tests

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

Uh oh!

linear-code Bot commented Jun 2, 2026

Uh oh!

qltysh Bot commented Jun 2, 2026

Uh oh!

qltysh Bot commented Jun 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

hercemer42 commented Jun 2, 2026 •

edited by macroscopeapp Bot

Loading