Skip to content

fix(gui): restore OpenAI account setup paths - #445

Closed
apple-ouyang wants to merge 1 commit into
lidge-jun:devfrom
apple-ouyang:codex/fix-openai-provider-entry
Closed

fix(gui): restore OpenAI account setup paths#445
apple-ouyang wants to merge 1 commit into
lidge-jun:devfrom
apple-ouyang:codex/fix-openai-provider-entry

Conversation

@apple-ouyang

@apple-ouyang apple-ouyang commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Fixes #443

Summary

  • always show OpenAI in Providers → Add Provider → Accounts, even when providers.openai is missing
  • let both the Accounts picker and Codex Auth restore a missing provider through the existing canonical preset API
  • re-enable a disabled OpenAI provider with PATCH { disabled: false }, preserving Direct/Pool mode and saved model settings
  • bypass resolved-DNS destination rejection only for the exact canonical built-in OpenAI seed, so Clash fake-IP DNS does not block setup while custom-provider SSRF checks remain unchanged

Validation

  • bun test tests/provider-payload.test.ts — 13 passed
  • bun test ./gui/tests/codex-auth-provider-enable.test.tsx — 1 passed
  • bun test tests/management-provider-validation.test.ts --test-name-pattern 'accepts canonical OpenAI modes|fake-IP DNS' — 2 passed
  • bun run typecheck
  • bun run lint:gui
  • bun run build:gui
  • browser-tested against an isolated source instance:
    • missing OpenAI → Enable OpenAI → canonical Pool provider created
    • disabled Direct provider → Enable OpenAI → provider enabled and Direct mode preserved
    • Providers → Add Provider → Accounts shows OpenAI

Local full-suite note

bun run prepush was attempted. On this macOS machine, Clash fake-IP DNS maps the existing test domains (for example api.example.test) into 198.18.0.0/15, so five pre-existing custom-provider management tests are rejected by the destination policy. The focused fake-IP boundary tests above pass; GitHub CI should provide the authoritative full-suite result under standard DNS.

Security review

This touches the authentication setup path and destination-policy boundary. Please apply the repository's explicit maintainer security review requirement. The DNS exception is limited to name === "openai" plus the existing strict canonical seed validator; custom providers still execute resolved destination validation.

Summary by CodeRabbit

  • New Features

    • Added an “Enable OpenAI” action when the OpenAI provider is unavailable or disabled, including “Enabling...” progress and error messaging.
    • Updated the Codex auth flow to recover OpenAI availability and refresh provider configuration after login.
    • Added localized OpenAI enablement text (English, German, Japanese, Korean, Russian, Chinese).
  • Bug Fixes

    • Canonical OpenAI provider setup is no longer blocked by destination validation.
  • Tests

    • Added/extended coverage for OpenAI enablement, provider enable/reactivation behavior, and provider validation scenarios.

@github-actions github-actions Bot added the bug Something isn't working label Jul 25, 2026
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds localized OpenAI enablement text, provider discovery and recovery helpers, canonical OpenAI validation handling, and UI flows that create or re-enable the provider before Codex authentication.

Changes

OpenAI provider recovery

