Skip to content

fix(plugin-npm): fix tag regex #1271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 30, 2020
Merged

fix(plugin-npm): fix tag regex #1271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea9c591
fix(plugin-npm): fix tag regex
paul-soporan Apr 30, 2020
65b8ad8
chore(release-workflow): set releases
paul-soporan Apr 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions .yarn/versions/ff3089cd.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
releases:
"@yarnpkg/cli": prerelease
"@yarnpkg/core": prerelease
"@yarnpkg/plugin-npm": prerelease

declined:
- "@yarnpkg/plugin-compat"
- "@yarnpkg/plugin-constraints"
- "@yarnpkg/plugin-dlx"
- "@yarnpkg/plugin-essentials"
- "@yarnpkg/plugin-exec"
- "@yarnpkg/plugin-file"
- "@yarnpkg/plugin-git"
- "@yarnpkg/plugin-github"
- "@yarnpkg/plugin-http"
- "@yarnpkg/plugin-init"
- "@yarnpkg/plugin-interactive-tools"
- "@yarnpkg/plugin-link"
- "@yarnpkg/plugin-node-modules"
- "@yarnpkg/plugin-npm-cli"
- "@yarnpkg/plugin-pack"
- "@yarnpkg/plugin-patch"
- "@yarnpkg/plugin-pnp"
- "@yarnpkg/plugin-stage"
- "@yarnpkg/plugin-typescript"
- "@yarnpkg/plugin-version"
- "@yarnpkg/plugin-workspace-tools"
- "@yarnpkg/builder"
- "@yarnpkg/doctor"
- "@yarnpkg/pnpify"
14 changes: 6 additions & 8 deletions packages/plugin-npm/sources/NpmTagResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import {ReportError, MessageName, Resolver, ResolveOptions, MinimalResolveOptions} from '@yarnpkg/core';
import {structUtils} from '@yarnpkg/core';
import {Descriptor, Locator, Package} from '@yarnpkg/core';
import {ReportError, MessageName, Resolver, ResolveOptions, MinimalResolveOptions, TAG_REGEXP} from '@yarnpkg/core';
import {structUtils} from '@yarnpkg/core';
import {Descriptor, Locator, Package} from '@yarnpkg/core';

import {NpmSemverFetcher} from './NpmSemverFetcher';
import {PROTOCOL} from './constants';
import * as npmHttpUtils from './npmHttpUtils';

export const TAG_REGEXP = /^(?!v)[a-z0-9-]+$/i;
import {NpmSemverFetcher} from './NpmSemverFetcher';
import {PROTOCOL} from './constants';
import * as npmHttpUtils from './npmHttpUtils';

export class NpmTagResolver implements Resolver {
supportsDescriptor(descriptor: Descriptor, opts: MinimalResolveOptions) {
Expand Down
2 changes: 1 addition & 1 deletion packages/yarnpkg-core/sources/ProtocolResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {Resolver, ResolveOptions, MinimalResolveOptions} from './Resolver';
import * as structUtils from './structUtils';
import {Descriptor, Locator, DescriptorHash, Package} from './types';

export const TAG_REGEXP = /^[a-z]+$/;
export const TAG_REGEXP = /^(?!v)[a-z0-9-\.]+$/i;
Copy link
Member

Choose a reason for hiding this comment

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

There's no reason dist-tags can't start with v

As far as I can see there's two limitations to dist-tags:

  • Tags may not have any characters that encodeURIComponent encodes. I copy pasted this from an npm error when trying to use a dist-tag with an @ in it. According to MDN that limits dist-tags to /[A-Za-z0-9_.!~*'()-]+/
  • Tags that can be interpreted as valid semver ranges will be rejected. (cf. https://docs.npmjs.com/cli/dist-tag)

The string v't*e!s(t) is a valid dist tag:

$ npm --registry http://localhost:4873 add @foo/test@"v't*e!s(t)"
npm WARN test No description
npm WARN test No repository field.
npm WARN test No license field.

+ @foo/[email protected]
updated 1 package and audited 1 package in 0.825s
found 0 vulnerabilities

Copy link
Member

Choose a reason for hiding this comment

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

The documentation recommends against:

The simplest way to avoid semver problems with tags is to use tags that do not begin with a number or the letter v.

Which ironically, was the case of the issue this PR closed (the problematic tag was starting with a number). I guess theoretically the foolproof way would be to do something like this:

^(?!v?[0-9]+(?:\.[0-9]+){2})

That starts to be pretty verbose though ...

Copy link
Member

@larixer larixer Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

the foolproof way would be to do something like this:

^(?!v?[0-9]+(?:\.[0-9]+){2})

That starts to be pretty verbose though ...

If we will do it this way we will match recommendation, but not the supported semantics which will break some packages that do not follow recommendation. Do we really want this?

Copy link
Member

@bgotink bgotink Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Or we could step away from regular expressions altogether?

function isValidTag(value: string) {
  return encodeURIComponent(value) === value && !semver.validRange(value);
}

Copy link
Member

@arcanis arcanis Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I find that overly broad. In particular, I'm considering a way to depend on workspaces by their name, and for now my candidate syntax is ~pkg-name, which would conflict with such a logic.

Frankly I'm fine with not supporting one-of-a-kind tags like v't*e!s(t), which are kinda asking for trouble anyway. The main thing is ensuring that semver versions aren't detected as tag... Maybe this?

const TAG_REGEXP = /^[a-z0-9][a-z0-9.-]*$/i;

function isValidTag(value: string) {
  return TAG_REGEXP.test(value) && !semver.valid(value);
}

Copy link
Member

Choose a reason for hiding this comment

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

I see two different things here: npm dist-tags in general and package descriptors using a dist-tag but don't have a protocol.

For the npm: protocol I would consider it dangerous to deviate from what the npm registry provides. In other words: I would expect yarn add "@foo/test@npm:v't*e!s(t)" to work, even though it's an esoteric contrived example.

Using npm dist-tags without protocol can definitely be more strict, it's a shorthand that uses sane defaults most people will be happy with. If you need something different, e.g. because you're using an esoteric dist-tag, you can always specify the protocol explicitly.

This way we don't break any dependency—every valid npm dist-tag can be used in berry—but we give berry more freedom in the protocol resolver.

Copy link
Member Author

@paul-soporan paul-soporan May 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @bgotink's idea.
Let's make the ProtocolResolver test tags using /^[a-z0-9-\.]+$/i or some other regex, while the NpmTagResolver can check using the function @bgotink proposed:

function isValidTag(value: string) {
  return encodeURIComponent(value) === value && !semver.validRange(value);
}

This way we can make the ProtocolResolver support more tag formats, as, for example, if we ever decide to create a PyPI Resolver / Fetcher (I've already played with the idea and created functioning Resolver and Fetcher prototypes a few days ago), Python uses a different version format (pep440), so using !semver.valid(value) might cause unepected problems for people that would set defaultProtocol to pypi: (or whatever other protocol would be used) for example.


export class ProtocolResolver implements Resolver {
supportsDescriptor(descriptor: Descriptor, opts: MinimalResolveOptions) {
Expand Down
1 change: 1 addition & 0 deletions packages/yarnpkg-core/sources/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export {AllDependencies, HardDependencies, Manifest, DependencyMeta, PeerDepende
export {MessageName} from './MessageName';
export {CommandContext, Hooks, Plugin} from './Plugin';
export {Project} from './Project';
export {TAG_REGEXP} from './ProtocolResolver';
export {ReportError, Report} from './Report';
export {Resolver, ResolveOptions, MinimalResolveOptions} from './Resolver';
export {StreamReport} from './StreamReport';
Expand Down