Ochenstarik bc2b658570 Phase 2.3.1: token refresh correctness gate

2026-08-18 14:37:09 +07:00

11 KiB

Raw Blame History

Randomayzer — Phase G-5 Authenticated VK Access / Token Lifecycle Review

Reviewer: Grok (xAI)
Date: 2026-08-18
Commit: d6f087c21efb593ee7db58f816be98a2d087b3e3
Scope: Phase 2.3 — VkAuthContextResolver, token refresh, SERVICE→USER fallback, credential ownership, capabilities, import auth.
Constraint: Review / tests / docs only. No production code changes.

1. Executive Verdicts

Area	Verdict
Credential isolation	PASS
Resolver	PASS WITH WARNINGS
Refresh	PASS WITH WARNINGS
Refresh concurrency	PASS WITH WARNINGS
Fallback	PASS WITH WARNINGS
Capabilities	PASS WITH WARNINGS
Token confidentiality	PASS
Participant import	PASS WITH WARNINGS
VK contract	PASS WITH WARNINGS
Overall Phase 2.3	PASS WITH FIXES

Безопасно ли переходить к реальному VK smoke test?

YES.

Real blockers: none for a controlled smoke test with:

configured VK_SERVICE_TOKEN / organizer USER login,
TOKEN_ENCRYPTION_KEY,
single-instance process (in-memory single-flight map).

Must watch during smoke: refresh single-flight under load, null expiresAt behaviour, fallback only on private/permission errors, no token in API responses.

2. Credential Ownership / IDOR

Participants POST/GET and other mutations call requireGiveawayOwner before any provider/resolver call.

const { giveaway, sessionUser } = await requireGiveawayOwner(req, id);
// ...
organizerId: sessionUser.id  // from session, not body

Organizer identity for resolver comes from trusted session after ownership check.
Client cannot pass another user’s userId / vkUserId / organizerId to decrypt or use their credential.
A’s giveaway never loads B’s UserCredential.
Null organizerId still Forbidden (prior phase invariant).

Horizontal privilege escalation: not found.

3. VkAuthContextResolver

Least-privilege default: SERVICE if configured; else USER if organizerId present.

Mode	Behaviour
preferred SERVICE	SERVICE env token; if missing + organizer → USER
preferred USER	requires organizerId → getOrRefreshUserToken
preferred COMMUNITY	env `VK_COMMUNITY_TOKEN_{id}` or USER fallback
automatic	SERVICE preferred; else USER; else AuthError

resolveUserFallbackContext(organizerId) is explicit and only used by provider on private/permission failure.

No silent arbitrary token switching outside documented paths.

WARN: COMMUNITY → USER fallback when community token missing is broad; acceptable if documented.

4. SERVICE → USER Fallback

Provider (e.g. fetchPost) only falls back when:

err instanceof VkPrivateResourceError || err instanceof VkPermissionError
&& activeAuth.type === 'SERVICE'
&& options?.organizerId

Error class	Fallback?
VkPrivateResourceError	YES (documented)
VkPermissionError	YES (documented; broader than pure private)
VkRateLimitError	NO
VkTemporaryError	NO
VkNetworkError / Timeout	NO
VkValidationError	NO
VkAuthError on SERVICE	NO (rethrows)

Rate-limit bypass via token switch: blocked.

WARN: Treating all VkPermissionError as fallback-eligible may include non-privacy permission failures; policy is explicit in VK_AUTHENTICATED_ACCESS.md.

Fallback loop: USER path does not re-enter SERVICE fallback → no SERVICE↔USER loop.

5. User Token Expiry

const isExpiredOrExpiring = cred.expiresAt
  ? now >= cred.expiresAt.getTime() - 30_000
  : false;

Case	Behaviour
future expiresAt	decrypt & use
within 30s of expiry	refresh
past expiry	refresh
null expiresAt	treated as non-expired → send without refresh
missing access token	ReauthenticationRequired

WARN: null/legacy expiresAt never triggers refresh. Prefer “unknown expiry → refresh or re-auth” for safety.

Expired token is not knowingly sent when expiresAt is present and past.

6–10. Refresh Security & Concurrency

Security

Refresh token decrypted only server-side in executeRefresh.
New access (and rotated refresh if present) encrypted before upsertUserWithTokens.
Failures → VkReauthenticationRequiredError; message may include generic error text, not raw tokens.
Refresh response user_id is not checked against stored vkUserId → WARN (account binding): should reject identity mismatch.

Single-flight

Map keyed by userId.
Existing test: 20 concurrent → 1 refresh call, same token to all (PASS in test).
WARN: every waiter runs finally { inFlightRefreshes.delete(userId) }. First completer clears the key; a new concurrent request can start a second refresh while other waiters still use the first promise. Prefer delete only if map.get(id) === thisFlight.
Locks are per-user → A does not block B (OK).
On error, waiters all reject; key cleared → subsequent call can retry (no permanent stuck lock).

Stale write

