[Spec Kit] Implementation progress

2026-04-22 19:41:14 -05:00
parent 24cfd85ac2
commit 110c2e961b
61 changed files with 5371 additions and 38 deletions
--- a/specs/001-oidc-proxy-script/research.md
+++ b/specs/001-oidc-proxy-script/research.md
@@ -0,0 +1,280 @@
+# Research: OIDC Proxy Script Authentication
+
+**Feature**: 001-oidc-proxy-script  
+**Phase**: 0 — Resolved unknowns  
+**Date**: 2025-07-17
+
+---
+
+## R-001 — Token Cache Persistence in VM Sandbox
+
+**Unknown**: How can `proxy.js` maintain a token cache that survives across requests when
+`server.js` creates a fresh `vm.createContext()` per request (resetting all bare `let` variables)?
+
+**Decision**: Store the cache as a property on the `adapter_settings` object
+(`adapter_settings._cache`).
+
+**Rationale**: `server.js` loads `adapter_settings.json` into `globalVariableContext.adapter_settings`
+once at startup. The per-request spread `vm.createContext({ ...globalVariableContext })` copies the
+*object reference* — not a clone — into each sandbox. Any mutation to `adapter_settings._cache`
+modifies the same underlying heap object and is therefore visible to every subsequent request.
+Bare `let`/`const` variables at the top level of `proxy.js` do **not** persist; they are
+sandbox-local and are reset to `undefined` on every invocation.
+
+```javascript
+// proxy.js — top of script (safe, no import/export)
+// adapter_settings is the same JS object reference every invocation:
+const _cache = adapter_settings._cache ||
+  (adapter_settings._cache = { token: null, expiry: 0, pendingFetch: null });
+```
+
+**Alternatives considered**:
+
+| Alternative | Why rejected |
+|-------------|-------------|
+| Bare `let cachedToken` in proxy.js | Resets on every `vm.createContext()` invocation |
+| Add dedicated `_cache` to `globalVariableContext` in server.js | Correct but requires server.js modification; feature spec says no server.js changes needed |
+| External Redis / file cache | Introduces infrastructure dependency; spec assumption explicitly rules this out |
+| Attach to `axios` or another injected object | Correct mechanism but semantically wrong; `adapter_settings` is the natural owner |
+
+**Verification**: `globalVariableContext` is a module-level `let` in `server.js` (line 27).
+The spread in `vm.createContext` (lines 172–177) copies each property value by reference for
+objects. `adapter_settings` is an object, so the sandbox and `globalVariableContext` share the
+same reference. Confirmed by reading `src/server.js`.
+
+---
+
+## R-002 — Token Stampede Prevention (Promise Sharing)
+
+**Unknown**: When multiple concurrent requests arrive while `_cache.token` is null, how do we
+ensure only one HTTP request is sent to the token service?
+
+**Decision**: Store the in-flight fetch promise on `_cache.pendingFetch`. Subsequent requests
+detect a non-null `pendingFetch` (via duck-type check) and `await` the same promise. A `finally`
+block clears `pendingFetch` after settlement.
+
+**Rationale**: The `pendingFetch` property is on the shared `adapter_settings._cache` object
+(same reference as R-001). All concurrent requests therefore see the same pending Promise.
+
+**Cross-realm safety**: Each `vm.createContext()` creates a new V8 realm with its own
+`Promise` constructor. `instanceof Promise` checks fail across realms. The Promises/A+ thenable
+protocol (`await` and `.then()`) works via duck-typing and is cross-realm safe.
+
+```javascript
+// Stampede guard — duck-type check, NOT instanceof
+if (_cache.pendingFetch !== null &&
+    typeof _cache.pendingFetch.then === 'function') {
+  // Queue on the existing fetch — await is thenable-protocol safe across V8 realms
+  try {
+    await _cache.pendingFetch;
+    res.writeHead(200, { 'Content-Type': 'text/plain' });
+    res.end('Authorized');
+  } catch (err) {
+    res.writeHead(401, { 'Content-Type': 'text/plain' });
+    res.end('Unauthorized: ' + err.message);
+  }
+  return;
+}
+
+// This invocation wins the race — build and share the fetch promise
+_cache.pendingFetch = (async () => {
+  // ... axios.post ...
+  _cache.token = id_token;
+  _cache.expiry = expires_in; // absolute Unix epoch seconds (FR-006)
+})();
+
+try {
+  await _cache.pendingFetch;
+  res.writeHead(200, { 'Content-Type': 'text/plain' });
+  res.end('Authorized');
+} catch (err) {
+  _cache.token = null;
+  _cache.expiry = 0;
+  res.writeHead(401, { 'Content-Type': 'text/plain' });
+  res.end('Unauthorized: ' + err.message);
+} finally {
+  _cache.pendingFetch = null; // clear after all awaiters have resolved/rejected
+}
+```
+
+**Why `finally` is safe for clearing `pendingFetch`**: By the time `finally` runs, all callers
+that `await`-ed `_cache.pendingFetch` have already received the settled value. Setting
+`pendingFetch = null` only affects future requests arriving after settlement.
+
+**Alternatives considered**:
+
+| Alternative | Why rejected |
+|-------------|-------------|
+| `instanceof Promise` guard | Fails across V8 realms — each sandbox has its own `Promise` constructor |
+| Mutex / lock via integer flag | More complex; promise sharing is idiomatic in async JS |
+| Separate request queue (array + callbacks) | Overkill; promise sharing achieves the same result with fewer lines |
+
+---
+
+## R-003 — Async Proxy Script in Synchronous `runInContext`
+
+**Unknown**: `server.js` calls `script.runInContext(context)` synchronously without `await`.
+How can `proxy.js` do async work (HTTP call) without leaving unhandled promise rejections?
+
+**Decision**: Wrap all proxy logic in a top-level immediately-invoked async function expression
+(IIFE). Catch all errors inside the IIFE and always send a response before the IIFE resolves.
+
+**Rationale**: The async IIFE returns a Promise but `server.js` does not `await` it. The
+outer `try/catch` in `server.js` only catches synchronous throws. All async errors must be
+handled within `proxy.js` itself.
+
+```javascript
+// proxy.js — entire body wrapped in async IIFE
+(async () => {
+  try {
+    // ... token logic ...
+    res.writeHead(200, { 'Content-Type': 'text/plain' });
+    res.end('Authorized');
+  } catch (err) {
+    console.error({ message: 'Auth failed', error: err.message });
+    res.writeHead(401, { 'Content-Type': 'text/plain' });
+    res.end('Unauthorized: ' + err.message);
+  }
+})();
+```
+
+**Constraint**: `res.end()` MUST be called in both the `try` and `catch` paths. If `res.end()`
+is not called, the HTTP connection hangs for the caller.
+
+---
+
+## R-004 — Absolute Epoch Expiry Check
+
+**Unknown**: The spec states `expires_in` is an absolute Unix epoch timestamp (not a
+relative duration). What is the correct expiry check?
+
+**Decision**: `Date.now() / 1000 < _cache.expiry` — token is valid when current Unix time
+(seconds) is strictly less than the stored `expires_in` value.
+
+**Rationale**: Confirmed in spec (line 99): "Expiry check: `Date.now() / 1000 < expires_in`".
+Example value `1532618185` is a Unix timestamp, not a duration.
+
+```javascript
+function isTokenValid() {
+  return _cache.token !== null && Date.now() / 1000 < _cache.expiry;
+}
+```
+
+**Edge cases**:
+- `expires_in` already in the past on receipt → `isTokenValid()` returns `false` immediately →
+  fresh token fetched (FR-006 compliance)
+- `expires_in` is `0` or negative → treated as expired
+- No safety buffer is applied; the spec does not require one
+
+---
+
+## R-005 — axios OIDC POST Pattern
+
+**Decision**: Pass `URLSearchParams` instance as body; set `Content-Type` header explicitly for
+clarity; use `timeout: 5000` for the 5-second limit (FR-014).
+
+```javascript
+const params = new URLSearchParams({
+  grant_type: 'password',
+  username:   adapter_settings.username,
+  password:   adapter_settings.password,
+  client_id:  adapter_settings.clientId,
+  scope:      adapter_settings.scope,
+});
+
+const response = await axios.post(adapter_settings.tokenUrl, params, {
+  headers: { 'Content-Type': 'application/x-www-form-urlencoded' },
+  timeout: 5000,
+});
+```
+
+**Content-Type note**: axios v1.x auto-detects `URLSearchParams` and sets
+`application/x-www-form-urlencoded` automatically. The explicit header is belt-and-suspenders
+for readability in the VM sandbox context.
+
+---
+
+## R-006 — axios Error Differentiation
+
+**Decision**: Treat **all** axios errors as authentication failures (FR-008). Derive the
+error message from the error type for clarity in the 401 body:
+
+| Error condition | `error.response` | `error.code` | Message strategy |
+|-----------------|-----------------|--------------|-----------------|
+| HTTP 4xx/5xx from token service | Populated | — | `HTTP ${error.response.status}` |
+| Timeout (5 s) | `undefined` | `'ECONNABORTED'` / `'ERR_CANCELED'` | `'token service timeout'` |
+| Network failure (DNS, TCP) | `undefined` | `'ERR_NETWORK'` | `error.message` |
+| Missing `id_token` in response | — | — | `'id_token missing from response'` |
+| Missing `adapter_settings` fields | — | — | `'missing required field: <name>'` |
+
+All cases route to the same 401 response path, satisfying FR-008 and SC-004.
+
+---
+
+## R-007 — Testing VM-Sandboxed Code with `node:test`
+
+**Decision**: Two test layers, no external test framework:
+
+1. **Unit tests** (`tests/unit/proxy.test.js`): Compile `proxy.js` once with `new vm.Script()`.
+   Each test creates a controlled fake context (mock `axios`, controllable `_cache`, mock `res`).
+   Use `await script.runInContext(ctx)` to drive the async IIFE.
+   Use `t.mock.fn()` for call-count assertions; `t.mock.timers` for time-travel expiry tests.
+
+2. **Contract tests** (`tests/contract/proxy-http.test.js`): Start an actual HTTP server with
+   a mock token endpoint (using `http.createServer`) and assert real HTTP responses. Validates
+   end-to-end behaviour including `server.js` context injection.
+
+**Key insight**: Because `proxy.js` receives **all** dependencies via the VM context object,
+dependency injection IS the test seam. No module-level mocking (`jest.mock` equivalent) is
+needed or available — the context object serves the same role.
+
+```javascript
+// tests/unit/proxy.test.js — shared setup pattern
+import { test, describe } from 'node:test';
+import assert from 'node:assert/strict';
+import vm from 'node:vm';
+import { readFileSync } from 'node:fs';
+import { fileURLToPath } from 'node:url';
+import { dirname, join } from 'node:path';
+
+const __dirname = dirname(fileURLToPath(import.meta.url));
+const proxyCode = readFileSync(
+  join(__dirname, '../../src/proxyScripts/proxy.js'), 'utf-8'
+);
+const script = new vm.Script(proxyCode, { filename: 'proxy.js' });
+
+function makeContext(t, overrides = {}) {
+  const _cache = { token: null, expiry: 0, pendingFetch: null };
+  let statusCode = null;
+  let body = '';
+  const res = {
+    writeHead: t.mock.fn((code) => { statusCode = code; }),
+    end:       t.mock.fn((b = '') => { body += b; }),
+    get statusCode() { return statusCode; },
+    get body()       { return body; },
+  };
+  return vm.createContext({
+    URLSearchParams,
+    URL,
+    console,
+    axios: {
+      post: t.mock.fn(async () => ({
+        data: { id_token: 'test-token', expires_in: 9_999_999_999 }
+      }))
+    },
+    adapter_settings: {
+      tokenUrl:  'https://auth.example.com/token',
+      username:  'user',
+      password:  'pass',
+      clientId:  'client',
+      scope:     'openid',
+      _cache,
+    },
+    req: { url: '/proxy', method: 'GET', headers: {} },
+    res,
+    ...overrides,
+  });
+}
+```
+
+**Run command**: `node --test tests/unit/proxy.test.js`