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

Jump to bottom

[BUG] Failed to decode path name with valid symbols #193

Open
1 task done
andrew-aladjev opened this issue Sep 12, 2024 · 6 comments · May be fixed by #200
Open
1 task done

[BUG] Failed to decode path name with valid symbols #193

andrew-aladjev opened this issue Sep 12, 2024 · 6 comments · May be fixed by #200
Labels
Bug thing that needs fixing Needs Triage needs an initial review

Comments

@andrew-aladjev
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

Parent issue: npm/cli#7779

cd /tmp
mkdir ' !"$%&'\''()*+,-.0123456789:;<=>@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~'
cd ' !"$%&'\''()*+,-.0123456789:;<=>@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~'
npm pack

4 silly config load:file:/tmp/ !"$%&'()*+,-.0123456789:;<=>@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~/.npmrc
5 silly config load:file:/home/puchuu/.npmrc
6 silly config load:file:/home/puchuu/.local/share/fnm/node-versions/v20.16.0/installation/etc/npmrc
7 verbose title npm pack
8 verbose argv "pack"
9 verbose logfile logs-max:10 dir:/home/puchuu/.npm/_logs/2024-09-12T12_18_03_676Z-
10 verbose logfile /home/puchuu/.npm/_logs/2024-09-12T12_18_03_676Z-debug-0.log
11 silly logfile start cleaning logs, removing 1 files
12 verbose stack URIError: URI malformed
12 verbose stack     at decodeURIComponent (<anonymous>)
12 verbose stack     at fromFile (/home/puchuu/.local/share/fnm/node-versions/v20.16.0/installation/lib/node_modules/npm/node_modules/npm-package-arg/lib/npa.js:279:22)

It is not possible to work with symbols available in path with URI. I don't know why you are trying to use URI encode and decode.

Expected Behavior

No response

Steps To Reproduce

No response

Environment

  • npm: 10.8.1
  • Node: v20.16.0
  • OS: Ubuntu 24.04.1 LTS
@andrew-aladjev andrew-aladjev added Bug thing that needs fixing Needs Triage needs an initial review labels Sep 12, 2024
@andrew-aladjev andrew-aladjev changed the title [BUG] Failed to decode path names with valid symbols [BUG] Failed to decode path name with valid symbols for path Sep 12, 2024
@andrew-aladjev andrew-aladjev changed the title [BUG] Failed to decode path name with valid symbols for path [BUG] Failed to decode path name with valid symbols Sep 12, 2024
@andrew-aladjev
Copy link
Author

Can you please describe what does let resolvedPath = decodeURIComponent(resolvedUrl.pathname) mean? pathname can include any ascii symbol except / and it can't be decoded using decodeURIComponent. Why are you using decodeURIComponent here?

@wraithgar
Copy link
Member

The reason is because this is parsing a URL object

    resolvedUrl = new URL(rawWithPrefix, `file://${path.resolve(where)}/`)

Normally pathname is URI encoded

