Skip to content

Conversation

@aks96
Copy link
Contributor

@aks96 aks96 commented Nov 17, 2025

Description

This PR upgrades express-openid-connect to use openid-client v6.8.1 while maintaining zero breaking changes for end users. The migration includes comprehensive Node.js 18+ compatibility with Web API polyfills and enhanced CI reliability.

  • openid-client: 4.9.1 → 6.8.1
  • nock: 11.9.1 → 14.0.10
  • undici: 5.29.0 (Web API polyfills)
  • Node.js engines: >=14 → >=18.0.0

Changes

  • Complete client rewrite - changed to adapt to v6 functional API
  • Custom TokenSet implementation - Compatibility layer for removed v6 class
  • Web API polyfills - Added fetch/crypto polyfills for Node.js 18 compatibility
  • ESM import handling - Dynamic imports with race condition protection
  • Enhanced CI reliability - Fixed GitHub Actions issues with dependency management

Backward Compatibility

  • Zero breaking changes for library users
  • All existing APIs work identically
  • Same configuration options and behavior

New Feature & Enhancement

  • Enhanced client authentication - Full support for all OAuth 2.1 methods
  • PAR support - Pushed Authorization Requests implementation
  • Better localhost development - HTTP bypass for local testing
  • Improved error handling - v4-compatible error messages
  • Performance improvements - Optimized client caching and timeouts

@aks96 aks96 requested a review from a team as a code owner November 17, 2025 18:53
@aks96 aks96 marked this pull request as draft November 17, 2025 18:58
@aks96 aks96 marked this pull request as ready for review November 18, 2025 12:54
@gyaneshgouraw-okta
Copy link
Contributor

@claude

@github-actions
Copy link

github-actions bot commented Nov 18, 2025

Claude finished @gyaneshgouraw-okta's task —— View job


Code Review Complete

Tasks

  • Read PR diff and understand changes
  • Read key modified files for deeper context
  • Perform code quality review
  • Perform security review
  • Perform performance review
  • Provide inline feedback for specific issues
  • Provide overall assessment

Overall Assessment

❌ This PR has critical security issues that must be addressed before merging, particularly around global state management and authentication validation.

Critical Issues:

  • Race condition in global fetch handling (lib/client.js:136-137) - Multiple concurrent requests will interfere with each other, potentially causing one client's configuration to be used for another's authentication flow
  • Insufficient state and nonce validation (lib/client.js:447-449, 467-469) - Does not properly enforce CSRF protection required by OpenID Connect specification
  • JWT parsing without validation (lib/tokenSet.js:16-19) - Decodes JWT claims without signature verification, creating potential security vulnerabilities

Security Concerns:

  • Global namespace pollution with crypto/fetch polyfills could conflict with other libraries
  • Automatic localhost HTTP bypass without explicit opt-in configuration flag
  • Silent error swallowing masks security-relevant events (tampering, replay attacks)
  • Test code in production paths increases attack surface

Code Quality Issues:

  • Significant code duplication in JWT assertion creation
  • Incomplete error handling loses valuable debugging context
  • Magic numbers and hardcoded test values in production code
  • Missing validation for token expiration values

Recommendations:

  1. Fix the critical global state race condition by using per-request fetch configuration instead of mutating globals
  2. Strengthen OpenID Connect validation to meet specification requirements for state and nonce
  3. Add explicit validation and error logging for security-relevant failures
  4. Refactor duplicated authentication code into shared helper functions
  5. Remove or isolate test-specific code from production code paths

While the migration from v4 to v6 is architecturally sound and the backward compatibility approach is well-designed, the implementation has security and reliability issues that must be resolved before production use.


Job Run | Branch: SDK-6785

Comment on lines +868 to +875
headers: Object.fromEntries(response.headers.entries()),
body: body,
// Include the original response for advanced usage
response,
};
},

async introspect() {

Choose a reason for hiding this comment

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

Code Quality: Missing Error Context

The error handling removes valuable debugging context by only preserving error.message. The original error stack trace, error code, and other properties are lost, making production debugging difficult.

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.

3 participants