Skip to content

fix(providers): remove OAuth credentials on provider delete #1579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
laerad777 wants to merge 2 commits into from
Closed

fix(providers): remove OAuth credentials on provider delete #1579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf61867
release: v2.14.0
lidge-jun Aug 12, 2026
018a7db
fix(providers): remove OAuth credentials on provider delete
laerad777 Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@bitkyc08/opencodex",
"version": "2.13.0",
"version": "2.14.0",
"description": "Universal provider proxy for OpenAI Codex & Claude Code — use any LLM with Codex CLI/App/SDK and Claude Code",
"type": "module",
"main": "./bin/package-main.mjs",
Expand Down
3 changes: 2 additions & 1 deletion src/server/management/provider-routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
submitManualLoginCode,
upsertOAuthProvider,
} from "../../oauth";
import { removeCredential } from "../../oauth/store";
import { replaceProviderAccountSet } from "../../oauth/store";
import { providerDestinationResolvedError } from "../../lib/destination-policy";
import { reconcileLiveStateStores } from "../../lib/state-store-registrations";
import { ProviderOutboundPolicyError, providerOutboundGet, providerOutboundPost, providerRedirectError } from "../../lib/provider-outbound";
Expand Down Expand Up @@ -765,6 +765,7 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise<Resp
const droppedCustomModels = dropProviderCustomModels(config, name);
setProviderContextCap(config, name, false);
save(config);
await replaceProviderAccountSet(name, null);
Comment on lines 767 to +768

@coderabbitai coderabbitai Bot Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Make config deletion and OAuth-store deletion failure-safe.

save(config) at Line 767 persists the provider removal before replaceProviderAccountSet(name, null) at Line 768 runs. If the OAuth-store write rejects, the request cannot return success after the config is already committed. The deleted provider’s OAuth account set remains on disk, and a retry cannot use this DELETE route because the provider is already absent.

Do not only reverse the call order. That creates the inverse partial commit if config persistence fails. Use a staged deletion or durable recovery record that makes both stores converge. Add a failure-path test.

Based on learnings: separate credential and runtime-config writes require staged deletion or durable multi-file recovery; a simple compensating credential restore can corrupt credential generation and validation metadata.

