Skip to content

Conversation

DmitriyLewen
Copy link
Contributor

@DmitriyLewen DmitriyLewen commented Mar 12, 2025

Description

We added workspace relationship.

So we can add Root and Workspace packages for yarn.
See #8012 for more details.

example:

➜ ./trivy -q fs /Users/dmitriy/work/repositories/trivy/pkg/fanal/analyzer/language/nodejs/yarn/testdata/project-with-workspace-in-subdir -f json --list-all-pkgs | jq '.Results[].Packages[]'
{
  "ID": "@test/[email protected]",
  "Name": "@test/foo",
  "Identifier": {
    "PURL": "pkg:npm/%40test/[email protected]",
    "UID": "78e9afcafeb460f1"
  },
  "Version": "1.0.0",
  "Licenses": [
    "MIT"
  ],
  "Relationship": "root",
  "DependsOn": [
    "@test/[email protected]"
  ],
  "Layer": {}
}
{
  "ID": "@test/[email protected]",
  "Name": "@test/bar-generators",
  "Identifier": {
    "PURL": "pkg:npm/%40test/[email protected]",
    "UID": "fbc8ab66d16b6431"
  },
  "Version": "0.0.1",
  "Relationship": "workspace",
  "DependsOn": [
    "[email protected]"
  ],
  "Layer": {}
}
{
  "ID": "[email protected]",
  "Name": "hoek",
  "Identifier": {
    "PURL": "pkg:npm/[email protected]",
    "UID": "dbafdf578b704907"
  },
  "Version": "6.1.3",
  "Relationship": "direct",
  "Layer": {},
  "Locations": [
    {
      "StartLine": 5,
      "EndLine": 8
    }
  ]
}

TODO:

  • Update yarn analyzer
  • handle cases when package.json doesn't have name and version
  • remove dev deps from DependsOn, if --include-dev-deps flag is not exist.
  • update unit tests
  • update integration tests
  • update docs

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

