url: origin of "blob:" URL containing inner non-"http(s):" URL#48168
url: origin of "blob:" URL containing inner non-"http(s):" URL#48168hemanth wants to merge 2 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
anonrig
left a comment
There was a problem hiding this comment.
You need to also update WPT using git node wpt url in the root of the Node.js repository, and include them in the same commit, since this is a breaking change.
More information can be found from: https://github.com/nodejs/node-core-utils/blob/main/docs/git-node.md#example-3
| const out = new URL(path); | ||
| if(out.protocol !== 'https:') { | ||
| return 'null'; | ||
| } |
There was a problem hiding this comment.
The standard says that
If pathURL’s scheme is not "http" and not "https", then return a new opaque origin.
For performance reasons, we avoid having string comparisons and calling getters which runs a string.prototype.slice. We can however use scheme_type numeric value.
If you look into the declaration of scheme_type variable in lib/internal/url.js, you can find the following comment:
/**
* Refers to `ada::scheme::type`
*
* enum type : uint8_t {
* HTTP = 0,
* NOT_SPECIAL = 1,
* HTTPS = 2,
* WS = 3,
* FTP = 4,
* WSS = 5,
* FILE = 6
* };
* @type {number}
*/
For this particular case: we need to change line 846 to:
if (out.#context.scheme_type === 0 || out.#context.scheme_type === 2)
There was a problem hiding this comment.
Ah, thank you. I did notice out.#context.scheme_type !== 1 but didn't pay attention to the comment.
There was a problem hiding this comment.
can this enum be exported to js in some way? would be good to not just have magic numbers floating around
There was a problem hiding this comment.
At the very least, please add a comment indicating where the numbers come from
|
@anonrig Currently, my official machine has some restrictions due to VPN: Ran out of my codespace, personal machine is broken. Stuck! |
|
Looks like something is broken in my local: 
|
Fixes #48157 ?
P.S: Will write tests, if this makes sense.