Skip to content
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

fix: use URL instead of url.parse #141

Merged
merged 2 commits into from
Sep 5, 2023
Merged

fix: use URL instead of url.parse #141

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93c7ef0
fix: use URL instead of url.parse
wraithgar Aug 24, 2023
ef82ddf
fix: tweak setGitCommittish
wraithgar Aug 25, 2023
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
95 changes: 46 additions & 49 deletions lib/npa.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module.exports.resolve = resolve
module.exports.toPurl = toPurl
module.exports.Result = Result

const url = require('url')
const { URL } = require('url')
const HostedGit = require('hosted-git-info')
const semver = require('semver')
const path = global.FAKE_WINDOWS ? require('path').win32 : require('path')
Expand Down Expand Up @@ -183,10 +183,11 @@ Result.prototype.toJSON = function () {
return result
}

function setGitCommittish (res, committish) {
// sets res.gitCommittish, res.gitRange, and res.gitSubdir
function setGitAttrs (res, committish) {
if (!committish) {
res.gitCommittish = null
return res
return
}

// for each :: separated item:
Expand Down Expand Up @@ -224,8 +225,6 @@ function setGitCommittish (res, committish) {
}
log.warn('npm-package-arg', `ignoring unknown key "${name}"`)
}

return res
}

function fromFile (res, where) {
Expand All @@ -245,8 +244,8 @@ function fromFile (res, where) {
const rawWithPrefix = prefix + res.rawSpec
let rawNoPrefix = rawWithPrefix.replace(/^file:/, '')
try {
resolvedUrl = new url.URL(rawWithPrefix, `file://${path.resolve(where)}/`)
specUrl = new url.URL(rawWithPrefix)
resolvedUrl = new URL(rawWithPrefix, `file://${path.resolve(where)}/`)
specUrl = new URL(rawWithPrefix)
} catch (originalError) {
const er = new Error('Invalid file: URL, must comply with RFC 8909')
throw Object.assign(er, {
Expand All @@ -260,17 +259,17 @@ function fromFile (res, where) {
// XXX backwards compatibility lack of compliance with RFC 8909
if (resolvedUrl.host && resolvedUrl.host !== 'localhost') {
const rawSpec = res.rawSpec.replace(/^file:\/\//, 'file:///')
resolvedUrl = new url.URL(rawSpec, `file://${path.resolve(where)}/`)
specUrl = new url.URL(rawSpec)
resolvedUrl = new URL(rawSpec, `file://${path.resolve(where)}/`)
specUrl = new URL(rawSpec)
rawNoPrefix = rawSpec.replace(/^file:/, '')
}
// turn file:/../foo into file:../foo
// for 1, 2 or 3 leading slashes since we attempted
// in the previous step to make it a file protocol url with a leading slash
if (/^\/{1,3}\.\.?(\/|$)/.test(rawNoPrefix)) {
const rawSpec = res.rawSpec.replace(/^file:\/{1,3}/, 'file:')
resolvedUrl = new url.URL(rawSpec, `file://${path.resolve(where)}/`)
specUrl = new url.URL(rawSpec)
resolvedUrl = new URL(rawSpec, `file://${path.resolve(where)}/`)
specUrl = new URL(rawSpec)
rawNoPrefix = rawSpec.replace(/^file:/, '')
}
// XXX end RFC 8909 violation backwards compatibility section
Expand Down Expand Up @@ -303,7 +302,8 @@ function fromHostedGit (res, hosted) {
res.hosted = hosted
res.saveSpec = hosted.toString({ noGitPlus: false, noCommittish: false })
res.fetchSpec = hosted.getDefaultRepresentation() === 'shortcut' ? null : hosted.toString()
return setGitCommittish(res, hosted.committish)
setGitAttrs(res, hosted.committish)
return res
}

function unsupportedURLType (protocol, spec) {
Expand All @@ -312,62 +312,59 @@ function unsupportedURLType (protocol, spec) {
return err
}

function matchGitScp (spec) {
// git ssh specifiers are overloaded to also use scp-style git
// specifiers, so we have to parse those out and treat them special.
// They are NOT true URIs, so we can't hand them to `url.parse`.
//
// This regex looks for things that look like:
// git+ssh://[email protected]:username/project.git#deadbeef
//
// ...and various combinations. The username in the beginning is *required*.
const matched = spec.match(/^git\+ssh:\/\/([^:#]+:[^#]+(?:\.git)?)(?:#(.*))?$/i)
return matched && !matched[1].match(/:[0-9]+\/?.*$/i) && {
fetchSpec: matched[1],
gitCommittish: matched[2] == null ? null : matched[2],
}
}

function fromURL (res) {
// eslint-disable-next-line node/no-deprecated-api
const urlparse = url.parse(res.rawSpec)
res.saveSpec = res.rawSpec
let rawSpec = res.rawSpec
res.saveSpec = rawSpec
if (rawSpec.startsWith('git+ssh:')) {
// git ssh specifiers are overloaded to also use scp-style git
// specifiers, so we have to parse those out and treat them special.
// They are NOT true URIs, so we can't hand them to URL.

// This regex looks for things that look like:
// git+ssh://[email protected]:username/project.git#deadbeef
// ...and various combinations. The username in the beginning is *required*.
const matched = rawSpec.match(/^git\+ssh:\/\/([^:#]+:[^#]+(?:\.git)?)(?:#(.*))?$/i)
if (matched && !matched[1].match(/:[0-9]+\/?.*$/i)) {
res.type = 'git'
setGitAttrs(res, matched[2])
res.fetchSpec = matched[1]
return res
}
} else if (rawSpec.startsWith('git+file://')) {
// URL can't handle windows paths
rawSpec = rawSpec.replace(/\\/g, '/')
}
const parsedUrl = new URL(rawSpec)
// check the protocol, and then see if it's git or not
switch (urlparse.protocol) {
switch (parsedUrl.protocol) {
case 'git:':
case 'git+http:':
case 'git+https:':
case 'git+rsync:':
case 'git+ftp:':
case 'git+file:':
case 'git+ssh:': {
case 'git+ssh:':
res.type = 'git'
const match = urlparse.protocol === 'git+ssh:' ? matchGitScp(res.rawSpec)
: null
if (match) {
setGitCommittish(res, match.gitCommittish)
res.fetchSpec = match.fetchSpec
setGitAttrs(res, parsedUrl.hash.slice(1))
if (parsedUrl.protocol === 'git+file:' && /^git\+file:\/\/[a-z]:/i.test(rawSpec)) {
// URL can't handle drive letters on windows file paths, the host can't contain a :
res.fetchSpec = `git+file://${parsedUrl.host.toLowerCase()}:${parsedUrl.pathname}`
} else {
setGitCommittish(res, urlparse.hash != null ? urlparse.hash.slice(1) : '')
Copy link
Member Author

Choose a reason for hiding this comment

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

the hash attribute of a URL instance is always set to a string object.

urlparse.protocol = urlparse.protocol.replace(/^git[+]/, '')
if (urlparse.protocol === 'file:' && /^git\+file:\/\/[a-z]:/i.test(res.rawSpec)) {
// keep the drive letter : on windows file paths
urlparse.host += ':'
urlparse.hostname += ':'
}
delete urlparse.hash
res.fetchSpec = url.format(urlparse)
parsedUrl.hash = ''
Copy link
Member Author

Choose a reason for hiding this comment

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

delete does not work on the URL instance's hash attribute like it did with url.parse

res.fetchSpec = parsedUrl.toString()
}
if (res.fetchSpec.startsWith('git+')) {
res.fetchSpec = res.fetchSpec.slice(4)
Copy link
Member Author

Choose a reason for hiding this comment

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

protocol is not able to be changed in a URL instance like it was in a url.parse result.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that might be related to nodejs/node#49319 - it's supposed to be changeable.

Copy link
Member Author

Choose a reason for hiding this comment

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

file: is a special spec so changing from git+file: (non special) to file: (special) appears to be forbidden.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, it seems that the spec forbids it, but all browsers allow it, which means the spec will likely change to allow it.

Copy link
Member Author

@wraithgar wraithgar Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Browsers do not obey the spec and also break in fun ways when you do change the spec

Firefox:

x = new URL("git+file://a")
URL { href: "git+file://a", origin: "null", protocol: "git+file:", username: "", password: "", host: "", hostname: "", port: "", pathname: "//a", search: "" }

x.toString()
"git+file://a"
x.protocol = "file:"
"file:"
x.toString()
"file:///"

Node silently ignores the setter with no warnings or errors (which I suppose is to spec since it just says return).

node.js

> x = new URL('git+file://a')
URL {
  href: 'git+file://a',
  origin: 'null',
  protocol: 'git+file:',
  username: '',
  password: '',
  host: 'a',
  hostname: 'a',
  port: '',
  pathname: '',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
> x.toString()
'git+file://a'
> x.protocol = 'file:'
'file:'
> x.protocol
'git+file:'
> x.toString()
'git+file://a'

Copy link
Contributor

Choose a reason for hiding this comment

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

lol, that is fun. the .protocol accessor correctly updates tho, even if the double slashes and default port behavior ends up being inconsistent.

(obv none of this helps this particular migration)

}
break
}
case 'http:':
case 'https:':
res.type = 'remote'
res.fetchSpec = res.saveSpec
break

default:
throw unsupportedURLType(urlparse.protocol, res.rawSpec)
throw unsupportedURLType(parsedUrl.protocol, rawSpec)
}

return res
Expand Down