🤖 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/server/management/provider-routes.ts` around lines 767 - 768, Make the
provider deletion flow around save(config) and replaceProviderAccountSet(name,
null) failure-safe without merely reordering the calls. Use staged deletion or a
durable recovery record so config and OAuth-store state converge after either
write fails, while preserving credential generation and validation metadata; add
a test covering the failing write and recovery behavior.

Source: Learnings

reconcileLiveStateStores();
const { clearModelCache: clearCache } = await import("../../codex/model-cache");
clearCache(name);
Expand Down
46 changes: 46 additions & 0 deletions tests/management-provider-validation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import { installIsolatedCodexHome, type IsolatedCodexHome } from "./helpers/isol
import * as destinationPolicy from "../src/lib/destination-policy";
import { catalogConvergenceFactory } from "./helpers/catalog-convergence";
import { LOCAL_PROVIDER_RELOAD_NAME_HEADER, LOCAL_PROVIDER_RELOAD_PATH } from "../src/lib/local-provider-reload-contract";
import { getAccountSet, saveCredential } from "../src/oauth/store";

// Full-suite Windows load: startServer + multi-step provider PATCH/GET flows exceed the
// default 5s per-test budget (same flake class as 810fa115 / claude-management-api).
Expand Down Expand Up @@ -1453,6 +1454,51 @@ describe("provider management validation", () => {
}
});

test("provider deletion removes the deleted provider's OAuth credential", async () => {
if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true });
mkdirSync(TEST_DIR, { recursive: true });
process.env.OPENCODEX_HOME = TEST_DIR;
saveConfig({
port: 0,
defaultProvider: "test-openai",
providers: {
"test-openai": {
adapter: "openai-chat",
baseUrl: "https://api.example.test/v1",
apiKey: "test-key",
},
removable: {
adapter: "openai-chat",
baseUrl: "https://api.removable.test/v1",
apiKey: "test-key",
},
},
});
await saveCredential("removable", {
access: "credential-to-delete",
refresh: "refresh-to-delete",
expires: Date.now() + 60_000,
});
await saveCredential("retained", {
access: "credential-to-keep",
refresh: "refresh-to-keep",
expires: Date.now() + 60_000,
});

const server = startServer(0);
try {
const response = await fetch(new URL("/api/providers?name=removable", server.url), {
method: "DELETE",
});
expect(response.status).toBe(200);

expect(getAccountSet("removable")).toBeNull();
expect(getAccountSet("retained")).not.toBeNull();
Comment on lines +1477 to +1496

@coderabbitai coderabbitai Bot Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Cover the full account-set and preservation contract.

The test creates only one account for removable, so it does not detect an implementation that deletes only the active account. The test checks only that retained is non-null, so a changed retained credential could also pass.

Save two distinct removable accounts and assert that the full set is null. Then assert that the retained credential values remain unchanged.

The PR objective requires full account-set deletion and preservation of unrelated provider credentials.

Proposed regression coverage 
     await saveCredential("removable", {
+      accountId: "removable-1",
       access: "credential-to-delete",
       refresh: "refresh-to-delete",
       expires: Date.now() + 60_000,
     });
+    await saveCredential("removable", {
+      accountId: "removable-2",
+      access: "second-credential-to-delete",
+      refresh: "second-refresh-to-delete",
+      expires: Date.now() + 60_000,
+    });
...
-      expect(getAccountSet("retained")).not.toBeNull();
+      const retained = getAccountSet("retained");
+      expect(retained).not.toBeNull();
+      expect(retained?.accounts[0]?.credential.access).toBe("credential-to-keep");
+      expect(retained?.accounts[0]?.credential.refresh).toBe("refresh-to-keep");
📝 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.

Suggested change
await saveCredential("removable", {
access: "credential-to-delete",
refresh: "refresh-to-delete",
expires: Date.now() + 60_000,
});
await saveCredential("retained", {
access: "credential-to-keep",
refresh: "refresh-to-keep",
expires: Date.now() + 60_000,
});
const server = startServer(0);
try {
const response = await fetch(new URL("/api/providers?name=removable", server.url), {
method: "DELETE",
});
expect(response.status).toBe(200);
expect(getAccountSet("removable")).toBeNull();
expect(getAccountSet("retained")).not.toBeNull();
await saveCredential("removable", {
accountId: "removable-1",
access: "credential-to-delete",
refresh: "refresh-to-delete",
expires: Date.now() + 60_000,
});
await saveCredential("removable", {
accountId: "removable-2",
access: "second-credential-to-delete",
refresh: "second-refresh-to-delete",
expires: Date.now() + 60_000,
});
await saveCredential("retained", {
access: "credential-to-keep",
refresh: "refresh-to-keep",
expires: Date.now() + 60_000,
});
const server = startServer(0);
try {
const response = await fetch(new URL("/api/providers?name=removable", server.url), {
method: "DELETE",
});
expect(response.status).toBe(200);
expect(getAccountSet("removable")).toBeNull();
const retained = getAccountSet("retained");
expect(retained).not.toBeNull();
expect(retained?.accounts[0]?.credential.access).toBe("credential-to-keep");
expect(retained?.accounts[0]?.credential.refresh).toBe("refresh-to-keep");
🤖 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 `@tests/management-provider-validation.test.ts` around lines 1477 - 1496,
Expand the test around the DELETE request in the provider credential deletion
case to save two distinct accounts under “removable” and assert the entire
account set is null afterward. Capture the original “retained” credential values
and assert its access, refresh, and expiration values remain unchanged, not
merely that the account set exists.

} finally {
await server.stop(true);
}
});

test("provider deletion removes stale provider context caps", async () => {
if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true });
mkdirSync(TEST_DIR, { recursive: true });
Expand Down
Loading