Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

First stab replacing url.parse with URL constructor (fixes #215) #231

Closed
wants to merge 1 commit into from

Conversation

mozfreddyb
Copy link
Contributor

Naive attempt :)

@XhmikosR
Copy link
Collaborator

XhmikosR commented Mar 4, 2019

Hehe, that's the problem :P We need a base path since we use relative URLs.

lib/helpers.js Outdated
if (isRelative) {
urlString = `https:${urlString}`;
if (missingScheme || bareHostname) {
urlString = `http:${urlString}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes the previous behavior though. Before we defaulted to https.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's an oddity I'd like to sort out:
In https://github.com/mozilla/srihash.org/blob/master/test/helpers.js#L30-L31, we expect example.com/script.js to be completed to HTTP, but below in https://github.com/mozilla/srihash.org/blob/master/test/helpers.js#L37-L38 we autofill the protocol-relative //example.com/script.js to HTTPS.

It's not in secureHosts.json, so why do we upgrade in the latter case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, yeah that doesn't seem right indeed. On a side note, since the input has type="url", the user doesn't seem to be able to enter a protocol-relative file. At least with FF 66 that I use.

Copy link
Collaborator

@XhmikosR XhmikosR Mar 4, 2019

Choose a reason for hiding this comment

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

This change was done in #156

So, if we trust the browsers' checks here, I think we can just drop the whole relative URLs logic (protocol-relative and schemeless).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We shouldn't trust the browser completely. We should try to parse with the URL constructor and return an error to the user interface if it throws

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then why did you merge #233 :P

You can add back any stuff here after rebasing this.

BTW you should open branches upstream so that it's easier to collaborate. I will start doing it too as long as we sort #234

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems I love rebasing after lunch.

lib/helpers.js Outdated
// Add a scheme to scheme-less URLs
const isRelative = Boolean(urlString.match(/^\/\//));
// For URLs that are protocol-relative or missing it completely
const missingScheme = urlString.indexOf(":") === -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd change this to const missingScheme = !urlString.includes(':');

@mozfreddyb
Copy link
Contributor Author

Not sure I understand the failures for generate(). I'll have to finish some other things today, so I'll give it another try tomorrow.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants