diff --git a/memory-bank/change-log.md b/memory-bank/change-log.md index c584470b..bab663e9 100644 --- a/memory-bank/change-log.md +++ b/memory-bank/change-log.md @@ -1148,3 +1148,44 @@ Validation: Follow-ups: - Optional next hardening increment: introduce endpoint-specific retry overrides for write paths (if future non-GET use is introduced) to keep retry policy conservative by operation type. + +--- + +### CL-031: TASK22236 P2-S3 Batch 2A policy tightening (retry classification, config clamping, log de-dup) + +date: 2026-03-24 +author: Cline +scope: `pages/api/middleware/relayForwarding.js`, `tests/phase21/relay-forwarding-hardening.test.cjs` +type: change +rationale: Execute P2-S3 Batch 2A by tightening relay retry policy and operational safety bounds while preventing duplicate error noise between relay-layer and endpoint-layer logging. +impact: Stronger resilience and cleaner observability with no endpoint contract changes; retry behavior is now explicitly conservative for deterministic client/auth failures. +status: completed + +Summary: + +- Updated relay retry policy: + - explicitly non-retryable statuses: `400`, `401`, `403`, `404`, `422` + - retries still allowed for transient classes (`408`, `429`, `5xx`) and selected transport error codes + - any other explicit numeric HTTP status now treated as non-retryable by default +- Added runtime-safe config clamping for relay knobs: + - timeout clamped to `100..30000ms` + - retries clamped to `0..4` + - retry delays clamped to `0..5000ms` + - both env-derived and per-call numeric overrides are sanitized +- Reduced duplicate logging noise: + - non-retry terminal relay failures are marked as already logged in middleware + - `relayGet` catch now skips `consoleLogger` when relay layer has already emitted a structured log +- Expanded relay hardening tests: + - no retry on `401` + - duplicate logging suppression path through `relayGet` + - env-value clamping behavior for timeout bounds + +Validation: + +- `node tests/phase21/relay-forwarding-hardening.test.cjs` -> pass (6/6) +- `node tests/phase21/endpoint-handler-contract.test.cjs` -> pass (152/152) +- `npm run lint` -> warnings only (pre-existing `react-hooks/exhaustive-deps`; no new lint errors) + +Follow-ups: + +- Batch 2B: update memory/context docs with canonical relay hardening policy, env knobs, and rollback/tuning guidance. diff --git a/pages/api/middleware/relayForwarding.js b/pages/api/middleware/relayForwarding.js index 8d9118e6..b420186b 100644 --- a/pages/api/middleware/relayForwarding.js +++ b/pages/api/middleware/relayForwarding.js @@ -13,6 +13,11 @@ const DEFAULT_TIMEOUT_MS = 8000; const DEFAULT_MAX_RETRIES = 2; const DEFAULT_RETRY_BASE_DELAY_MS = 200; const DEFAULT_RETRY_MAX_DELAY_MS = 1200; +const MAX_TIMEOUT_MS = 30000; +const MAX_RETRIES = 4; +const MAX_RETRY_DELAY_MS = 5000; + +const NON_RETRYABLE_STATUS_CODES = new Set([400, 401, 403, 404, 422]); const RETRYABLE_STATUS_CODES = new Set([408, 429, 500, 502, 503, 504]); const RETRYABLE_ERROR_CODES = new Set([ @@ -29,24 +34,66 @@ const parsePositiveInt = (value, fallback) => { return Number.isFinite(parsed) && parsed >= 0 ? parsed : fallback; }; +const clamp = (value, min, max) => { + return Math.min(max, Math.max(min, value)); +}; + +const sanitizeNumberConfig = ({ + value, + fallback, + min, + max, + allowZero = false +}) => { + const parsed = parsePositiveInt(value, fallback); + const lowerBound = allowZero ? 0 : min; + return clamp(parsed, lowerBound, max); +}; + +const resolveNumericOverride = ({ + overrideValue, + fallbackValue, + min, + max, + allowZero = false +}) => { + if (typeof overrideValue !== "number" || !Number.isFinite(overrideValue)) { + return fallbackValue; + } + + const lowerBound = allowZero ? 0 : min; + return clamp(Math.floor(overrideValue), lowerBound, max); +}; + const getRelayConfig = () => { return { - timeoutMs: parsePositiveInt( - process.env.RELAY_TIMEOUT_MS, - DEFAULT_TIMEOUT_MS - ), - maxRetries: parsePositiveInt( - process.env.RELAY_RETRY_MAX, - DEFAULT_MAX_RETRIES - ), - retryBaseDelayMs: parsePositiveInt( - process.env.RELAY_RETRY_BASE_DELAY_MS, - DEFAULT_RETRY_BASE_DELAY_MS - ), - retryMaxDelayMs: parsePositiveInt( - process.env.RELAY_RETRY_MAX_DELAY_MS, - DEFAULT_RETRY_MAX_DELAY_MS - ) + timeoutMs: sanitizeNumberConfig({ + value: process.env.RELAY_TIMEOUT_MS, + fallback: DEFAULT_TIMEOUT_MS, + min: 100, + max: MAX_TIMEOUT_MS + }), + maxRetries: sanitizeNumberConfig({ + value: process.env.RELAY_RETRY_MAX, + fallback: DEFAULT_MAX_RETRIES, + min: 0, + max: MAX_RETRIES, + allowZero: true + }), + retryBaseDelayMs: sanitizeNumberConfig({ + value: process.env.RELAY_RETRY_BASE_DELAY_MS, + fallback: DEFAULT_RETRY_BASE_DELAY_MS, + min: 0, + max: MAX_RETRY_DELAY_MS, + allowZero: true + }), + retryMaxDelayMs: sanitizeNumberConfig({ + value: process.env.RELAY_RETRY_MAX_DELAY_MS, + fallback: DEFAULT_RETRY_MAX_DELAY_MS, + min: 0, + max: MAX_RETRY_DELAY_MS, + allowZero: true + }) }; }; @@ -66,7 +113,9 @@ const shouldRetryRelayError = ({ error, attempt, maxRetries }) => { if (attempt >= maxRetries) return false; const status = error?.response?.status; + if (NON_RETRYABLE_STATUS_CODES.has(status)) return false; if (RETRYABLE_STATUS_CODES.has(status)) return true; + if (typeof status === "number") return false; const code = error?.code; return RETRYABLE_ERROR_CODES.has(code); @@ -92,7 +141,9 @@ export const relayGet = async ({ : data ); } catch (error) { - consoleLogger(error); + if (!error?.__relayAlreadyLogged) { + consoleLogger(error); + } return respondError(res, errorResponse); } }; @@ -122,25 +173,36 @@ export const forwardGetData = async ({ }) => { const relayConfig = getRelayConfig(); - const resolvedTimeoutMs = - typeof timeoutMs === "number" && timeoutMs >= 0 - ? timeoutMs - : relayConfig.timeoutMs; + const resolvedTimeoutMs = resolveNumericOverride({ + overrideValue: timeoutMs, + fallbackValue: relayConfig.timeoutMs, + min: 100, + max: MAX_TIMEOUT_MS + }); - const resolvedMaxRetries = - typeof maxRetries === "number" && maxRetries >= 0 - ? maxRetries - : relayConfig.maxRetries; + const resolvedMaxRetries = resolveNumericOverride({ + overrideValue: maxRetries, + fallbackValue: relayConfig.maxRetries, + min: 0, + max: MAX_RETRIES, + allowZero: true + }); - const resolvedRetryBaseDelayMs = - typeof retryBaseDelayMs === "number" && retryBaseDelayMs >= 0 - ? retryBaseDelayMs - : relayConfig.retryBaseDelayMs; + const resolvedRetryBaseDelayMs = resolveNumericOverride({ + overrideValue: retryBaseDelayMs, + fallbackValue: relayConfig.retryBaseDelayMs, + min: 0, + max: MAX_RETRY_DELAY_MS, + allowZero: true + }); - const resolvedRetryMaxDelayMs = - typeof retryMaxDelayMs === "number" && retryMaxDelayMs >= 0 - ? retryMaxDelayMs - : relayConfig.retryMaxDelayMs; + const resolvedRetryMaxDelayMs = resolveNumericOverride({ + overrideValue: retryMaxDelayMs, + fallbackValue: relayConfig.retryMaxDelayMs, + min: 0, + max: MAX_RETRY_DELAY_MS, + allowZero: true + }); const tokenAccessToken = typeof accessToken === "string" && accessToken.length > 0 @@ -184,6 +246,7 @@ export const forwardGetData = async ({ }); if (!retryEligible) { + error.__relayAlreadyLogged = true; structuredRelayLog("relay_request_failed", { queryUrl, baseUrl: resolvedBaseUrl, diff --git a/tests/phase21/relay-forwarding-hardening.test.cjs b/tests/phase21/relay-forwarding-hardening.test.cjs index cd94e20b..fd238806 100644 --- a/tests/phase21/relay-forwarding-hardening.test.cjs +++ b/tests/phase21/relay-forwarding-hardening.test.cjs @@ -111,6 +111,118 @@ test("forwardGetData does not retry non-retryable HTTP status", async () => { assert.strictEqual(callCount, 1); }); +test("forwardGetData does not retry unauthorized status", async () => { + let callCount = 0; + + const mod = loadRelayForwardingModule({ + axios: { + get: async () => { + callCount += 1; + const error = new Error("unauthorized"); + error.response = { status: 401 }; + throw error; + } + }, + getToken: async () => ({ access_token: "token" }), + hashAPIPath: () => "&hash=abc", + azureHeaders: () => ({ headers: {} }), + redactSensitive: (value) => value, + consoleLogger: () => {} + }); + + let thrown = null; + try { + await mod.forwardGetData({ + queryUrl: "incidents?$top=1", + maxRetries: 3, + retryBaseDelayMs: 0, + retryMaxDelayMs: 0 + }); + } catch (error) { + thrown = error; + } + + assert.ok(thrown); + assert.strictEqual(callCount, 1); +}); + +test("relayGet avoids duplicate consoleLogger when relay layer already logged", async () => { + let consoleLoggerCalls = 0; + let respondErrorCalls = 0; + + const mod = loadRelayForwardingModule({ + axios: { + get: async () => { + const error = new Error("bad request"); + error.response = { status: 400 }; + throw error; + } + }, + getToken: async () => ({ access_token: "token" }), + hashAPIPath: () => "&hash=abc", + azureHeaders: () => ({ headers: {} }), + redactSensitive: (value) => value, + consoleLogger: () => { + consoleLoggerCalls += 1; + }, + respondSuccess: () => {}, + respondError: () => { + respondErrorCalls += 1; + } + }); + + await mod.relayGet({ + queryUrl: "incidents?$top=1", + res: {}, + errorResponse: { code: "FAILED" } + }); + + assert.strictEqual(consoleLoggerCalls, 0); + assert.strictEqual(respondErrorCalls, 1); +}); + +test("forwardGetData clamps invalid env config values to safe bounds", async () => { + const previousEnv = { + RELAY_TIMEOUT_MS: process.env.RELAY_TIMEOUT_MS, + RELAY_RETRY_MAX: process.env.RELAY_RETRY_MAX, + RELAY_RETRY_BASE_DELAY_MS: process.env.RELAY_RETRY_BASE_DELAY_MS, + RELAY_RETRY_MAX_DELAY_MS: process.env.RELAY_RETRY_MAX_DELAY_MS + }; + + process.env.RELAY_TIMEOUT_MS = "999999"; + process.env.RELAY_RETRY_MAX = "999"; + process.env.RELAY_RETRY_BASE_DELAY_MS = "-1"; + process.env.RELAY_RETRY_MAX_DELAY_MS = "999999"; + + let capturedOptions = null; + + const mod = loadRelayForwardingModule({ + axios: { + get: async (url, options) => { + capturedOptions = options; + return { data: { ok: true } }; + } + }, + getToken: async () => ({ access_token: "token" }), + hashAPIPath: () => "&hash=abc", + azureHeaders: () => ({ headers: {} }), + redactSensitive: (value) => value, + consoleLogger: () => {} + }); + + await mod.forwardGetData({ + queryUrl: "incidents?$top=1" + }); + + assert.strictEqual(capturedOptions.timeout, 30000); + + process.env.RELAY_TIMEOUT_MS = previousEnv.RELAY_TIMEOUT_MS; + process.env.RELAY_RETRY_MAX = previousEnv.RELAY_RETRY_MAX; + process.env.RELAY_RETRY_BASE_DELAY_MS = + previousEnv.RELAY_RETRY_BASE_DELAY_MS; + process.env.RELAY_RETRY_MAX_DELAY_MS = previousEnv.RELAY_RETRY_MAX_DELAY_MS; +}); + test("forwardGetData applies timeout and appendHash=false behavior", async () => { let capturedUrl = null; let capturedOptions = null;