HarperFast / HarperFast/oauth

mcp.signingKeyPem resolving to empty string silently switches from pinned key to self-generated (no startup validation)

Open
#221 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
1
Forks
1
Avg merge
2d 16h
Merged PRs (30d)
12

Description

## `mcp.signingKeyPem` that resolves to an empty string silently switches the AS from pin mode to self-generated keys

Found while triaging a downstream boot warning (tpsdev-ai/flair#1456). Version: `@harperfast/oauth@2.4.0`. Behavior derived from shipped `dist/` source; line references below are to `dist/lib/mcp/keyStore.js` and `dist/lib/config.js` in the published package.

### The seam

Config supports env expansion, and `expandEnvVar` returns the **original placeholder** when the variable is unset, but the **empty string** when it is set-but-empty:

```js
return envValue !== undefined ? envValue : value;
```

The key store then branches on truthiness (`keyStore.js:320`):

```js
if (mcpConfig?.signingKeyPem) {
return this.getOrPersistPinnedKey(mcpConfig.signingKeyPem, accessTtl);
}
// :327 — falsy falls through to single-flight generateAndPersistFirstKey() (:502)
```

So the same operator intent — *"pin the signing key from `${MY_SIGNING_KEY}`"* — produces **three different behaviors** depending on how the env var fails:

| env var state | value reaching keyStore | behavior |
|---|---|---|
| set to a valid PEM | the PEM | pin mode ✓ |
| **unset** | literal `${MY_SIGNING_KEY}` | truthy → pin path → `createPublicKey` throws `ERR_OSSL_UNSUPPORTED` at first mint → 500 on `/token`. Fails closed, but only because a parser throws — there is no startup validation of this slot (the only boot-time check is the `signingKeyPem && keyRotationInterval` warning). |
| **set but empty** | `""` | falsy → **generation path**: the AS mints and persists its own RS256 key and serves it via JWKS. The pin intent is silently discarded. |

### Why the empty-string case matters

`VAR="$(cat /path/to/key.pem)"` with a missing/unreadable file exports an **empty** variable and succeeds. That is a common deploy-script failure, and it lands in the one row of the table that *keeps working* — tokens mint fine, JWKS serves — just under a key nobody provisioned. In a multi-node setup pinned to a shared key, the symptom is cross-node verification failures with no error pointing at the cause; in a single-node setup there is no symptom at all.

Self-generation itself is clearly a deliberate zero-config feature (`keyStore.js:38` — "pin wins — ALWAYS"), and this issue is not asking to remove it. The defect is that **"pin configured but resolved empty" is indistinguishable from "no pin configured"**, so a config error silently changes the trust model instead of failing.

### Suggested fix

At startup (component init, where the rotation-interval warning already lives): if the `signingKeyPem` config key is **present** but resolves to an empty string or an unparseable key, fail loudly (throw or refuse to serve the MCP endpoints) rather than proceeding — distinguishing:

- key **absent from config** → zero-config self-generation, as today;
- key **present, unresolved placeholder** → startup error naming the env var (today: boots, then 500s at first mint with a generic `server_error`);
- key **present, empty/unparseable** → startup error (today: silent mode switch).

A `createPublicKey` round-trip at init on any configured value would cover both bad rows and costs one parse at boot.

### Repro sketch

1. Configure `mcp.signingKeyPem: ${MY_SIGNING_KEY}` with `MY_SIGNING_KEY=""` in the environment, OAuth enabled.
2. Boot, request a token: mint succeeds; JWKS serves a key with a UUID kid that matches nothing the operator provisioned.
3. Same config with `MY_SIGNING_KEY` unset: mint returns 500 `server_error` with no earlier diagnostic.

(Trace is from the published source as cited; happy to add a runnable runtime repro on request.)

Contributor guide

Open the contributing guide

Research direction

Start in dist/lib/config.js at expandEnvVar and dist/lib/mcp/keyStore.js around the signingKeyPem branch, getOrPersistPinnedKey, and generateAndPersistFirstKey. Trace the startup initialization where the rotation-interval warning is emitted, then reproduce with an empty and unset MY_SIGNING_KEY; done means configured empty or unparseable values fail at startup while an absent key still permits self-generation.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
authentication
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.