Skip to content

fix(auth): remove deprecated OpenAuth dependency - #661

Merged
ndycode merged 2 commits into
ndycode:mainfrom
sussdorff:fix/upstream/remove-deprecated-openauth
Aug 13, 2026
Merged

fix(auth): remove deprecated OpenAuth dependency#661
ndycode merged 2 commits into
ndycode:mainfrom
sussdorff:fix/upstream/remove-deprecated-openauth

Conversation

@sussdorff

@sussdorff sussdorff commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace the sole @openauthjs/openauth/pkce usage with Node's built-in crypto APIs
  • remove OpenAuth and its deprecated Arctic/Oslo dependency chain
  • verify the RFC 7636 verifier constraints and S256 challenge derivation

Motivation

@openauthjs/openauth@0.4.3 declares arctic ^2.2.2 as a peer
dependency. npm therefore installs deprecated versions of arctic and
@oslojs/* for every installation of codex-multi-auth.

codex-multi-auth uses OpenAuth only for generatePKCE. Since the package
already requires Node >=18.17, the same operation can be implemented directly
with node:crypto without carrying the additional dependency chain.

The implementation preserves the existing behavior:

  • 64 cryptographically random bytes
  • base64url-encoded verifier
  • SHA-256 S256 challenge
  • unchanged asynchronous authorization-flow API

Verification

  • npm ci — clean install without Arctic/Oslo deprecation warnings
  • npm test -- test/auth.test.ts — 64 tests passed
  • npm run lint
  • npm run typecheck
  • npx --yes npm@11.6.2 run pack:check — 811,749 bytes across 920 files

The full suite was also attempted locally on macOS with Node 24.17:
5,326 tests passed and 28 unrelated existing platform/wrapper/storage tests
failed. None of the failing files overlap this change.

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

the pr replaces the deprecated openauth pkce helper with node’s built-in cryptographic apis and removes the obsolete dependency chain.

  • generates an rfc 7636 verifier from 64 random bytes and derives its s256 challenge.
  • removes openauth, arctic, oslo, and associated transitive packages.
  • adds focused vitest coverage for verifier constraints and challenge derivation.
  • no concurrency behavior, token persistence, or windows filesystem paths are changed.

Confidence Score: 5/5

the pr appears safe to merge.

no blocking failure remains.

Important Files Changed

Filename Overview
lib/auth/auth.ts replaces the external pkce helper with a node-compatible implementation that preserves the authorization-flow contract.
test/auth.test.ts directly verifies verifier length, character constraints, and s256 challenge derivation; no relevant vitest coverage gap remains.
package.json removes the sole direct openauth dependency without affecting package exports or runtime requirements.
package-lock.json removes openauth and its deprecated transitive chain while remaining synchronized with the root manifest.

Reviews (2): Last reviewed commit: "test(auth): assert PKCE verifier alphabe..." | Re-trigger Greptile

Context used:

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

this is a minor-risk dependency removal. lib/auth/auth.ts:1 replaces the deprecated @openauthjs/openauth PKCE helper with Node crypto APIs and preserves the asynchronous authorization-flow API. no security or data-loss regression is expected. regression tests in test/auth.test.ts:... cover verifier length, allowed characters, and the s256 challenge.

reviewers should focus on the local PKCE implementation and compatibility across supported runtimes and windows environments. concurrent authorization flows should also be checked for independent verifier state. validation includes 64 auth tests, linting, type checking, package checks, and 5,326 passing tests in the full suite. 28 unrelated platform, wrapper, and storage tests failed.

changes

  • remove @openauthjs/openauth from package.json:....
  • generate 64 random bytes with node:crypto in lib/auth/auth.ts:....
  • encode the verifier with base64url.
  • derive the s256 challenge with sha-256.
  • retain the existing asynchronous authorization-flow behavior.
  • add PKCE verifier and challenge assertions in test/auth.test.ts:....

risks

  • windows-specific behavior remains unverified.
  • no explicit concurrency regression test is reported.
  • full-suite failures remain unrelated according to the validation report, but reviewers should confirm this during review.

Walkthrough

the authorization flow now generates pkce values locally with randomBytes and web crypto sha-256. the external runtime dependency is removed. tests validate verifier constraints and challenge derivation.

Changes

local pkce localization

Layer / File(s) Summary
pkce generation and flow wiring
lib/auth/auth.ts:1, lib/auth/auth.ts:412, lib/auth/auth.ts:430, package.json:174
the authorization flow generates the pkce verifier and challenge locally. the external dependency is removed.
pkce validation coverage
test/auth.test.ts:2, test/auth.test.ts:218
the test validates verifier length, allowed characters, and sha-256 base64url challenge derivation. no windows-specific regression test is added. the change introduces no shared mutable state or concurrency-specific behavior.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: ndycode

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning the description explains the change and validation in lib/auth/auth.ts:1 and test/auth.test.ts:1, but omits required template sections. add the what changed, docs and governance, risk and rollback, and additional notes sections; explicitly record regression tests, windows edge cases, and concurrency risks.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed the title accurately summarizes the dependency removal and pkce replacement in lib/auth/auth.ts:1 and package.json:1.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

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.

❤️ Share

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

@sussdorff
sussdorff marked this pull request as ready for review August 11, 2026 07:28
@sussdorff
sussdorff requested a review from ndycode as a code owner August 11, 2026 07:28
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🤖 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 `@test/auth.test.ts`:
- Around line 218-224: Extend the PKCE verifier assertions in the relevant auth
test to validate that flow.pkce.verifier contains only the unpadded base64url
alphabet generated by the auth implementation, rejecting +, /, and = characters.
Keep the existing deterministic length and SHA-256 challenge assertions
unchanged.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c7022269-4453-44e5-88bf-80352890f73a

📥 Commits

Reviewing files that changed from the base of the PR and between 7f5c61b and 5c498fd.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (3)
  • lib/auth/auth.ts
  • package.json
  • test/auth.test.ts
💤 Files with no reviewable changes (1)
  • package.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (10)
test/**/*.test.ts

