Chore: Upgrade jest-environment-jsdom and auth0.js #2682
Chore: Upgrade jest-environment-jsdom and auth0.js #2682ankita10119 wants to merge 10 commits intomasterfrom
Conversation
…t, function or class Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Claude finished @ankita10119's task —— View job PR Review CompleteI've completed a comprehensive review of this PR. Here's my assessment: Overall Assessment❌ This PR is not ready to merge - Critical issues must be addressed: Critical Issues (Must Fix)
Major Concerns (Should Fix)
Positive Changes
Recommendations
Security AssessmentThe PR achieves its primary goal of fixing the form-data vulnerability through dependency upgrades. However, the cheerio downgrade introduces new security concerns that offset some of these gains. Detailed feedback provided in inline comments above. |
| targets: { | ||
| esmodules: false |
There was a problem hiding this comment.
Security & Maintenance Risk: No explicit Node.js target specified
By only setting esmodules: false without specifying a concrete Node.js version target, the transpilation behavior becomes less predictable and depends on Babel's default browserslist configuration. This can lead to:
- Security risk: No explicit lower bound means the code may not transpile newer syntax that could cause runtime errors on older environments
- Inconsistent builds: Different developers/CI environments might produce different output depending on their browserslist database version
While I understand from the review comments that you're keeping this approach to maintain ES2017 compatibility for the es-check validation, I recommend:
- Either add an explicit
node: '8.0'target (as discussed in comments) OR - Update the project to drop Node 8 support and set a modern target like
node: '14', then update the es-check script accordingly
The current configuration creates an implicit dependency on browserslist defaults which reduces build reproducibility.
| "enzyme": { | ||
| "cheerio": "0.22.0" | ||
| } |
There was a problem hiding this comment.
Pinning Cheerio Version: Why 0.22.0?
Cheerio 0.22.0 was released in 2017 and has known security vulnerabilities. Pinning to this specific old version raises concerns:
- Security risk: Very old version likely contains unpatched vulnerabilities
- Why this version?: The comment mentions "maintain compatibility" but doesn't specify what breaks with newer versions
- Enzyme limitation: If this is required for enzyme@3.11.0 compatibility, it indicates enzyme itself may be outdated
Recommendation:
- Document in a comment WHY this specific version is required
- Investigate upgrading to React Testing Library instead of Enzyme (Enzyme is no longer actively maintained)
- If keeping Enzyme, verify if newer cheerio versions work with enzyme 3.11.0
| import { configure } from 'enzyme'; | ||
| import Adapter from '@cfaester/enzyme-adapter-react-18'; | ||
|
|
||
| import { TextEncoder, TextDecoder } from 'util'; |
There was a problem hiding this comment.
Node.js Specific Import in Browser Test Environment
Importing TextEncoder and TextDecoder from Node.js's util module in a browser-focused test environment (jsdom) is a workaround for missing globals.
Concerns:
- Semantic mismatch: You're polyfilling browser APIs with Node.js implementations
- Behavior differences: Node.js TextEncoder/TextDecoder may have subtle behavioral differences from browser implementations
- Better alternatives: Consider using a proper polyfill package like
text-encodingorfast-text-encodingthat provides browser-compliant implementations
Recommendation:
// Instead of Node.js util, use a proper polyfill:
import { TextEncoder, TextDecoder } from 'text-encoding';However, if this is working in practice and jest-environment-jsdom 30.x requires it, this is acceptable with a comment explaining why.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Closing this PR as these changes are covered in another PR - #2690 |
Changes
This PR includes updates to dependencies and test configurations, targeting improved test reliability, security fixes, and compatibility enhancements.
Security Fixes
These changes are primarily aimed at resolving a security vulnerability reported in the transitive dependency
form-data, which was pulled in via:auth0-jsjsdom(used byjest-environment-jsdom)By upgrading:
auth0-jstov9.29.0jest-environment-jsdomtov30.2.0, we eliminate the vulnerable versions and reduce exposure to known issues in the dependency graph.
Dependency & Environment Updates
auth0-jsfromv9.27.0→v9.29.0enzymetov3.11.0with an override forcheerio@0.22.0to maintain compatibility.jest-environment-jsdomtov30.2.0npmrcfile to enforce consistent registry resolution via npmjs.orgTest Setup Enhancements
setWindowLocation()utility usingjsdomto mockwindow.locationin tests.Auth0WebApi.setupClient()to:- Handle same-origin and cross-origin cases.
- Simulate
CordovaandElectronenvironments viawindow.cordova / window.electron.- Updated setup-tests.js to polyfill:
- TextEncoder
- TextDecoder
This ensures better compatibility with libraries relying on these globals (e.g., jsdom > 20).
Known Issues & Fix
Adding an override for
jest-environment-jsdomintroduced a version mismatch withjest-environment-jsdom-global, which was relying on an older version.This mismatch caused tests to fail with global / window inconsistencies.
The issue was resolved by:
jsdomsetup (viasetWindowLocation).No production logic was modified, all changes are test or config related.
Testing
Updated unit test cases to handle these changes.
Checklist