Skip to content

[Snyk] Fix for 2 vulnerabilities#97055

Open
MelvinBot wants to merge 2 commits into
mainfrom
snyk-fix-3ec3b4cdc0381172408c94029916b22c
Open

[Snyk] Fix for 2 vulnerabilities#97055
MelvinBot wants to merge 2 commits into
mainfrom
snyk-fix-3ec3b4cdc0381172408c94029916b22c

Conversation

@MelvinBot

@MelvinBot MelvinBot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Fixed Issues

$
PROPOSAL:

Tests

  • Verify that no errors appear in the JS console

Offline tests

QA Steps

// TODO: These must be filled out, or the issue title must include "[No QA]."

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

@MelvinBot
MelvinBot requested a review from a team as a code owner July 26, 2026 19:03
@MelvinBot

Copy link
Copy Markdown
Contributor Author

Merge Risk: High

This release includes major version upgrades for critical development tools including ESLint, Jest, and Glob, each introducing significant breaking changes. Action is required to ensure compatibility, particularly for ESLint's new configuration system and Jest's updated API.

Top 3 Most Impactful Upgrades

  • eslint 9.36.010.0.0 (High Risk)
    This major upgrade requires migrating from the legacy .eslintrc format to the modern eslint.config.js (flat config) system. The old format is no longer supported.

    Key Breaking Changes:

    • Node.js Requirement: Support for Node.js versions older than v20.19.0 has been dropped.
    • Configuration Overhaul: The .eslintrc configuration system and related CLI flags (--rulesdir, --ignore-path) have been completely removed.
    • New Config Lookup: ESLint now locates eslint.config.js by searching up from the directory of each linted file, which improves behavior in monorepos.
    • API Removals: Several deprecated SourceCode and Linter methods have been removed, which may affect custom rules and integrations.

    Recommendation: Before merging, run the official migration script (npx @eslint/migrate-config) to convert your configuration to the new flat config format. Ensure your development and CI environments are using Node.js v20.19.0 or newer.

  • jest & jest-cli 29.7.030.0.0 (High Risk)
    Jest 30 introduces breaking changes related to environment support, API methods, and dependencies. Snapshots may require updating due to formatting changes.

    Key Breaking Changes:

    • Node.js Requirement: Support for Node.js 14, 16, 19, and 21 is dropped. The minimum required version is now 18.
    • Matcher Aliases Removed: Deprecated expect matcher aliases (e.g., toBeCalledWith) have been removed. A codemod is available to automate the replacement.
    • JSDOM Upgrade: The jest-environment-jsdom package was upgraded to jsdom v26, which may cause subtle changes in DOM-related tests.
    • CLI Flag Renamed: The --testPathPattern flag has been renamed to --testPathPatterns.

    Recommendation: Verify your project is running on a supported Node.js version. Use eslint-plugin-jest or a codemod to automatically fix the removed matcher aliases in your codebase. Review tests that rely on specific DOM behavior after the upgrade.

  • glob 10.5.012.0.0 (Medium Risk)
    This upgrade spans two major versions and fully resolves a security vulnerability present in earlier versions.

    Key Breaking Changes:

    • Security Fix: Version 12 provides a complete fix for a command injection vulnerability (CVE-2025-64756) by removing the unsafe --shell option from the CLI. Previous versions (including v

Notice 🤖: This content was augmented using artificial intelligence. AI-generated content may contain errors and should be reviewed for accuracy before use.

@melvin-bot
melvin-bot Bot requested a review from Gonals July 26, 2026 19:03
@melvin-bot

melvin-bot Bot commented Jul 26, 2026

Copy link
Copy Markdown

@Gonals Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@melvin-bot
melvin-bot Bot removed the request for review from a team July 26, 2026 19:03

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e317e6cd1

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread package.json
"diff-so-fancy": "^1.3.0",
"dotenv": "^16.0.3",
"eslint": "^9.36.0",
"eslint": "^10.0.0",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Regenerate the dependency lockfile

These four version changes leave package-lock.json unchanged: its root entries still request ESLint 9.36, glob 10.4, Jest 29.7, and jest-cli 29.7 and resolve those old majors. I checked the install step in .github/actions/composite/setupNode/action.yml, which runs npm ci; npm's ci documentation explicitly states that it exits with an error when lockfile dependencies do not match package.json. Consequently, clean CI installs fail, while cached installs continue using the old dependency tree and do not apply the advertised vulnerability fixes.

Useful? React with 👍 / 👎.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

verify check failure — root cause

The verify job fails at the dependency-install step with ERESOLVE unable to resolve dependency tree (run log):

npm error Found: eslint@10.8.0
npm error   dev eslint@"^10.0.0" from the root project
npm error Could not resolve dependency:
npm error peer eslint@"^7.0.0 || ^8.0.0 || ^9.0.0" from eslint-plugin-jsdoc@60.8.3

This is caused directly by this PR. Snyk bumped eslint to ^10.0.0, but the repo pins several plugins to eslint 9:

  • eslint-plugin-jsdoc@^60.7.0 declares peer eslint@"^7.0.0 || ^8.0.0 || ^9.0.0" — it does not accept eslint 10.
  • The overrides block itself force-pins eslint@^9.36.0 for eslint-plugin-react-native-a11y, which now contradicts the top-level ^10.0.0.

On top of that, the PR also bumps glob^12.0.0 and jest/jest-cli30.0.0 (breaking majors), and package-lock.json was never regenerated — it still resolves the old tree, including the exact brace-expansion@1.1.11 this PR is meant to patch. So the change also doesn't actually achieve its security goal.

Why Snyk did this

The two advisories (SNYK-JS-BRACEEXPANSION-18313044, SNYK-JS-JSYAML-18313070) are for transitive packages. Snyk tried to pull patched versions in indirectly by bumping the dev tools that depend on them (eslint, glob, jest), but those major bumps are incompatible with the repo's pinned eslint-9 plugins, so npm can't resolve the tree at all.

Recommended fix

The clean, low-risk fix is to not do the breaking major bumps and instead patch the transitive packages directly:

  1. Revert eslint, glob, jest, and jest-cli in package.json back to their pre-PR versions (^9.36.0, ^10.4.5, 29.7.0, 29.7.0).
  2. Add overrides forcing the vulnerable transitive deps to patched versions (e.g. brace-expansion1.1.12/2.0.2, js-yaml → its patched release per the advisory).
  3. Regenerate package-lock.json (npm install) and run ESLint + Jest so the lock is consistent and the check passes.

Alternatively, if the team wants to move to eslint 10 / jest 30 / glob 12 deliberately, that's a real migration — eslint-plugin-jsdoc and the other eslint-9-capped plugins/overrides would need to be bumped to eslint-10-compatible versions first, then the lockfile regenerated and the full lint/test suite fixed up.

Given the breaking scope and the fact that the lockfile wasn't updated, closing this Snyk PR in favor of a targeted overrides-based patch is likely the better path.

I wasn't able to push the fix myself: safely resolving a dependency-tree failure requires regenerating package-lock.json and re-running the ESLint/Jest suites, and this environment has no npm-registry access to do or verify that. An unverified lockfile change would risk breaking the PR further.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

I can't push an automated fix for this one, and it's worth explaining why rather than pushing something unverified.

The root cause is already detailed in my earlier comment: Snyk bumped eslint^10.0.0 (plus glob^12, jest/jest-cli30), but the repo pins eslint-plugin-jsdoc and an overrides entry to eslint 9, so npm can't resolve the tree — and package-lock.json was never regenerated (it still contains the vulnerable brace-expansion@1.1.11 this PR was meant to patch).

A correct fix here cannot be a package.json-only edit: it requires regenerating package-lock.json and running the ESLint + Jest suites to verify. My environment has no npm-registry access to do or verify that, so anything I pushed would be an unverified lockfile change that risks breaking the PR further — which I won't do.

Recommended path (needs a human with a working npm):

  1. Revert eslint, glob, jest, jest-cli back to their pre-PR versions.
  2. Add overrides forcing just the vulnerable transitive deps to patched releases (brace-expansion^1.1.12/^2.0.2; js-yaml → its patched version per SNYK-JS-JSYAML-18313070).
  3. npm install to regenerate the lockfile, then run lint + tests.

Given the breaking scope and that the lockfile wasn't updated, closing this Snyk PR in favor of that targeted overrides patch is likely the cleaner move.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Re-checked on this retry — the situation is unchanged, so I won't repeat the full write-up.

Root cause and recommended fix are already covered in my root-cause analysis and follow-up above: Snyk's eslint^10, glob^12, jest/jest-cli30 bumps can't resolve against the repo's eslint-9-pinned plugins, and package-lock.json was never regenerated.

I still can't push a verified fix from here: this needs package-lock.json regenerated and the ESLint + Jest suites re-run, and npm has no registry access in my environment — so anything I pushed would be an unverified lockfile change. The fix needs a human with a working npm to run the steps in my earlier comment (revert the four breaking bumps, add targeted overrides for brace-expansion/js-yaml, npm install, then lint + test), or to close this Snyk PR in favor of that targeted overrides patch.

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