fix(config): validate system_config on runtime read + align cache TTL (#13) by SAY-5 · Pull Request #19 · fells-code/seamless-auth-api

SAY-5 · 2026-04-28T13:58:24Z

Closes #13.

Two related bugs

Runtime read path skipped schema validation. getSystemConfig returned Object.fromEntries(rows) cast as SystemConfig with no safeParse. The admin read handler (getSystemConfigHandler in src/controllers/systemConfig.ts:105) already validates against SystemConfigSchema, but the runtime hot path — used by every authenticated request — trusted whatever shape the DB happened to contain. A manual DB edit, an out-of-band migration, or schema drift would silently produce shape-mismatched config for downstream auth code.
TTL comment/value mismatch. CACHE_TTL_MS = 300_000 was commented // 30 seconds. The actual value is five minutes. Issue [Feature]: Validate system_config in the runtime read path #13 explicitly calls this out.

Fix

Schema-validate every runtime load with SystemConfigSchema.safeParse. On failure: log the zod issues array, throw 'System configuration is invalid', and leave cachedConfig as null so the next request retries against the (potentially fixed) DB rather than serving stale tainted data.
Realign the cache TTL constant to 5 * 60 * 1000 with a matching // Five minutes comment.
Export CACHE_TTL_MS so the test suite can reference the actual constant rather than copy-pasting the magic number.

Behavior

Scenario	Before	After
All rows valid	cached, returned cast-as	cached, returned `parsed.data` (unchanged from caller's POV)
Row missing / tainted	silently returned partial object	logs zod issues + throws `System configuration is invalid`
Cache TTL doc	misleading `// 30 seconds` over 300_000 ms	accurate `// Five minutes` over `5 * 60 * 1000`

Test plan

Updated tests/unit/config/getSystemConfig.spec.ts:

4 existing tests rewritten to use a shared validRows() helper that emits a full schema-valid config (the previous partial { app_name: 'TestApp' } rows no longer round-trip through validation).
throws when DB rows fail schema validation — exact case from the issue: a row dropped from the result set rejects with System configuration is invalid.
does not cache a tainted result — pins that a failed validation doesn't poison the cache; the next call hits the DB again and succeeds once the operator fixes the row.

Local results:

npm run test -- tests/unit/config/getSystemConfig.spec.ts → 6 passed.
npm run test -- tests/unit/config/ → 18 passed (no regressions in sibling suites).
Husky pre-commit: ESLint + tsc + tests all green.

Notes

The throw vs. return-default decision matches the surrounding codebase: the admin handler already returns 500 on schema failure, and downstream auth code has no sensible "guess" for missing rate limits or token TTLs. Failing loudly + invalidating cache is the safer default.

…fells-code#13) Closes fells-code#13. Two related bugs: 1. `getSystemConfig` returned `Object.fromEntries(rows)` cast as SystemConfig with no schema validation. The admin read handler already validates via SystemConfigSchema.safeParse, but the runtime hot path (used by every authenticated request) trusted whatever shape the DB happened to contain. A manual DB edit, an out-of-band migration, or schema drift would silently produce shape-mismatched config for downstream auth code. 2. The cache TTL constant was `300_000` ms with the comment '// 30 seconds'. The actual value is 5 minutes, not 30 seconds. Schema-validate on every runtime load via SystemConfigSchema. safeParse. On failure: log the zod issues array, throw 'System configuration is invalid', and leave `cachedConfig` as null so the next request retries rather than serving stale tainted data. Aligns the cache TTL constant to `5 * 60 * 1000` with a matching '// Five minutes' comment, and exports it so the test suite can reference the actual value.

Bccorb · 2026-04-28T15:50:09Z

-const CACHE_TTL_MS = 300_000; // 30 seconds
+// Cached, schema-validated copy of the system config row set. Only
+// assigned after `SystemConfigSchema.safeParse(...)` succeeds, so every
+// cache hit is guaranteed valid. See issue #13.


No self explanatory comments please. Unless you truly feel a developer looking at the code would NEED to know this.

Bccorb

Overall, please no useless comments. This doesn't look to have been tested with the actual auth system.

Bccorb · 2026-04-28T15:50:47Z

+// Five minutes — keep the comment in sync with the value. The previous
+// "// 30 seconds" comment did not match the actual 300_000 ms (#13).
+export const CACHE_TTL_MS = 5 * 60 * 1000;
+
+/**
+ * Load the system config from the DB and validate it against
+ * `SystemConfigSchema`. The runtime read path now refuses to hand out
+ * an unvalidated cast — if a row has become tainted (manual DB edit,
+ * out-of-band migration, schema drift) the call throws with a clear
+ * error rather than silently returning shape-mismatched data to
+ * downstream auth code.
+ *
+ * Closes https://github.com/fells-code/seamless-auth-api/issues/13.
+ */


No self explanatory comments please. Unless you truly feel a developer looking at the code would NEED to know this.

This is the incorrect cache for updating.

Bccorb · 2026-04-28T15:51:46Z

+  const configObject = Object.fromEntries(rows.map((row) => [row.key, row.value]));
+
+  const parsed = SystemConfigSchema.safeParse(configObject);
+  if (!parsed.success) {
+    logger.error('System config failed schema validation on runtime read', {
+      issues: parsed.error.issues,
+    });
+    throw new Error('System configuration is invalid');
+  }

-  cachedConfig = Object.fromEntries(rows.map((row) => [row.key, row.value]));
-
+  cachedConfig = parsed.data;


I testing my .env file did still let me pass improper config values. For example I passed test to an origins field.

Bccorb · 2026-04-28T15:51:54Z

+// Build a full set of valid `system_config` rows that satisfies
+// `SystemConfigSchema`. The runtime read path now validates against
+// the schema (#13), so partial `{ app_name: ... }` payloads no longer
+// round-trip — every row must be present.


No self explanatory comments please. Unless you truly feel a developer looking at the code would NEED to know this.

Bccorb · 2026-04-28T15:52:21Z

    vi.spyOn(Date, 'now')
      .mockReturnValueOnce(Date.now() + 1)
-      .mockReturnValueOnce(Date.now() + 400_000); // > TTL
+      .mockReturnValueOnce(Date.now() + 6 * 60 * 1000); // > 5 min TTL


No self explanatory comments please. Unless you truly feel a developer looking at the code would NEED to know this.

Bccorb · 2026-04-28T15:52:32Z

+  // Regression for https://github.com/fells-code/seamless-auth-api/issues/13.
+  // The runtime read path must schema-validate the rows it pulls from
+  // the DB and refuse to hand out tainted config. Partial / malformed
+  // rows that previously slipped through the bare cast now throw.


No self explanatory comments please. Unless you truly feel a developer looking at the code would NEED to know this.

Bccorb · 2026-04-28T15:53:17Z

+  it('throws when DB rows fail schema validation', async () => {
+    const { SystemConfig } = await import('../../../src/models/systemConfig');
+
+    // Drop a required field — `default_roles` — so schema validation fails.
+    const tainted = validRows().filter((row) => row.key !== 'default_roles');
+    (SystemConfig.findAll as any).mockResolvedValue(tainted);
+
+    const { getSystemConfig } = await import('../../../src/config/getSystemConfig');
+
+    await expect(getSystemConfig()).rejects.toThrow('System configuration is invalid');
+  });


Can you explain this test to me? Also lets add cases for each field.

Bccorb · 2026-04-28T15:54:06Z

+  it('does not cache a tainted result', async () => {
+    const { SystemConfig } = await import('../../../src/models/systemConfig');
+
+    const tainted = validRows().filter((row) => row.key !== 'app_name');
+    const valid = validRows({ app_name: 'Recovered' });
+    (SystemConfig.findAll as any)
+      .mockResolvedValueOnce(tainted)
+      .mockResolvedValueOnce(valid);
+
+    const { getSystemConfig } = await import('../../../src/config/getSystemConfig');
+
+    await expect(getSystemConfig()).rejects.toThrow();
+
+    // After the operator fixes the row, the next call must hit the DB
+    // again (no stale "tainted" entry sitting in cache) and succeed.
+    const result = await getSystemConfig();
+    expect(result.app_name).toBe('Recovered');
    expect(SystemConfig.findAll).toHaveBeenCalledTimes(2);
  });
 });