📄 CodeRabbit inference engine (test/AGENTS.md)

test/**/*.test.ts: Write Vitest test suites with globals enabled (describe, it, expect)
Maintain 80%+ coverage threshold across statements, branches, functions, and lines
Use removeWithRetry() for Windows filesystem cleanup instead of bare fs.rm to handle EBUSY, EPERM, and ENOTEMPTY errors
Do not rely on dist/ in tests; use source files instead
Do not skip tests without justification
Relax lint rules for test files as configured in eslint.config.js

Files:

  • test/auth.test.ts
test/**/auth.test.ts

📄 CodeRabbit inference engine (test/AGENTS.md)

Test OAuth PKCE flow and JWT decoding in auth.test.ts

Files:

  • test/auth.test.ts
**/*.{ts,js,mjs}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,js,mjs}: Use ESM modules throughout the project; the package is configured with "type": "module".
Do not use as any, @ts-ignore, or @ts-expect-error.

Files:

  • test/auth.test.ts
  • lib/auth/auth.ts
test/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Windows-sensitive filesystem tests and helpers must use retry handling for transient lock-related cleanup and write failures.

Files:

  • test/auth.test.ts
**/*

📄 CodeRabbit inference engine (AGENTS.md)

Source changes belong in index.ts, lib/, and scripts/; dist/ is generated output and local temporary/cache directories must not be edited.

Files:

  • test/auth.test.ts
  • lib/auth/auth.ts
**/*.{js,ts,mjs,cjs}

📄 CodeRabbit inference engine (README.md)

**/*.{js,ts,mjs,cjs}: Do not publish or replace a global codex binary; official OpenAI installation paths must retain ownership of the codex command.
Keep OAuth credentials local and restrict runtime rotation and local bridges to loopback interfaces.
Require hashed local client tokens to protect the optional loopback bridge.
Responses background: true compatibility must remain opt-in; requests using it must use stateful store=true routing rather than stateless store=false routing.
Never run npm install or update commands automatically; only display a manual upgrade notice when appropriate.
Experimental synchronization and backup flows must be non-destructive by default: preview before applying sync, preserve destination-only accounts, and fail safely on backup filename collisions.
Keep account storage project-scoped under the configured multi-auth root when operating in repo-specific workflows.

Files:

  • test/auth.test.ts
  • lib/auth/auth.ts
test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

  • test/auth.test.ts
lib/**/*.ts

📄 CodeRabbit inference engine (lib/AGENTS.md)

lib/**/*.ts: Route all public exports through lib/index.ts or documented package subpaths.
Keep module dependencies acyclic and preserve the layering types/constants → storage → accounts → runtime → manager/CLI; lower layers must not import higher layers.
Preserve runtime rotation pass-through semantics except for intentionally changed auth or provider headers.
Deduplicate emails using normalizeEmailKey(), which trims and lowercases the email.
Use classes for state requiring multiple independent instances or dependency injection, including AccountManager, CircuitBreaker, SessionAffinityStore, and the CodexError hierarchy. Reserve module-level state for genuinely process-global concerns and provide a test reset helper for such state.
Never import from dist/ in source tests or library code.
Never suppress type errors.
Never patch official Codex application binaries for desktop routing.
Never use bare recursive cleanup in Windows-sensitive paths without retry handling.

Files:

  • lib/auth/auth.ts
lib/auth/**/*.ts

📄 CodeRabbit inference engine (lib/AGENTS.md)

Never hardcode OAuth ports; use the existing authentication constants and helpers.

Do not hardcode the OAuth callback port; use existing constants or helpers. The callback port is 1455.

Files:

  • lib/auth/auth.ts
lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

  • lib/auth/auth.ts
🧠 Learnings (2)
📚 Learning: 2026-06-04T06:14:18.093Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/scheduling-strategy-config.test.ts:1-1
Timestamp: 2026-06-04T06:14:18.093Z
Learning: In ndycode/codex-multi-auth, do not flag explicit imports from "vitest" (e.g., describe, it, expect, beforeEach/afterEach, etc.) in test files as issues—even if the Vitest config sets `globals: true`. The repo’s established convention is to keep these imports for consistency with neighboring tests; removing them would make files outliers.

Applied to files:

  • test/auth.test.ts
📚 Learning: 2026-06-04T06:14:24.975Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/runtime-rotation-proxy.test.ts:2478-2491
Timestamp: 2026-06-04T06:14:24.975Z
Learning: In ndycode/codex-multi-auth test files (e.g. `test/*.test.ts`), when creating V3 storage fixtures for accounts, it’s an intentional convention to use `as never` for deliberately minimal stored-account objects that only include `refreshToken`, `addedAt`, and `lastUsed`. Do not treat `as never` here as a type-safety problem: optional/other fields are expected to be populated by the runtime during execution, and the cast is used solely to keep the fixture minimal and consistent across existing tests.

Applied to files:

  • test/auth.test.ts
🔇 Additional comments (2)
lib/auth/auth.ts (1)

1-1: LGTM!

Also applies to: 412-421, 430-430

test/auth.test.ts (1)

2-2: LGTM!

Comment thread test/auth.test.ts
@ndycode
ndycode merged commit 8bbc1e7 into ndycode:main Aug 13, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants