feat(auth): import Cockpit Tools accounts through safe adapters - #1357
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded Cockpit Tools Antigravity JSON account import through the OAuth API, CLI, and dashboard. The change validates input and responses, refreshes Google identity, persists credentials, updates caches, reports results, and documents supported usage. ChangesAntigravity account import
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ProviderAuthPanel
participant OAuthImportRoute
participant importAccounts
participant Google
participant OAuthStore
User->>ProviderAuthPanel: Select Cockpit Tools JSON file
ProviderAuthPanel->>OAuthImportRoute: POST account import document
OAuthImportRoute->>importAccounts: Validate and process records
importAccounts->>Google: Refresh token and fetch identity
Google-->>importAccounts: Normalized identity and credentials
importAccounts->>OAuthStore: Insert or update credential
OAuthStore-->>importAccounts: Record outcome
importAccounts-->>OAuthImportRoute: Aggregate import result
OAuthImportRoute-->>ProviderAuthPanel: Safe result counts
ProviderAuthPanel-->>User: Display completion status
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
the
intake: hygiene-blocked
|
⏳ DRAFT
What to do
Review readiness checklist
3/4 boxes ticked. Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Что сделано: - объединена существующая ветка PR lidge-jun#1357 со свежим upstream/dev; - перенесён проверенный v2.11.1 candidate и post-review исправления отмены, DTO и GUI; - добавлена турецкая локализация и устранено предупреждение React Doctor. Зачем: - сохранить историю существующего PR и обновить её обычным fast-forward push без force; - привязать безопасный импорт credentials к актуальной интеграционной ветке. Результат: - двухродительский merge commit сохраняет старый PR head и свежий dev в ancestry; - итоговый diff содержит ровно 32 проверенных пути. Проверка: - runtime: 10434 pass, 7 skip, 0 fail; - GUI: 721 pass, 0 fail; - focused backend: 133 pass, 0 fail; - typecheck, privacy scan, React Doctor, GUI/docs builds и git diff --check: PASS.
This comment was marked as outdated.
This comment was marked as outdated.
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Requesting changes on the current reviewed head be0db410b062116aee17adcd1ab2674e6b20485c.
I found two P2 lifecycle/availability issues in the account-import path:
-
Cancellation after a partial commit skips post-write reconciliation/cache invalidation.
importAccounts()can commit one or more records and then collapse a later abort to{ ok: false, status: 408, code: "import_cancelled" }, discarding the already-completed results. The management route returns immediately on!imported.ok, soreconcileLiveStateStores(), model-cache clearing, routed-model inflight clearing, and quota-cache clearing do not run even thoughauth.jsonmay already have changed. Please preserve an internal changed/committed signal (or conservatively reconcile onimport_cancelled) and add a deterministic regression where record 1 commits, record 2 blocks, then the request aborts. -
The 10-minute server deadline does not bound request-body ingestion. The route creates a deadline
AbortController, butreadBoundedJsonRequestBody(req, ...)reads withreq.signal; the deadline controller is only passed later toimportAccounts(). A slow trickling upload can therefore continue beyond the advertised server deadline. Please let the bounded body reader accept a signal override (or otherwise compose the deadline withreq.signal) and ensure a deadline/abort is reported as408 import_cancelled, not400 invalid_document. Add a regression for a body read that remains active until the deadline fires.
The broader security design looks solid: exact provider/format admission before credential traversal, bounded input, provider-derived identity/project validation before persistence, atomic identity upsert, and fixed secret-free API/CLI/GUI projections. I did not find a credential leak or unsafe credential-admission path beyond the lifecycle issues above.
Wibias
added
the
maintainer-sponsored
ChatGPT Codex Connector
github-actions
Bot
removed
the
intake: hygiene-blocked
|
@Wibias — both requested P2 lifecycle fixes are now included in exact head
Deterministic regressions cover same-record and later-record post-commit cancellation, route-level partial commit plus cache invalidation, stalled body cancellation, and malformed JSON classification. The branch was fast-forwarded without force through a two-parent bridge containing both the prior PR head Exact-head local verification:
Please re-review the resolved P2 findings on this exact head when convenient. The PR remains draft until the new GitHub CI is green. |
|
Please explain what to do next. |
|
The next step is to rebase this branch onto the current I verified the current PR head Please:
Do not mark it ready or treat the previous local/full-suite report as current until that rebased head is independently reviewed. |
91556e8 to
d80a412
Compare
|
Addressed on tip Maintainer P2 from the earlier
Also restored Turkish |
|
Guys, I'm so fed up with this. I'm a complete noob at coding—I'm writing all this code using AI. Help me and make it right. I gave you the idea and wrote the code as best I could, but I'm just exhausted. You're talking about something I'm probably incapable of doing for you! Then pull yourself together and finish the code. I don't understand what you want and what I should finish—so finish the job yourself. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@Wibias, I apologize for my last two comments. They were inappropriate and unfair to you, especially after you had already spent time rebasing the branch and fixing the two P2 lifecycle issues. I was frustrated and overwhelmed, but that does not excuse the tone or the personal remarks. Thank you for the work you did on head I understand that the PR is closed and that CodeRabbit posted ten new findings after the closure. I will not reopen this PR or force-push the branch without maintainer direction. I am validating and addressing the current findings against the exact remote head. When the candidate is fully reverified, please let me know which route you prefer:
I will follow that decision and keep future communication technical and respectful. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/oauth/store.ts (1)
541-547: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not match a different stable identity by email.
If
safe.accountIdis present and an existing account has a differentaccountId, Lines 543-547 still match on email. A reused or reassigned email can then overwrite the stored credential for a different Google identity.Use email fallback only for legacy accounts that have no
accountId. Add a regression that imports two differentaccountIdvalues with the same email and verifies that both accounts remain distinct.Proposed fix
const matches = (account: ProviderAccount): boolean => { - if (safe.accountId && account.credential.accountId === safe.accountId) return true; + if (safe.accountId) { + if (account.credential.accountId) { + return account.credential.accountId === safe.accountId; + } + return Boolean( + safe.email + && account.credential.email + && account.credential.email.toLowerCase() === safe.email.toLowerCase(), + ); + } return Boolean( safe.email && account.credential.email🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/oauth/store.ts` around lines 541 - 547, Update the matches predicate to use email fallback only when both safe.accountId and account.credential.accountId are absent; when safe.accountId is present, require an exact accountId match and never match a different stable identity by email. Add a regression covering two imports with different accountId values and the same email, verifying both accounts remain distinct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/oauth/store.ts`:
- Around line 541-547: Update the matches predicate to use email fallback only
when both safe.accountId and account.credential.accountId are absent; when
safe.accountId is present, require an exact accountId match and never match a
different stable identity by email. Add a regression covering two imports with
different accountId values and the same email, verifying both accounts remain
distinct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d852e628-cc0b-4c4d-b919-08f59c3f0714
📒 Files selected for processing (11)
src/cli/account-extended.tssrc/oauth/account-import/index.tssrc/oauth/account-import/parser.tssrc/oauth/account-import/types.tssrc/oauth/google-antigravity.tssrc/oauth/store.tssrc/server/management/oauth-account-routes.tssrc/server/request-decompress.tstests/account-import.test.tstests/cli-account.test.tstests/oauth-accounts-api.test.ts
78d68cc to
629dd15
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/account-import.test.ts`:
- Around line 314-337: Add a regression assertion to the existing
duplicate-update test around upsertCredentialByIdentity: after the first insert,
set needsReauth on the stored account before performing the second import, then
verify the updated account no longer contains needsReauth while retaining the
second credentials. If the in-memory account is not the object mutated by the
store, update auth.json directly between the two calls.
- Around line 185-214: Update the delayed-validation fixture in the test around
createAntigravityAccountImportAdapter to use the imported
ValidatedAntigravityCredential type for both resolveValidation and
delayedValidation, removing the duplicated inline object shape while preserving
the existing test behavior. If the repository’s CI type-check configuration
excludes tests, add the test files to the appropriate typecheck configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 51650137-312e-4c3e-81eb-3a03da22e5e6
📒 Files selected for processing (4)
docs-site/src/content/docs/guides/providers.mddocs-site/src/content/docs/zh-cn/guides/providers.mdsrc/oauth/store.tstests/account-import.test.ts
Что сделано: - добавлен общий bounded adapter framework импорта с v1 allowlist для Google Antigravity; - реализованы file/stdin CLI, management API и локализованный GUI file picker; - добавлены live identity/project validation, атомарный identity upsert и secret-free результаты; - обновлены документация и негативные тесты для лимитов, persistence и canary-утечек. Зачем: - заменить узкий старый PR расширяемым контрактом без привязки CLI/API к одному формату реализации; - исключить попадание refresh token в argv, ответы, логи и DOM; - сохранять только проверенные провайдером аккаунты поверх актуального dev. Результат: - импорт поддерживает доказанный Cockpit Tools Antigravity JSON через файл или stdin; - неизвестные провайдеры/форматы fail closed, а Codex/OpenAI не включены в v1; - GUI проверяет размер файла до чтения и строго валидирует успешный DTO до отображения. Проверка: - focused backend/API: 16 pass; - focused CLI: 1 pass; - focused GUI: 4 pass, full GUI: 711 pass; - typecheck, ESLint, GUI/docs builds, privacy scan и diff-check прошли; - полный bun run test: 10331 pass, 7 skip, 1 fail — нерешённый тест lidge-jun#1007 про piped OAuth login URL; Draft PR не заявляет local CI green.
Что сделано: - добавлен проверенный screenshot панели импорта Google Antigravity без email и токенов. Зачем: - политика репозитория требует встроенное визуальное доказательство для изменения интерфейса; - авторизованная браузерная загрузка GitHub недоступна в текущей среде. Результат: - Draft PR может встроить стабильный raw asset из ветки форка без стороннего хостинга. Проверка: - screenshot визуально проверен основным агентом, QA и reviewer; - на изображении нет credential content, email или token-shaped данных.
Что сделано: - исправлен запуск subprocess-теста через канонический fileURLToPath; - добавлена строгая fail-closed проверка CLI import-result DTO; - добавлены негативные fixtures для counts, индексов, status/code и утечки canary. Зачем: - закрыть сбой lidge-jun#1007 в worktree-путях со скобками; - не допустить отображения или нормализации повреждённого ответа API. Результат: - валидный импорт остаётся совместимым; - повреждённый HTTP 200 завершается фиксированной безопасной ошибкой. Проверка: - bun run test: 10333 pass, 7 skip, 0 fail; - bun run typecheck и bun run privacy:scan; - GUI 711/711, lint/build, docs build; - focused import/API/CLI 113/113.
Что сделано: - сохранён успешный GUI-результат при сбое обновления списка аккаунтов; - исправлен порядок CLI-флагов и добавлен ограниченный таймаут import POST; - восстановлена barrel-граница request-size constant; - расширены негативные CLI/GUI fixtures и исправлена русская сводка. Зачем: - закрыть шесть подтверждённых замечаний CodeRabbit без расширения OAuth-контракта; - исключить ложный статус ошибки, зависание CLI и непокрытые admission-ветки. Результат: - import остаётся fail-closed и secret-safe; - все подтверждённые review cases покрыты регрессиями. Проверка: - bun run test: 10337 pass, 7 skip, 0 fail; - focused import/API/CLI: 117/117; - GUI: 712/712, focused 5/5; - typecheck, privacy, lint, build и diff-check.
Re-apply the composed request/deadline AbortSignal, partial-commit reconciliation, Turkish cockpitImport keys, and paired GUI result validation that the rebase onto latest dev dropped.
Share one import deadline constant, preserve the active account on identity upsert, reject in-document duplicate emails, store Google userinfo id, and clarify CLI timeout versus unreachable diagnostics.
Require exact accountId when the incoming credential has one, and only fall back to email when both sides lack a stable id. Add a regression for two Google subjects sharing the same email.
Allow accountId-bearing imports to match legacy email-only rows for in-place migration, while keeping distinct stable Google IDs separate and blocking email-only imports from overwriting stable-ID rows.
Pin the delayed-validation fixture to ValidatedAntigravityCredential and assert needsReauth is cleared when a verified identity is re-imported.
fb23d90 to
c6f0c1c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/guides/providers.md`:
- Line 140: Update the provider guidance text in
docs-site/src/content/docs/guides/providers.md:140-140 to state that other
Cockpit providers are rejected in v1, and update
docs-site/src/content/docs/zh-cn/guides/providers.md:122-122 with equivalent
explicit rejection wording for imports.
In `@tests/account-import.test.ts`:
- Around line 126-162: Add a focused case to the existing mapping test using a
separate adapter whose validate method resolves with a credential having an
absent or malformed email, while retaining the other required credential fields.
Call importRecord with the existing input and assert it returns { status:
"failed", code: "credential_rejected" }, specifically covering the
safeProviderEmail rejection branch in createAntigravityAccountImportAdapter.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 62a4398b-00b9-4fdc-ae7f-f9b3b88ccc4d
📒 Files selected for processing (11)
docs-site/src/content/docs/guides/providers.mddocs-site/src/content/docs/zh-cn/guides/providers.mdgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/tr.tsgui/src/i18n/zh.tssrc/oauth/store.tstests/account-import.test.ts
State explicitly that other Cockpit providers are rejected in v1, and cover the safeProviderEmail credential_rejected branch in adapter tests.
3 participants
Summary
pws.cockpitImport*keys, and paired GUI status/code validation after rebasing onto latestdev.Verification
Rebased onto
831b008819e8fd41c86d7c77b438320d27874ce8(devat validation time). Final PR head:c6f0c1caa1574f7bcb57e334961e9569d228afce(9 commits ahead ofdev, 0 behind). Branch history was rewritten to a linear cherry-pick stack; the self-merge at982ee800that regressed identity migration was removed.Identity contract
accountId;accountIdvalues are never merged by email;Identity regression tests (final head):
PASS: legacy email-only → stable
accountIdmigration (upgrades an email-only legacy row…)PASS: different stable IDs + same email remain distinct (
keeps distinct accountId identities…)PASS: email-only incoming cannot overwrite stable-ID row (
does not overwrite a stable accountId row…)PASS:
needsReauthcleared on verified re-import (clears a terminal reauth flag…)PASS:
bun run typecheck.PASS:
bun test tests/account-import.test.ts tests/oauth-accounts-api.test.ts tests/bounded-body.test.ts tests/cli-account.test.ts— 158 pass, 0 fail, 724 assertions.PASS:
cd gui && bun test tests/provider-account-import.test.tsx— 6 pass, 0 fail, 77 assertions.PASS:
cd gui && bun test tests— 755 pass, 0 fail, 3,577 assertions across 134 files.PASS:
cd gui && bun test tests/i18n-locales.test.ts— 8 pass (exact English key set + interpolation placeholders for every locale, including Turkish).PASS:
cd gui && bun run lintandbun run lint:i18n.PASS: GUI TypeScript/Vite build.
PASS:
bun run privacy:scan.PASS:
git diff --check.PENDING: exact-head Cross-platform CI and React Doctor on
c6f0c1caa1574f7bcb57e334961e9569d228afce(queued after force-push).Dashboard screenshot
Security review
This changes OAuth credential admission and persistence and therefore requires explicit maintainer security review before merge. The maintainer-owned
maintainer-sponsoredlabel is present on the PR.Local independent review found no security release blocker. The implementation uses file/stdin-only secret input, exact provider/format admission, bounded parsing, provider-derived identity and project validation before persistence, atomic duplicate upsert, fixed result codes, strict fail-closed API/CLI/GUI projection, request cancellation with a bounded deadline, canary non-leakage tests, and post-write state/cache reconciliation.
Residual P3: cancellation that arrives after the atomic upsert has already started can produce an intentionally ambiguous
import_cancelledoutcome even though the already-validated credential may have committed. The operation remains identity-idempotent and never persists an unvalidated credential. Partial commits now reconcile live stores and invalidate model/routed-model/quota caches before returning the secret-free 408 DTO.Checklist
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
google-antigravitythrough the dashboard or CLI.Documentation
Localization