Skip to content

[deps]: Update @yao-pkg/pkg to v6.11.0#953

Closed
r-tome wants to merge 2 commits intomainfrom
ac/pm-24898/update-yao-pkg-to-v6
Closed

[deps]: Update @yao-pkg/pkg to v6.11.0#953
r-tome wants to merge 2 commits intomainfrom
ac/pm-24898/update-yao-pkg-to-v6

Conversation

@r-tome
Copy link
Copy Markdown
Contributor

@r-tome r-tome commented Dec 17, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-24898

📔 Objective

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 16.01%. Comparing base (bb16544) to head (3be7420).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #953   +/-   ##
=======================================
  Coverage   16.01%   16.01%           
=======================================
  Files          67       67           
  Lines        2791     2791           
  Branches      481      481           
=======================================
  Hits          447      447           
  Misses       2306     2306           
  Partials       38       38           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 17, 2025

Logo
Checkmarx One – Scan Summary & Details7256513b-35ec-4e9d-9782-2d2843c2c228

New Issues (3)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
HIGH CVE-2025-13639 Npm-electron-39.2.1
detailsDescription: Inappropriate implementation in WebRTC in Google Chrome prior to 143.0.7499.41 allowed a remote attacker to perform arbitrary read/write via a craf...
Attack Vector: NETWORK
Attack Complexity: LOW

ID: Mlb9grDoUmgUsa%2FF7uxQ%2FRup6kR8tRiXpByNNNf%2FdEk%3D
Vulnerable Package
MEDIUM CVE-2025-13632 Npm-electron-39.2.1
detailsDescription: Inappropriate implementation in DevTools in Google Chrome prior to 143.0.7499.41 allowed an attacker who convinced a user to install a malicious ex...
Attack Vector: NETWORK
Attack Complexity: LOW

ID: K%2FgB42vEt7eKcC%2BGqoI7ezyxPs%2BnIpSRS5wpTVyJNS4%3D
Vulnerable Package
LOW CVE-2025-13640 Npm-electron-39.2.1
detailsDescription: Inappropriate implementation in Passwords in Google Chrome prior to 143.0.7499.41 allowed a local attacker to bypass authentication via physical ac...
Attack Vector: PHYSICAL
Attack Complexity: LOW

ID: 8hJ511PQPDVyuhTmNhG%2BQaB8KkjjwTmPbFiwLczfgTM%3D
Vulnerable Package

@r-tome r-tome requested a review from eliykat December 17, 2025 15:49
@r-tome r-tome marked this pull request as ready for review December 17, 2025 15:49
@r-tome r-tome requested a review from a team as a code owner December 17, 2025 15:49
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Dec 17, 2025

Claude finished @r-tome's task in 4m 14s —— View job


Code Review Complete

I've reviewed the dependency updates for @yao-pkg/pkg (v5.16.1 → v6.5.1) and Node.js (v20 → v22).

✅ What's Correct

The PR correctly implements the required changes per the VULN-251 security vetting:

  • ✅ @yao-pkg/pkg upgraded to exactly v6.5.1 (as required)
  • ✅ .nvmrc updated to v22
  • ✅ package.json engines field updated to node ~22
  • ✅ Electron 39.2.1 already uses Node 22 (properly aligned)
  • ✅ package-lock.json properly updated with all dependencies

❌ Critical Issue - Changes Requested

README.md line 51 currently states:

- [Node.js](https://nodejs.org) v18 (LTS)

This must be updated to reflect the new Node.js requirement:

- [Node.js](https://nodejs.org) v22 (LTS)

Without this change, developers following the README will install the wrong Node version and encounter build failures due to the engines constraint in package.json.

⚠️ Testing Recommendations

  1. Native Module Compatibility: Verify keytar@7.9.0 works with Node 22:

    • Run npm run reset && npm run rebuild
    • Test credential storage functionality in the desktop app
    • Build and test CLI packages: npm run dist:cli
  2. Full Test Suite: Run integration tests to ensure directory connectors work correctly with Node 22

  3. CI/CD Validation: Confirm all GitHub Actions workflows pass (they should automatically use Node 22 from .nvmrc)

📝 Optional Improvements

Consider adding a comment in package.json near the @yao-pkg/pkg dependency explaining the VULN-251 security requirement for version 6.5.1. This prevents accidental version changes and documents why this specific version is locked.

🔒 Security Assessment

This upgrade follows proper security vetting procedures. The specific version (6.5.1) should be maintained and not upgraded without re-vetting per VULN-251.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Dec 17, 2025

Overall Assessment: REQUEST CHANGES

Important Issues:

  • package.json:52 - Removal of macOS ARM64 (Apple Silicon) support needs justification and documentation

Additional Questions:

  • PR description objective section is empty - should explain the purpose of this update

See inline comments for details.

Copy link
Copy Markdown
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

This is a special case, so we have some specific requirements around this upgrade:

  • We need to upgrade to yao-pkg/pkg@6.5.1 exactly - this package has been vetted at that version. See https://bitwarden.atlassian.net/browse/VULN-251 for more context.
  • We also need to upgrade to node v22, because that is the version of node that has been vetted in that package. That can be done by updating .nvmrc in the root of the repo. Also update engines in package.json.
  • This determines the node version for CLI, but it's also worth checking the version of node used by Electron - ideally they should match. We're using Electron 39.2.1 which already uses node 22, so no changes required there.

@r-tome r-tome force-pushed the ac/pm-24898/update-yao-pkg-to-v6 branch from 996833c to 3be7420 Compare December 18, 2025 11:07
@r-tome
Copy link
Copy Markdown
Contributor Author

r-tome commented Dec 19, 2025

Keytar does not support node 22

@r-tome r-tome closed this Dec 19, 2025
@r-tome r-tome deleted the ac/pm-24898/update-yao-pkg-to-v6 branch December 19, 2025 11:01
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