Skip to content

fix(security): address pentest findings#2347

Closed
jeanduplessis wants to merge 9 commits intomainfrom
pentest-fixes
Closed

fix(security): address pentest findings#2347
jeanduplessis wants to merge 9 commits intomainfrom
pentest-fixes

Conversation

@jeanduplessis
Copy link
Copy Markdown
Contributor

@jeanduplessis jeanduplessis commented Apr 13, 2026

Summary

Code-related pentest findings [private access to Kilo team members only] are now addressed without broad product rewrites. Each fix focuses on closing the reported behavior while preserving existing auth, analytics, payments, and device-token flows.

M2: Web session revocation after logout

Related commits: a8b5a06ab, be7dad5f

  • Why needed: NextAuth logout only cleared the client cookie. A copied or stale web session cookie could still authenticate until normal JWT expiry.
  • Solution chosen: Added kilocode_users.web_session_version, stores that version in the NextAuth JWT/session at sign-in, and rejects requests in getUserFromAuth when the session version no longer matches the database row.
  • Logout behavior: Added POST /api/auth/revoke-web-session and call it from normal sign-out, auto sign-out, and dev account nuke flows before client-side sign-out.
  • Concurrent login behavior: This does not enforce a single active browser session. Multiple browsers can stay logged in at the same time because each sign-in receives the current version. Logging out from any browser increments the version and invalidates all existing web sessions for that user.
  • Counter decision: A numeric increment is sufficient because web_session_version is not a secret. It is a revocation counter inside a signed/encrypted NextAuth JWT. Knowing the next number does not help unless an attacker can mint a valid NextAuth token.
  • Trade-off: Did not rotate api_token_pepper on logout. That would have fixed stale web sessions, but it would also revoke CLI/device/API tokens unexpectedly. A separate web-only version keeps logout scoped to browser sessions.

M2: Route-level admin API authorization

Related commit: a8b5a06ab

  • Why needed: Admin credit category APIs were relying on middleware as the effective auth boundary. Middleware JWT state alone should not be the final authorization check for admin APIs.
  • Solution chosen: Added route-level getUserFromAuth({ adminOnly: true }) checks to the credit category admin API endpoints.
  • Current coverage: Non-credit admin API paths are not known to share the same gap. Most already call direct route-level admin auth. The remaining enrichment upsert route delegates through tRPC, where context calls getUserFromAuth and adminProcedure enforces ctx.user.is_admin.
  • Trade-off: Kept the change targeted to the middleware-only gaps identified in the plan instead of refactoring all admin routing helpers. If we want every /admin/api/** file to visibly call direct admin auth, the tRPC-backed enrichment route can be tightened separately for consistency.

M3: Global Content Security Policy

Related commits: 6f1a18ca, be809b65, 8041b017

  • Why needed: App pages did not send a global CSP, leaving script, frame, image, and connect sources unconstrained.
  • Solution chosen: Added a nonce-backed CSP builder and generates a per-request nonce in proxy.ts. GTM/Impact bootstrap code is served from same-origin script routes instead of inline root-layout scripts, so the root layout no longer calls headers() and does not force site-wide dynamic rendering just for CSP.
  • Allowlist decision: Included sources needed by current browser flows: Stripe, Stytch/login domains, GTM, Impact, PostHog, Turnstile, app websocket/connect origins, and lottie WASM fallback hosts observed during browser verification.
  • Production risk: CSP can still block production-only sources. GTM can inject tags configured outside the repo, third-party SDKs can load secondary hosts, and Stripe 3DS/SCA or Stytch telemetry can vary by tenant, card flow, region, and runtime condition.
  • Operational flexibility: Added directive-specific env extensions (CSP_ADDITIONAL_SCRIPT_SRC, CSP_ADDITIONAL_CONNECT_SRC, CSP_ADDITIONAL_IMG_SRC, CSP_ADDITIONAL_STYLE_SRC, CSP_ADDITIONAL_FONT_SRC, CSP_ADDITIONAL_FRAME_SRC, CSP_ADDITIONAL_WORKER_SRC, CSP_ADDITIONAL_MEDIA_SRC) so production-only third-party sources can be added without code changes.
  • Trade-off: Kept style-src 'unsafe-inline' because Next/runtime styles commonly require it. This is safer than breaking rendering and can be tightened later with more focused style nonce/hash work. Env extensions reject values containing ; to avoid directive injection.
  • Rollout note: If production breakage risk is too high, the CSP can be staged as report-only first with report-uri/report-to, then enforced after reviewing real production violation reports.

L1: Missing security headers

Related commit: abb0e5e0

  • Why needed: The app already sent several security headers, but the pentest flagged missing hardening headers and X-Powered-By exposure.
  • Solution chosen: Disabled Next's powered-by header and added XSS protection disablement, referrer policy, permissions policy, CORP, and COEP report-only headers while preserving existing HSTS, COOP, X-Frame-Options, and nosniff headers.
  • Trade-off: Shipped Cross-Origin-Embedder-Policy-Report-Only instead of enforced COEP. Enforced COEP can break third-party scripts/assets unless every provider sends compatible CORS/CORP headers.

L2: Email exposure guardrails

Related commits: 72dce744, 6a0ce82e

  • Why needed: Historical email exposure requires operational cleanup, but current code should avoid personal email literals and should not expose authenticated PII to third-party tag scripts.
  • Solution chosen: Moved contributor champion personal email allowlisting to CONTRIBUTOR_CHAMPION_TEAM_EMAILS, removed raw email/name from GTM dataLayer events, and added a production-source email literal guardrail test with explicit role-alias allowlist.
  • Trade-off: Kept intentional public role aliases such as hi@kilo.ai and sales@kilocode.ai; removing them would change product contact behavior rather than close accidental exposure.

I1 CAA DNS and Wayback removal are operational follow-ups outside repo code.

Verification

  • Spec alignment: matched .plans/pentest-findings.md for code-related M2, M3, L1, and L2 scope. I1 CAA and Wayback removal remain operational follow-ups outside repo code.
  • Code evaluations: security, logic, types, data, resource, and style review agents ran across the implementation. Security review found a GTM dataLayer PII issue, the issue was fixed, and security re-review reported no findings.
  • Browser evaluations: agent-browser verified fake user login, CSP presence with nonce/Stripe sources, /api/user returning 401 after revocation with stale cookie, re-login after revocation, and stale admin route rejection for /admin/api/credit-categories.

Visual Changes

N/A

Reviewer Notes

Code Reviewer Notes
  • Session revocation uses a new kilocode_users.web_session_version column and stores the version in the NextAuth JWT/session at sign-in.
  • Numeric session versions are counters, not secrets. Security relies on NextAuth token integrity; the DB comparison provides revocation.
  • This is logout-everywhere for web sessions. It does not block concurrent browser logins.
  • getUserFromAuth rejects stale web sessions when the session version no longer matches the database row.
  • Logout entry points call POST /api/auth/revoke-web-session before client-side signOut; the endpoint is intentionally idempotent and only increments web session state.
  • The API token pepper check was not reused for logout because that would unexpectedly revoke CLI/device/API tokens.
  • Credit category admin APIs now have direct route-level admin auth; other admin APIs are either directly protected or protected through tRPC adminProcedure.
  • CSP is built in apps/web/src/lib/security-headers.ts and applied from proxy.ts; GTM/Impact bootstraps are served through same-origin external script routes to avoid headers() in the root layout.
  • CSP source env extensions are directive-specific and additive only; env values containing semicolons are ignored to avoid injecting extra directives.
  • COEP is Cross-Origin-Embedder-Policy-Report-Only for compatibility with Stripe/Stytch/Impact/GTM/PostHog/Turnstile resources.
  • Contributor champion personal email allowlist is now env-backed via CONTRIBUTOR_CHAMPION_TEAM_EMAILS; approved public aliases remain explicit in the guardrail test.

@kilo-code-bot
Copy link
Copy Markdown
Contributor

kilo-code-bot bot commented Apr 13, 2026

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0

Fix these issues in Kilo Cloud

Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/contributor-champions/service.ts 35 When CONTRIBUTOR_CHAMPION_TEAM_EMAILS is unset, the removed fallback means internal personal accounts are treated as external contributors and can receive contributor rewards.
Other Observations (not in diff)

None.

Files Reviewed (30 files)
  • apps/web/next.config.mjs - 0 issues
  • apps/web/src/app/(app)/components/SidebarUserFooter.tsx - 0 issues
  • apps/web/src/app/admin/api/credit-categories/[key]/route.ts - 0 issues
  • apps/web/src/app/admin/api/credit-categories/route.ts - 0 issues
  • apps/web/src/app/api/auth/revoke-web-session/route.ts - 0 issues
  • apps/web/src/app/api/cloud-agent/sessions/prepare/route.test.ts - 0 issues
  • apps/web/src/app/api/marketing-tags/gtm/route.ts - 0 issues
  • apps/web/src/app/api/marketing-tags/impact/route.ts - 0 issues
  • apps/web/src/app/auto-signout/page.tsx - 0 issues
  • apps/web/src/app/layout.tsx - 0 issues
  • apps/web/src/components/DataLayerProvider.tsx - 0 issues
  • apps/web/src/components/dev/actions.ts - 0 issues
  • apps/web/src/lib/config.server.ts - 0 issues
  • apps/web/src/lib/contributor-champions/service.test.ts - 0 issues
  • apps/web/src/lib/contributor-champions/service.ts - 1 issue
  • apps/web/src/lib/email-literal-guardrail.test.ts - 0 issues
  • apps/web/src/lib/marketing-tag-scripts.test.ts - 0 issues
  • apps/web/src/lib/marketing-tag-scripts.ts - 0 issues
  • apps/web/src/lib/security-headers.test.ts - 0 issues
  • apps/web/src/lib/security-headers.ts - 0 issues
  • apps/web/src/lib/token.test.ts - 0 issues
  • apps/web/src/lib/user.server.ts - 0 issues
  • apps/web/src/lib/web-session-revocation.test.ts - 0 issues
  • apps/web/src/lib/web-session-revocation.ts - 0 issues
  • apps/web/src/proxy.ts - 0 issues
  • apps/web/src/tests/helpers/user.helper.ts - 0 issues
  • apps/web/src/types/datalayer.d.ts - 0 issues
  • apps/web/src/types/next-auth.d.ts - 0 issues
  • packages/db/src/migrations/0086_cool_night_thrasher.sql - 0 issues
  • packages/db/src/schema.ts - 0 issues

Reviewed by gpt-5.4-2026-03-05 · 1,908,661 tokens

@@ -0,0 +1,9 @@
export function buildGoogleTagManagerScript(gtmId: string): string {
const encodedGtmId = JSON.stringify(gtmId);
return `(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start':new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0],j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src='https://www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f);})(window,document,'script','dataLayer',${encodedGtmId});`;

export function buildImpactUttScript(impactUttId: string): string {
const encodedScriptUrl = JSON.stringify(`https://utt.impactcdn.com/${impactUttId}.js`);
return `(function(a,b,c,d,e,f,g){e.ire_o=c;e[c]=e[c]||function(){(e[c].a=e[c].a||[]).push(arguments)};f=d.createElement(b);g=d.getElementsByTagName(b)[0];f.async=1;f.src=a;g.parentNode.insertBefore(f,g);})(${encodedScriptUrl},'script','ire',document,window);`;
@jeanduplessis
Copy link
Copy Markdown
Contributor Author

Superseding this monolithic PR with one PR per pentest finding for easier rollout and rollback:\n\n- L1 security headers: #2350\n- M2 web session revocation/admin auth: #2351\n- L2 email/privacy exposure: #2352\n- M3 CSP: #2353

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