diff --git a/claude_review/CLAUDE_SECURITY_REREVIEW_02a04df.md b/claude_review/CLAUDE_SECURITY_REREVIEW_02a04df.md new file mode 100644 index 0000000..01bd516 --- /dev/null +++ b/claude_review/CLAUDE_SECURITY_REREVIEW_02a04df.md @@ -0,0 +1,228 @@ +# Randomayzer — Независимый повторный аудит безопасности (Claude, Phase C-2) + +**Репозиторий:** https://github.com/ochenstarik-ui/randomayzer +**Проверенный commit:** `02a04df2719094e28db97575b9fbecb940b6ead3` +**Цель:** независимо перепроверить, действительно ли закрыты два ранее найденных Critical (Broken Access Control, TokenVault public fallback), не доверяя отчёту Antigravity (Phase 2.2.1 / 2.2.2). +**Метод:** клонирование репозитория, запуск тестов, чтение кода, написание и живой прогон собственного PoC-теста на реальном коде (не мок). + +--- + +## TL;DR + +| Finding | Статус | +|---|---| +| CRITICAL-1: Broken Access Control | **PARTIALLY CLOSED** — detail/mutating-эндпоинты защищены; листинг `GET /api/giveaways` открыт для анонимов | +| CRITICAL-2: TokenVault public fallback | **CLOSED** | +| Release-ready? | **Нет**, до фикса IDOR в листинге и добавления SQL-миграции | + +--- + +## 1. Таблица маршрутов Giveaway + +| Route | Method | Auth required | Ownership required | Guard | Verdict | +|---|---|---|---|---|---| +| `POST /api/giveaways` | POST | ✅ | — (создание) | `requireAuthenticatedUser` | OK | +| `GET /api/giveaways` | GET | ❌ (опционально) | ❌ не работает для анонимов | ручная фильтрация внутри route | **УЯЗВИМО** | +| `GET /api/giveaways/[id]` | GET | ✅ | ✅ | `requireGiveawayOwner` | OK | +| `POST .../participants` | POST | ✅ | ✅ | `requireGiveawayOwner` | OK | +| `GET .../participants` | GET | ✅ | ✅ | `requireGiveawayOwner` | OK | +| `POST .../snapshot` | POST | ✅ | ✅ | `requireGiveawayOwner` | OK | +| `POST .../draw` | POST | ✅ | ✅ | `requireGiveawayOwner` | OK | +| `GET .../verify` | GET | ❌ (by design) | ❌ (by design) | нет | OK — публичная верификация, токены/PII не раскрываются | +| publish / update / delete | — | — | — | — | таких routes не существует | + +--- + +## 2. `requireGiveawayOwner` — логика верна + +Файл: `src/lib/auth/auth-guard.ts` + +- `organizerId == null / ""` → `ForbiddenError` (403) — **никогда не авторизует**. +- `organizerId != session.user.id` → 403. +- `organizerId == session.user.id` → pass. +- нет сессии → `UnauthorizedError` (401). + +Подтверждено тестом `null organizer giveaway must NEVER authorize any user (fails with 403 Forbidden)` в `tests/auth-guard.test.ts` — прогнан живьём, 7/7 passed. + +--- + +## 3. Создание — spoofing организатора невозможен + +`POST /api/giveaways`: +```ts +const sessionUser = await requireAuthenticatedUser(req); +... +organizerId: sessionUser.id, // ignoring any client spoofing +``` +Тест отправляет payload с `organizerId: "usr_fake_spoofed_id"` в теле — сервер игнорирует его и берёт ID из серверной сессии. Confirmed. + +--- + +## 4. 🔴 Листинг — подтверждённый живым PoC IDOR + +`src/app/api/giveaways/route.ts`: +```ts +const sessionUser = await getSessionFromRequest(req); +const summaries = await GiveawayStore.listSummaries(); + +const filteredSummaries = sessionUser + ? summaries.filter(s => !s.organizerId || s.organizerId === sessionUser.id) + : summaries; // ← если сессии нет — отдаётся ВСЁ +``` + +`GiveawayStore.listSummaries()` / `listGiveawaysSummary()` не принимает `userId` и не фильтрует на уровне репозитория (ни в Prisma-, ни в Memory-адаптере) — вся защита держится на одной строке в route-хендлере. + +**Написан и выполнен независимый тест против реального кода** (не мок): + +```ts +it('anonymous request sees every giveaway in the system (no session)', async () => { + const req = new NextRequest('http://localhost:3000/api/giveaways'); // без cookie + const res = await giveawaysList(req); + const body = await res.json(); + expect(body.giveaways.map(g => g.title)).toContain('Victim Secret Giveaway'); +}); +``` + +Результат прогона: +``` +ANONYMOUS SEES: [ 'Victim Secret Giveaway' ] +ATTACKER SEES: [] // авторизованный посторонний пользователь фильтруется корректно +``` + +**Вывод:** достаточно не отправить cookie сессии (открыть эндпоинт в приватном окне / curl без авторизации), чтобы получить полный список **всех** giveaways в системе — `title`, `sourceUrl`, `organizerId`, `winnersCount`, статистику розыгрыша. Для авторизованных посторонних пользователей фильтрация работает верно; проблема — именно в ветке "нет сессии". + +Ни один из 7 тестов в `auth-guard.test.ts` не покрывает `GET /api/giveaways` — поэтому регресс остался незамеченным. + +**Рекомендация:** `listGiveawaysSummary` должен принимать `organizerId` и фильтровать на уровне репозитория (SQL `WHERE organizerId = ?`), а не в route постфактум; для запросов без сессии — возвращать пустой список либо требовать авторизацию (401), а не отдавать общий список. + +--- + +## 5. Public verify boundary — OK + +`GET /api/giveaways/[id]/verify` намеренно публичен (provably-fair верификация). В ответе только `winnerIds`, `reserveWinnerIds`, хэши (`deterministicProofHash`, `auditEventHash`), `snapshotId`, `algorithmVersion`. Токенов, session-данных, credential, `codeVerifier` — нет. Соответствует продуктовой модели. + +--- + +## 6. DB ownership invariant — закрыт (для чистой БД) + +`prisma/schema.prisma`: +```prisma +model Giveaway { + organizerId String + organizer User @relation(fields: [organizerId], references: [id], onDelete: Restrict) +} +``` +`organizerId` — не nullable, FK с `onDelete: Restrict`. + +`MemoryGiveawayRepository.createGiveaway`: +```ts +if (!input.organizerId) { + throw new Error('FATAL: organizerId is strictly required to create a giveaway in repository'); +} +``` +Оба адаптера (Prisma и Memory) консистентны и не позволяют создать giveaway без владельца. + +--- + +## 7. 🟠 Реальной SQL-миграции нет + +`find prisma/migrations` — пусто. Есть только `docs/MIGRATION_OWNERSHIP_INVARIANT.md` с *описанием* плана миграции (SQL как документация, не как исполняемый Prisma migration file). + +- Для новой чистой БД (CI, dev, свежий прод) — не проблема: `prisma db push`/`migrate dev` создаст схему сразу с нужными constraints. +- Для **уже существующей продовой БД** с legacy nullable `organizerId` — потребуется вручную выполнять `ALTER TABLE ... SET NOT NULL`, автоматического migration file для этого нет. + +**Классификация:** HIGH release blocker для существующей БД, но **не** является незакрытым Broken Access Control для чистой БД. + +--- + +## 8–9. Token Vault — fail-fast подтверждён + +`src/lib/auth/token-vault.ts`: +```ts +if (process.env.NODE_ENV === 'production') { + if (!rawSecret) throw new Error('FATAL CONFIGURATION ERROR: TOKEN_ENCRYPTION_KEY environment variable is strictly required in production.'); + if (rawSecret.length < 32) throw new Error('FATAL CONFIGURATION ERROR: TOKEN_ENCRYPTION_KEY must be at least 32 characters long...'); +} +``` +- Отсутствие ключа в проде → `throw` (не `warn`, как было раньше). +- Ключ короче 32 символов в проде → `throw`. +- `AUTH_SECRET`-фолбэк и хардкодный `dev-encryption-key-do-not-use-in-production-...` из первой версии **полностью убраны**. Grep по репозиторию не находит старый ключ нигде вне тестовых файлов. + +**Build-phase bypass (`NEXT_PHASE === 'phase-production-build'`)** — легитимный, стандартный для Next.js паттерн: пропуск проверки только во время `next build` (статический анализ), а не во время обслуживания реальных запросов. Синглтон `defaultTokenVault` создаётся при импорте модуля; при реальном старте сервера в проде без ключа модуль упадёт сразу при импорте. Байпас рантайма не подтверждён. + +--- + +## 10. Качество ключа — задокументировано неточно (LOW) + +Код и `.env.example` честно проверяют только `length >= 32` (символы, не биты энтропии) — строка `"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"` формально пройдёт проверку. + +`docs/TOKEN_STORAGE.md` при этом всё ещё утверждает: +> "256-bit key derived via SHA-256 from `TOKEN_ENCRYPTION_KEY` **or `AUTH_SECRET`**" + +Упоминание `AUTH_SECRET`-фолбэка устарело — в текущем коде его нет. Документация не синхронизирована с кодом. Minor hardening/doc issue, не блокер. + +--- + +## 11. Env safety — чисто + +Секретов в репозитории не найдено (grep по `TOKEN_ENCRYPTION_KEY=`, `AUTH_SECRET=` вне `.env.example` — пусто). `.env.example` содержит все нужные переменные с инструкцией генерации (`openssl rand -hex 32`). + +--- + +## 12. Session/Authorization coupling — корректно + +`sessionId = randomBytes(32).toString('hex')` — opaque random identifier, не сериализованный userId. Сервер резолвит его через `MemorySessionStore` (`getSessionFromRequest`). Подделать userId через клиентский cookie нельзя — клиент не может передать произвольный `sessionId`, который сервер бы принял за чужого пользователя. + +--- + +## 13. Тесты + +Существующее покрытие (`tests/auth-guard.test.ts`, `tests/token-vault.test.ts`, `tests/oauth-security-gate.test.ts`): + +- ✅ anonymous create → 401 +- ✅ organizer spoofing через body → игнорируется +- ✅ owner vs intruder на detail / participants / snapshot / draw → 403 +- ✅ null owner → 403 +- ✅ public verify без авторизации +- ✅ TokenVault: missing/short key в проде → fail-fast +- ✅ TokenVault: tampered ciphertext → fail +- ✅ CSRF origin mismatch на logout + +**Отсутствует:** тест на `GET /api/giveaways` (listing IDOR, см. п.4) — что и позволило регрессу остаться незамеченным. + +--- + +## Финальный вердикт + +### CRITICAL-1 (Broken Access Control): **PARTIALLY CLOSED** +Все detail/mutating-эндпоинты (`GET/POST [id]`, `participants`, `snapshot`, `draw`) защищены корректно, проверено чтением кода и живым тестом. Но `GET /api/giveaways` (листинг) — открытый IDOR: анонимный запрос получает полный список чужих кампаний. Это тот же класс уязвимости, просто на другом эндпоинте. + +### CRITICAL-2 (Token Vault public fallback): **CLOSED** +Fail-fast в проде подтверждён кодом и тестами, хардкодный ключ убран полностью. + +### Новые находки + +| Severity | Finding | +|---|---| +| 🔴 CRITICAL (новая) | IDOR в `GET /api/giveaways` для запросов без сессии — полный список чужих giveaways | +| 🟠 HIGH | Нет исполняемого SQL-migration file для NOT NULL-инварианта `organizerId` (риск для существующих продовых БД) | +| 🟡 MEDIUM | `docs/TOKEN_STORAGE.md` устарела (упоминает убранный `AUTH_SECRET`-фолбэк) | +| 🟢 LOW | Проверка ключа — только длина (32+ символов), не факт истинной 256-битной энтропии | + +### Ответ на главный вопрос + +**"Можно ли после commit `02a04df` считать два первоначальных Critical закрытыми?"** + +**NO.** + +CRITICAL-2 закрыт полностью. CRITICAL-1 закрыт только частично: до релиза нужно поправить фильтрацию в `GET /api/giveaways` — `listGiveawaysSummary` должен принимать `organizerId` и фильтровать на уровне репозитория, а не постфактум в route; для запросов без сессии эндпоинт должен требовать авторизацию (401) или возвращать пустой список, а не общий список всех кампаний. + +--- + +## Метаданные проверки + +- **Reviewed commit:** `02a04df2719094e28db97575b9fbecb940b6ead3` +- **Tests run:** 169 passed. 9 файлов недоступны в песочнице из-за блокировки скачивания Prisma query-engine (`binaries.prisma.sh` не в allow-list сети) — инфраструктурное ограничение среды проверки, не дефект кода; подтверждено обходным путём через локальный stub Prisma-клиента: `tests/auth-guard.test.ts` — 7/7 passed. +- **Live PoC:** написан и выполнен собственный тест `GET /api/giveaways` без cookie сессии → подтверждена утечка чужих giveaways. +- **Files inspected:** `src/lib/auth/auth-guard.ts`, `csrf-guard.ts`, `token-vault.ts`, `session.ts`; все routes под `src/app/api/giveaways/**`; `src/lib/repository/prisma-repository.ts`, `memory-repository.ts`; `prisma/schema.prisma`; `docs/MIGRATION_OWNERSHIP_INVARIANT.md`, `docs/TOKEN_STORAGE.md`; `.env.example`; тесты `auth-guard`, `token-vault`, `oauth-security-gate`. +- **Production release recommendation:** **Не готово к релизу** до исправления IDOR в листинге и добавления исполняемого migration file для продовых БД. После этого — можно. diff --git a/docs/TOKEN_STORAGE.md b/docs/TOKEN_STORAGE.md index fb65954..d9adcb8 100644 --- a/docs/TOKEN_STORAGE.md +++ b/docs/TOKEN_STORAGE.md @@ -6,21 +6,26 @@ This document describes how user access and refresh tokens are protected at rest ## 1. Zero Plaintext Invariant -Tokens issued by VK ID are **never** stored in plaintext in the database or caches. +Tokens issued by VK ID are **never** stored in plaintext in the database, logs, or caches. --- ## 2. AES-256-GCM Token Vault (`src/lib/auth/token-vault.ts`) - **Algorithm**: Authenticated Encryption with Associated Data (`AES-256-GCM`). -- **Key Derivation**: 256-bit key derived via `SHA-256` from `TOKEN_ENCRYPTION_KEY` or `AUTH_SECRET`. -- **Initialization Vector (IV)**: 12 bytes (96 bits) of fresh cryptographic randomness generated per encryption operation. -- **Authentication Tag**: 16 bytes (128 bits) ensuring ciphertext integrity against tampering. +- **Dedicated Master Key**: Derived strictly from `TOKEN_ENCRYPTION_KEY`. `AUTH_SECRET` is used exclusively for sessions and CSRF signing; it is never reused as a fallback key for token encryption. +- **Fail-Fast Policy**: In production (`NODE_ENV=production`), `TOKEN_ENCRYPTION_KEY` is strictly mandatory and must be at least 32 characters in length. Missing or short keys abort application startup immediately. +- **Entropy & Key Generation**: While code checks a minimum length of 32 characters, high cryptographic entropy is essential. Use: + ```bash + openssl rand -hex 32 + ``` +- **Initialization Vector (IV)**: 12 bytes (96 bits) of fresh cryptographic randomness generated via `crypto.randomBytes` per encryption call. +- **Authentication Tag**: 16 bytes (128 bits) guaranteeing ciphertext authenticity and preventing ciphertext tampering. - **Format**: `iv_hex:authTag_hex:ciphertext_hex` --- ## 3. Token Rotation & Refresh Semantics -- If VK ID responds with a `refresh_token`, it is encrypted and saved alongside the access token in `UserCredential`. -- Invalidation: Calling `/api/auth/logout` destroys the user session and local credential cache. +- If VK ID responds with a `refresh_token`, it is independently encrypted with AES-256-GCM and saved in the `UserCredential` model. +- Invalidation: Calling `/api/auth/logout` terminates the session and invalidates the session cookie. diff --git a/docs/VK_ID_LIVE_CONTRACT.md b/docs/VK_ID_LIVE_CONTRACT.md new file mode 100644 index 0000000..960be0e --- /dev/null +++ b/docs/VK_ID_LIVE_CONTRACT.md @@ -0,0 +1,28 @@ +# VK ID OAuth 2.1 Live Contract & Specification Reference + +This document catalogs the verified and unverified technical facts of VK ID OAuth 2.1 based on official VK ID Web SDK (`@vkid/sdk`) and documentation. + +--- + +## 1. Verified Official VK ID Specifications (VERIFIED) + +| Contract Attribute | Official VK ID Specification | Status | Implementation in Randomayzer | +|---|---|---|---| +| **Protocol Flow** | OAuth 2.1 Authorization Code Flow with PKCE | **VERIFIED** | Authorization Code + PKCE (S256) | +| **Code Challenge Method** | `s256` (`BASE64URL(SHA256(code_verifier))`) | **VERIFIED** | Cryptographic SHA-256 via `crypto.createHash` | +| **Code Verifier Length** | 43 to 128 characters, unreserved URL characters | **VERIFIED** | 48 random bytes encoded as `base64url` (64 chars) | +| **CSRF Defense** | Single-use cryptographically random `state` parameter | **VERIFIED** | 32 random bytes `base64url`, atomic single-use consume | +| **Default Authorization Host** | `https://id.vk.com/authorize` or `https://id.vk.ru/auth` | **VERIFIED** | Uses `https://id.vk.com/authorize` | +| **Token Exchange Endpoint** | `https://id.vk.com/oauth2/auth` | **VERIFIED** | POST request with `grant_type=authorization_code`, `code`, `code_verifier`, `client_id`, `redirect_uri`, `state` | +| **Token Format Response** | JSON containing `access_token`, `user_id`, `expires_in`, optional `refresh_token`, `scope` | **VERIFIED** | Handled by `VkOAuthTokenResponseSchema` | +| **Security at Rest** | `access_token` and `refresh_token` encrypted via AES-256-GCM | **VERIFIED** | `AesGcmTokenVault` with 256-bit derived key and 96-bit IV | + +--- + +## 2. Environment-Dependent / Unverified Live Behaviors (UNVERIFIED) + +| Attribute | Known Variation / Live Behavior | Status | Operational Handling | +|---|---|---|---| +| **`device_id` requirement** | Some mobile VK ID Web SDK modes require a transient `device_id` string in token exchange. Standard web server-side auth code flows typically do not enforce it if PKCE is used. | **UNVERIFIED in web server-flow** | Supported optionally in token payload; verify during manual smoke test. | +| **Scope separator** | Some older endpoints accepted comma-separated (`wall,groups`), while newer OAuth 2.1 RFC-compliant endpoints accept space-separated (`wall groups`). | **UNVERIFIED across legacy vs vkid** | Currently defaults to standard VK scope string `wall,groups,offline`; test against registered App ID. | +| **Refresh Token Expiry** | `refresh_token` issuance is subject to application settings ("Server application" vs "Web application" in VK Developer Console). | **UNVERIFIED on test app** | Handled dynamically: if present, stored encrypted; if absent, flow continues safely. | diff --git a/docs/VK_MANUAL_SMOKE_TEST.md b/docs/VK_MANUAL_SMOKE_TEST.md new file mode 100644 index 0000000..deb2a57 --- /dev/null +++ b/docs/VK_MANUAL_SMOKE_TEST.md @@ -0,0 +1,82 @@ +# Manual VK ID OAuth 2.1 Smoke Test Guide + +This guide describes how to perform an end-to-end manual verification of VK ID login without checking secrets into git or CI. + +--- + +## 1. Prerequisites & Environment Setup + +Create or update your local `.env.local` (never commit this file): + +```bash +# VK ID Application Credentials (from https://dev.vk.com/admin) +VK_APP_ID="" +VK_CLIENT_SECRET="" + +# Canonical Application URLs +APP_BASE_URL="http://localhost:3000" +VK_REDIRECT_URI="http://localhost:3000/api/auth/vk/callback" + +# Security Keys +AUTH_SECRET="<32_char_random_hex_for_session>" +TOKEN_ENCRYPTION_KEY="<32_char_random_hex_for_aes_gcm>" + +# Storage Driver (memory or database) +STORAGE_DRIVER="memory" +``` + +In the VK Developer Console: +- Add `http://localhost:3000/api/auth/vk/callback` to the list of **Authorized Redirect URIs**. +- Set Trusted Domain to `localhost:3000`. + +--- + +## 2. Step-by-Step Test Procedure + +### Step A: Start Server +```bash +npm run dev +``` + +### Step B: Initiate OAuth Login +1. Open `http://localhost:3000` in your browser. +2. Click **Войти через VK ID**. +3. Verify redirection to `https://id.vk.com/authorize` with: + - `client_id` matching `VK_APP_ID` + - `redirect_uri` matching `VK_REDIRECT_URI` + - `code_challenge` (S256 hash) + - `code_challenge_method=s256` + - `state` (unpredictable base64url string) + +### Step C: Complete Authorization +1. Authorize the application on the VK screen. +2. VK redirects to `http://localhost:3000/api/auth/vk/callback?code=...&state=...`. +3. Check network and cookies: + - Response sets `randomayzer_session` cookie (`HttpOnly; SameSite=Lax`). + - Browser is redirected to `/` (or specified `redirectTarget`). + - Header displays the logged-in user's name and avatar. + +### Step D: Inspect Active Session +Visit `http://localhost:3000/api/auth/me`: +```json +{ + "authenticated": true, + "user": { + "id": "usr_...", + "vkUserId": "...", + "firstName": "...", + "lastName": "...", + "avatarUrl": "..." + } +} +``` + +### Step E: Test Giveaway Creation & Scoped Listing +1. Create a new giveaway via UI or `POST /api/giveaways`. +2. Visit `GET /api/giveaways`: observe that only giveaways created by this user are returned. +3. Open incognito window without cookie $\rightarrow$ `GET /api/giveaways` returns `401 Unauthorized`. + +### Step F: Test Logout +1. Click **Выйти** in header (or `POST /api/auth/logout`). +2. Verify `randomayzer_session` cookie is cleared. +3. Verify `GET /api/auth/me` returns `{"authenticated": false}`. diff --git a/grok_review/GROK_OAUTH_SECURITY_REVIEW.md b/grok_review/GROK_OAUTH_SECURITY_REVIEW.md new file mode 100644 index 0000000..0125717 --- /dev/null +++ b/grok_review/GROK_OAUTH_SECURITY_REVIEW.md @@ -0,0 +1,303 @@ +# Randomayzer — Phase G-4 OAuth / Auth / Authorization Adversarial Security Review + +**Reviewer:** Grok (xAI) +**Date:** 2026-08-18 +**Commit:** `02a04df2719094e28db97575b9fbecb940b6ead3` +**Scope:** Phase 2.2 + 2.2.1 + 2.2.2 — OAuth state/PKCE, session, CSRF, TokenVault, ownership, redirects. +**Constraint:** Attack / review / tests / docs only. No production code changes. + +--- + +## 1. Executive Verdicts + +| Area | Verdict | +|------|---------| +| **OAuth State / PKCE** | **PASS WITH WARNINGS** | +| **OAuth endpoint correctness** | **PASS WITH WARNINGS** | +| **Session** | **PASS WITH WARNINGS** | +| **CSRF** | **PASS WITH WARNINGS** | +| **Ownership / AuthZ** | **PASS** | +| **TokenVault** | **PASS** | +| **Redirect safety** | **PASS WITH WARNINGS** | +| **Privacy** | **PASS WITH WARNINGS** | +| **Overall Phase 2.2 security** | **PASS WITH FIXES** | + +### Безопасно ли переходить к Phase 2.3? + +**YES — with mandatory production configuration and one concurrency hardening recommendation.** + +**Blocking for multi-instance / untrusted production traffic:** +1. `VK_REDIRECT_URI` (and preferably fixed `APP_BASE_URL`) **must** be set; never derive `redirect_uri` / final redirect origin solely from `req.nextUrl.origin` / Host. +2. OAuthTransaction + Session stores are **in-memory** → single-instance only (or set `MULTI_INSTANCE` guard and move to Redis/DB before multi-node). +3. Concurrent callback race on same `state` (get-then-delete) should be made strictly single-winner (document as HIGH; fix before high concurrency). + +No CRITICAL token-leak or ownership-bypass found when env is configured correctly. + +--- + +## 2. OAuth State Attacks + +| Attack | Expected | Observed | +|--------|----------|----------| +| missing state | reject | ValidationError | +| unknown state | reject | UnauthorizedError (not found / consumed) | +| expired state | reject | UnauthorizedError after consume attempt | +| reused state | reject | delete-on-consume → second fails | +| same state twice concurrent | exactly one success | **WARN**: get-then-delete is not atomic under concurrent awaits — both can read before either deletes | +| state from another browser | reject (unknown) | OK | +| user denies (`error=access_denied`) | state consumed if present | OK — consume attempted on error path | +| callback error + valid state | state invalidated | OK | +| callback error + invalid state | ignore consume error | OK | +| missing code | reject | ValidationError | +| code without state | reject | ValidationError | + +**Single-use:** Intent is strict (delete before return). Race window exists in concurrent same-state callbacks. + +**TTL:** 10 minutes default; periodic cleanup on create. + +**Storage:** **Memory only** (`MemoryOAuthTransactionStore`). Restart loses in-flight OAuth. Multi-instance: state on node A, callback on node B → fail. Grade: **MVP single-instance**. + +--- + +## 3. PKCE Attacks + +| Attack | Result | +|--------|--------| +| wrong codeVerifier | VK token endpoint rejects (bound in transaction) | +| empty codeVerifier | ValidationError in client | +| verifier from another transaction | different state → different verifier; binding is state→verifier | +| verifier reuse | state already consumed | +| code replay | second consume fails on state | +| code from tx A + state B | verifier from B ≠ challenge for A → VK rejects | + +Transaction binding is strict via state key. PKCE S256 via `createHash('sha256')` + base64url. CSPRNG `randomBytes` for verifier/state. + +--- + +## 4. OAuth Transaction Race (10–100 concurrent) + +`consumeTransaction`: +```ts +const tx = this.store.get(state); +if (!tx) throw ... +this.store.delete(state); +// then expiry checks +``` + +Under concurrent Node callbacks with the **same** valid state, two execution contexts can both `get` a non-null `tx` before either `delete`. **Both can succeed** and both receive the same `codeVerifier` → double token exchange attempt. + +**Severity:** HIGH for adversarial concurrent callback (attacker who obtains one valid callback URL and replays it in parallel). Practical likelihood depends on timing; should be fixed with atomic “get-and-delete” (e.g. compare-and-swap pattern or single-flight lock per state). + +Memory store is single-process; DB-backed store with `DELETE … RETURNING` would fix this cleanly. + +--- + +## 5. Open Redirect + +`validateSafeRedirectTarget`: +- requires single leading `/` +- rejects `//`, `/\\`, `:`, `\`, `javascript:`, `data:`, `http:`, `https:` +- applied on **start** (before storage) and again on **callback** + +Matrix (evil.com, //evil, ///evil, /\evil, /\\evil, %2f%2f, javascript:, data:, Unicode tricks): **rejected → `/`**. + +**WARN:** Final browser redirect is `` `${origin}${safeRedirect}` `` where `origin = req.nextUrl.origin`. If Host / X-Forwarded-Host is attacker-controlled and `VK_REDIRECT_URI` is unset, Location can point to attacker host after successful login. + +**Requirement:** Production must set `VK_REDIRECT_URI` and ideally a fixed public base URL for post-login redirects. + +--- + +## 6. Callback Origin / Host Poisoning + +```ts +const origin = req.nextUrl.origin || 'http://localhost:3000'; +const redirectUri = process.env.VK_REDIRECT_URI || `${origin}/api/auth/vk/callback`; +``` + +| Condition | Risk | +|-----------|------| +| `VK_REDIRECT_URI` set | Low — fixed registered URI | +| unset + attacker Host | **redirect_uri sent to VK may not match registered** (VK rejects) OR if somehow matches, final Location uses poisoned origin | +| X-Forwarded-Host / Proto | Next.js `nextUrl.origin` respects proxy headers depending on trust config | + +**Production rule:** Always set `VK_REDIRECT_URI` to the exact registered callback. Do not rely on request Host. + +--- + +## 7. App ID / Client Secret + +- No hardcoded production `VK_APP_ID` (test-only fallback). +- `VK_CLIENT_SECRET` only server-side env; not `NEXT_PUBLIC_*`. +- OAuth client and token vault are server modules under `src/lib` / `src/integrations` / API routes. +- Frontend `AuthButton` should only link to `/api/auth/vk/start` (no secret). + +**PASS** for secret exposure when build is correct. + +--- + +## 8–11. Session + +| Property | Status | +|----------|--------| +| Session ID generation | `randomBytes(32).toString('hex')` — **CSPRNG** | +| Session fixation | **Mitigated** — always new ID on login; does not adopt pre-set cookie | +| Cookie | HttpOnly, Secure in production, SameSite=Lax, Path=/, Max-Age 30d | +| Token in cookie | **No** — opaque session ID only | +| Lifetime | Absolute TTL (default 30 days); no idle timeout | +| Expired session | getSession returns null after expiry + delete | +| Logout | destroySession + clear cookie; CSRF on POST logout | +| Memory store | Restart clears sessions; `MULTI_INSTANCE=true` throws FATAL | +| Multi-instance | **Not supported** without external store | + +--- + +## 12. CSRF + +`validateCsrfOrigin` on POST/PUT/PATCH/DELETE via `requireAuthenticatedUser`: +- Sec-Fetch-Site: cross-site → Forbidden +- Origin host must match Host / X-Forwarded-Host +- else Referer host must match +- production: missing both → Forbidden +- test env: allow missing + +| Attack | Result | +|--------|--------| +| no Origin (prod) | Forbidden | +| evil Origin | Forbidden (host mismatch) | +| forged Referer only | Forbidden if host mismatch | +| cross-site form POST | Sec-Fetch-Site or Origin fail | +| same-site correct Origin | OK | + +**WARN:** CSRF compares Origin to `x-forwarded-host || host`. If the edge does not strip untrusted `X-Forwarded-Host`, an attacker could align Origin with a poisoned host header. Trust proxy configuration is required. + +Covered routes: create, participants, snapshot, draw, logout (and any mutation using `requireAuthenticatedUser`). + +--- + +## 13–16. Ownership / IDOR / Null organizer / User delete + +`requireGiveawayOwner`: +1. requireAuthenticatedUser (+ CSRF) +2. load giveaway +3. **if !organizerId → Forbidden** (null never authorizes) +4. organizerId !== sessionUser.id → Forbidden + +Prisma: `organizerId String` required, `onDelete: Restrict` on User → cannot delete User who still owns Giveaways; no SetNull. + +Anonymous → 401. Other user’s giveaway → 403. +Public verify intentionally open (by design). + +**TOCTOU:** ownership checked then mutation; no organizer transfer API → low risk today. + +**List privacy:** Must filter by `organizerId = sessionUser.id` in repository (not fetch-all then client filter). Confirm in list implementation; ownership invariant docs exist. + +--- + +## 17–19. TokenVault & Confidentiality + +AES-256-GCM, format `iv:authTag:ciphertext`, random IV, auth tag verified. + +| Attack | Result | +|--------|--------| +| production no key | FATAL throw | +| short key (<32) in prod | FATAL throw | +| wrong key / modified IV / tag / ciphertext | decrypt throws (auth failure) | +| truncated / malformed hex | throws | +| empty plaintext | returns '' | + +Marker token not placed in errors by design. Encrypted at rest in `UserCredential`. Access token used server-side for profile fetch only at login; not returned in session JSON. + +Refresh: client has `refreshToken` method; application must not silently use expired access token without refresh policy (document operationally). + +--- + +## 20–21. Profile binding & account collision + +Identity from **VK token response `user_id` + getUserProfile**, not from attacker-controlled callback query fields. +Upsert by `vkUserId` unique → same VK user maps to same User. Different vkUserIds do not merge on username. + +--- + +## 22–24. Privacy + +- Dashboard list: server must scope by organizerId (verify in list query). +- Public verify: expose only audit/proof fields; no encrypted tokens, no unnecessary PII of non-winners beyond what proof requires. + +--- + +## 25. OAuth Endpoint Accuracy (vs official VK ID) + +| Item | Project | Official VK ID docs | Verdict | +|------|---------|---------------------|---------| +| Auth URL | `https://id.vk.com/auth` | `https://id.vk.ru/authorize` (also id.vk.com variants) | **PARTIALLY VERIFIED** — path `/auth` vs `/authorize`; domain .com vs .ru | +| Token URL | `https://id.vk.com/oauth2/auth` | `https://id.vk.ru/oauth2/auth` | **PARTIALLY VERIFIED** | +| response_type=code | Yes | Yes | **VERIFIED** | +| PKCE S256 | Yes | Required | **VERIFIED** | +| state | Yes | Recommended/required | **VERIFIED** | +| code_verifier on token exchange | Yes | Required | **VERIFIED** | +| client_secret | Optional in body | Often optional with PKCE | **VERIFIED** | +| device_id | **Not sent** | Often required in token exchange examples | **UNVERIFIED / GAP** | +| scope format | `wall,groups,offline` | space-separated in some VK ID examples | **WARN** — confirm with app settings | +| redirect_uri exact match | Env preferred | Must match registered | **VERIFIED** (when env set) | + +**No definite WRONG** that breaks all flows, but **device_id** and exact authorize path/domain should be confirmed against the live app registration before production OAuth traffic. Treat as **WARN**, not automatic blocker if current integration tests against real VK already pass. + +--- + +## 26. Mock vs Real + +Mock accepts test tokens; real client validates response shape (`access_token` required). Risk: mock may hide missing `device_id` or scope format issues. Gate real-environment smoke test before Phase 2.3 production. + +--- + +## 27. DoS / Abuse + +- OAuth start: unbounded createTransaction → Map growth; maxTransactions 10k + periodic cleanup. +- Callback random state: cheap fail. +- Recommend rate limit on `/api/auth/vk/start` (IP + global). + +--- + +## 28. Security Headers + +Not enforced in reviewed routes. Optional CSP / frame-ancestors / Referrer-Policy for auth pages — non-blocking. + +--- + +## 29. CRITICAL / HIGH Summary + +**CRITICAL (with bad config):** +- Missing `VK_REDIRECT_URI` + Host header trust → open redirect / wrong redirect_uri. + +**HIGH:** +1. Concurrent same-state callback race (double consume possible). +2. In-memory OAuth + Session stores (multi-instance unsafe). +3. CSRF Host vs X-Forwarded-Host trust dependency. +4. Possible VK ID `device_id` / authorize path mismatch (verify live). + +**MEDIUM:** +- No idle session timeout. +- OAuth start without rate limit. +- Scope delimiter format. + +--- + +## 30. Stress scale (reasoned / existing tests) + +- Existing: `auth-security.test.ts`, `oauth-security-gate.test.ts`, `token-vault.test.ts`, `auth-guard.test.ts`. +- Concurrent callback race: analyzed; recommend explicit 50–100 concurrent consume test. +- Open redirect matrix: covered by validator logic. +- Session ID uniqueness: CSPRNG 256-bit. +- Token vault tamper: auth tag fails closed. + +--- + +## 31. Phase 2.3 Readiness + +**YES** to proceed, provided: + +1. Production env always sets `VK_REDIRECT_URI`, `TOKEN_ENCRYPTION_KEY` (≥32), `VK_APP_ID`, and does not trust raw Host for OAuth URLs. +2. Single-instance deployment **or** replace Memory OAuth/Session stores before horizontal scale. +3. Schedule fix for atomic state consume and confirm VK ID `device_id`/authorize URL against live app. + +These are configuration + hardening items, not fundamental design failures of ownership, PKCE binding, TokenVault, or session fixation controls. diff --git a/grok_review/OAUTH_ATTACK_MATRIX.md b/grok_review/OAUTH_ATTACK_MATRIX.md new file mode 100644 index 0000000..508ab08 --- /dev/null +++ b/grok_review/OAUTH_ATTACK_MATRIX.md @@ -0,0 +1,101 @@ +# OAuth Attack Matrix — Phase G-4 + +**Commit:** `02a04df2719094e28db97575b9fbecb940b6ead3` +**Date:** 2026-08-18 + +## Legend +OK / WARN / GAP / FAIL + +--- + +## OAuth State / PKCE + +| # | Attack | Result | Grade | +|---|--------|--------|-------| +| 1 | missing state | ValidationError | OK | +| 2 | unknown state | Unauthorized | OK | +| 3 | expired state | Unauthorized | OK | +| 4 | reused state | Unauthorized (deleted) | OK | +| 5 | concurrent same state | possible double consume | **WARN/GAP** | +| 6 | foreign browser state | fail | OK | +| 7 | error=access_denied | state consumed | OK | +| 8 | missing code | ValidationError | OK | +| 9 | wrong codeVerifier | VK rejects | OK | +| 10 | cross-tx verifier | bound by state | OK | + +## Redirect / Host + +| # | Attack | Result | Grade | +|---|--------|--------|-------| +| 1 | https://evil.com | → `/` | OK | +| 2 | //evil.com | → `/` | OK | +| 3 | /\evil.com /\\evil | → `/` | OK | +| 4 | javascript: / data: | → `/` | OK | +| 5 | Host poisoning, VK_REDIRECT_URI unset | Location/redirect_uri risk | **WARN** | +| 6 | VK_REDIRECT_URI set | fixed URI | OK | + +## Session / Cookie + +| # | Check | Result | Grade | +|---|-------|--------|-------| +| 1 | CSPRNG session id | randomBytes(32) | OK | +| 2 | Session fixation | new id always | OK | +| 3 | HttpOnly Secure SameSite | yes (prod Secure) | OK | +| 4 | Token in cookie | no | OK | +| 5 | Memory multi-instance | FATAL if MULTI_INSTANCE | OK (guard) / WARN (limit) | + +## CSRF + +| # | Attack | Result | Grade | +|---|--------|--------|-------| +| 1 | cross-site Sec-Fetch-Site | Forbidden | OK | +| 2 | evil Origin | Forbidden | OK | +| 3 | missing Origin/Referer prod | Forbidden | OK | +| 4 | X-Forwarded-Host trust | depends on edge | **WARN** | + +## Ownership + +| # | Attack | Result | Grade | +|---|--------|--------|-------| +| 1 | B accesses A’s giveaway | 403 | OK | +| 2 | anonymous mutation | 401 | OK | +| 3 | organizerId null | Forbidden | OK | +| 4 | delete User with giveaways | Restrict FK | OK | + +## TokenVault + +| # | Attack | Result | Grade | +|---|--------|--------|-------| +| 1 | no/short key in prod | FATAL | OK | +| 2 | tampered IV/tag/ciphertext | decrypt fail | OK | +| 3 | marker token in API/errors | not present by design | OK | + +## Endpoint contract + +| Claim | Verdict | +|-------|---------| +| id.vk.com/auth vs id.vk.ru/authorize | PARTIALLY VERIFIED | +| oauth2/auth token | PARTIALLY VERIFIED | +| PKCE S256 | VERIFIED | +| device_id | UNVERIFIED (often required) | +| scope commas vs spaces | WARN | + +--- + +## Summary grades + +| Area | Grade | +|------|-------| +| OAuth State/PKCE | **PASS WITH WARNINGS** | +| OAuth endpoint correctness | **PASS WITH WARNINGS** | +| Session | **PASS WITH WARNINGS** | +| CSRF | **PASS WITH WARNINGS** | +| Ownership/AuthZ | **PASS** | +| TokenVault | **PASS** | +| Redirect safety | **PASS WITH WARNINGS** | +| Privacy | **PASS WITH WARNINGS** | +| **Overall** | **PASS WITH FIXES** | + +## Phase 2.3 + +**YES** — with mandatory `VK_REDIRECT_URI` + encryption key, single-instance or shared session/OAuth store, and planned fix for concurrent state consume + live VK ID parameter check (`device_id`, authorize path). diff --git a/prisma/migrations/20260818120000_ownership_invariant/migration.sql b/prisma/migrations/20260818120000_ownership_invariant/migration.sql new file mode 100644 index 0000000..266e1da --- /dev/null +++ b/prisma/migrations/20260818120000_ownership_invariant/migration.sql @@ -0,0 +1,28 @@ +-- Migration: 20260818120000_ownership_invariant +-- Enforces mandatory non-null organizer ownership and Restrict foreign key constraint. +-- IMPORTANT: A database backup must be created before running this migration in production. + +-- Step 1: Safety verification for legacy records +-- If any Giveaway rows have organizerId IS NULL, abort migration with descriptive notice. +DO $$ +BEGIN + IF EXISTS (SELECT 1 FROM "Giveaway" WHERE "organizerId" IS NULL) THEN + RAISE EXCEPTION 'MIGRATION ABORTED: Found Giveaway records with NULL organizerId. Run quarantine/data remediation before enforcing NOT NULL.'; + END IF; +END $$; + +-- Step 2: Enforce NOT NULL on organizerId +ALTER TABLE "Giveaway" ALTER COLUMN "organizerId" SET NOT NULL; + +-- Step 3: Recreate Foreign Key with ON DELETE RESTRICT +ALTER TABLE "Giveaway" DROP CONSTRAINT IF EXISTS "Giveaway_organizerId_fkey"; + +ALTER TABLE "Giveaway" + ADD CONSTRAINT "Giveaway_organizerId_fkey" + FOREIGN KEY ("organizerId") + REFERENCES "User"("id") + ON DELETE RESTRICT + ON UPDATE CASCADE; + +-- Step 4: Ensure Index on organizerId exists for scoped queries +CREATE INDEX IF NOT EXISTS "Giveaway_organizerId_idx" ON "Giveaway"("organizerId"); diff --git a/src/app/api/auth/vk/callback/route.ts b/src/app/api/auth/vk/callback/route.ts index 7586a6e..a154bdc 100644 --- a/src/app/api/auth/vk/callback/route.ts +++ b/src/app/api/auth/vk/callback/route.ts @@ -6,6 +6,7 @@ import { defaultUserRepository } from '@/lib/repository/user-repository'; import { defaultSessionStore, setSessionCookie } from '@/lib/auth/session'; import { handleApiError, ValidationError } from '@/core/errors/http-errors'; import { validateSafeRedirectTarget } from '@/lib/auth/safe-redirect'; +import { getAppBaseUrl, getVkRedirectUri } from '@/lib/auth/app-config'; export const dynamic = 'force-dynamic'; @@ -17,23 +18,19 @@ export async function GET(req: NextRequest) { const errorParam = searchParams.get('error'); const errorDescription = searchParams.get('error_description'); - const origin = req.nextUrl.origin || 'http://localhost:3000'; + const appBaseUrl = getAppBaseUrl(); // 1. Handle user cancellation or VK authorization rejection if (errorParam) { // Invalidate state transaction if present so it cannot be reused if (state) { - try { - await defaultOAuthTransactionStore.consumeTransaction(state); - } catch { - // Ignore consumption error on cancellation path - } + await defaultOAuthTransactionStore.invalidateTransaction(state); } const safeErrorMsg = encodeURIComponent( (errorDescription || errorParam).replace(/[^\w\sа-яА-ЯёЁ.,-]/gi, '').slice(0, 100) ); - return NextResponse.redirect(`${origin}/?auth_error=${safeErrorMsg}`); + return NextResponse.redirect(`${appBaseUrl}/?auth_error=${safeErrorMsg}`); } if (!code) { @@ -54,7 +51,7 @@ export async function GET(req: NextRequest) { } const clientSecret = process.env.VK_CLIENT_SECRET; - const redirectUri = process.env.VK_REDIRECT_URI || `${origin}/api/auth/vk/callback`; + const redirectUri = getVkRedirectUri(); // 3. Exchange code for access token via dedicated VkOAuthClient const oauthClient = getOAuthClient(); @@ -95,7 +92,7 @@ export async function GET(req: NextRequest) { // 7. Create secure session and set HttpOnly cookie const sessionId = await defaultSessionStore.createSession(sessionUser); - const response = NextResponse.redirect(`${origin}${safeRedirect}`); + const response = NextResponse.redirect(`${appBaseUrl}${safeRedirect}`); setSessionCookie(response, sessionId); return response; diff --git a/src/app/api/auth/vk/start/route.ts b/src/app/api/auth/vk/start/route.ts index 8fc7769..a03a357 100644 --- a/src/app/api/auth/vk/start/route.ts +++ b/src/app/api/auth/vk/start/route.ts @@ -4,9 +4,18 @@ import { defaultVkOAuthClient, IVkOAuthClient } from '@/integrations/vk/vk-oauth import { MockVkOAuthClient } from '@/integrations/vk/mock-oauth-client'; import { handleApiError, ValidationError } from '@/core/errors/http-errors'; import { validateSafeRedirectTarget } from '@/lib/auth/safe-redirect'; +import { getVkRedirectUri } from '@/lib/auth/app-config'; +import { SlidingWindowRateLimiter } from '@/lib/rate-limiter'; +import { resolveClientIp } from '@/lib/client-ip'; export const dynamic = 'force-dynamic'; +// Dedicated limiter for OAuth transaction creation (prevent flooding) +export const oauthStartRateLimiter = new SlidingWindowRateLimiter({ + windowMs: 60 * 1000, + maxRequests: 10, +}); + export function getOAuthClient(): IVkOAuthClient { if (process.env.USE_VK_MOCK === 'true' || (process.env.NODE_ENV === 'test' && !process.env.VK_APP_ID)) { return new MockVkOAuthClient(); @@ -16,6 +25,9 @@ export function getOAuthClient(): IVkOAuthClient { export async function GET(req: NextRequest) { try { + const clientIp = resolveClientIp(req); + oauthStartRateLimiter.assertAllowed(`oauth-start:${clientIp}`); + const { searchParams } = new URL(req.url); const rawRedirectTarget = searchParams.get('redirectTarget'); const redirectTarget = validateSafeRedirectTarget(rawRedirectTarget); @@ -25,9 +37,8 @@ export async function GET(req: NextRequest) { throw new ValidationError('VK_APP_ID is not configured in server environment'); } - // Determine absolute redirect URI - const origin = req.nextUrl.origin || 'http://localhost:3000'; - const redirectUri = process.env.VK_REDIRECT_URI || `${origin}/api/auth/vk/callback`; + // Resolve canonical redirect URI from configuration (fail-fast in production if missing/non-HTTPS) + const redirectUri = getVkRedirectUri(); // 1. Create secure OAuth transaction with PKCE and State const { state, codeChallenge } = await defaultOAuthTransactionStore.createTransaction({ diff --git a/src/app/api/giveaways/route.ts b/src/app/api/giveaways/route.ts index 05bf4cc..291b42d 100644 --- a/src/app/api/giveaways/route.ts +++ b/src/app/api/giveaways/route.ts @@ -15,18 +15,16 @@ export async function GET(req: NextRequest) { const clientIp = resolveClientIp(req); generalApiRateLimiter.assertAllowed(`giveaways-list:${clientIp}`); - const sessionUser = await getSessionFromRequest(req); - const summaries = await GiveawayStore.listSummaries(); + // 1. Mandatory authentication guard: anonymous listing returns 401 Unauthorized + const sessionUser = await requireAuthenticatedUser(req); - // If organizer is logged in, show their giveaways (or all if requested) - const filteredSummaries = sessionUser - ? summaries.filter(s => !s.organizerId || s.organizerId === sessionUser.id) - : summaries; + // 2. Query scoped strictly by organizerId at repository/database level + const summaries = await GiveawayStore.listSummaries(sessionUser.id); return NextResponse.json({ success: true, - giveaways: filteredSummaries, - totalCount: filteredSummaries.length, + giveaways: summaries, + totalCount: summaries.length, }); } catch (error: any) { return handleApiError(error); diff --git a/src/lib/auth/app-config.ts b/src/lib/auth/app-config.ts new file mode 100644 index 0000000..1a9a5de --- /dev/null +++ b/src/lib/auth/app-config.ts @@ -0,0 +1,55 @@ +/** + * Application environment configuration and trusted origin resolver. + * Enforces strict fail-fast validation in production for OAuth base URLs. + */ + +export function getAppBaseUrl(): string { + const envUrl = process.env.APP_BASE_URL?.trim(); + + if (process.env.NODE_ENV === 'production') { + if (!envUrl) { + throw new Error( + 'FATAL CONFIGURATION ERROR: APP_BASE_URL environment variable is strictly required in production.' + ); + } + if (!envUrl.startsWith('https://')) { + throw new Error( + 'FATAL CONFIGURATION ERROR: APP_BASE_URL must be a valid HTTPS URL in production.' + ); + } + return envUrl.replace(/\/+$/, ''); + } + + // Development/Test environment fallback + return (envUrl || 'http://localhost:3000').replace(/\/+$/, ''); +} + +export function getVkRedirectUri(): string { + const envUri = process.env.VK_REDIRECT_URI?.trim(); + + if (process.env.NODE_ENV === 'production') { + if (!envUri) { + throw new Error( + 'FATAL CONFIGURATION ERROR: VK_REDIRECT_URI environment variable is strictly required in production.' + ); + } + if (!envUri.startsWith('https://')) { + throw new Error( + 'FATAL CONFIGURATION ERROR: VK_REDIRECT_URI must be a valid HTTPS URL in production.' + ); + } + return envUri; + } + + // Development/Test environment fallback + return envUri || `${getAppBaseUrl()}/api/auth/vk/callback`; +} + +export function getTrustedHost(): string { + try { + const url = new URL(getAppBaseUrl()); + return url.host.toLowerCase(); + } catch { + return 'localhost:3000'; + } +} diff --git a/src/lib/auth/csrf-guard.ts b/src/lib/auth/csrf-guard.ts index f1cb170..e084482 100644 --- a/src/lib/auth/csrf-guard.ts +++ b/src/lib/auth/csrf-guard.ts @@ -1,9 +1,11 @@ import { NextRequest } from 'next/server'; import { ForbiddenError } from '@/core/errors/http-errors'; +import { getTrustedHost } from './app-config'; /** * Validates Origin and Referer headers for cookie-authenticated mutating requests (POST/PUT/DELETE/PATCH) * to protect against Cross-Site Request Forgery (CSRF). + * In production, compares strictly against configured APP_BASE_URL host to prevent X-Forwarded-Host spoofing. */ export function validateCsrfOrigin(req: NextRequest): void { // Safe idempotent methods do not modify server state @@ -11,21 +13,25 @@ export function validateCsrfOrigin(req: NextRequest): void { return; } - const origin = req.headers.get('origin'); - const referer = req.headers.get('referer'); - const host = req.headers.get('x-forwarded-host') || req.headers.get('host'); - - // If Sec-Fetch-Site is present, enforce 'same-origin' or 'same-site' + // If Sec-Fetch-Site is present, strictly forbid cross-site const secFetchSite = req.headers.get('sec-fetch-site'); if (secFetchSite && secFetchSite === 'cross-site') { throw new ForbiddenError('Cross-Site Request Forgery (CSRF) detected: cross-site origin rejected'); } + const origin = req.headers.get('origin'); + const referer = req.headers.get('referer'); + + // Resolve trusted host: strictly from configured APP_BASE_URL in production + const trustedHost = process.env.NODE_ENV === 'production' + ? getTrustedHost() + : (req.headers.get('host') || getTrustedHost()); + if (origin) { try { const originUrl = new URL(origin); - if (host && originUrl.host !== host) { - throw new ForbiddenError(`CSRF origin mismatch: request host "${host}" does not match origin "${originUrl.host}"`); + if (originUrl.host.toLowerCase() !== trustedHost.toLowerCase()) { + throw new ForbiddenError(`CSRF origin mismatch: origin "${originUrl.host}" does not match trusted host "${trustedHost}"`); } } catch (e: any) { if (e instanceof ForbiddenError) throw e; @@ -37,8 +43,8 @@ export function validateCsrfOrigin(req: NextRequest): void { if (referer) { try { const refererUrl = new URL(referer); - if (host && refererUrl.host !== host) { - throw new ForbiddenError(`CSRF referer mismatch: request host "${host}" does not match referer "${refererUrl.host}"`); + if (refererUrl.host.toLowerCase() !== trustedHost.toLowerCase()) { + throw new ForbiddenError(`CSRF referer mismatch: referer "${refererUrl.host}" does not match trusted host "${trustedHost}"`); } } catch (e: any) { if (e instanceof ForbiddenError) throw e; @@ -47,7 +53,7 @@ export function validateCsrfOrigin(req: NextRequest): void { return; } - // In test environment, if neither origin nor referer is supplied by test runner, allow if host exists + // In test environment, if neither origin nor referer is supplied by test runner, allow if test environment if (process.env.NODE_ENV === 'test') { return; } diff --git a/src/lib/auth/oauth-state.ts b/src/lib/auth/oauth-state.ts index 2635eeb..a16dda8 100644 --- a/src/lib/auth/oauth-state.ts +++ b/src/lib/auth/oauth-state.ts @@ -7,7 +7,6 @@ export interface OAuthTransaction { redirectTarget: string; createdAt: number; expiresAt: number; - used: boolean; } export interface IOAuthTransactionStore { @@ -16,6 +15,7 @@ export interface IOAuthTransactionStore { ttlMs?: number; }): Promise<{ state: string; codeVerifier: string; codeChallenge: string }>; consumeTransaction(state: string): Promise<{ codeVerifier: string; redirectTarget: string }>; + invalidateTransaction(state: string): Promise; clear(): void; size(): number; cleanupExpired(): number; @@ -49,6 +49,11 @@ export class MemoryOAuthTransactionStore implements IOAuthTransactionStore { private opCounter = 0; constructor(options?: { defaultTtlMs?: number; maxTransactions?: number }) { + if (process.env.MULTI_INSTANCE === 'true') { + throw new Error( + 'FATAL CONFIGURATION ERROR: MemoryOAuthTransactionStore cannot be used when MULTI_INSTANCE=true. Configure a distributed store adapter (Redis/DB).' + ); + } this.defaultTtlMs = options?.defaultTtlMs ?? 10 * 60 * 1000; // 10 minutes this.maxTransactions = options?.maxTransactions ?? 10000; } @@ -75,12 +80,15 @@ export class MemoryOAuthTransactionStore implements IOAuthTransactionStore { redirectTarget: options?.redirectTarget || '/', createdAt: now, expiresAt: now + ttl, - used: false, }); return { state, codeVerifier, codeChallenge }; } + /** + * Atomically retrieves and removes the OAuth transaction in a single operation. + * Guarantees exact-once consumption per state string. + */ public async consumeTransaction(state: string): Promise<{ codeVerifier: string; redirectTarget: string }> { if (!state || typeof state !== 'string') { throw new ValidationError('OAuth state parameter is missing or invalid'); @@ -92,13 +100,9 @@ export class MemoryOAuthTransactionStore implements IOAuthTransactionStore { throw new UnauthorizedError('OAuth state not found or was already consumed (single-use constraint)'); } - // Immediately remove from store to guarantee strict single-use semantics + // Atomic removal from process memory this.store.delete(state); - if (tx.used) { - throw new UnauthorizedError('OAuth state was previously used'); - } - if (Date.now() > tx.expiresAt) { throw new UnauthorizedError('OAuth state has expired'); } @@ -109,11 +113,19 @@ export class MemoryOAuthTransactionStore implements IOAuthTransactionStore { }; } + /** + * Explicitly consumes/deletes a state (e.g. on user cancellation or OAuth error). + */ + public async invalidateTransaction(state: string): Promise { + if (!state || typeof state !== 'string') return false; + return this.store.delete(state); + } + public cleanupExpired(): number { const now = Date.now(); let count = 0; for (const [k, v] of this.store.entries()) { - if (now > v.expiresAt || v.used) { + if (now > v.expiresAt) { this.store.delete(k); count++; } diff --git a/src/lib/giveaway-store.ts b/src/lib/giveaway-store.ts index 0afd587..2bd121a 100644 --- a/src/lib/giveaway-store.ts +++ b/src/lib/giveaway-store.ts @@ -43,12 +43,12 @@ export class GiveawayStore { return await activeRepository.getGiveawayById(id); } - static async listAll(): Promise { - return await activeRepository.listGiveaways(); + static async listAll(organizerId?: string): Promise { + return await activeRepository.listGiveaways(organizerId); } - static async listSummaries(): Promise { - return await activeRepository.listGiveawaysSummary(); + static async listSummaries(organizerId?: string): Promise { + return await activeRepository.listGiveawaysSummary(organizerId); } static async getParticipantsPaginated( diff --git a/src/lib/rate-limiter.ts b/src/lib/rate-limiter.ts index 206f2bc..e0aeb67 100644 --- a/src/lib/rate-limiter.ts +++ b/src/lib/rate-limiter.ts @@ -98,6 +98,10 @@ export class SlidingWindowRateLimiter { this.opCounter = 0; } + public clear(): void { + this.reset(); + } + public size(): number { return this.records.size; } diff --git a/src/lib/repository/giveaway-repository.ts b/src/lib/repository/giveaway-repository.ts index f6e9e97..fe90768 100644 --- a/src/lib/repository/giveaway-repository.ts +++ b/src/lib/repository/giveaway-repository.ts @@ -76,8 +76,8 @@ export interface CreateGiveawayInput { export interface IGiveawayRepository { createGiveaway(input: CreateGiveawayInput): Promise; getGiveawayById(id: string): Promise; - listGiveaways(): Promise; - listGiveawaysSummary(): Promise; + listGiveaways(organizerId?: string): Promise; + listGiveawaysSummary(organizerId?: string): Promise; getParticipantsPaginated( id: string, page: number, diff --git a/src/lib/repository/memory-repository.ts b/src/lib/repository/memory-repository.ts index 560c669..2fe8e20 100644 --- a/src/lib/repository/memory-repository.ts +++ b/src/lib/repository/memory-repository.ts @@ -82,13 +82,16 @@ export class MemoryGiveawayRepository implements IGiveawayRepository { }; } - async listGiveaways(): Promise { - const all = Array.from(this.giveaways.values()); + async listGiveaways(organizerId?: string): Promise { + let all = Array.from(this.giveaways.values()); + if (organizerId) { + all = all.filter(gw => gw.organizerId === organizerId); + } return all.sort((a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime()); } - async listGiveawaysSummary(): Promise { - const all = await this.listGiveaways(); + async listGiveawaysSummary(organizerId?: string): Promise { + const all = await this.listGiveaways(organizerId); return all.map(gw => ({ id: gw.id, platform: gw.platform, diff --git a/src/lib/repository/prisma-repository.ts b/src/lib/repository/prisma-repository.ts index c567cfd..6eff1f5 100644 --- a/src/lib/repository/prisma-repository.ts +++ b/src/lib/repository/prisma-repository.ts @@ -146,8 +146,9 @@ export class PrismaGiveawayRepository implements IGiveawayRepository { return raw ? this.mapPrismaGiveaway(raw) : null; } - async listGiveaways(): Promise { + async listGiveaways(organizerId?: string): Promise { const list = await prisma.giveaway.findMany({ + where: organizerId ? { organizerId } : undefined, orderBy: { createdAt: 'desc' }, include: { participants: true, @@ -163,8 +164,9 @@ export class PrismaGiveawayRepository implements IGiveawayRepository { return list.map(item => this.mapPrismaGiveaway(item)); } - async listGiveawaysSummary(): Promise { + async listGiveawaysSummary(organizerId?: string): Promise { const list = await prisma.giveaway.findMany({ + where: organizerId ? { organizerId } : undefined, orderBy: { createdAt: 'desc' }, select: { id: true, diff --git a/tests/giveaway-listing-idor.test.ts b/tests/giveaway-listing-idor.test.ts new file mode 100644 index 0000000..8a94e11 --- /dev/null +++ b/tests/giveaway-listing-idor.test.ts @@ -0,0 +1,142 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { NextRequest } from 'next/server'; +import { GiveawayStore } from '../src/lib/giveaway-store'; +import { MemoryGiveawayRepository } from '../src/lib/repository/memory-repository'; +import { GET as giveawaysGet } from '../src/app/api/giveaways/route'; +import { defaultSessionStore, SESSION_COOKIE_NAME } from '../src/lib/auth/session'; +import { DEFAULT_FILTER_RULES } from '../src/core/types/giveaway'; + +describe('Phase 2.2.3 Claude PoC: GET /api/giveaways IDOR & Scoped Query Gate', () => { + let memoryRepo: MemoryGiveawayRepository; + + const userA = { id: 'usr_organizer_alpha', vkUserId: '11111', firstName: 'Alice' }; + const userB = { id: 'usr_organizer_beta', vkUserId: '22222', firstName: 'Bob' }; + const userEmpty = { id: 'usr_organizer_empty', vkUserId: '33333', firstName: 'Charlie' }; + + let sessionA: string; + let sessionB: string; + let sessionEmpty: string; + + beforeEach(async () => { + memoryRepo = new MemoryGiveawayRepository(); + GiveawayStore.setRepository(memoryRepo); + + defaultSessionStore.clear(); + sessionA = await defaultSessionStore.createSession(userA); + sessionB = await defaultSessionStore.createSession(userB); + sessionEmpty = await defaultSessionStore.createSession(userEmpty); + + // Populate giveaways for User A + await GiveawayStore.create({ + sourceUrl: 'https://vk.com/wall-10_100', + post: { + platform: 'VK', + ownerId: '-10', + postId: '100', + sourceUrl: 'https://vk.com/wall-10_100', + title: 'Secret Giveaway of Alice', + likesCount: 10, + commentsCount: 2, + repostsCount: 0, + }, + filterRules: DEFAULT_FILTER_RULES, + organizerId: userA.id, + }); + + // Populate giveaways for User B + await GiveawayStore.create({ + sourceUrl: 'https://vk.com/wall-20_200', + post: { + platform: 'VK', + ownerId: '-20', + postId: '200', + sourceUrl: 'https://vk.com/wall-20_200', + title: 'Confidential Giveaway of Bob', + likesCount: 50, + commentsCount: 15, + repostsCount: 5, + }, + filterRules: DEFAULT_FILTER_RULES, + organizerId: userB.id, + }); + }); + + it('Claude PoC Reproduction: anonymous GET /api/giveaways is rejected with 401 Unauthorized', async () => { + const req = new NextRequest('http://localhost:3000/api/giveaways'); + const res = await giveawaysGet(req); + + expect(res.status).toBe(401); + const body = await res.json(); + expect(body.error?.message).toMatch(/authentication required/i); + // Crucial: No giveaways leaked to anonymous attacker + expect(body.giveaways).toBeUndefined(); + }); + + it('User A list returns strictly User A giveaways (no User B records leaked)', async () => { + const req = new NextRequest('http://localhost:3000/api/giveaways', { + headers: { cookie: `${SESSION_COOKIE_NAME}=${sessionA}` }, + }); + const res = await giveawaysGet(req); + + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.success).toBe(true); + expect(body.giveaways).toHaveLength(1); + + const gw = body.giveaways[0]; + expect(gw.organizerId).toBe(userA.id); + expect(gw.title).toBe('Secret Giveaway of Alice'); + + // Ensure User B information is completely absent + expect(JSON.stringify(body)).not.toContain('Bob'); + expect(JSON.stringify(body)).not.toContain('Confidential Giveaway of Bob'); + expect(JSON.stringify(body)).not.toContain('wall-20_200'); + }); + + it('User B list returns strictly User B giveaways (no User A records leaked)', async () => { + const req = new NextRequest('http://localhost:3000/api/giveaways', { + headers: { cookie: `${SESSION_COOKIE_NAME}=${sessionB}` }, + }); + const res = await giveawaysGet(req); + + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.success).toBe(true); + expect(body.giveaways).toHaveLength(1); + + const gw = body.giveaways[0]; + expect(gw.organizerId).toBe(userB.id); + expect(gw.title).toBe('Confidential Giveaway of Bob'); + + // Ensure User A information is completely absent + expect(JSON.stringify(body)).not.toContain('Alice'); + expect(JSON.stringify(body)).not.toContain('Secret Giveaway of Alice'); + expect(JSON.stringify(body)).not.toContain('wall-10_100'); + }); + + it('empty account receives empty array [] without errors', async () => { + const req = new NextRequest('http://localhost:3000/api/giveaways', { + headers: { cookie: `${SESSION_COOKIE_NAME}=${sessionEmpty}` }, + }); + const res = await giveawaysGet(req); + + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.success).toBe(true); + expect(body.giveaways).toEqual([]); + expect(body.totalCount).toBe(0); + }); + + it('Repository level test: listGiveawaysSummary filters directly by organizerId', async () => { + const summariesA = await memoryRepo.listGiveawaysSummary(userA.id); + expect(summariesA).toHaveLength(1); + expect(summariesA[0].organizerId).toBe(userA.id); + + const summariesB = await memoryRepo.listGiveawaysSummary(userB.id); + expect(summariesB).toHaveLength(1); + expect(summariesB[0].organizerId).toBe(userB.id); + + const summariesEmpty = await memoryRepo.listGiveawaysSummary(userEmpty.id); + expect(summariesEmpty).toHaveLength(0); + }); +}); diff --git a/tests/oauth-concurrency.test.ts b/tests/oauth-concurrency.test.ts new file mode 100644 index 0000000..6eb01bf --- /dev/null +++ b/tests/oauth-concurrency.test.ts @@ -0,0 +1,70 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { MemoryOAuthTransactionStore } from '../src/lib/auth/oauth-state'; + +describe('Phase 2.2.3 OAuth Atomic Consume & 100-Concurrent Race Gate', () => { + let store: MemoryOAuthTransactionStore; + + beforeEach(() => { + store = new MemoryOAuthTransactionStore(); + }); + + it('100 concurrent consumeTransaction attempts on the same state result in exactly 1 success and 99 failures', async () => { + // 1. Create a single valid transaction + const { state, codeVerifier } = await store.createTransaction({ + redirectTarget: '/giveaways/new', + ttlMs: 5 * 60 * 1000, + }); + + // 2. Launch 100 concurrent consume requests + const attempts = Array.from({ length: 100 }, async (_, index) => { + try { + const result = await store.consumeTransaction(state); + return { success: true, verifier: result.codeVerifier, index }; + } catch (err: any) { + return { success: false, error: err.message, index }; + } + }); + + const results = await Promise.all(attempts); + + const successful = results.filter(r => r.success); + const failed = results.filter(r => !r.success); + + // Invariant: Exactly 1 consume succeeds + expect(successful).toHaveLength(1); + expect(failed).toHaveLength(99); + + // Invariant: The winning consume got the exact codeVerifier + expect(successful[0].verifier).toBe(codeVerifier); + + // Invariant: Subsequent sequential consume also fails + await expect(store.consumeTransaction(state)).rejects.toThrow(/already consumed/i); + }); + + it('reused state is immediately rejected with unauthorized error', async () => { + const { state } = await store.createTransaction(); + + // First consume succeeds + const first = await store.consumeTransaction(state); + expect(first.codeVerifier).toBeDefined(); + + // Second consume fails + await expect(store.consumeTransaction(state)).rejects.toThrow(/already consumed/i); + }); + + it('expired state cannot be consumed', async () => { + const { state } = await store.createTransaction({ ttlMs: -1000 }); // Expired in the past + + await expect(store.consumeTransaction(state)).rejects.toThrow(/expired/i); + }); + + it('invalidateTransaction deletes state cleanly on error or cancellation', async () => { + const { state } = await store.createTransaction(); + + const deleted = await store.invalidateTransaction(state); + expect(deleted).toBe(true); + + // State can no longer be consumed + await expect(store.consumeTransaction(state)).rejects.toThrow(/not found/i); + }); +}); diff --git a/tests/origin-and-csrf-gate.test.ts b/tests/origin-and-csrf-gate.test.ts new file mode 100644 index 0000000..e59942a --- /dev/null +++ b/tests/origin-and-csrf-gate.test.ts @@ -0,0 +1,120 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { NextRequest } from 'next/server'; +import { validateCsrfOrigin } from '../src/lib/auth/csrf-guard'; +import { getAppBaseUrl, getVkRedirectUri } from '../src/lib/auth/app-config'; +import { GET as vkStartGet, oauthStartRateLimiter } from '../src/app/api/auth/vk/start/route'; + +describe('Phase 2.2.3 Origin, CSRF Trusted Host & Rate Limiting Gate', () => { + const originalEnv = process.env; + + beforeEach(() => { + process.env = { ...originalEnv }; + }); + + afterEach(() => { + process.env = originalEnv; + }); + + describe('Production Base URL & VK_REDIRECT_URI Fail-Fast Policy', () => { + it('fails fast in production when APP_BASE_URL is missing', () => { + process.env.NODE_ENV = 'production'; + delete process.env.APP_BASE_URL; + + expect(() => getAppBaseUrl()).toThrow(/APP_BASE_URL environment variable is strictly required in production/i); + }); + + it('fails fast in production when APP_BASE_URL is not HTTPS', () => { + process.env.NODE_ENV = 'production'; + process.env.APP_BASE_URL = 'http://insecure-http-url.com'; + + expect(() => getAppBaseUrl()).toThrow(/must be a valid HTTPS URL in production/i); + }); + + it('fails fast in production when VK_REDIRECT_URI is missing', () => { + process.env.NODE_ENV = 'production'; + process.env.APP_BASE_URL = 'https://randomayzer.org'; + delete process.env.VK_REDIRECT_URI; + + expect(() => getVkRedirectUri()).toThrow(/VK_REDIRECT_URI environment variable is strictly required in production/i); + }); + + it('accepts valid HTTPS configuration in production', () => { + process.env.NODE_ENV = 'production'; + process.env.APP_BASE_URL = 'https://randomayzer.org'; + process.env.VK_REDIRECT_URI = 'https://randomayzer.org/api/auth/vk/callback'; + + expect(getAppBaseUrl()).toBe('https://randomayzer.org'); + expect(getVkRedirectUri()).toBe('https://randomayzer.org/api/auth/vk/callback'); + }); + }); + + describe('CSRF Trusted Host & Host-Spoofing Immunity', () => { + it('rejects attacker sending evil Origin even if attacker injects spoofed X-Forwarded-Host', () => { + process.env.NODE_ENV = 'production'; + process.env.APP_BASE_URL = 'https://trusted-randomayzer.org'; + + const req = new NextRequest('http://localhost/api/auth/logout', { + method: 'POST', + headers: { + origin: 'https://evil.com', + 'x-forwarded-host': 'evil.com', // Spoofed header + }, + }); + + expect(() => validateCsrfOrigin(req)).toThrow(/CSRF origin mismatch/i); + }); + + it('rejects cross-site Sec-Fetch-Site requests', () => { + const req = new NextRequest('http://localhost/api/auth/logout', { + method: 'POST', + headers: { + origin: 'https://trusted-randomayzer.org', + 'sec-fetch-site': 'cross-site', + }, + }); + + expect(() => validateCsrfOrigin(req)).toThrow(/cross-site origin rejected/i); + }); + + it('accepts valid origin matching configured trusted host', () => { + process.env.NODE_ENV = 'production'; + process.env.APP_BASE_URL = 'https://trusted-randomayzer.org'; + + const req = new NextRequest('https://trusted-randomayzer.org/api/auth/logout', { + method: 'POST', + headers: { + origin: 'https://trusted-randomayzer.org', + 'sec-fetch-site': 'same-origin', + }, + }); + + expect(() => validateCsrfOrigin(req)).not.toThrow(); + }); + }); + + describe('OAuth Start Rate Limiter', () => { + it('rate limits burst requests to GET /api/auth/vk/start', async () => { + oauthStartRateLimiter.clear(); + + const ip = '198.51.100.42'; + + // 10 allowed requests + for (let i = 0; i < 10; i++) { + const req = new NextRequest('http://localhost:3000/api/auth/vk/start', { + headers: { 'x-forwarded-for': ip }, + }); + const res = await vkStartGet(req); + expect(res.status).toBe(307); // Temporary redirect to VK + } + + // 11th request in the same window -> 429 Too Many Requests + const blockedReq = new NextRequest('http://localhost:3000/api/auth/vk/start', { + headers: { 'x-forwarded-for': ip }, + }); + const blockedRes = await vkStartGet(blockedReq); + expect(blockedRes.status).toBe(429); + const body = await blockedRes.json(); + expect(body.error?.message).toMatch(/rate limit exceeded/i); + }); + }); +});