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

url: validate pathToFileURL(path) argument as string #49161

Merged

Conversation

LiviaMedeiros
Copy link
Contributor

Since url.pathToFileURL() is exposed as a part of public API, it should validate input value.

Before this change, non-strings were rejected in path.resolve() with confusing The "paths[0]" argument must be of type string. message.
On Windows, input was directly passed into StringPrototypeStartsWith(), causing either confusing errors or implicit coercion to string.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 14, 2023

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Aug 14, 2023
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023
@nodejs-github-bot
Copy link
Collaborator

@@ -1443,6 +1444,8 @@ function encodePathChars(filepath) {
}

function pathToFileURL(filepath) {
validateString(filepath, 'path');
Copy link
Member

Choose a reason for hiding this comment

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

This is an extremely hot path, hence my comment:

We already validate this for !(isWindows && StringPrototypeStartsWith(filepath, '\\\\')) case (path.resolve handles the validation). Can you make sure this only runs when the OS is windows? No need to run it twice for non-windows environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check also fixes The "paths[0]" argument must be of type string. error message that kinda implies that this method may take more than one argument.

I guess we can keep the internal/url version as is (so it won't affect performance in core), but instead of re-exporting it directly in url make a thin wrapper that validates input, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds good to me.

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53632/

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53668/

PR-URL: nodejs#49161
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@LiviaMedeiros LiviaMedeiros force-pushed the url-pathtofileurl-validate-string branch from c940437 to 57e78bc Compare August 31, 2023 12:35
@LiviaMedeiros
Copy link
Contributor Author

Landed in 57e78bc

@LiviaMedeiros LiviaMedeiros merged commit 57e78bc into nodejs:main Aug 31, 2023
34 checks passed
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
PR-URL: #49161
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49161
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Nov 27, 2023
PR-URL: #49161
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#49161
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#49161
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants