Skip to content

url: handle quasi-WHATWG URLs in urlToOptions() - #26226

Merged
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:url
Feb 26, 2019
Merged

url: handle quasi-WHATWG URLs in urlToOptions()#26226
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:url

Conversation

@cjihrig

@cjihrigcjihrig commented Feb 21, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

This commit prevents the internal urlToOptions() from throwing when a malformed WHATWG URL is encountered. This is more of a bandaid fix, as the return value will still be pretty useless, but garbage in, garbage out I guess.

I don't think we want to slow this specific function down with a lot of extra validation code, but maybe others feel differently.

Refs: #26198

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot

nodejs-github-bot commented Feb 21, 2019

Copy link
Copy Markdown
Collaborator

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2678/pipeline

lpinca
lpinca approved these changes Feb 21, 2019
View reviewed changes

@lpincalpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It still throws if url.pathname is a Symbol for example, but that is very unlikely I guess.

BridgeAR
BridgeAR approved these changes Feb 21, 2019
View reviewed changes

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we add a test for this as well?

@cjihrig

cjihrig commented Feb 21, 2019

Copy link
Copy Markdown
ContributorAuthor

Should we add a test for this as well?

Test added.

It still throws if url.pathname is a Symbol for example, but that is very unlikely I guess.

Yea, the point was to handle the case where a copied URL object passes our brand checking (the existence of several symbols). It seems unlikely that url.pathname would be a Symbol unless someone was actively trying to break things. The point wasn't necessarily to be bulletproof (it doesn't handle throwing getters, etc. either).

@cjihrig

cjihrig commented Feb 21, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

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

@cjihrig
cjihrigforce-pushed the url branch 2 times, most recently from 03dcf8a to 1b6ebffCompareFebruary 26, 2019 20:04
@cjihrig
url: handle quasi-WHATWG URLs in urlToOptions()
4900863
PR-URL: nodejs#26226
Refs: nodejs#26198
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@cjihrig

cjihrig commented Feb 26, 2019

Copy link
Copy Markdown
ContributorAuthor

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

@cjihrig
cjihrig merged commit 4900863 into nodejs:masterFeb 26, 2019
@cjihrig
cjihrig deleted the url branch February 26, 2019 21:11
addaleax pushed a commit that referenced this pull request Mar 1, 2019
@cjihrig@addaleax
url: handle quasi-WHATWG URLs in urlToOptions()
0017855
PR-URL: #26226
Refs: #26198
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@lpincalpinca mentioned this pull request Mar 3, 2019
Closed
@BridgeARBridgeAR mentioned this pull request Mar 4, 2019
Merged
@jacobjenksjacobjenks mentioned this pull request Dec 18, 2019
Closed
2 tasks
@ericmustinericmustin mentioned this pull request Jan 17, 2020
Closed
@szmarczak

szmarczak commented Feb 15, 2020

Copy link
Copy Markdown
Member

Will this be merged into Node.js 10?

This was referenced Mar 23, 2020
Closed
Closed
BethGriggs pushed a commit that referenced this pull request Mar 24, 2020
@cjihrig@BethGriggs
url: handle quasi-WHATWG URLs in urlToOptions()
4390674
PR-URL: #26226
Backport-PR-URL: #32446
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs added a commit that referenced this pull request Mar 24, 2020
@BethGriggs
2020-03-24, Version 10.20.0 'Dubnium' (LTS)
22927bf
Notable changes:
- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
[#19691](#19691)
- deps:
- update npm to 6.14.3 (Myles Borins)
[#32368](#32368)
- upgrade openssl sources to 1.1.1e (Hassaan Pasha)
[#32328](#32328)
- upgrade to libuv 1.34.2 (cjihrig)
[#31477](#31477)
- n-api:
- add napi\_get\_all\_property\_names (himself65)
[#30006](#30006)
- add APIs for per-instance state management (Gabriel Schulhof)
[#28682](#28682)
- turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
[#26128](#26128)
- tls:
- expose keylog event on TLSSocket (Alba Mendez)
[#27654](#27654)
- support TLS min/max protocol defaults in CLI (Sam Roberts)
[#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
[#26226](#26226)
PR-URL: #31984
@BethGriggsBethGriggs mentioned this pull request Mar 24, 2020
Merged
BethGriggs added a commit that referenced this pull request Mar 25, 2020
@BethGriggs
2020-03-25, Version 10.20.0 'Dubnium' (LTS)
f5f4a75
Notable changes:
- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
[#19691](#19691)
- deps:
- update npm to 6.14.3 (Myles Borins)
[#32368](#32368)
- upgrade openssl sources to 1.1.1e (Hassaan Pasha)
[#32328](#32328)
- upgrade to libuv 1.34.2 (cjihrig)
[#31477](#31477)
- n-api:
- add napi\_get\_all\_property\_names (himself65)
[#30006](#30006)
- add APIs for per-instance state management (Gabriel Schulhof)
[#28682](#28682)
- turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
[#26128](#26128)
- tls:
- expose keylog event on TLSSocket (Alba Mendez)
[#27654](#27654)
- support TLS min/max protocol defaults in CLI (Sam Roberts)
[#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
[#26226](#26226)
PR-URL: #31984
BethGriggs added a commit that referenced this pull request Mar 26, 2020
@BethGriggs
2020-03-26, Version 10.20.0 'Dubnium' (LTS)
314adeb
Notable changes:
- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
[#19691](#19691)
- deps:
- update npm to 6.14.3 (Myles Borins)
[#32368](#32368)
- upgrade openssl sources to 1.1.1e (Hassaan Pasha)
[#32328](#32328)
- upgrade to libuv 1.34.2 (cjihrig)
[#31477](#31477)
- n-api:
- add napi\_get\_all\_property\_names (himself65)
[#30006](#30006)
- add APIs for per-instance state management (Gabriel Schulhof)
[#28682](#28682)
- turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
[#26128](#26128)
- tls:
- expose keylog event on TLSSocket (Alba Mendez)
[#27654](#27654)
- support TLS min/max protocol defaults in CLI (Sam Roberts)
[#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
[#26226](#26226)
PR-URL: #31984
BethGriggs added a commit that referenced this pull request Apr 3, 2020
@BethGriggs
2020-03-26, Version 10.20.0 'Dubnium' (LTS)
997dbf1
Notable changes:
- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
[#19691](#19691)
- build: macOS package notarization (Rod Vagg)
[#31459](#31459)
- deps:
- update npm to 6.14.3 (Myles Borins)
[#32368](#32368)
- upgrade openssl sources to 1.1.1e (Hassaan Pasha)
[#32328](#32328)
- upgrade to libuv 1.34.2 (cjihrig)
[#31477](#31477)
- n-api:
- add napi\_get\_all\_property\_names (himself65)
[#30006](#30006)
- add APIs for per-instance state management (Gabriel Schulhof)
[#28682](#28682)
- turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
[#26128](#26128)
- tls:
- expose keylog event on TLSSocket (Alba Mendez)
[#27654](#27654)
- support TLS min/max protocol defaults in CLI (Sam Roberts)
[#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
[#26226](#26226)
PR-URL: #31984
BethGriggs added a commit that referenced this pull request Apr 6, 2020
@BethGriggs
2020-04-07, Version 10.20.0 'Dubnium' (LTS)
0326fc1
Notable changes:
- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
[#19691](#19691)
- build: macOS package notarization (Rod Vagg)
[#31459](#31459)
- deps:
- update npm to 6.14.3 (Myles Borins)
[#32368](#32368)
- upgrade openssl sources to 1.1.1e (Hassaan Pasha)
[#32328](#32328)
- upgrade to libuv 1.34.2 (cjihrig)
[#31477](#31477)
- n-api:
- add napi\_get\_all\_property\_names (himself65)
[#30006](#30006)
- add APIs for per-instance state management (Gabriel Schulhof)
[#28682](#28682)
- define release 6
[#32058](#32058)
- turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
[#26128](#26128)
- tls:
- expose keylog event on TLSSocket (Alba Mendez)
[#27654](#27654)
- support TLS min/max protocol defaults in CLI (Sam Roberts)
[#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
[#26226](#26226)
PR-URL: #31984
BethGriggs added a commit that referenced this pull request Apr 7, 2020
@BethGriggs
2020-04-07, Version 10.20.0 'Dubnium' (LTS)
d5f87c5
macOS package notarization and a change in builder configuration
The macOS binaries for this release, and future 10.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing .pkg files on macOS
10.15 and later. Previous builds of Node.js 10.x were compiled on macOS
10.7 (Lion). As binaries are still being compiled to support a minimum
of macOS 10.7 (Lion) we do not anticipate this having a negative impact
on Node.js 10.x users with older versions of macOS.
Notable changes:
- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
[#19691](#19691)
- build: macOS package notarization (Rod Vagg)
[#31459](#31459)
- deps:
- update npm to 6.14.3 (Myles Borins)
[#32368](#32368)
- upgrade openssl sources to 1.1.1e (Hassaan Pasha)
[#32328](#32328)
- upgrade to libuv 1.34.2 (cjihrig)
[#31477](#31477)
- n-api:
- add napi\_get\_all\_property\_names (himself65)
[#30006](#30006)
- add APIs for per-instance state management (Gabriel Schulhof)
[#28682](#28682)
- define release 6
[#32058](#32058)
- turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
[#26128](#26128)
- tls:
- expose keylog event on TLSSocket (Alba Mendez)
[#27654](#27654)
- support TLS min/max protocol defaults in CLI (Sam Roberts)
[#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
[#26226](#26226)
PR-URL: #31984
BethGriggs added a commit that referenced this pull request Apr 7, 2020
@BethGriggs
2020-04-07, Version 10.20.0 'Dubnium' (LTS)
b6d4334
macOS package notarization and a change in builder configuration
The macOS binaries for this release, and future 10.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing .pkg files on macOS
10.15 and later. Previous builds of Node.js 10.x were compiled on macOS
10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion).
As binaries are still being compiled to support a minimum of macOS 10.7
(Lion) we do not anticipate this having a negative impact on Node.js
10.x users with older versions of macOS.
Notable changes:
- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
[#19691](#19691)
- build: macOS package notarization (Rod Vagg)
[#31459](#31459)
- deps:
- update npm to 6.14.3 (Myles Borins)
[#32368](#32368)
- upgrade openssl sources to 1.1.1e (Hassaan Pasha)
[#32328](#32328)
- upgrade to libuv 1.34.2 (cjihrig)
[#31477](#31477)
- n-api:
- add napi\_get\_all\_property\_names (himself65)
[#30006](#30006)
- add APIs for per-instance state management (Gabriel Schulhof)
[#28682](#28682)
- define release 6
[#32058](#32058)
- turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
[#26128](#26128)
- tls:
- expose keylog event on TLSSocket (Alba Mendez)
[#27654](#27654)
- support TLS min/max protocol defaults in CLI (Sam Roberts)
[#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
[#26226](#26226)
PR-URL: #31984
BethGriggs added a commit that referenced this pull request Apr 8, 2020
@BethGriggs
2020-04-08, Version 10.20.0 'Dubnium' (LTS)
246eede
macOS package notarization and a change in builder configuration
The macOS binaries for this release, and future 10.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing .pkg files on macOS
10.15 and later. Previous builds of Node.js 10.x were compiled on macOS
10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion).
As binaries are still being compiled to support a minimum of macOS 10.7
(Lion) we do not anticipate this having a negative impact on Node.js
10.x users with older versions of macOS.
Notable changes:
- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
[#19691](#19691)
- build: macOS package notarization (Rod Vagg)
[#31459](#31459)
- deps:
- update npm to 6.14.3 (Myles Borins)
[#32368](#32368)
- upgrade openssl sources to 1.1.1e (Hassaan Pasha)
[#32328](#32328)
- upgrade to libuv 1.34.2 (cjihrig)
[#31477](#31477)
- n-api:
- add napi\_get\_all\_property\_names (himself65)
[#30006](#30006)
- add APIs for per-instance state management (Gabriel Schulhof)
[#28682](#28682)
- define release 6
[#32058](#32058)
- turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
[#26128](#26128)
- tls:
- expose keylog event on TLSSocket (Alba Mendez)
[#27654](#27654)
- support TLS min/max protocol defaults in CLI (Sam Roberts)
[#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
[#26226](#26226)
PR-URL: #31984
BethGriggs added a commit that referenced this pull request Apr 8, 2020
@BethGriggs
2020-04-08, Version 10.20.0 'Dubnium' (LTS)
9b46e3f
macOS package notarization and a change in builder configuration
The macOS binaries for this release, and future 10.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing .pkg files on macOS
10.15 and later. Previous builds of Node.js 10.x were compiled on macOS
10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion).
As binaries are still being compiled to support a minimum of macOS 10.7
(Lion) we do not anticipate this having a negative impact on Node.js
10.x users with older versions of macOS.
Notable changes:
- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
[#19691](#19691)
- build: macOS package notarization (Rod Vagg)
[#31459](#31459)
- deps:
- update npm to 6.14.3 (Myles Borins)
[#32368](#32368)
- upgrade openssl sources to 1.1.1e (Hassaan Pasha)
[#32328](#32328)
- upgrade to libuv 1.34.2 (cjihrig)
[#31477](#31477)
- n-api:
- add napi\_get\_all\_property\_names (himself65)
[#30006](#30006)
- add APIs for per-instance state management (Gabriel Schulhof)
[#28682](#28682)
- define release 6
[#32058](#32058)
- turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
[#26128](#26128)
- tls:
- expose keylog event on TLSSocket (Alba Mendez)
[#27654](#27654)
- support TLS min/max protocol defaults in CLI (Sam Roberts)
[#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
[#26226](#26226)
PR-URL: #31984
BethGriggs added a commit that referenced this pull request Apr 14, 2020
@BethGriggs
2020-04-08, Version 10.20.0 'Dubnium' (LTS)
8d8095f
macOS package notarization and a change in builder configuration
The macOS binaries for this release, and future 10.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing .pkg files on macOS
10.15 and later. Previous builds of Node.js 10.x were compiled on macOS
10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion).
As binaries are still being compiled to support a minimum of macOS 10.7
(Lion) we do not anticipate this having a negative impact on Node.js
10.x users with older versions of macOS.
Notable changes:
- buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc)
[#19691](#19691)
- build: macOS package notarization (Rod Vagg)
[#31459](#31459)
- deps:
- update npm to 6.14.3 (Myles Borins)
[#32368](#32368)
- upgrade openssl sources to 1.1.1e (Hassaan Pasha)
[#32328](#32328)
- upgrade to libuv 1.34.2 (cjihrig)
[#31477](#31477)
- n-api:
- add napi\_get\_all\_property\_names (himself65)
[#30006](#30006)
- add APIs for per-instance state management (Gabriel Schulhof)
[#28682](#28682)
- define release 6
[#32058](#32058)
- turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen)
[#26128](#26128)
- tls:
- expose keylog event on TLSSocket (Alba Mendez)
[#27654](#27654)
- support TLS min/max protocol defaults in CLI (Sam Roberts)
[#27946](#27946)
- url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig)
[#26226](#26226)
PR-URL: #31984
@trentmtrentm mentioned this pull request Apr 13, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+1 more reviewer
@devsnekdevsnekdevsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@cjihrig@nodejs-github-bot@szmarczak@jasnell@lpinca@devsnek@BridgeAR