No version/CAS on credential update. Late refresh can overwrite a credential updated by a concurrent login/refresh.
Severity: MEDIUM–HIGH under concurrent refresh+relogin; lower if single-flight holds for most cases.

Refresh failure matrix (expected)

VK mock outcome	Result
invalid/expired refresh	ReauthenticationRequired
network/timeout/429/500	wrapped ReauthenticationRequired
missing access_token	ReauthenticationRequired
malformed	ReauthenticationRequired
No plaintext token in thrown message by design	OK

11. Account Binding

Upsert on refresh uses DB user.vkUserId, not token response identity.
Silent rebind to another VK account: not implemented.
GAP: no explicit reject if refresh response user_id ≠ stored vkUserId.

12–13. Token Confidentiality

Markers must not appear in API JSON, participant responses, giveaway detail, capabilities, audit proof.

Design:

Credentials only via vault decrypt on server.
POST participants returns summary counts only.
Session cookie is opaque ID.
UserCredential not spread into public DTOs.

Encrypted ciphertext also should not be returned to frontend — repository responses used by API must omit credential fields (verify list/detail serializers).

Token leak result (static review): no intentional plaintext path found. Smoke test should grep responses/logs for markers.

14–16. Participant Import Auth Flow

Order:

Rate limit
requireGiveawayOwner (session + ownership + CSRF)
Validate body
Idempotency lookup
Provider fetchParticipants with organizerId: sessionUser.id
Pipeline / persist
Summary response + idempotency store

No provider call before ownership. Client organizer id cannot control resolver.

Partial import + fallback: if SERVICE fails mid-pagination with private error, fallback restarts USER fetch. Provider should not merge partial SERVICE pages with USER result as one complete set without clear restart. WARN: confirm import path fully restarts on fallback (likes/comments) rather than appending mixed auth pages.

Idempotency: key includes operation + giveawayId + payload; successful USER completion after SERVICE deny should cache final result; replay returns cache (design intent).

17–18. Runtime Capabilities

Docs define method matrix + fallback rules. Static provider.capabilities still flag reposts/adminDetection false.

TOCTOU: UI capability snapshot can go stale if token expires before import; import path re-resolves via refresher → revalidation on execution (OK). Do not trust UI-only flags for authorization.

WARN: Ensure API “effectiveCapabilities” for a giveaway reflects actual SERVICE availability + organizer credential presence, not only static provider flags.

19–20. Subscription / Preview

groups.isMember batching remains 500; auth via resolver/organizerId. Prefer consistent token for all batches of one import (no mixed SERVICE/USER batches unless intentional full restart).

/api/posts/preview: must not accept foreign organizer tokens; use session or SERVICE only; no token fields in response. (Confirm route does not take client-supplied user tokens.)

21–22. Rate Limit & Refresh Storm

Global VK limiter can starve short calls during large import (ops issue, not security).

100 concurrent + refresh 429/500: single-flight should yield one attempt then shared failure; after key clear, retries possible — avoid unbounded client retry amplification (application/API rate limits).

23. Credential Invalidation

Confirmed auth failure → VkReauthenticationRequiredError → client reconnect.
No infinite retry of bad refresh token in-process without new user action (OK).
Optional: clear stored refresh on definitive invalid_grant (product choice).

24–25. Versioning / Audit Isolation

Ciphertext has no explicit key-version field → TECH DEBT, non-blocking.

Auth mode / token metadata must not enter Randomizer/AuditProof inputs — unchanged core; PASS.

26–27. Mock vs Real / VK Contract

Claim	Implementation	Official	Verdict
Refresh endpoint oauth2/auth	Yes	VK ID docs	VERIFIED (path)
grant_type refresh_token	via oauth client	Required	VERIFIED if client sends it
device_id on refresh	optional / often absent	Sometimes required	UNVERIFIED
access + optional refresh rotation	Yes	Common	VERIFIED pattern
expires_in handling	Yes (+30s skew)	Yes	PARTIAL (null expiry)
SERVICE→USER only on private/permission	Yes	Product policy	VERIFIED policy
Scope separator	comma default	space in some VK ID	UNVERIFIED live

No definite WRONG refresh contract found that blocks smoke. Confirm device_id and scope format on the registered app during smoke.

28. Performance

Resolver + decrypt + expiry check are O(1) vs network/pagination. Overhead negligible vs 100k import.

29. CRITICAL / HIGH

CRITICAL: none for controlled smoke with proper env.

HIGH:

Single-flight finally deletes key for every waiter → possible double refresh under overlap.
null expiresAt never refreshes.
No CAS/version on credential write (stale refresh overwrite).
No refresh response user_id vs stored vkUserId check.

MEDIUM:

Fallback includes all PermissionError.
Partial SERVICE pages + USER restart semantics.
Global limiter starvation (ops).

30. Refresh stress scale

Existing test: 20 concurrent → 1 refresh.
Design targets 50–100; recommend extending test with the “delete only if same promise” fix verification.

31. Phase 2.3 readiness for real VK smoke