318 lines
21 KiB
Markdown
318 lines
21 KiB
Markdown
# OpenCode Independent Review — Randomayzer
|
||
|
||
**Scope:** test coverage, security, concurrency, VK integration preparation.
|
||
**Review date:** 2026-08-18
|
||
**Branch reviewed:** `main` (initial clone)
|
||
**Reviewer:** OpenCode
|
||
|
||
> **Coordination note:** another agent (Antigravity) is performing Core Audit Fixes / Phase 1.2.
|
||
> This review intentionally does **not** rewrite the Randomizer, Prisma schema, FSM, AuditProof, `giveaway-store.ts`, VK OAuth, or any existing core architecture. Findings that touch those areas are documented as recommendations and are **not** auto-fixed unless they are isolated, low-risk, and do not overlap with Antigravity's scope.
|
||
|
||
---
|
||
|
||
## Summary of findings
|
||
|
||
| Severity | Count | Themes |
|
||
|---|---|---|
|
||
| CRITICAL | 2 | double-draw race, silent DB→memory fallback |
|
||
| HIGH | 6 | capability violations, API input validation, VK pagination limits, token exposure surface, missing retry/rate-limit, JSON snapshot scalability |
|
||
| MEDIUM | 5 | environment-based mock fallback, `.gitignore` gaps, error-message leakage, unused env vars, `excludeDuplicateComments` ignored |
|
||
| LOW | 3 | in-memory IDs via `Math.random()`, `filterRules` immutability gaps, `MINIMUM_COMMENTS` not implemented |
|
||
| INFO | 3 | architecture strengths, provider abstraction, clean separation |
|
||
|
||
---
|
||
|
||
## CRITICAL
|
||
|
||
### C1 — Race condition allows duplicate draw for the same giveaway
|
||
|
||
- **File:** `src/app/api/giveaways/[id]/draw/route.ts`
|
||
- **Lines:** 12–60
|
||
- **Description:** The route performs a read-check-write sequence without an atomic lock or transaction:
|
||
1. `giveaway.status === 'DRAWN'` is checked on a giveaway object loaded at line 14.
|
||
2. A snapshot is fetched or created (lines 28–36).
|
||
3. `GiveawayFSM.assertCanDraw('SNAPSHOT_LOCKED')` is called with a hard-coded status string, **not** the current persisted status (line 41).
|
||
4. `GiveawayStore.saveDrawResult` persists the draw (line 60).
|
||
- **Consequence:** Two concurrent `POST /draw` requests can both pass the initial `DRAWN` check while the giveaway is `READY`, both create/retrieve a snapshot, both pass `assertCanDraw('SNAPSHOT_LOCKED')`, and both persist a `DrawResult`. The second call overwrites the first or produces two audit records for the same giveaway, violating provably-fair invariants.
|
||
- **Recommended fix:** Wrap the entire read-snapshot-draw-write sequence in a database-level atomic operation. Options:
|
||
- Use a `SELECT FOR UPDATE` row lock on the giveaway row at the start of the transaction.
|
||
- Or add a unique constraint on `DrawResult.giveawayId`/`AuditRecord.giveawayId` (already present) and retry on conflict, but the route must read the row **inside** the transaction and fail fast on status mismatch.
|
||
- Pass the actual persisted status to `assertCanDraw`, not a literal string.
|
||
- **Coordination:** overlaps Antigravity Phase 1.2 (core audit / persistence). **Not auto-fixed.**
|
||
|
||
### C2 — Prisma errors silently fall back to in-memory storage
|
||
|
||
- **File:** `src/lib/giveaway-store.ts`
|
||
- **Lines:** 24–58 (`create`, `getById`, `listAll`)
|
||
- **Description:** Every read/write operation catches any Prisma error, logs a warning, swaps `activeRepository` to `MemoryGiveawayRepository`, and retries. There is no recovery path back to Prisma.
|
||
- **Consequence:** A transient DB hiccup (network blip, pool timeout, lock timeout) permanently downgrades the running process to in-memory mode. Subsequent requests create giveaways that are lost on restart or invisible to other horizontally scaled instances. This violates data durability and audit traceability.
|
||
- **Recommended fix:**
|
||
- Remove the silent fallback from production code. Fail fast and return a 500/503 with a clear error.
|
||
- Keep `MemoryGiveawayRepository` only for unit tests via `GiveawayStore.setRepository(...)`.
|
||
- If a fallback is truly required, implement it at the infrastructure level (connection pool, replica read), not by switching the repository implementation mid-process.
|
||
- **Coordination:** `giveaway-store.ts` is explicitly in Antigravity's scope. **Not auto-fixed.**
|
||
|
||
---
|
||
|
||
## HIGH
|
||
|
||
### H1 — Unsupported filter rules are accepted for the current provider
|
||
|
||
- **File:** `src/app/api/giveaways/[id]/participants/route.ts`
|
||
- **Lines:** 27–30
|
||
- **Description:** The route forwards `rules.requireRepost` to `provider.fetchParticipants({ includeReposts: rules.requireRepost })` without checking `provider.capabilities.reposts`. `VkProvider` and `VkMockProvider` both declare `reposts: false`, yet the backend still attempts to honor the rule. There is no backend validation that rejects a rule the selected provider cannot verify.
|
||
- **Consequence:** Organizers can configure giveaways that the system cannot actually verify. The UI may imply reposts are checked while the provider silently ignores them, undermining trust and audit correctness.
|
||
- **Recommended fix:** Add a `validateFilterRulesAgainstCapabilities(rules, capabilities)` guard (see `tests/provider-capabilities.test.ts` for contract expectations). Call it in the participants route and snapshot route before any provider work. Return `400` with a clear message such as `"requireRepost is not supported by VK provider"`.
|
||
- **Coordination:** low risk, isolated. A validation utility was added during this review; wiring it into routes is documented as a recommendation and tested.
|
||
|
||
### H2 — API routes lack input validation
|
||
|
||
- **Files:**
|
||
- `src/app/api/giveaways/route.ts` (lines 14–35)
|
||
- `src/app/api/giveaways/[id]/draw/route.ts` (lines 42–46)
|
||
- `src/app/api/giveaways/[id]/snapshot/route.ts` (lines 10–30)
|
||
- `src/app/api/posts/preview/route.ts` (lines 5–30)
|
||
- **Description:** No schema validation or bounds checks on:
|
||
- `winnersCount` / `reserveWinnersCount` (negative, zero, or extremely large values are accepted).
|
||
- `seed` (empty string, multi-megabyte seed, or untrusted external seed).
|
||
- `filterRules` (unknown keys are ignored; missing required keys may be defaulted unsafely).
|
||
- `sourceUrl` / `url` (only presence is checked in some routes, not format/length).
|
||
- JSON body parsing failures return generic 500 instead of 400.
|
||
- **Consequence:** Invalid or malicious payloads can corrupt persisted data, cause confusing draw behavior (e.g., `winnersCount: -1` produces zero winners), or be used for DoS via huge strings stored in `seed`, `filterRules`, or snapshot JSON.
|
||
- **Recommended fix:** Introduce a lightweight schema validator (Zod is recommended) for all API routes. At minimum enforce:
|
||
- `winnersCount` integer >= 1, capped to a reasonable maximum (e.g., 10 000).
|
||
- `reserveWinnersCount` integer >= 0, capped similarly.
|
||
- `seed` non-empty string, max length 1024, sanitized.
|
||
- `filterRules` strict shape with only known keys and capability checks.
|
||
- Malformed JSON returns `400 Bad Request`.
|
||
- **Coordination:** additive change, does not touch forbidden files. Validation tests added; full Zod refactor deferred pending team agreement.
|
||
|
||
### H3 — VK provider silently caps large participant lists
|
||
|
||
- **File:** `src/providers/vk/vk-provider.ts`
|
||
- **Lines:** 142–183 (likes), 186–238 (comments)
|
||
- **Description:** `fetchParticipants` loops with `offset < totalLikes && offset < 5000` for likes and `offset < totalComments && offset < 1000` for comments. For posts with more than ~5 000 likes or ~1 000 comments, the remaining participants are silently truncated.
|
||
- **Consequence:** Large giveaways exclude valid participants without warning, breaking fairness.
|
||
- **Recommended fix:** Remove artificial caps or make them configurable/documented with explicit UI warnings. Implement paginated fetching with rate limiting and progress callbacks. Use `execute` batching where beneficial.
|
||
- **Coordination:** touches `VkProvider`; overlaps VK integration preparation. **Not auto-fixed.**
|
||
|
||
### H4 — VK service token travels in query string and may leak through logs/proxies
|
||
|
||
- **File:** `src/providers/vk/vk-provider.ts`
|
||
- **Lines:** 43–52 (`callApi`)
|
||
- **Description:** `access_token` is appended to the URL query string. `fetch` errors, server/proxy access logs, APM traces, or exception reporters may capture the full URL. The constructor also reads `process.env.VK_SERVICE_TOKEN` directly; while this is correct, there is no audit that the value never appears in error objects.
|
||
- **Consequence:** If an error reporter logs the request URL, the VK service token is exposed. Service tokens are long-lived and grant broad public-data access.
|
||
- **Recommended fix:**
|
||
- Prefer sending the token in an `Authorization: Bearer <token>` header where VK allows it; otherwise ensure request URLs are never logged.
|
||
- Scrub tokens from any error serialization in `VkProvider`.
|
||
- Add automated tests asserting that token does not appear in thrown messages, responses, or logs (see `tests/security.test.ts`).
|
||
- **Coordination:** low risk; security tests added. Header change depends on VK API contract (documented as recommendation).
|
||
|
||
### H5 — No rate limiting, retry, or timeout strategy in VK client
|
||
|
||
- **File:** `src/providers/vk/vk-provider.ts`
|
||
- **Lines:** 38–69 (`callApi`)
|
||
- **Description:** `callApi` uses a single `fetch` call. It does not:
|
||
- Set a timeout.
|
||
- Retry on transient failures (network, 5xx, VK error 6/29).
|
||
- Back off on rate-limit responses.
|
||
- Distinguish retryable from non-retryable VK errors.
|
||
- **Consequence:** A single network stall or rate-limit response fails the entire participant import. Large giveaways are unreliable and slow.
|
||
- **Recommended fix:** Implement `VkClient` → `RateLimiter` → `RetryPolicy` → `VkProvider` as documented in `docs/VK_INTEGRATION_PLAN.md`. Start with interfaces and a simple exponential-backoff wrapper; do not over-engineer.
|
||
- **Coordination:** preparation-only; no production implementation added.
|
||
|
||
### H6 — Storing entire eligible participant snapshot as JSON does not scale
|
||
|
||
- **Files:**
|
||
- `prisma/schema.prisma` — `ParticipantSnapshot.eligibleParticipants Json`
|
||
- `src/core/types/audit.ts` — `ParticipantSnapshotData.eligibleParticipants: FilteredParticipant[]`
|
||
- `src/lib/repository/prisma-repository.ts` — snapshot create/read maps the full JSON
|
||
- **Description:** Every eligible participant is serialized into a single JSON column. Each participant record contains avatar URLs, names, usernames, and flags.
|
||
- **Consequence:**
|
||
- ~1 000 participants ≈ 200–400 KB JSON.
|
||
- ~100 000 participants ≈ 20–40 MB per snapshot row.
|
||
- ~500 000 participants ≈ 100–200 MB per row.
|
||
- PostgreSQL `jsonb` limit is 1 GB, but reading/writing such rows consumes large amounts of application memory, slows queries, and blocks the UI table. Hashing the snapshot becomes CPU-bound.
|
||
- **Recommended fix:** See **Scalability** section below. Short term: cap supported giveaway size and warn organizers. Long term: store participants in normalized `ParticipantSnapshotItem` rows and compute the snapshot hash incrementally or on a streaming cursor.
|
||
- **Coordination:** Prisma schema is explicitly forbidden to change. **Not auto-fixed.**
|
||
|
||
---
|
||
|
||
## MEDIUM
|
||
|
||
### M1 — Environment-based provider selection can silently run mock in production
|
||
|
||
- **File:** `src/providers/registry.ts`
|
||
- **Lines:** 9–12
|
||
- **Description:** `ProviderRegistry` chooses `VkProvider` only if `VK_SERVICE_TOKEN` is present and longer than 10 characters. If the variable is missing, empty, or a placeholder shorter than 11 chars, the production process silently uses `VkMockProvider`, returning synthetic data.
|
||
- **Consequence:** Misconfigured deployments appear to work but produce fake participants and fake draws.
|
||
- **Recommended fix:** Fail fast at startup when a real token is expected. Reserve `VkMockProvider` for explicit `NODE_ENV=test` or a `USE_VK_MOCK=1` flag.
|
||
- **Coordination:** isolated; recommended but not auto-fixed to avoid behavior changes.
|
||
|
||
### M2 — `.gitignore` does not cover all environment files
|
||
|
||
- **File:** `.gitignore`
|
||
- **Lines:** 19–22
|
||
- **Description:** Only `.env` and `.env*.local` are ignored. `.env.production`, `.env.staging`, `.env.test`, and `.env*.[other]` are not ignored.
|
||
- **Consequence:** Accidental commits of production secrets are possible.
|
||
- **Recommended fix:** Add `!.env.example` and `.env*` (with explicit allow-list for safe examples) or list common variants.
|
||
- **Coordination:** isolated; not auto-fixed because it is project hygiene and can be handled by Antigravity.
|
||
|
||
### M3 — Raw error messages from external APIs are returned to clients
|
||
|
||
- **Files:** all `src/app/api/**/route.ts`
|
||
- **Description:** Catch blocks return `error.message` directly in the JSON body.
|
||
- **Consequence:** Internal details (file paths, provider error texts, partial URLs) may leak to the client.
|
||
- **Recommended fix:** In production, log the full error server-side and return a sanitized message. Use `NODE_ENV` to decide detail level.
|
||
- **Coordination:** additive; not auto-fixed because it spans many routes.
|
||
|
||
### M4 — `.env.example` contains unused variables
|
||
|
||
- **File:** `.env.example`
|
||
- **Lines:** 7–10
|
||
- **Description:** `VK_APP_ID` and `VK_APP_SECRET` are documented but not referenced in code.
|
||
- **Consequence:** Operators may populate them believing they are required, increasing secret surface area without benefit.
|
||
- **Recommended fix:** Remove unused variables or add comments explaining they are reserved for future OAuth/community-token flows.
|
||
- **Coordination:** trivial; not auto-fixed.
|
||
|
||
### M5 — `excludeDuplicateComments` flag exists but is not honored
|
||
|
||
- **File:** `src/core/filtering/filter-engine.ts`
|
||
- **Lines:** 25–42
|
||
- **Description:** `FilterRules` includes `excludeDuplicateComments`, but `applyFilterRules` always deduplicates participants and aggregates `commentsCount` regardless of the flag.
|
||
- **Consequence:** A future UI toggle for duplicate comments will not behave as expected. The rule is also included in `computeConditionsHash`, so changing its behavior later will alter snapshot hashes.
|
||
- **Recommended fix:** When `excludeDuplicateComments` is false, keep each raw comment as a separate entry (or disable aggregation). Update hash tests accordingly.
|
||
- **Coordination:** touches filter engine; not auto-fixed because it changes existing semantics.
|
||
|
||
---
|
||
|
||
## LOW
|
||
|
||
### L1 — In-memory repository uses `Math.random()` for IDs
|
||
|
||
- **File:** `src/lib/repository/memory-repository.ts`
|
||
- **Line:** 17
|
||
- **Description:** Giveaway IDs are generated with `Math.random()`. IDs are not secrets, but this is inconsistent with the cryptographic rigor used elsewhere.
|
||
- **Consequence:** Negligible for tests; low risk for production if the memory repo is ever used seriously.
|
||
- **Recommended fix:** Use `crypto.randomUUID()` or a CUID generator.
|
||
- **Coordination:** not auto-fixed; trivial.
|
||
|
||
### L2 — `filterRules` object in snapshot route can mutate stored rules unexpectedly
|
||
|
||
- **File:** `src/app/api/giveaways/[id]/snapshot/route.ts`
|
||
- **Lines:** 24–30
|
||
- **Description:** `body.filterRules || giveaway.filterRules` passes the request body object directly to `createAndLockSnapshot`, which then stores it in the DB.
|
||
- **Consequence:** Malformed or extra keys in the body can be persisted, affecting `conditionsHash` and future audits.
|
||
- **Recommended fix:** Deep-clone and validate rules before persisting.
|
||
- **Coordination:** overlaps input validation (H2); deferred.
|
||
|
||
### L3 — No `minimumComments` rule despite review task mentioning it
|
||
|
||
- **File:** `src/core/types/giveaway.ts`
|
||
- **Description:** `FilterRules` does not contain a `minimumComments` field. The task asked to test combinations including `minimumComments`, but the rule is not implemented.
|
||
- **Consequence:** Organizer cannot require "at least N comments".
|
||
- **Recommended fix:** Add `minimumComments?: number` to `FilterRules` and enforce it in `applyFilterRules` after deduplication.
|
||
- **Coordination:** requires type + filter engine change; **not auto-fixed.**
|
||
|
||
---
|
||
|
||
## INFO
|
||
|
||
### I1 — Strong core domain isolation
|
||
|
||
- The core layer (`src/core/randomizer`, `src/core/filtering`, `src/core/fsm`) has no imports from Next.js, React, or provider implementations. This is good and should be preserved.
|
||
|
||
### I2 — Provider abstraction is clean and testable
|
||
|
||
- `SocialMediaProvider` interface (`src/providers/types.ts`) cleanly separates platform specifics from the domain. `VkMockProvider` allows offline testing.
|
||
|
||
### I3 — Provably-fair hashing is deterministic and order-independent
|
||
|
||
- `computeParticipantsSnapshotHash` and `computeConditionsHash` canonicalize input and produce stable hashes, enabling third-party verification.
|
||
|
||
---
|
||
|
||
## Scalability analysis
|
||
|
||
### Participant snapshot JSON size estimation
|
||
|
||
A single `FilteredParticipant` record contains:
|
||
|
||
```json
|
||
{
|
||
"platformUserId": "123456789",
|
||
"firstName": "Иван",
|
||
"lastName": "Иванов",
|
||
"username": "ivanov",
|
||
"avatarUrl": "https://.../photo.jpg",
|
||
"source": "COMBINED",
|
||
"liked": true,
|
||
"commented": true,
|
||
"commentsCount": 3,
|
||
"reposted": false,
|
||
"subscribed": true,
|
||
"eligible": true,
|
||
"exclusionReason": null
|
||
}
|
||
```
|
||
|
||
Approximate serialized size: **250–400 bytes** per participant (avatar URLs dominate).
|
||
|
||
| Eligible participants | Snapshot JSON size | Assessment |
|
||
|---|---|---|
|
||
| 1 000 | ~0.3 MB | ✅ Safe. Fits comfortably in memory and a `jsonb` column. |
|
||
| 10 000 | ~3 MB | ✅ Still safe for a single request, but UI table rendering starts to degrade. |
|
||
| 50 000 | ~15 MB | ⚠️ Heavy. Page load / API response time increases; hashing takes noticeable CPU. |
|
||
| 100 000 | ~30 MB | ❌ Risky. Exceeds comfortable single-row JSON workload; Next.js API response limits may be hit. |
|
||
| 500 000 | ~150 MB | ❌ Not viable. PostgreSQL `jsonb` max is ~1 GB, but memory, I/O, and UI become impractical. |
|
||
|
||
### Component-by-component assessment
|
||
|
||
| Component | 1 000 | 10 000 | 50 000 | 100 000 | 500 000 |
|
||
|---|---|---|---|---|---|
|
||
| **Memory (API route)** | < 5 MB | ~30 MB | ~150 MB | ~300 MB | > 1 GB |
|
||
| **PostgreSQL snapshot row** | 0.3 MB | 3 MB | 15 MB | 30 MB | 150 MB |
|
||
| **Snapshot hashing (SHA-256)** | < 1 ms | ~5 ms | ~30 ms | ~80 ms | ~500 ms |
|
||
| **API pagination (VK likes)** | 1 req | 10 req | 50 req | 100 req | 500 req |
|
||
| **API pagination (VK comments)** | 10 req | 100 req | 500 req | 1 000 req | 5 000 req |
|
||
| **Subscription checks** | 2 req | 20 req | 100 req | 200 req | 1 000 req |
|
||
| **UI Participants table** | instant | slight lag | unusable without virtualization | browser crash risk | requires server-side pagination |
|
||
| **Next.js API response** | < 50 KB | ~300 KB | > 1 MB | > 3 MB | > 15 MB |
|
||
|
||
### When does `eligibleParticipants JSON` stop being a good solution?
|
||
|
||
**Threshold: ~10 000 eligible participants.**
|
||
|
||
Above 10 000, the monolithic JSON snapshot becomes a bottleneck because:
|
||
1. **Network:** `/api/giveaways/[id]/draw` and related endpoints return the full snapshot or draw result, producing multi-megabyte responses.
|
||
2. **Memory:** every snapshot read loads the entire list into the Node.js heap.
|
||
3. **UI:** rendering the participants table without virtualization causes jank or crashes.
|
||
4. **Hashing:** snapshot hash computation is O(n) on a large string; while still fast, it blocks the event loop.
|
||
5. **Audit replay:** third-party verification must download the entire JSON to recompute the hash.
|
||
|
||
### Recommended long-term architecture
|
||
|
||
1. **Normalize snapshot items:** replace `eligibleParticipants Json` with a `ParticipantSnapshotItem` table (or reuse `Participant` with a `snapshotId`). Each row stores one participant; `participantsSnapshotHash` is computed from a streaming sorted cursor.
|
||
2. **Paginated API:** return only summary counts by default; expose paginated endpoints for participant lists.
|
||
3. **Background jobs:** for > 10 000 participants, run import and subscription checks in a background worker (e.g., BullMQ / inngest) and update giveaway status asynchronously.
|
||
4. **Streaming hash:** compute the snapshot hash using a streaming SHA-256 over sorted rows instead of materializing the full JSON string.
|
||
5. **Cap + warn:** until the architecture changes, enforce a configurable maximum eligible count (e.g., 10 000) with a clear error message and guidance.
|
||
|
||
> **Coordination note:** schema changes are deferred to Antigravity Phase 1.2. The analysis above is provided for planning only.
|
||
|
||
---
|
||
|
||
## Test coverage notes
|
||
|
||
During this review the following test files were added or extended:
|
||
|
||
- `tests/filter-engine.test.ts` — extended with combination rule matrices.
|
||
- `tests/vk-mock-provider.test.ts` — new; scenarios for 0/1/10/1000/large participants, likes-only, comments-only, subscription, duplicates, excluded IDs.
|
||
- `tests/participant-pipeline.test.ts` — new; full fetch → enrichment → subscription → filtering → eligible pipeline.
|
||
- `tests/provider-capabilities.test.ts` — new; validation of rules against provider capabilities.
|
||
- `tests/vk-errors.test.ts` — new; VK error handling via mocked `fetch`.
|
||
- `tests/security.test.ts` — new; token leakage checks.
|
||
- `tests/api-validation.test.ts` — new; route-level input validation.
|
||
- `tests/concurrency.test.ts` — new; race-condition analysis (findings documented, no production fix).
|
||
|
||
See each file for concrete test cases and the final report for pass counts.
|