- add root package (parse package.json)
- add workspaces (parse workspace package.json files)
- add workspaces as children of root package
@DmitriyLewen DmitriyLewen self-assigned this Mar 12, 2025
@DmitriyLewen DmitriyLewen changed the title feat(yarn): add root and workspace packages feat(nodejs): add root and workspace for yarn packages Mar 12, 2025
- use name|name@version for IDs
- use hash of packagejson.Package if name is empty
- rename types to ftypes
- update tests
- restructure yarn paragraph
- add info about root/workspace packages
Packages: types.Packages{
Packages: ftypes.Packages{
{
ID: "4b1aec5d292e8d22",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we want to add some prefix for hash.

We also will need to use hash for cargo.
So it can be one prefix for all type, or we can relate prefix with type.

@DmitriyLewen DmitriyLewen marked this pull request as ready for review March 14, 2025 12:01
@DmitriyLewen DmitriyLewen requested a review from knqyf263 as a code owner March 14, 2025 12:01
@knqyf263
Copy link
Collaborator

  "ID": "82636613e66143cd",
  "Identifier": {
    "PURL": "pkg:npm/",
    "UID": "976bb38dc3a7a8fb"
  },

The name is empty. Is it intentional? I thought it would be possible to retrieve the workspace name from the package.json within the workspace.

@@ -25,7 +27,7 @@ import (
"github.com/aquasecurity/trivy/pkg/fanal/analyzer"
"github.com/aquasecurity/trivy/pkg/fanal/analyzer/language"
"github.com/aquasecurity/trivy/pkg/fanal/analyzer/language/nodejs/license"
"github.com/aquasecurity/trivy/pkg/fanal/types"
ftypes "github.com/aquasecurity/trivy/pkg/fanal/types"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reasons for renaming it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We often use types and ftypes in the same file, so I'm used to using ftypes.
To make it more uniform - I made this change.

Copy link
Collaborator

@knqyf263 knqyf263 Mar 19, 2025

Choose a reason for hiding this comment

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

Ideally, we should not use pkg/types under fanal as all necessary types for fanal are defined in pkg/fanal/types. So, they will not conflict. Also, I think we can rename it once they actually conflict.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to rename it, I don't mind it, but can we change it in another PR? I want to focus on changes for this enhancement. It currently looks like there are many changes on GitHub, and I need to carefully distinguish which lines are changes due to ftypes and which lines are related to the actual feature additions.

Additionally, for a personal reason, since I am reviewing this while traveling and do not have a large display, I have to scroll a lot to skip the ftypes changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. It's not really important for me.
Also you're right - yarn analyzer package is already in fanal, so it's not necessary to point it out again.

I returned types in 9fb33ff

@DmitriyLewen
Copy link
Contributor Author

The name is empty. Is it intentional? I thought it would be possible to retrieve the workspace name from the package.json within the workspace.

https://docs.npmjs.com/cli/v11/configuring-npm/package-json#name
name and version can be empty ( If you don't plan to publish your package, the name and version fields are optional.).

So possible cases when package.json from workspace doesn't have name/version

"PURL": "pkg:npm/",

But perhaps we need to update logic for purl (don't create purl, if name is empty)

@knqyf263
Copy link
Collaborator

knqyf263 commented Mar 17, 2025

name and version can be empty ( If you don't plan to publish your package, the name and version fields are optional.).

It can be, but is it the default behavior? I mean I'm wondering if the workspace name will be empty by default. I didn't get how it works by default from the document.
https://yarnpkg.com/features/workspaces

@DmitriyLewen
Copy link
Contributor Author

Add the following in a package.json file. Starting from now on, we’ll call this directory the “workspace root”:

IIUC (i didn't find info about cli flags) - you need to add workspace dir into root package.json manually (Add the following in a package.json file. Starting from now on, we’ll call this directory the “workspace root”:).

So you need to create package.json yourself.

yarn has -w flag for init command. But it looks like it same as for init (or broken).

I tested a bit:
yarn init (and yarn init -w) command uses dir name as name, and you can't create empty name.
So but default yarn init create name and version in package.json file (the exception is when you create a file in the root of the system - then you can create an empty name).

But IIRC - i already saw empty names in package.json, so i can say that users still use package.json files with empty name.

@knqyf263
Copy link
Collaborator

If an empty name is an edge case, I think it's better to use a normal case in the documentation and PR examples. Of course, it's okay to mention that empty names are also supported, but if they are not the typical case, it would be better to explain them as an exception.

@DmitriyLewen
Copy link
Contributor Author

in the documentation and PR examples.

I thought you want to change logic for these cases.

About docs - i think your are right. There is no need to confuse users once again.
I changed PR description.

DmitriyLewen and others added 9 commits March 17, 2025 13:44
- don't create purl, if pkg doesn't have name
…ling

- Updated test case naming for consistency.
- Refactored yarn dependency analysis to enhance clarity and functionality.
- Introduced methods for resolving root and workspace dependencies.
- Improved error handling and logging for package.json parsing.
- Removed deprecated dependency structures and streamlined the code.

This refactor aims to enhance the maintainability and performance of the yarn analyzer.
…tionships

- Updated package ID assignment logic to use file paths when package.json lacks a name or version.
- Refactored workspace relationship assignments for improved clarity.
- Removed deprecated methods related to package ID generation.
- Updated the documentation for Yarn dependency analysis to clarify the handling of additional files and package relationships.
- Added sections on development dependencies and license detection to provide comprehensive coverage of Yarn's functionality.
- Improved clarity in the explanation of how Trivy analyzes `package.json` files alongside `yarn.lock`.
@knqyf263
Copy link
Collaborator

In the end, I used the file path as the package ID when the name or version is empty. Using a hash value would also be possible, but since file paths are used in package-lock.json in npm, I thought this approach would be more common in Node.js projects. Cases where the name or version is empty are rare, so let's start with this simple approach for now. If any issues come up, we can switch to using hashes.

@knqyf263 knqyf263 added this pull request to the merge queue Apr 30, 2025
Merged via the queue into aquasecurity:main with commit bf4cd4f Apr 30, 2025
17 checks passed
@DmitriyLewen DmitriyLewen deleted the feat/root-workspace-yarn branch May 5, 2025 10:58
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.

feat(yarn): add workspace as dependencies and use workspace relationship for them
2 participants