Skip to content

Finalize staging cleanup and safety hardening - #4

Open
haydenkoch wants to merge 11 commits into
stagingfrom
codex/staging-cleanup
Open

Finalize staging cleanup and safety hardening#4
haydenkoch wants to merge 11 commits into
stagingfrom
codex/staging-cleanup

Conversation

@haydenkoch

Copy link
Copy Markdown
Owner

Summary

  • bring the latest runtime-config and delivery hardening commit onto staging
  • carry over the staging delivery safety commits on a linear branch that satisfies branch rules
  • preserve the integrated CI, Playwright, and source-ops test behavior verified locally

Verification

  • pnpm --config.engine-strict=false lint:ci
  • pnpm --config.engine-strict=false check
  • pnpm --config.engine-strict=false test:unit:ci

Notes

  • local verification was run under Node v25.9.0 with engine-strict disabled because the branch now declares Node 24.x
  • GitHub requires PR-based updates to staging and rejects merge commits on that branch

@haydenkoch
haydenkoch enabled auto-merge (squash) April 8, 2026 10:27
Comment thread .github/workflows/pr-title.yml Fixed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3f587bb7c4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-migrations-ci.mjs Outdated
@greptile-apps

greptile-apps Bot commented Apr 8, 2026

Copy link
Copy Markdown

Greptile Summary

This PR consolidates a series of incremental staging hardening commits onto a linear branch to satisfy branch protection rules. The primary substance is a refactored runtime-configuration and secret-validation layer (runtime-config-core.ts, runtime-secrets.ts) alongside a significantly expanded CI pipeline that adds sharded unit/HTTP/search test jobs, ephemeral database bootstrapping, and Playwright E2E coverage.

Key changes:

  • Runtime config system: readBoolean is now exported from runtime-config-core.ts (resolving the previously noted duplication in email.ts); inspectRuntimeConfig gained environment-aware validation covering all service endpoints, secrets, and SMTP security posture across development, ci, staging, and production tiers.
  • CI pipeline: Jobs now run on Node 24 with pinned action SHAs; migration-check guard (check-migrations-ci.mjs) correctly uses execFileSync array args; pr-title.yml uses pull_request_target with empty permissions and no checkout — a safe configuration.
  • Auth email templates (auth.ts): User-supplied fields (user.name, newEmail) are interpolated directly into HTML without encoding, which can produce malformed email markup when those fields contain <, >, or &.
  • Privileged API endpoints (/api/reindex, /api/source-ops/run-due): Token comparison uses === rather than crypto.timingSafeEqual; practical exploitability is negligible given rate limiting and token length, but the pattern is worth addressing.

Confidence Score: 5/5

Safe to merge — the prior duplication concern is resolved and no blocking issues were introduced.

The PR delivers on its stated goal: it linearises the staging branch, exports readBoolean to fix the previously flagged duplicate, wires up a robust environment-aware config validation layer, and adds well-structured CI coverage. The two remaining comments (HTML encoding in email templates, === vs timingSafeEqual) are both P2 style/best-practice items with negligible real-world impact — neither blocks production reliability or creates a meaningful security exposure. Prior review concerns are addressed.

src/lib/server/auth.ts (email template encoding); src/routes/api/reindex/+server.ts and src/routes/api/source-ops/run-due/+server.ts (token comparison idiom)

Vulnerabilities

  • HTML injection in email templates (src/lib/server/auth.ts lines 34–89): user.name, user.email, and newEmail are embedded in HTML email bodies without encoding. Exploitation is self-contained (user receives their own email) and email clients don't execute scripts, but malformed characters can break email HTML structure.
  • Non-constant-time token comparison (src/routes/api/reindex/+server.ts:34, src/routes/api/source-ops/run-due/+server.ts:29): Secret header values are compared with === rather than crypto.timingSafeEqual. Rate limiting and token length make timing oracle attacks impractical, but the pattern deviates from the Node.js security idiom.
  • No secrets committed to source; CI credentials are all ephemeral/local values.
  • pull_request_target workflow does not check out code, uses empty permissions — no pwn-request risk.

Important Files Changed