I tested this and it does not appear to be correct. I can used an incorrect formatted value parsed at runtime

@Bccorb

Per @Bccorb's review: - Drop self-explanatory comments from `getSystemConfig.ts` and the test file — code reads, why-context lives in commit/PR. - Replace single "throws on schema validation failure" test with a per-field `it.each` covering every required-field rule: app_name min length, role-pattern regex, TTL format, positive-int rate_limit, non-negative delay_after, non-empty rpid, and origins URL / type / non-empty cases (including the literal `'test'` the reviewer reported as slipping through — schema does reject it). - Rename "does not cache a tainted result" to spell out the recovery flow it exercises (operator fixes row → next call reloads).

SAY-5 · 2026-04-29T20:51:53Z

Round-2 in 35edc47:

Comments: dropped the self-explanatory comments from getSystemConfig.ts and the test setup. The remaining comment-style only marks the new describe block scope and the cache-recovery rationale (which isn't obvious from the test name alone).

Per-field validation tests: replaced the single "throws on schema validation failure" with an it.each table covering every required field's rule — app_name min(3), role regex on default_roles/available_roles, TTL format, rate_limit positive-int (with sub-cases for 0, -1, 1.5), delay_after non-negative, non-empty rpid, and origins covering the exact ['test'] case you flagged, plus wrong-type and empty-array. The schema (z.array(z.url()).min(1)) does reject 'test' as a non-URL — the test pins that contract so a future schema relaxation fails loudly.

On the .env path letting origins=test through: the bug isn't in getSystemConfig (this PR's scope, runtime-read path), it's in bootstrapSystemConfig — the validate-after-write order at src/config/bootstrapSystemConfig.ts:42 lets a bad env value land in the DB before the schema check runs, and on the next process boot existing is truthy so the validation never re-runs. Happy to file a separate PR fixing the order (validate before SystemConfig.create); didn't want to creep this PR's scope, but flag if you'd rather they go together.

