GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — url: validate `pathToFileURL(path)` argument as string by LiviaMedeiros · Pull Request #49161 · nodejs/node

Conversation

@LiviaMedeiros

@LiviaMedeiros LiviaMedeiros commented Aug 14, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2023 •
edited by LiviaMedeiros
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/url
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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. Only starts once the PR has an approving review. label Aug 14, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 14, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53302/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung
joyeecheung approved these changes Aug 14, 2023
View reviewed changes
anonrig
anonrig reviewed Aug 14, 2023
View reviewed changes
Comment thread lib/internal/url.js Outdated

@anonrig anonrig Aug 14, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@LiviaMedeiros LiviaMedeiros Aug 15, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aduh95 aduh95 Aug 17, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
This was referenced Aug 15, 2023
CI Reliability 2023-08-15 nodejs/reliability#637
Open
CI Reliability 2023-08-16 nodejs/reliability#638
Open
CI Reliability 2023-08-17 nodejs/reliability#639
Open
aduh95
aduh95 approved these changes Aug 21, 2023
View reviewed changes
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 22, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 22, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
This was referenced Aug 23, 2023
CI Reliability 2023-08-23 nodejs/reliability#643
Open
CI Reliability 2023-08-24 nodejs/reliability#644
Open
CI Reliability 2023-08-25 nodejs/reliability#645
Open
CI Reliability 2023-08-26 nodejs/reliability#646
Open
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 26, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 26, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
This was referenced Aug 27, 2023
CI Reliability 2023-08-27 nodejs/reliability#647
Open
CI Reliability 2023-08-28 nodejs/reliability#648
Open
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
This was referenced Aug 29, 2023
CI Reliability 2023-08-29 nodejs/reliability#649
Open
CI Reliability 2023-08-30 nodejs/reliability#650
Open
@nodejs-github-bot

nodejs-github-bot commented Aug 30, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53632/
LiviaMedeiros reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 30, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 30, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions Bot mentioned this pull request Aug 31, 2023
CI Reliability 2023-08-31 nodejs/reliability#651
Open
18 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Aug 31, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53668/
LiviaMedeiros reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@LiviaMedeiros
url: validate pathToFileURL(path) argument as string
57e78bc 
PR-URL: nodejs#49161
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@LiviaMedeiros
LiviaMedeiros force-pushed the url-pathtofileurl-validate-string branch from c940437 to 57e78bc Compare August 31, 2023 12:35
@LiviaMedeiros

LiviaMedeiros commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

Landed in 57e78bc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@LiviaMedeiros
LiviaMedeiros merged commit 57e78bc into nodejs:main Aug 31, 2023
This was referenced Sep 1, 2023
CI Reliability 2023-09-01 nodejs/reliability#652
Open
CI Reliability 2023-09-02 nodejs/reliability#653
Open
CI Reliability 2023-09-03 nodejs/reliability#654
Open
CI Reliability 2023-09-04 nodejs/reliability#655
Open
CI Reliability 2023-09-05 nodejs/reliability#656
Open
CI Reliability 2023-09-06 nodejs/reliability#657
Open
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@LiviaMedeiros @UlisesGascon
url: validate pathToFileURL(path) argument as string
ae40cb1 
PR-URL: #49161
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
v20.7.0 proposal #49592
Merged
targos pushed a commit that referenced this pull request Nov 27, 2023
@LiviaMedeiros @targos
url: validate pathToFileURL(path) argument as string
ac27431 
PR-URL: #49161
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
url: validate pathToFileURL(path) argument as string
8d57e0f 
PR-URL: nodejs/node#49161
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
url: validate pathToFileURL(path) argument as string
9a8e69a 
PR-URL: nodejs/node#49161
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig left review comments
@joyeecheung joyeecheung joyeecheung approved these changes
@aduh95 aduh95 aduh95 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@LiviaMedeiros @nodejs-github-bot @anonrig @joyeecheung @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.