> new URL('file:///test dir')
URL {
  href: 'file:///test%20dir',
  origin: 'null',
  protocol: 'file:',
  username: '',
  password: '',
  host: '',
  hostname: '',
  port: '',
  pathname: '/test%20dir',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
> new URL('file:test dir')
URL {
  href: 'file:///test%20dir',
  origin: 'null',
  protocol: 'file:',
  username: '',
  password: '',
  host: '',
  hostname: '',
  port: '',
  pathname: '/test%20dir',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

This actually looks like it could be a node bug. (new URL).pathname is supposed to be URI encoded, and able to be URI decoded.

> new URL(`file:///tmp/ !"$%&'()*+,-.0123456789:;<=>@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_\`abcdefghijklmnopqrstuvwxyz{|}~`)
URL {
  href: "file:///tmp/%20!%22$%&'()*+,-.0123456789:;%3C=%3E@ABCDEFGHIJKLMNOPQRSTUVWXYZ[/]^_%60abcdefghijklmnopqrstuvwxyz%7B|%7D~",
  origin: 'null',
  protocol: 'file:',
  username: '',
  password: '',
  host: '',
  hostname: '',
  port: '',
  pathname: "/tmp/%20!%22$%&'()*+,-.0123456789:;%3C=%3E@ABCDEFGHIJKLMNOPQRSTUVWXYZ[/]^_%60abcdefghijklmnopqrstuvwxyz%7B|%7D~",
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
> decodeURIComponent("/tmp/%20!%22$%&'()*+,-.0123456789:;%3C=%3E@ABCDEFGHIJKLMNOPQRSTUVWXYZ[/]^_%60abcdefghijklmnopqrstuvwxyz%7B|%7D~")
Uncaught URIError: URI malformed
    at decodeURIComponent (<anonymous>)
> decodeURIComponent((new URL(`file:///tmp/ !"$%&'()*+,-.0123456789:;<=>@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_\`abcdefghijklmnopqrstuvwxyz{|}~`)).pathname)
Uncaught URIError: URI malformed
    at decodeURIComponent (<anonymous>)

@wraithgar
Copy link
Member

This would be an issue w/ ecmascript itself, since this error also exists in browsers. The issue here is that % is not encoded in the pathname, and since it doesn't have a pair and is not a legitimate escape we get this error.

@wraithgar
Copy link
Member

Looks like this issue is handled in node via fileURLToPath

@ljharb
Copy link
Contributor

ljharb commented Dec 10, 2024 •
edited
Loading

(URL isn't in ecmascript, it's in WHATWG and WinterCG)

@wraithgar
Copy link
Member

The root cause here is that new URL doesn't actually uri encode. It looks like it does cause it encodes some things. But the new URL will happily parse a string whose components will fail URI decoding.

> new URL('http://foo.bar/path%name space')
URL {
  href: 'http://foo.bar/path%name%20space',
> encodeURI('http://foo.bar/path%name space')
'http://foo.bar/path%25name%20space'

new URL is not sufficient here.

wraithgar added a commit that referenced this issue Dec 16, 2024
@wraithgar
fix: properly parse paths with non-url characters
f41822e 
Also removed the magic windows global for testing, fixing the tests to mock process.platform instead.

Closes: #193
wraithgar added a commit that referenced this issue Dec 16, 2024
@wraithgar
fix: properly parse paths with non-url characters
d6b7cd5 
Also removed the magic windows global for testing, fixing the tests to mock process.platform instead.

Closes: #193
wraithgar added a commit that referenced this issue Dec 18, 2024
@wraithgar
fix: properly parse non-url encoded file specs
6e6f8e9 
Properly creates file package args that contain characters that need to be url encoded.

There is also a refactor/cleanup in here
 - Removed the magic windows global for testing, fixing the tests to mock process.platform instead.
 - Moved inline regexes up to where the others are defined
 - Renamed a few variables to be more correct (i.e. isFilename to isFileType)
 - Refactored Result to be a proper Class instead of a function w/ prototypes

Closes: #193
@wraithgar wraithgar linked a pull request Dec 18, 2024 that will close this issue
Open
wraithgar added a commit that referenced this issue Dec 20, 2024
@wraithgar
fix: properly parse non-url encoded file specs
7a06c88 
Properly creates file package args that contain characters that need to be url encoded.

There is also a refactor/cleanup in here
 - Removed the magic windows global for testing, fixing the tests to mock process.platform instead.
 - Moved inline regexes up to where the others are defined
 - Renamed a few variables to be more correct (i.e. isFilename to isFileType)
 - Refactored Result to be a proper Class instead of a function w/ prototypes

Closes: #193
wraithgar added a commit that referenced this issue Dec 20, 2024
@wraithgar
fix: properly parse non-url encoded file specs
3e28f1e 
Properly creates file package args that contain characters that need to be url encoded.

There is also a refactor/cleanup in here
 - Removed the magic windows global for testing, fixing the tests to mock process.platform instead.
 - Moved inline regexes up to where the others are defined
 - Renamed a few variables to be more correct (i.e. isFilename to isFileType)
 - Refactored Result to be a proper Class instead of a function w/ prototypes

Closes: #193
wraithgar added a commit that referenced this issue Dec 20, 2024
@wraithgar
fix: properly parse non-url encoded file specs
ce6c05b 
Properly creates file package args that contain characters that need to be url encoded.

There is also a refactor/cleanup in here
 - Removed the magic windows global for testing, fixing the tests to mock process.platform instead.
 - Moved inline regexes up to where the others are defined
 - Renamed a few variables to be more correct (i.e. isFilename to isFileType)
 - Refactored Result to be a proper Class instead of a function w/ prototypes

Closes: #193
wraithgar added a commit that referenced this issue Dec 20, 2024
@wraithgar
fix: properly parse non-url encoded file specs
3c876a6 
Properly creates file package args that contain characters that need to be url encoded.

There is also a refactor/cleanup in here
 - Removed the magic windows global for testing, fixing the tests to mock process.platform instead.
 - Moved inline regexes up to where the others are defined
 - Renamed a few variables to be more correct (i.e. isFilename to isFileType)
 - Refactored Result to be a proper Class instead of a function w/ prototypes

Closes: #193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug thing that needs fixing Needs Triage needs an initial review
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: properly parse non-url encoded file specs npm/npm-package-arg
3 participants
@wraithgar @ljharb @andrew-aladjev