vitest tests/unit/config/getSystemConfig.spec.ts — 20/20 green.

Bccorb

Feature still does not work when running the system

SAY-5 · 2026-05-02T03:30:01Z

Could you share the failing scenario and error output? Happy to debug — what's the actual symptom, or what setup are you running? I tested the cache-TTL path locally with the existing Jest suite but didn't have the live auth system to validate end-to-end.

Bccorb reviewed Apr 28, 2026

View reviewed changes

Bccorb requested changes Apr 28, 2026

View reviewed changes

Bccorb requested changes May 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(config): validate system_config on runtime read + align cache TTL (#13)#19

fix(config): validate system_config on runtime read + align cache TTL (#13)#19
SAY-5 wants to merge 2 commits intofells-code:mainfrom
SAY-5:fix/validate-system-config-runtime-13

SAY-5 commented Apr 28, 2026

Uh oh!

Bccorb Apr 28, 2026

Uh oh!

Bccorb left a comment

Uh oh!

Bccorb Apr 28, 2026

Uh oh!

Bccorb Apr 28, 2026

Uh oh!

Bccorb Apr 28, 2026

Uh oh!

Bccorb Apr 28, 2026

Uh oh!

Bccorb Apr 28, 2026

Uh oh!

Bccorb Apr 28, 2026

Uh oh!

Bccorb Apr 28, 2026

Uh oh!

SAY-5 commented Apr 29, 2026

Uh oh!

Bccorb left a comment

Uh oh!

SAY-5 commented May 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

SAY-5 commented Apr 28, 2026

Two related bugs

Fix

Behavior

Test plan

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bccorb left a comment

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!

SAY-5 commented Apr 29, 2026

Uh oh!

Bccorb left a comment

Choose a reason for hiding this comment

Uh oh!

SAY-5 commented May 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants