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.

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: properly parse non-url encoded file specs #200

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wraithgar
Copy link
Member

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 requested a review from a team as a code owner December 18, 2024 22:16
lib/npa.js Dismissed Show dismissed Hide dismissed
@wraithgar
Copy link
Member Author

There still appears to be some incorrect behavior here, that was here even before this PR. saveSpec is a file: url but it is not url encoded. Most things this will generate won't cause decodeURI to throw:

> require('.')('/path/br[acket')
Result {
  type: 'directory',
  registry: undefined,
  where: '/Users/wraithgar/Development/npm/npm-package-arg/branches/gar_path-url-encoding',
  raw: '/path/br[acket',
  rawSpec: '/path/br[acket',
  saveSpec: 'file:/path/br[acket',
  fetchSpec: '/path/br[acket',
  gitRange: undefined,
  gitCommittish: undefined,
  gitSubdir: undefined,
  hosted: undefined
}
> decodeURI('file:/path/br[acket')
'file:/path/br[acket'

but some new characters that this now allows (like %) will:

> require('.')('/path/per%cent')
Result {
  type: 'directory',
  registry: undefined,
  where: '/Users/wraithgar/Development/npm/npm-package-arg/branches/gar_path-url-encoding',
  raw: '/path/per%cent',
  rawSpec: '/path/per%cent',
  saveSpec: 'file:/path/per%cent',
  fetchSpec: '/path/per%cent',
  gitRange: undefined,
  gitCommittish: undefined,
  gitSubdir: undefined,
  hosted: undefined
}
> decodeURI('file:/path/per%cent')
Uncaught URIError: URI malformed

I chose not to encode saveSpec in this PR because it's likely a breaking change. Consumers should always be decoding that, but they may not be currently.

@wraithgar
Copy link
Member Author

wraithgar commented Dec 18, 2024

Still some \ shenanigans to work out, evidently. The saveSpec normalization in the tests is likely hiding potential bugs.

@wraithgar wraithgar marked this pull request as draft December 18, 2024 22:46
@wraithgar wraithgar force-pushed the gar/path-url-encoding branch 5 times, most recently from 3e28f1e to ce6c05b Compare December 20, 2024 18:30

const normalizePath = p => p && p.replace(/^[a-zA-Z]:/, '').replace(/\\/g, '/')

const cwd = normalizePath(process.cwd())
process.cwd = () => cwd
const normalizePaths = spec => {
spec.saveSpec = normalizePath(spec.saveSpec)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was masking bugs.

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 force-pushed the gar/path-url-encoding branch from ce6c05b to 3c876a6 Compare December 20, 2024 18:36
@wraithgar wraithgar marked this pull request as ready for review December 20, 2024 18:39
return isWindowsFile.test(spec)
}
// We never hit this in windows tests, obviously
/* istanbul ignore next */
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 fact that this was covered before is concerning.

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.

[BUG] Failed to decode path name with valid symbols
1 participant