Filename Overview
src/lib/config/runtime-config-core.ts Core runtime config validation engine — exports readBoolean (previously private), adds comprehensive environment-aware validation for all required secrets and service endpoints, with proper file-backed secret support.
src/lib/config/runtime-secrets.ts Configuration catalog and snapshot resolver; defines all known config keys, their required environments, and the file-backed secret resolution logic. No issues found.
src/lib/server/runtime-config.ts Thin server-side adapter that wires getRuntimeConfigValues() and readSecretFile into the core inspection logic; exposes assertProductionRuntimeConfig and resolveRuntimeOrigin. Clean and well-structured.
src/lib/server/email.ts Nodemailer transport configured from runtime config values; previously imported a private readBoolean copy — now correctly imports the exported helper from runtime-config-core. Clean refactor.
src/lib/server/auth.ts Better-auth setup with email/password and optional Google OAuth; email callback templates interpolate user-supplied values (user.name, newEmail) directly into HTML without encoding, which can produce malformed email markup.
.github/workflows/ci.yml Comprehensive CI matrix: static checks, sharded unit/HTTP/search tests, E2E Playwright; all jobs use pinned action SHAs, Node 24, ephemeral Postgres/Meilisearch services, and proper artifact hand-off. No issues found.
.github/workflows/pr-title.yml PR title validation using pull_request_target (needed for fork PRs) with empty permissions and no checkout — PR title is passed via env variable, not interpolated into the shell script, so no injection risk.
scripts/check-migrations-ci.mjs Migration guard script that detects schema changes without committed migration artifacts; uses execFileSync with argument arrays (no shell injection), handles zero-SHA base refs and missing diffs gracefully.
src/routes/api/reindex/+server.ts Reindex endpoint with rate limiting and dual auth (session or secret header); token comparison uses === rather than crypto.timingSafeEqual, though exploitability is negligible given rate limiting and token length.
src/routes/api/source-ops/run-due/+server.ts Source-ops trigger endpoint; same rate-limit + dual-auth pattern as reindex, same non-timing-safe comparison note applies. Logic and structure are otherwise correct.
src/routes/admin/settings/integrations/+page.server.ts Admin integrations page loader that surfaces Mapbox, Meilisearch, PostHog, and SMTP config status; correctly imports readBoolean from runtime-config-core and uses readRuntimeConfigValue throughout. No issues.
tests/runtime-config.test.ts Comprehensive unit tests covering production, staging (file-backed secrets), Railway-domain fallback, partial OAuth, CI contract, and invalid config scenarios. Good coverage of the validation surface.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    ENV["process.env / SvelteKit env"] --> RSS["runtime-secrets.ts\ngetRuntimeConfigValues()"]
    FILE["_FILE env vars\n(Docker secrets)"] --> RSS
    RSS --> SNAP["resolveRuntimeConfigSnapshot()\nruntime-secrets.ts (lib/config)"]
    SNAP --> CORE["inspectRuntimeConfig()\nruntime-config-core.ts"]
    CORE -->|ok=false| ASSERT["assertProductionRuntimeConfig()\nthrows on staging/prod"]
    CORE -->|health report| ADMIN["Admin Integrations Page\n/admin/settings/integrations"]
    CORE -->|origin| AUTH["auth.ts\nbetterAuth baseURL"]
    RSS --> EMAIL["email.ts\nnodemailer transport"]
    RSS --> MEILI["meilisearch.ts\nMeiliSearch client"]
    RSS --> S3["object-storage.ts\nS3Client"]
    RSS --> REINDEX["/api/reindex\ntoken check + reindex"]
    RSS --> SOURCEOPS["/api/source-ops/run-due\ntoken check + scheduler"]
Loading

Comments Outside Diff (1)

  1. src/lib/server/auth.ts, line 34-45 (link)

    P1 Unescaped HTML in email templates

    user.name and user.email are interpolated directly into HTML strings without encoding. A display name containing <, >, &, or " characters will produce malformed HTML in the email body — e.g. a name like Alice & Bob will be rendered literally in many email clients rather than as the expected entity &amp;. The same applies to newEmail in the sendChangeEmailVerification callback (line 77) and to the ${url} text content on line 43.

    A minimal fix is to HTML-encode user-supplied values before interpolation:

    function escapeHtml(s: string): string {
      return s.replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;').replace(/"/g, '&quot;');
    }

    Then use escapeHtml(user.name ?? user.email) and escapeHtml(newEmail) in the template strings. The url argument from better-auth is system-generated and low-risk, but encoding it would also be consistent.

Reviews (2): Last reviewed commit: "fix: clear staging merge blockers" | Re-trigger Greptile

Comment thread src/lib/server/email.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d9e26cb11a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

if (headers.length === 0) return [];

const rows: Array<Record<string, unknown>> = [];
for (let rowNumber = 2; rowNumber <= worksheet.actualRowCount; rowNumber += 1) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Iterate XLSX rows by row index instead of actualRowCount

The loop bound uses worksheet.actualRowCount, but in ExcelJS that value is the count of non-empty rows, not the highest row index. When a sheet contains gaps (for example, a header row followed by blank spacer rows and then data), rowNumber <= worksheet.actualRowCount stops too early and silently drops later records. This is a data-loss regression in ingestion because valid rows after gaps are never parsed.

Useful? React with 👍 / 👎.

@greptile-apps

greptile-apps Bot commented Apr 8, 2026

Copy link
Copy Markdown

Tip:

Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.

Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

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