feat: implement npmMinimalAgeGate and npmPreapprovedPackages config options
#6901
Conversation
|
This is so good, I just started working on this but thought I'd double check no one else had. This feature will move the needle substantially for folks that care 👍 Would be great to see this land |
| const rcEnv: Record<string, any> = {}; | ||
| for (const [key, value] of Object.entries(config)) | ||
| rcEnv[`YARN_${key.replace(/([A-Z])/g, `_$1`).toUpperCase()}`] = Array.isArray(value) ? value.join(`;`) : value; | ||
| rcEnv[`YARN_${key.replace(/([A-Z])/g, `_$1`).toUpperCase()}`] = Array.isArray(value) ? value.join(`,`) : value; |
There was a problem hiding this comment.
This seems like an a bug leftover from a while ago or something. My tests were failing because the array was not being parsed correctly -- they are comma-delimited when passed as YARN_ - see
berry/packages/yarnpkg-core/sources/Configuration.ts
Lines 754 to 759 in 5aad466
| const RELEASE_DATE_PACKAGES: Record<string, Record<string, number | string>> = { | ||
| "release-date": { | ||
| "1.0.0": new Date(new Date().getTime() - /* 10 days */ 1000 * 60 * 60 * 24 * 10).toISOString(), | ||
| "1.1.0": new Date(new Date().getTime() - /* 5 days */ 1000 * 60 * 60 * 24 * 5).toISOString(), | ||
| "1.1.1": new Date().toISOString(), | ||
| }, | ||
| "release-date-transitive": { | ||
| "1.0.0": new Date(new Date().getTime() - /* 10 days */ 1000 * 60 * 60 * 24 * 10).toISOString(), | ||
| "1.1.0": new Date(new Date().getTime() - /* 5 days */ 1000 * 60 * 60 * 24 * 5).toISOString(), | ||
| "1.1.1": new Date().toISOString(), | ||
| }, | ||
| "@scoped/release-date": { | ||
| "1.0.0": new Date(new Date().getTime() - /* 10 days */ 1000 * 60 * 60 * 24 * 10).toISOString(), | ||
| "1.1.0": new Date(new Date().getTime() - /* 5 days */ 1000 * 60 * 60 * 24 * 5).toISOString(), | ||
| "1.1.1": new Date().toISOString(), | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
open to a better way of storing/computing this metadata.
The only other thought I had was to store a delta (e.g. 5 days) in the fixture package.json metadata and calculate the registry response as now - delta from that. Less hardcoding that way.
There was a problem hiding this comment.
The way you implemented it seems good to me 👍
| function extractInferenceParametersFromRequest(request: Descriptor): InferenceParameters { | ||
| if (request.range === `unknown`) | ||
| return {type: `resolve`, range: `latest`}; | ||
| return {type: `resolve`, range: `*`}; |
There was a problem hiding this comment.
See eccba6c for context on this. Without this, add on new packages or up wouldn't resolve correctly if the latest package was newer than the minimum age.
Not sure what kind of implications (performance or otherwise) this might have on those operations or others.
There was a problem hiding this comment.
Hm this is a little problematic - some packages like to release new versions without immediately turning them latest.
Could we rather have a check in the tag resolver so that if the tag version is too new it picks the highest version that's lower than the tag and matches the gates?
| structUtils.stringifyIdent(descriptor) === exclude | ||
| || structUtils.stringifyLocator(structUtils.makeLocator(descriptor, version)) === exclude | ||
| || structUtils.stringifyLocator(structUtils.makeLocator(descriptor, `${PROTOCOL}:${version}`)) === exclude | ||
| || micromatch.isMatch(structUtils.stringifyDescriptor({...descriptor, range: rawRange}), exclude) | ||
| || micromatch.isMatch(structUtils.stringifyDescriptor(descriptor), exclude), |
There was a problem hiding this comment.
I think I covered the bases here, but let me know if there are other cases to be considered, or any comparison functions that might already exist.
There was a problem hiding this comment.
Shouldn't we just do the glob matching, perhaps after normalizing the glob to turn foo into foo@*?
There was a problem hiding this comment.
I'm not sure. I was thinking users could specify:
- scope + name - @aws-sdk/types
- locator, with or without the protocol prefix - @aws-sdk/[email protected] OR @aws-sdk/types@npm:1.2.3
- descriptor, again, with our without the protocol prefix - @aws-sdk/types@^1.2.3 OR @aws-sdk/types@^npm:1.2.3
- any glob that would match on the descriptor. so
@aws-sdk/*,@aws-sdk/types@*or evenaws*.
I think it would be more complicated to condense these down into a narrower set of checks that it would be to just enumerate them.
There was a problem hiding this comment.
I think I'd prefer to start with those two:
- An ident
- A descriptor with a semver range (and only that; no
npm:at all), just like peer dependency ranges
I suspect checking on arbitrary descriptors / locators will be more confusing than not, based on the experience we had with the resolutions field.
bienzaaron
changed the title
minimumNpmReleaseAge and minimumNpmReleaseAgeExclude config optionsnpmMinimumReleaseAge and npmMinimumReleaseAgeExclude config options
arcanis
left a comment
arcanis
left a comment
There was a problem hiding this comment.
Outstanding work, thanks! I left a couple of requests, let me know what you think.
| npmMinimumReleaseAge: { | ||
| description: `Minimum age of a package version according to the publish date on the npm registry in minutes to be considered for installation`, | ||
| type: SettingsType.NUMBER, | ||
| default: 0, | ||
| }, | ||
| npmMinimumReleaseAgeExclude: { | ||
| description: `Array of package name glob patterns to exclude from the minimum release age check`, | ||
| type: SettingsType.STRING, | ||
| isArray: true, | ||
| default: [], | ||
| }, |
There was a problem hiding this comment.
Can you move these settings into plugin-npm?
| type: SettingsType.STRING, | ||
| default: `throw`, | ||
| }, | ||
| npmMinimumReleaseAge: { |
There was a problem hiding this comment.
| npmMinimumReleaseAge: { | |
| npmMinimalAgeGate: { |
I could envision multiple types of gates.
| type: SettingsType.NUMBER, | ||
| default: 0, | ||
| }, | ||
| npmMinimumReleaseAgeExclude: { |
There was a problem hiding this comment.
| npmMinimumReleaseAgeExclude: { | |
| npmPreapprovedPackages: { |
Same reason, if there were multiple gates we'd want a single setting to bypass them all.
| const RELEASE_DATE_PACKAGES: Record<string, Record<string, number | string>> = { | ||
| "release-date": { | ||
| "1.0.0": new Date(new Date().getTime() - /* 10 days */ 1000 * 60 * 60 * 24 * 10).toISOString(), | ||
| "1.1.0": new Date(new Date().getTime() - /* 5 days */ 1000 * 60 * 60 * 24 * 5).toISOString(), | ||
| "1.1.1": new Date().toISOString(), | ||
| }, | ||
| "release-date-transitive": { | ||
| "1.0.0": new Date(new Date().getTime() - /* 10 days */ 1000 * 60 * 60 * 24 * 10).toISOString(), | ||
| "1.1.0": new Date(new Date().getTime() - /* 5 days */ 1000 * 60 * 60 * 24 * 5).toISOString(), | ||
| "1.1.1": new Date().toISOString(), | ||
| }, | ||
| "@scoped/release-date": { | ||
| "1.0.0": new Date(new Date().getTime() - /* 10 days */ 1000 * 60 * 60 * 24 * 10).toISOString(), | ||
| "1.1.0": new Date(new Date().getTime() - /* 5 days */ 1000 * 60 * 60 * 24 * 5).toISOString(), | ||
| "1.1.1": new Date().toISOString(), | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
The way you implemented it seems good to me 👍
| function extractInferenceParametersFromRequest(request: Descriptor): InferenceParameters { | ||
| if (request.range === `unknown`) | ||
| return {type: `resolve`, range: `latest`}; | ||
| return {type: `resolve`, range: `*`}; |
There was a problem hiding this comment.
Hm this is a little problematic - some packages like to release new versions without immediately turning them latest.
Could we rather have a check in the tag resolver so that if the tag version is too new it picks the highest version that's lower than the tag and matches the gates?
| const shouldExclude = minimumReleaseAgeExclude.some(exclude => | ||
| structUtils.stringifyIdent(descriptor) === exclude | ||
| || structUtils.stringifyLocator(structUtils.makeLocator(descriptor, version)) === exclude | ||
| || structUtils.stringifyLocator(structUtils.makeLocator(descriptor, `${PROTOCOL}:${version}`)) === exclude | ||
| || micromatch.isMatch(structUtils.stringifyDescriptor({...descriptor, range: rawRange}), exclude) | ||
| || micromatch.isMatch(structUtils.stringifyDescriptor(descriptor), exclude), | ||
| ); | ||
| if (!shouldExclude) { | ||
| const versionTime = new Date(registryData.time[version]); | ||
| const ageMinutes = (new Date().getTime() - versionTime.getTime()) / 60 / 1000; | ||
| if (ageMinutes < minimumReleaseAge) { | ||
| return miscUtils.mapAndFilter.skip; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you move that logic into an npmConfigUtils helper?
| structUtils.stringifyIdent(descriptor) === exclude | ||
| || structUtils.stringifyLocator(structUtils.makeLocator(descriptor, version)) === exclude | ||
| || structUtils.stringifyLocator(structUtils.makeLocator(descriptor, `${PROTOCOL}:${version}`)) === exclude | ||
| || micromatch.isMatch(structUtils.stringifyDescriptor({...descriptor, range: rawRange}), exclude) | ||
| || micromatch.isMatch(structUtils.stringifyDescriptor(descriptor), exclude), |
There was a problem hiding this comment.
Shouldn't we just do the glob matching, perhaps after normalizing the glob to turn foo into foo@*?
This reverts commit eccba6c.
…es not pass gates
|
I slightly tweaked the code; instead of checking the original descriptor (ie whatever was in the Waiting for the tests to run one last time then it'll be good to merge and you'll be able to run it by using |
|
Released in 4.10 - thanks a lot ! |
|
Does it also support transitive dependencies? |
clemyan
changed the title
npmMinimumReleaseAge and npmMinimumReleaseAgeExclude config optionsnpmMinimalAgeGate and npmPreapprovedPackages config options
4 participants
What's the problem this PR addresses?
closes #6899. See rationale in pnpm/pnpm#9921 and pnpm/pnpm#9957, but the tl;dr is that with the recent uptick in compromised npm packages, this can offer some level of protection to prevent end-users from installing malware prior to detection and removal from registries.
There are a few differences from the pnpm implementation. I felt these differences made sense with some of the other features
yarnsupports, but I also understand the desire for parity between package managers, so open to thoughts there.npmadded in the option names (npmMinimumReleaseAgeversusminimumReleaseAge). Since yarn implements many resolvers and this is only implemented in the case of the npm resolver, I felt that this made the behavior of the options more clear.npmMinimumReleaseAgeExcludesupports not only package names like pnpm's implementation, but it also supports:@aws-sdk/[email protected]or@aws-sdk/types@npm:3.877.0)@aws-sdk/types@^3.0.0,@aws-sdk/types@npm:^3.0.0,@aws-sdk/types@*or@aws-sdk/*)The rationale here is mostly in the case that you know certain package versions that are affected (e.g.
[email protected]) or if you need to upgrade to an excluded version, but its part of a monorepo -- that's where the micromatch glob comes in handy.How did you fix it?
I added the options and checked for them within the NPM semver resolver when evaluating candidates. I'm new to this codebase -- I think I've made all the updates needed and was able to test some scenarios successfully (see below).
Testing manually
yarn add @aws-sdk/[email protected]fails ✅.yarnrc.ymlinstall command
yarn add @aws-sdk/types@^3.0.0resolves prior version ✅.yarnrc.ymlinstall command
yarn add @aws-sdk/types@^3.0.0resolves most recent version with package exclude =@aws-sdk/types✅.yarnrc.ymlinstall command - installs
@aws-sdk/[email protected]✅yarn add @aws-sdk/types@^3.0.0resolves most recent version with package exclude =@aws-sdk/*✅.yarnrc.ymlinstall command - installs
@aws-sdk/[email protected]✅yarn add @aws-sdk/types@^3.0.0resolves most recent version with package exclude =@aws-sdk/[email protected]✅.yarnrc.ymlinstall command - installs
@aws-sdk/[email protected]✅yarn add @aws-sdk/types@^3.0.0resolves most recent version with package exclude =@aws-sdk/types@^3.0.0✅.yarnrc.ymlinstall command - installs
@aws-sdk/[email protected]✅yarn add @aws-sdk/types@^3.5.0resolves prior version with package exclude =@aws-sdk/types@^3.0.0✅.yarnrc.ymlinstall command - installs
@aws-sdk/[email protected]✅yarn add @aws-sdk/types@^3.5.0resolves prior version with package exclude =@aws-sdk/types@npm:^3.0.0✅.yarnrc.ymlinstall command - installs
@aws-sdk/[email protected]✅❯ yarn add @aws-sdk/types@^3.5.0 ➤ YN0000: · Yarn 4.9.4-git.20250917.hash-a05df867e ➤ YN0000: ┌ Resolution step ➤ YN0085: │ + @aws-sdk/types@npm:3.862.0, @smithy/types@npm:4.5.0, tslib@npm:2.8.1 ➤ YN0000: └ Completed ➤ YN0000: ┌ Fetch step ➤ YN0000: └ Completed ➤ YN0000: ┌ Link step ➤ YN0000: └ Completed ➤ YN0000: · Done in 0s 223ms ❯ yarn why @aws-sdk/types └─ test-yarn-project@workspace:. └─ @aws-sdk/types@npm:3.862.0 (via npm:^3.5.0)yarn add @aws-sdk/types@^3.0.0resolves most recent version with package exclude =@aws-sdk/types@npm:^3.0.0✅.yarnrc.ymlinstall command - installs
@aws-sdk/[email protected]✅Checklist