feat: add account-specific verification action in auth menu #414
Conversation
WalkthroughAdds a Google account verification subsystem (types, URL normalization, verification request/response handling, token refresh, and browser-open helpers). Integrates verification state into account storage and AccountManager (fields, persistence, mark/clear methods). Embeds verification flows into CLI and plugin UI (new "verify" and "verify-all" actions, prompts, per-account verification handling, and optional browser opening). Extends select UI with non-selectable headings, ANSI-safe truncation, optional screen clearing, and windowed rendering. Also updates several UI prompts and account-action flows. Lines changed: +871/-70. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)src/plugin/ui/select.ts (1)
src/plugin.ts (4)
🔇 Additional comments (12)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile OverviewGreptile SummaryThis PR reworks the interactive auth login menu to add section headings, clearer per-account labels, and verification actions (verify one / verify all). It also introduces a verification probe that refreshes an account�s token and makes a small Antigravity request to detect The main integration is in Merge blockers found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
autonumber
participant User as User (CLI)
participant Menu as auth-menu/select
participant Plugin as createAntigravityPlugin
participant Storage as antigravity-accounts.json
participant AM as AccountManager
participant API as Antigravity API
participant Browser as System browser
User->>Menu: opencode auth login
Menu->>Plugin: promptLoginMode(existingAccounts)
alt Verify one account
Menu-->>Plugin: {mode:"verify", verifyAccountIndex?}
Plugin->>Plugin: verifyAccountAccess(account)
Plugin->>API: streamGenerateContent alt=sse (ping)
API-->>Plugin: 200 OK
Plugin->>Plugin: clearStoredAccountVerificationRequired(enable=true)
Plugin->>Storage: saveAccounts(if changed)
Plugin->>AM: clearAccountVerificationRequired(index, wasRequired)
else Verify blocked (403 VALIDATION_REQUIRED)
API-->>Plugin: 403 + body
Plugin->>Plugin: extractVerificationErrorDetails(body)
Plugin->>Plugin: markStoredAccountVerificationRequired(enabled=false, metadata)
Plugin->>Storage: saveAccounts(if changed)
Plugin->>AM: markAccountVerificationRequired(index, reason, url)
opt Open URL
Plugin->>User: promptOpenVerificationUrl()
User-->>Plugin: yes
Plugin->>Browser: openBrowser(verifyUrl)
end
end
opt Runtime request path
Plugin->>API: fetch(prepared.request)
API-->>Plugin: 403 VALIDATION_REQUIRED
Plugin->>Plugin: extractVerificationErrorDetails(body)
Plugin->>AM: markAccountVerificationRequired(...)
Plugin->>AM: markAccountCoolingDown(..., "validation-required")
Plugin->>AM: markRateLimited(...)
Plugin-->>Plugin: switch to next account
end
|
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin/ui/auth-menu.ts
Line: 17:21
Comment:
**Removed manage action**
`showAuthMenu` no longer returns `{ type: 'manage' }`, but `promptLoginMode` still routes `'toggle'` account actions to `{ mode: "manage" }` (`src/plugin/cli.ts:134`). This makes the TTY menu inconsistent with the non-TTY fallback (which can no longer select manage) and effectively removes the ability to enter “manage” from the main menu. If “manage” is still intended, re-add a Manage entry in `AuthMenuAction`/menu items; otherwise remove the `manage` mode path (and any callers) so toggle/enable-disable remains reachable in both flows.
How can I resolve this? If you propose a fix, please make it concise.
In Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin/cli.ts
Line: 60:64
Comment:
**Manage option lost in fallback**
In `promptLoginModeFallback` the prompt removed the `(m)anage` option and no longer returns `{ mode: "manage" }`, but `LoginMode` still includes `"manage"` and the TTY flow still supports toggling accounts via `{ mode: "manage", toggleAccountIndex }` (`src/plugin/cli.ts:134` + `src/plugin.ts:2607`). This means non-TTY environments can’t enable/disable accounts anymore, which is a functional regression for CI/remote shells and pipes.
How can I resolve this? If you propose a fix, please make it concise. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return out + suffix; | ||
| } | ||
|
|
There was a problem hiding this comment.
ANSI truncation breaks styling
truncateAnsi() appends the suffix without ensuring ANSI styles are reset, so a truncated colored label can "bleed" its color/bold into the rest of the menu (including following lines). This will happen whenever an item/message/subtitle is longer than the terminal width.
Consider appending ANSI.reset before the suffix (or tracking open SGR state) so truncated strings always end in a reset.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/ui/select.ts
Line: 64:66
Comment:
**ANSI truncation breaks styling**
`truncateAnsi()` appends the suffix without ensuring ANSI styles are reset, so a truncated colored label can "bleed" its color/bold into the rest of the menu (including following lines). This will happen whenever an item/message/subtitle is longer than the terminal width.
Consider appending `ANSI.reset` before the suffix (or tracking open SGR state) so truncated strings always end in a reset.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
This should pass through the stored value ( Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin.ts
Line: 2580:2582
Comment:
**Wrong enabled flag saved**
`existingAccounts` passes `enabled: acc.enabled !== false`, which flips the meaning (it’s `true` when enabled). But `promptLoginMode()` forwards that into `AccountInfo.enabled`, and `showAuthMenu()` treats `enabled === false` as disabled. With the current mapping, disabled accounts will be displayed as enabled and vice‑versa, and the "[disabled]" badge will be wrong.
This should pass through the stored value (`acc.enabled`) or normalize consistently across UI and storage.
How can I resolve this? If you propose a fix, please make it concise. |
| const walk = (value: unknown, key?: string): void => { | ||
| if (typeof value === "string") { | ||
| const normalizedValue = decodeEscapedText(value); | ||
| const lowerValue = normalizedValue.toLowerCase(); | ||
| const lowerKey = key?.toLowerCase() ?? ""; |
There was a problem hiding this comment.
Verification timestamp always changes
markStoredAccountVerificationRequired() compares verificationRequiredAt to a freshly computed Date.now() and will therefore mark the account as changed on every call (and continually rewrite storage), even if the account was already in verificationRequired state with the same reason/url. This makes storageUpdated effectively always true in the verify flows and can cause unnecessary disk writes.
A simple fix is to only set verificationRequiredAt when transitioning from not-required → required (or if it was previously unset), rather than every time the function runs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin.ts
Line: 364:368
Comment:
**Verification timestamp always changes**
`markStoredAccountVerificationRequired()` compares `verificationRequiredAt` to a freshly computed `Date.now()` and will therefore mark the account as changed on every call (and continually rewrite storage), even if the account was already in `verificationRequired` state with the same reason/url. This makes `storageUpdated` effectively always true in the verify flows and can cause unnecessary disk writes.
A simple fix is to only set `verificationRequiredAt` when transitioning from not-required → required (or if it was previously unset), rather than every time the function runs.
How can I resolve this? If you propose a fix, please make it concise.| unique.sort((a, b) => { | ||
| const score = (value: string): number => { | ||
| let total = 0; | ||
| if (value.includes("plt=")) total += 4; | ||
| if (value.includes("/signin/continue")) total += 3; | ||
| if (value.includes("continue=")) total += 2; | ||
| if (value.includes("service=cloudcode")) total += 1; | ||
| return total; | ||
| }; | ||
| return score(b) - score(a); | ||
| }); | ||
| return unique[0]; | ||
| } | ||
|
|
||
| function extractVerificationErrorDetails(bodyText: string): { | ||
| validationRequired: boolean; | ||
| message?: string; | ||
| verifyUrl?: string; | ||
| } { | ||
| const decodedBody = decodeEscapedText(bodyText); | ||
| const lowerBody = decodedBody.toLowerCase(); | ||
| let validationRequired = lowerBody.includes("validation_required"); | ||
| let message: string | undefined; | ||
| const verificationUrls = new Set<string>(); | ||
|
|
||
| const collectUrlsFromText = (text: string): void => { | ||
| for (const match of text.matchAll(/https:\/\/accounts\.google\.com\/[^\s"'<>]+/gi)) { | ||
| if (match[0]) { | ||
| verificationUrls.add(match[0]); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| collectUrlsFromText(decodedBody); |
There was a problem hiding this comment.
Verify prompt index mismatch
promptAccountIndexForVerification() returns the 0-based index the user typed minus 1, but it’s passed existingAccounts built from existingStorage.accounts.map((acc, idx) => ({ index: idx, ... })), and the selection flow later uses that returned value to index existingStorage.accounts. If existingAccounts is ever filtered/reordered (e.g. non-account rows, future sorting), this will verify the wrong account. To keep this robust, validate/resolve the selection by the stored account.index (or pass the full existingStorage.accounts and use its indices directly), rather than relying on list position.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin.ts
Line: 303:336
Comment:
**Verify prompt index mismatch**
`promptAccountIndexForVerification()` returns the 0-based index the user typed minus 1, but it’s passed `existingAccounts` built from `existingStorage.accounts.map((acc, idx) => ({ index: idx, ... }))`, and the selection flow later uses that returned value to index `existingStorage.accounts`. If `existingAccounts` is ever filtered/reordered (e.g. non-account rows, future sorting), this will verify the wrong account. To keep this robust, validate/resolve the selection by the stored `account.index` (or pass the full `existingStorage.accounts` and use its indices directly), rather than relying on list position.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugin.ts (1)
244-268:⚠️ Potential issue | 🟡 MinorPotential command injection in
openBrowserwith URL arguments.
openBrowserpasses the URL directly into shell commands viaexec()with string interpolation (e.g.,exec(`open "${url}"`)). While this function is pre-existing, this PR introduces a new call path at line 2892 where the URL originates from API error responses (viaextractVerificationErrorDetails→selectBestVerificationUrl).Although
normalizeGoogleVerificationUrlvalidates that the hostname isaccounts.google.com, the URL's query string could theoretically contain shell metacharacters (e.g.,$, backticks) that surviveURL.toString(). The risk is mitigated by the hostname validation, but a safer approach would useexecFilewith argument arrays instead of shell interpolation.Safer alternative for openBrowser
+import { execFile } from "node:child_process"; + async function openBrowser(url: string): Promise<boolean> { try { if (process.platform === "darwin") { - exec(`open "${url}"`); + execFile("open", [url]); return true; } if (process.platform === "win32") { - exec(`start "" "${url}"`); + execFile("cmd", ["/c", "start", "", url]); return true; } // ... similar for other platformsAlso applies to: 2889-2898
🤖 Fix all issues with AI agents
In `@src/plugin/storage.ts`:
- Line 180: Rename the CooldownReason variant from "validation-required" to
"verification-required" in the CooldownReason type declaration (symbol:
CooldownReason) and update all places that construct or compare that string
(e.g., the place in plugin.ts that currently emits/compares
"validation-required") so they use "verification-required" consistently; ensure
any switch/case, equality checks, stored values, and user-facing mappings are
updated to the new literal to keep type and runtime strings aligned.
🧹 Nitpick comments (6)
src/plugin/accounts.ts (1)
1015-1018: Consider omitting falsy verification fields from persisted storage.Currently
verificationRequired: falseandundefinedvalues for the other fields will be serialized. Other fields in this mapping use conditional expressions (e.g., line 1008 forrateLimitResetTimes). For consistency and to avoid storing noise:Optional cleanup
- verificationRequired: a.verificationRequired, - verificationRequiredAt: a.verificationRequiredAt, - verificationRequiredReason: a.verificationRequiredReason, - verificationUrl: a.verificationUrl, + verificationRequired: a.verificationRequired || undefined, + verificationRequiredAt: a.verificationRequiredAt, + verificationRequiredReason: a.verificationRequiredReason, + verificationUrl: a.verificationUrl,src/plugin.ts (5)
317-435:extractVerificationErrorDetailsis thorough but complex — consider a brief doc comment.The function handles multiple response formats (plain JSON, SSE
data:lines, nested structures) with recursive walking. Thevisitedset (line 363) correctly prevents infinite traversal, and the broadlowerKey === "url"match (line 383) is safely gated bynormalizeGoogleVerificationUrldownstream.One subtle behavior: line 374 captures only the first string value with a message-like key (
!message && ...). If the most descriptive message appears after a less useful one in the walk order (object key iteration order), it will be missed. This is an acceptable trade-off for simplicity.
437-547: Hardcoded model"gemini-3-flash"in verification probe — consider extracting as a constant.The verification probe (lines 494-499) hardcodes the model name
"gemini-3-flash"in two places within the request body. If this model is renamed or deprecated, the verification check will silently break (returning "error" instead of the true verification status).Suggested extraction
+const VERIFICATION_PROBE_MODEL = "gemini-3-flash"; + async function verifyAccountAccess( // ... ): Promise<VerificationProbeResult> { // ... const requestBody = { - model: "gemini-3-flash", + model: VERIFICATION_PROBE_MODEL, request: { - model: "gemini-3-flash", + model: VERIFICATION_PROBE_MODEL, contents: [{ role: "user", parts: [{ text: "ping" }] }], generationConfig: { maxOutputTokens: 1, temperature: 0 }, }, };
590-666: Duplication of verification state management logic betweenplugin.tsandaccounts.ts.
markStoredAccountVerificationRequired/clearStoredAccountVerificationRequiredinplugin.tsduplicate the logic inAccountManager.markAccountVerificationRequired/clearAccountVerificationRequiredinaccounts.ts. The plugin.ts versions operate on plain storage objects while accounts.ts versions operate onManagedAccountinstances.This duplication is understandable since the login flow works with raw storage before the
AccountManageris initialized, but it creates a maintenance risk — changes to verification logic must be synchronized in two places. Consider extracting shared pure-logic helpers that both can call, or documenting the relationship.
598-634:markStoredAccountVerificationRequired: timestamp comparison is always true.Line 611:
if (account.verificationRequiredAt !== timestamp)—timestampis freshly obtained fromDate.now()on line 610, so this will virtually always differ from the stored value. Thechangedflag tracking here adds complexity without meaningful optimization. This is not a bug, but the granular change-tracking forverificationRequiredAtis effectively a no-op guard.
2744-2907: Verify flow is well-implemented — one readability suggestion.The verify-all and verify-single paths are comprehensive and correctly sync both storage and in-memory state. A minor readability improvement: this ~160-line block could be extracted into helper functions (e.g.,
handleVerifyAll,handleVerifySingle) to reduce nesting depth in the already-largeauthorizemethod.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
src/plugin.tssrc/plugin/accounts.tssrc/plugin/storage.tssrc/plugin/ui/auth-menu.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/plugin/storage.ts (1)
src/plugin/accounts.ts (1)
CooldownReason(12-12)
src/plugin.ts (6)
src/plugin/types.ts (1)
PluginClient(42-42)src/plugin/auth.ts (2)
parseRefreshParts(12-19)formatRefreshParts(24-28)src/plugin/token.ts (1)
AntigravityTokenRefreshError(60-80)src/constants.ts (3)
ANTIGRAVITY_DEFAULT_PROJECT_ID(71-71)ANTIGRAVITY_HEADERS(94-98)ANTIGRAVITY_ENDPOINT_PROD(34-34)src/plugin/rotation.ts (1)
getHealthTracker(441-446)src/plugin/storage.ts (1)
saveAccounts(651-677)
src/plugin/ui/auth-menu.ts (2)
src/plugin/ui/ansi.ts (1)
ANSI(6-25)src/plugin/ui/select.ts (2)
MenuItem(3-12)select(77-312)
🔇 Additional comments (10)
src/plugin/storage.ts (1)
196-200: LGTM — verification fields onAccountMetadataV3.The new optional fields (
verificationRequired,verificationRequiredAt,verificationRequiredReason,verificationUrl) are well-typed, properly optional, and align with theManagedAccountadditions inaccounts.tsand the persistence logic insaveToDisk.src/plugin/accounts.ts (3)
166-169: LGTM —ManagedAccountverification fields.Fields correctly mirror
AccountMetadataV3and are properly optional.
383-386: LGTM — Verification fields loaded from storage.Straight-through mapping from stored metadata to in-memory account.
827-876: LGTM —markAccountVerificationRequiredandclearAccountVerificationRequiredare well-structured.Both methods correctly guard against invalid indices, update state atomically, and trigger persistence through
setAccountEnabledorrequestSaveToDiskas appropriate. TheclearAccountVerificationRequiredmethod's conditional re-enable logic (only whenenableAccountis true and the account was previously verification-blocked) is a nice touch.One minor observation:
clearAccountVerificationRequiredsetsverificationRequired = false(line 864) instead ofundefined, which means"verificationRequired": falsewill be serialized to the JSON file. This is harmless but adds a small amount of noise to the stored data compared to omitting the key entirely.src/plugin/ui/auth-menu.ts (3)
56-65: LGTM — Menu restructuring with section headings.The use of
kind: 'heading'for non-selectable section titles and separators between logical groups is clean and aligns with theselect.tscontract. The dummy{ type: 'cancel' }value for non-selectable items is a reasonable pattern given theMenuItem<T>interface requires a value.
68-74: LGTM — Enhanced account label rendering.Numbered labels with status badges provide good visual clarity. The ordering (number, name, current badge, status badge, disabled badge) reads naturally.
106-127: LGTM —showAccountDetailswith composite header and verify action.The header/subtitle restructuring improves context visibility, and the new "Verify account access" action integrates naturally.
src/plugin.ts (3)
270-315: LGTM — Verification URL types and normalization.
normalizeGoogleVerificationUrlcorrectly validates the hostname to prevent open-redirect attacks, andselectBestVerificationUrluses a reasonable scoring heuristic for selecting the most useful verification URL.
2254-2294: Verification-required handling on 403 looks correct.The flow correctly:
- Clones the response to read the body without consuming the original
- Uses the extracted details to mark the account
- Applies a 10-minute cooldown and rate limit
- Switches to another account
One consideration: the 10-minute hardcoded cooldown (line 2260) means users who complete verification quickly still have to wait or manually re-enable through the auth menu. Since
clearAccountVerificationRequiredcan re-enable the account, and the verify flow is now available, this seems like an acceptable trade-off.
2562-2584: LGTM — Verification status takes priority over rate-limited status.The
if (acc.verificationRequired)check before the rate-limit status determination correctly surfaces the root cause rather than the downstream cooldown.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| } | ||
|
|
||
| export type CooldownReason = "auth-failure" | "network-error" | "project-error"; | ||
| export type CooldownReason = "auth-failure" | "network-error" | "project-error" | "validation-required"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of "validation-required" and "verification-required"
echo "=== Occurrences of 'validation-required' ==="
rg -n "validation-required" --type=ts
echo ""
echo "=== Occurrences of 'verification-required' ==="
rg -n "verification-required" --type=tsRepository: NoeFabris/opencode-antigravity-auth
Length of output: 998
Inconsistent naming: "validation-required" vs "verification-required"
The CooldownReason type uses "validation-required" but the rest of the codebase consistently uses "verification" terminology: AccountStatus has 'verification-required' (src/plugin/ui/auth-menu.ts), user-facing display shows "[needs verification]" (auth-menu.ts:49), and debug output uses "verification-required" (plugin.ts:2275). This mismatch is confusing and error-prone for future consumers of CooldownReason.
Required changes
Update src/plugin/storage.ts line 180:
-export type CooldownReason = "auth-failure" | "network-error" | "project-error" | "validation-required";
+export type CooldownReason = "auth-failure" | "network-error" | "project-error" | "verification-required";Update src/plugin.ts line 2263:
-accountManager.markAccountCoolingDown(account, cooldownMs, "validation-required");
+accountManager.markAccountCoolingDown(account, cooldownMs, "verification-required");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type CooldownReason = "auth-failure" | "network-error" | "project-error" | "validation-required"; | |
| export type CooldownReason = "auth-failure" | "network-error" | "project-error" | "verification-required"; |
🤖 Prompt for AI Agents
In `@src/plugin/storage.ts` at line 180, Rename the CooldownReason variant from
"validation-required" to "verification-required" in the CooldownReason type
declaration (symbol: CooldownReason) and update all places that construct or
compare that string (e.g., the place in plugin.ts that currently emits/compares
"validation-required") so they use "verification-required" consistently; ensure
any switch/case, equality checks, stored values, and user-facing mappings are
updated to the new literal to keep type and runtime strings aligned.
| markAccountVerificationRequired(accountIndex: number, reason?: string, verifyUrl?: string): boolean { | ||
| const account = this.accounts[accountIndex]; | ||
| if (!account) { | ||
| return false; | ||
| } | ||
|
|
||
| account.verificationRequired = true; | ||
| account.verificationRequiredAt = nowMs(); | ||
| account.verificationRequiredReason = reason?.trim() || undefined; |
There was a problem hiding this comment.
Verification timestamp churn
AccountManager.markAccountVerificationRequired() overwrites verificationRequiredAt on every call (account.verificationRequiredAt = nowMs()), even if the account was already marked verificationRequired. This makes repeated 403/VALIDATION_REQUIRED responses continuously change the stored metadata and trigger extra disk writes/toast logic downstream. Consider only setting verificationRequiredAt when transitioning from not-required
to required (or when it�s currently unset), similar to the storage helper in src/plugin.ts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/accounts.ts
Line: 827:835
Comment:
**Verification timestamp churn**
`AccountManager.markAccountVerificationRequired()` overwrites `verificationRequiredAt` on every call (`account.verificationRequiredAt = nowMs()`), even if the account was already marked `verificationRequired`. This makes repeated 403/`VALIDATION_REQUIRED` responses continuously change the stored metadata and trigger extra disk writes/toast logic downstream. Consider only setting `verificationRequiredAt` when transitioning from not-required
to required (or when it�s currently unset), similar to the storage helper in `src/plugin.ts`.
How can I resolve this? If you propose a fix, please make it concise.| if (response.status === 403) { | ||
| const errorBodyText = await response.clone().text().catch(() => ""); | ||
| const extracted = extractVerificationErrorDetails(errorBodyText); | ||
|
|
||
| if (extracted.validationRequired) { | ||
| const verificationReason = extracted.message ?? "Google requires account verification."; | ||
| const cooldownMs = 10 * 60 * 1000; | ||
|
|
||
| accountManager.markAccountVerificationRequired(account.index, verificationReason, extracted.verifyUrl); | ||
| accountManager.markAccountCoolingDown(account, cooldownMs, "validation-required"); | ||
| accountManager.markRateLimited(account, cooldownMs, family, headerStyle, model); | ||
|
|
There was a problem hiding this comment.
Validation-required treated as rate-limit
When a request gets 403 + VALIDATION_REQUIRED, the code marks the account as cooling down and also calls accountManager.markRateLimited(account, cooldownMs, family, headerStyle, model). This makes a verification block look like a temporary quota/rate-limit and can affect rotation and UI status (e.g. showing �rate-limited�) even though the account is actually disabled for manual verification.
If the intent is to remove the account from rotation until the user verifies, the enabled=false + verificationRequired=true state already does that; treating it as a rate limit can mislead selection logic and status reporting.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin.ts
Line: 2255:2266
Comment:
**Validation-required treated as rate-limit**
When a request gets 403 + `VALIDATION_REQUIRED`, the code marks the account as cooling down and also calls `accountManager.markRateLimited(account, cooldownMs, family, headerStyle, model)`. This makes a verification block look like a temporary quota/rate-limit and can affect rotation and UI status (e.g. showing �rate-limited�) even though the account is actually disabled for manual verification.
If the intent is to remove the account from rotation until the user verifies, the `enabled=false` + `verificationRequired=true` state already does that; treating it as a rate limit can mislead selection logic and status reporting.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Safer options are to use Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin.ts
Line: 244:264
Comment:
**Command injection via URL**
`openBrowser()` interpolates `url` into a shell command (`exec(�open "${url}"�)`, `xdg-open`, `wslview`, etc.). In this PR, that URL can come from remote API error bodies (`extractVerificationErrorDetails()` �3 `verifyUrl` �2 `openBrowser()`), so a crafted URL containing quotes/backticks can execute arbitrary commands on the user�s machine. This will happen whenever the backend returns a verification URL that includes shell metacharacters.
Safer options are to use `execFile`/`spawn` with argument arrays (no shell interpolation), or strictly validate/escape URLs before passing them to the shell.
How can I resolve this? If you propose a fix, please make it concise. |
1 participant
Summary
VALIDATION_REQUIRED, automatically disable that account, persist verification-required metadata, switch away from it, and show a guiding toastLinked Issues
Testing