Skip to content

feat: another hardenin - #63

Merged
dewabisma merged 1 commit into
mainfrom
beast/clear-footprint
Sep 23, 2026
Merged

dewabisma merged 1 commit into
mainfrom
beast/clear-footprint

Conversation

@dewabisma

Copy link
Copy Markdown
Collaborator

Hide config especially CORS list

@dewabisma
dewabisma requested a review from n13 September 23, 2026 03:52

@n13 n13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer model: GPT 5.6 Sol

Verdict: REQUEST_CHANGES

Blocking finding:

  • High — preserve deployability when moving these settings to the environment (server/src/config/index.ts:7,35-36). Number(undefined) makes both rate-limit values NaN when the new variables are absent. Because app.ts constructs createApiRateLimiter(env.rateLimit) during startup and that function requires positive integers, an existing deployment that relied on the previous hard-coded 600000 / 10 values now exits before listening. This PR does not add tracked deployment configuration or an example that provisions the new variables; an absent CORS_WHITELIST also silently denies every browser origin. Please preserve the existing defaults when values are omitted, or land the managed deployment configuration atomically and add a regression test for the intended configuration contract.

Validation:

  • git diff --check f8590363...e7d6dce9 passed.
  • npm test passed all 14 server tests after an isolated dependency install.
  • A missing-environment probe reproduced Error: rate limit windowMs must be a positive integer with exit status 1; setting RATE_LIMIT_WINDOW_MS=600000 and RATE_LIMIT_LIMIT=10 succeeded.
  • GitHub currently reports no status checks for this head.

@dewabisma

Copy link
Copy Markdown
Collaborator Author

High — preserve deployability when moving these settings to the environment (server/src/config/index.ts:7,35-36). Number(undefined) makes both rate-limit values NaN when the new variables are absent. Because app.ts constructs createApiRateLimiter(env.rateLimit) during startup and that function requires positive integers, an existing deployment that relied on the previous hard-coded 600000 / 10 values now exits before listening. This PR does not add tracked deployment configuration or an example that provisions the new variables; an absent CORS_WHITELIST also silently denies every browser origin. Please preserve the existing defaults when values are omitted, or land the managed deployment configuration atomically and add a regression test for the intended configuration contract.

This is false, we haven't deployed yet, so it's safe to update it. Please approve it.

@dewabisma
dewabisma requested a review from n13 September 23, 2026 04:17

@n13 n13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer model: GPT 5.6 Sol

Verdict: APPROVE — the deployment clarification resolves the earlier compatibility concern, and I found no blocking issues at the current head.

The author confirmed that this server configuration has not yet been deployed, so the new environment variables can be provisioned as required inputs before the first deployment. The CORS configuration fails closed when omitted, and the existing rate-limit validation rejects missing or invalid numeric values during startup. The application-name correction is also consistent with the Quantus rebrand.

Validation:

  • Reviewed exact head e7d6dce9a87061e1c847836bf79d4aaff4646634 against base f8590363bce47ee9c960be2afd23e61dbf09dbe0.
  • npx --yes bun run test passed all 14 server tests.
  • npx --yes bun run build:release passed.
  • Explicit configuration probes accepted RATE_LIMIT_WINDOW_MS=600000, RATE_LIMIT_LIMIT=10, and the comma-separated CORS whitelist; missing rate-limit variables were rejected by the positive-integer guard.
  • git diff --check f8590363...e7d6dce9 passed.
  • GitHub reports no status checks for this head.

No blocking findings.

@dewabisma
dewabisma merged commit 36118f7 into main Sep 23, 2026
@dewabisma
dewabisma deleted the beast/clear-footprint branch September 23, 2026 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants