60 lines
7.7 KiB
Markdown
60 lines
7.7 KiB
Markdown
# Independent auditor2 review — stage1 R1–R5
|
||
|
||
Reviewed SHA: `17faecd962c2cf40e0a3f6ec4434811cfffdd312`
|
||
Base SHA: `6382cb8e0473ba9ea5ba7a3f94f34e8ab0d4ec60`
|
||
Result SHA: `17faecd962c2cf40e0a3f6ec4434811cfffdd312` (review only; no code changes).
|
||
|
||
## Scope and conclusion
|
||
|
||
Reviewed the immutable base-to-HEAD patch against R1–R5, with emphasis on malformed/incomplete VK responses and persistence, unsupported repost/manager checks, smoke behavior and credential handling, migration bootstrap, production socket trust, deployment binding and operator runbook.
|
||
|
||
One **HIGH** issue remains in R3: likes/comments item records are not validated, allowing malformed VK payload elements to be silently accepted or omitted and the resulting list to be persisted as complete. One **LOW** issue remains in capability metadata: COMMUNITY auth is reported as enabling admin detection despite no manager-list fetch implementation. No CRITICAL findings. This is not a phase-closure decision.
|
||
|
||
## Findings
|
||
|
||
### HIGH — malformed likes/comments can produce a persisted “complete” participant list
|
||
|
||
Evidence: [vk-provider.ts](/srv/projects/Agent%20projects/randomayzer/src/providers/vk/vk-provider.ts:272) converts `item.id` directly with `String(item.id)` and inserts the participant without checking that the ID is a valid user identifier. The comments path accepts any `from_id` satisfying JavaScript `> 0` and silently ignores other values at [vk-provider.ts](/srv/projects/Agent%20projects/randomayzer/src/providers/vk/vk-provider.ts:308). The pagination guard at [vk-client.ts](/srv/projects/Agent%20projects/randomayzer/src/integrations/vk/vk-client.ts:249) validates array size and reported totals, but not element shape. The route then saves whatever the provider/pipeline returned at [participants/route.ts](/srv/projects/Agent%20projects/randomayzer/src/app/api/giveaways/%5Bid%5D/participants/route.ts:95).
|
||
|
||
For example, a likes page with a reported matching count and an item missing `id` becomes participant ID `"undefined"`; multiple such records collapse in the map. A malformed comment `from_id` is counted by pagination but omitted from participants. Either path can silently change draw eligibility while completing the import and replacing the previous list. The added R3 tests cover missing/short pages and incomplete `groups.isMember` results, but do not exercise malformed likes/comments item payloads.
|
||
|
||
The provider should reject malformed participant records with the explicit incomplete-data error before returning; a regression test should assert the existing participant list survives each malformed payload case.
|
||
|
||
### LOW — COMMUNITY capability metadata claims admin detection without an implementation
|
||
|
||
Evidence: [vk-capabilities.ts](/srv/projects/Agent%20projects/randomayzer/src/providers/vk/vk-capabilities.ts:34) sets `adminDetection: true` whenever the auth type is `COMMUNITY` and clears the explanation. The provider implementation has no manager-list lookup, and [participant-enricher.ts](/srv/projects/Agent%20projects/randomayzer/src/core/pipeline/participant-enricher.ts:28) only enriches subscriptions. Actual participant/snapshot validation currently uses the provider’s static capabilities (`adminDetection: false`), so this metadata discrepancy does not bypass the current server guard; however, the preview API can return a capability consumers could treat as available.
|
||
|
||
Keep this capability false until a complete manager-detection implementation exists, or derive it from a separately verified capability; add a test for COMMUNITY auth metadata. Current end-user flow selecting manager exclusion is disabled with a clear explanation, and the server validator rejects it.
|
||
|
||
## Areas reviewed without additional findings
|
||
|
||
- Pagination rejects invalid/changing totals, short/empty pages before total, oversized pages, missing totals for incomplete data, and max-page truncation. Subscription enrichment validates response shape, unexpected/duplicate/missing IDs and member values. The import route writes only after all fetch/enrichment succeeds.
|
||
- Reposts and manager exclusion are rejected by provider capability validation with reasons; the creation UI visibly disables both unsupported options and explains why. The code makes ordinary paginated calls and does not use `execute`.
|
||
- Smoke configuration requires a non-symlink regular mode-0600 file, allowlists keys and URL origins, keeps cookie/basic-auth material in curl config under a private temp directory, does not print response bodies, and reports `SKIP`/`MANUAL` distinctly. Exit status is zero when executed checks pass even when others are skipped/manual, but the script prints skip counts and its documentation explicitly says skipped checks are not passed; I did not classify that as a false PASS.
|
||
- The new initial migration intentionally models the pre-ownership schema; the following ownership migration aborts on null owner rows, then makes the column non-null and changes FK behavior. The runbook explicitly requires backup/schema/history inspection and says not to apply CREATE TABLE over existing tables. The recorded disposable-PostgreSQL migration evidence includes a zero-difference Prisma schema check.
|
||
- The production entrypoint replaces proxy metadata before Next receives the request, derives trust from `socket.remoteAddress`, strips proxy headers from untrusted peers, and allows only the configured `10.77.0.1` peer in the domain profile. Tests cover forged headers from several peers and a real loopback socket. Compose publishes only loopback plus the explicit WireGuard address in the domain overlay; DB has no host port. The runbook gates the actual VPS/WireGuard source identity and firewall persistence on operator checks.
|
||
- The deployment runbook includes startup, migration, backup/restore, update/rollback and two-host domain procedures. It requires the correct existing proxy and firewall manager and avoids changing the tunnel or unrelated services.
|
||
|
||
## Commands run by this review
|
||
|
||
- `git status --short --branch` — branch `codex/stage1-completion`; pre-existing `M AGENTS.md` preserved.
|
||
- `git rev-parse HEAD` — `17faecd962c2cf40e0a3f6ec4434811cfffdd312`.
|
||
- `USE_VK_MOCK=true npx vitest run tests/vk-r3-safety.test.ts tests/production-transport.test.ts tests/deployment-profile.test.ts` — exit 0; 3 files, 26 tests passed.
|
||
- `git diff --check 6382cb8e0473ba9ea5ba7a3f94f34e8ab0d4ec60..17faecd962c2cf40e0a3f6ec4434811cfffdd312` — exit 0, no whitespace errors.
|
||
|
||
Not run by this reviewer: `npm ci`, Prisma generation, full unit/integration suites, lint, build, dependency audit, Docker stack rebuild, live VK smoke, live VPS/domain/network acceptance. Coordinator owns the full gates and Docker work.
|
||
|
||
## UNVERIFIED and remaining blockers
|
||
|
||
- Actual live VK response schemas and official behavior for manager listing, private-profile reposts, pagination consistency and method limits remain unverified. This review makes no claim about external VK contracts.
|
||
- Whether the deployed Docker/VPS path preserves the trusted socket peer as `10.77.0.1`, and whether firewall rules persist correctly, still require operator verification on the target hosts.
|
||
- Live VK consent, domain login, TLS/basic-auth behavior, and external/LAN/WireGuard isolation remain unverified.
|
||
- R3 malformed participant item validation is the release-blocking code issue identified here; regression coverage and repair remain outstanding.
|
||
|
||
## Files changed by this review session
|
||
|
||
- `agents/codex/inbox/2026-10-19-auditor2.md` — assignment record.
|
||
- `agents/codex/done/2026-10-19-auditor2.md` — this report.
|
||
- `/srv/projects/AI-Memory/01_PROJECTS/randomayzer/worklog/2026-10-19_auditor2_stage1-review.md` — individual memory worklog.
|
||
|
||
No production code or tests were changed; no commit was created. The pre-existing `AGENTS.md` modification was left untouched.
|