Layer / File(s) Summary
Provider discovery and provisioning
gui/src/provider-payload.ts, tests/provider-payload.test.ts
OpenAI is always ordered first; missing providers are created from presets, disabled providers are re-enabled, and request behavior is tested.
Canonical OpenAI validation exception
src/server/management/provider-routes.ts, tests/management-provider-validation.test.ts
Destination resolution is bypassed only for the canonical OpenAI provider; custom providers remain validated.
Codex Auth and provider picker recovery
gui/src/pages/CodexAuth.tsx, gui/src/pages/Providers.tsx, gui/src/i18n/*.ts, gui/tests/codex-auth-provider-enable.test.tsx
The UI adds enable, busy, error, login-preparation, refresh, and localized translation states, with banner rendering tests.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Providers
  participant ensureOpenAiProvider
  participant ProviderAPI
  participant CodexLogin
  Providers->>ensureOpenAiProvider: prepare OpenAI account provider
  ensureOpenAiProvider->>ProviderAPI: create or re-enable OpenAI
  ProviderAPI-->>ensureOpenAiProvider: provider result
  ensureOpenAiProvider-->>Providers: preparation complete
  Providers->>CodexLogin: open Codex login flow
  CodexLogin-->>Providers: account added
  Providers->>Providers: refresh config, OAuth, quotas, and models
Loading

Suggested reviewers: lidge-j, wibias, ingwannu

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: restoring OpenAI account setup and recovery paths in the GUI.
Linked Issues check ✅ Passed The changes in gui/src/pages/Providers.tsx, gui/src/pages/CodexAuth.tsx, gui/src/provider-payload.ts, and src/server/management/provider-routes.ts satisfy #443.
Out of Scope Changes check ✅ Passed The added i18n strings, tests, and route/helper changes all support the OpenAI recovery flow and do not appear unrelated.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

chatgpt-codex-connector[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

原因:
- OpenAI account credentials can outlive the built-in provider, leaving no GUI path to restore routing.
- Disabled legacy or API-key OpenAI rows must not be treated as canonical Codex account providers.
- Clash fake-IP DNS can reject the exact canonical OpenAI preset after canonical seed validation.

改动:
- Keep OpenAI visible in the unified account picker and add in-place recovery on Codex Auth.
- Create missing canonical providers, re-enable only canonical disabled providers, and reject noncanonical rows without overwriting them.
- Preserve destination checks for custom providers and cover provider-state, banner-state, test isolation, and fake-IP boundaries.
@apple-ouyang
apple-ouyang force-pushed the codex/fix-openai-provider-entry branch from d4b66ef to ba5d9ca Compare July 25, 2026 09:59
coderabbitai[bot]

This comment was marked as outdated.

@lidge-jun

Copy link
Copy Markdown
Owner

🔒 Under maintainer review — detailed feedback incoming

@lidge-jun (maintainer) has this PR in an active review pass. Please do not merge or close
it
until the detailed review lands; a full comment with specific file:line findings,
failure modes, and suggested fixes is being prepared.

This PR is not part of the current integration batch. It is held back deliberately —
either because it touches a security boundary that MAINTAINERS.md requires an explicit
security review for, because it needs to be split into reviewable units, or because it needs
a rebase onto changes landing in dev right now.

Heads-up on rebasing: dev is receiving a coordinated integration batch today. If this PR
touches the same files, expect to rebase after that batch lands. The upcoming review comment
will name the exact overlapping paths so you only have to do it once.

No action needed from you until then. Thanks for the contribution and for your patience.

Review tracker: devlog/_plan/260725_pr_issue_rework · marker posted by the maintainer review pass

@lidge-jun

Copy link
Copy Markdown
Owner

Thank you for restoring the OpenAI account setup paths. The disabled-provider recovery logic currently bypasses the stricter canonical-provider validation and can re-enable an unusable OpenAI row.

At gui/src/pages/CodexAuth.tsx:72-73, codexAccountModeState treats any disabled providers.openai object as the built-in provider before openAiAccountProviderState verifies the canonical adapter/base URL/auth contract. The server's disabled-only PATCH path has the same problem: it can toggle that malformed row back on without canonicalizing or rejecting it. The UI can therefore report a recovered Codex account path while routing remains broken or points at a non-canonical OpenAI configuration.

Please make disabled rows pass the same openAiAccountProviderState canonical checks before they are considered built-in, and make the server validate/canonicalize the complete OpenAI row before a disabled-only re-enable is persisted. Add a regression with a disabled openai entry carrying a wrong adapter/base URL/auth mode and prove the UI does not offer the built-in recovery path and the PATCH cannot reactivate it unchanged.

This changes authentication/provider-validation behavior, so it requires explicit maintainer security review. I appreciate the recovery UX work; canonical validation has to remain the single gate for both enabled and disabled rows.

Please also rebase onto current dev, which now includes #449's provider-workspace integration in gui/src/pages/Providers.tsx and the newly added dashboard i18n keys; resolve against those current surfaces.


Reviewed as part of a maintainer integration pass. dev moved to ebc62d1f today (7 PRs integrated); Cross-platform CI is green on ubuntu, macOS, and Windows.

Repository owner deleted a comment from coderabbitai Bot Jul 25, 2026
@Wibias
Wibias marked this pull request as draft July 25, 2026 19:47
@Wibias

Wibias commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Thanks @apple-ouyang — this was a solid recovery UX fix.

We cherry-picked your #445 commits onto current dev and addressed the maintainer review (canonical checks for disabled rows in Codex Auth + server reject of noncanonical disabled-only re-enable) in:

#469

You're credited in the commit (Co-authored-by), PR summary, and changelog note. Once #469 lands, this PR can be closed as superseded.

@Wibias Wibias closed this Jul 25, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

Thank you for spotting and fixing the broken OpenAI account setup path — the problem you identified is real and worth fixing. Before this can land, there's a security defect in the recovery gate that I need to flag directly.

High — a disabled, noncanonical OpenAI endpoint can be re-enabled and then receive forwarded account credentials. gui/src/provider-payload.ts:28 decides a row is recoverable from adapter and authMode alone and never checks baseUrl. src/server/management/provider-routes.ts:238 then deliberately skips merged-shape and destination validation for a disabled-only patch. So a row like {adapter: "openai-responses", authMode: "forward", baseUrl: "https://attacker.example", disabled: true} receives PATCH {disabled: false}, goes live, and OpenAI account credentials are subsequently forwarded to a destination the user never authorized. The fix is to require the normalized canonical ChatGPT Codex URL in the GUI and independently call the canonical OpenAI predicate in the server-side re-enable path, so neither layer trusts the other.

Policy — this area also needs explicit security review. gui/src/provider-payload.ts:99 and src/server/management/provider-routes.ts:238 change authentication recovery and destination-policy behavior, which MAINTAINERS.md gates behind maintainer security approval even after the defect above is repaired.

Recommendation: close this in favor of #469. #469 carries the identical original patch — the patch IDs match, so your authorship is preserved — and adds exactly the base-URL gate and server-side re-enable validation described above, plus regressions for malformed disabled rows. Keeping both open risks someone reviewing and landing the weaker implementation by mistake. This branch is also 129 commits behind the current baseline and still a draft, so rebasing it to reach the same end state would duplicate work already done in #469.

Test gap that allowed this: no regression uses a disabled openai row with a canonical adapter and auth mode but a noncanonical baseUrl, and no backend test proves disabled-only re-enablement performs canonical validation. Green CI here reflects that missing coverage rather than a safe gate.

Genuinely appreciate the contribution — the diagnosis was right, and it's carried forward in #469.


Reviewed as part of a maintainer review pass against dev at bef0dfbe, snapshot 2026-07-26T08:35+09:00, at head ba5d9ca1.

Wibias added a commit to Wibias/opencodex that referenced this pull request Jul 25, 2026
Cherry-picked lidge-jun#445 from @apple-ouyang (Ouyang Xingyuan) onto current
dev, then require the same adapter/baseUrl/authMode checks for disabled
rows in Codex Auth and reject noncanonical openai re-enable via PATCH.

Co-authored-by: Ouyang Xingyuan <galaxyroaming@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants