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: handle quasi-WHATWG URLs in urlToOptions() #26226

Merged
merged 1 commit into from
Feb 26, 2019
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Feb 21, 2019

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

Copy link
Member

@lpinca lpinca left a comment

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.

Copy link
Member

@BridgeAR BridgeAR left a comment

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
Copy link
Contributor Author

cjihrig commented Feb 21, 2019

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
Copy link
Contributor Author

cjihrig commented Feb 21, 2019

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
Copy link
Contributor Author

cjihrig commented Feb 26, 2019

@cjihrig cjihrig merged commit 4900863 into nodejs:master Feb 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
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>
@BridgeAR BridgeAR mentioned this pull request Mar 4, 2019
@szmarczak
Copy link
Member

Will this be merged into Node.js 10?

BethGriggs pushed a commit that referenced this pull request Mar 24, 2020
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
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 BethGriggs mentioned this pull request Mar 24, 2020
BethGriggs added a commit that referenced this pull request Mar 25, 2020
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
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
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
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
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
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
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
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
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
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.

7 participants