1559356 - Regression: URL class does not parse registered custom protocols correctly

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[sam]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

1. Register a protocol handler using the [libdweb](](https://github.com/mozilla/libdweb) experimental API.

browser.protocol.registerProtocol('dweb', (request) => { ... });

2. Parse a URL of this protocol using the URL class to get it's origin (in extension or page context).

new URL('dweb://example.com/test').origin

Actual results:

On 69: origin is null
Before 69: origin is dweb://example.com

Expected results:

origin is dweb://example.com

Comment 1

[sam]

I think fixing this issue (https://bugzilla.mozilla.org/show_bug.cgi?id=1374505) would also solve this, as parsing unknown protocols as-per the spec should give the desired result in this case.

Comment 2

[sam]

On IRC it was suggested that this could have been caused by these changes: https://groups.google.com/forum/#!topic/mozilla.dev.platform/ILkf8vTRZsI

Comment 3

[[:fabrice] Fabrice Desré]

Valentin, can you confirm?

Comment 4

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Yes, this is fallout from bug 1536744.
At the moment, this code is no longer run:
https://github.com/mozilla/libdweb/blob/4d6cdee2ac43dfafffa234fcfc9a3e1cc4873e7e/src/toolkit/components/extensions/ExtensionProtocol.js#L1280

If we want dweb to continue having a host, I suggest we add the scheme to NS_NewURI in order to instantiate an nsStandardURL.

(In reply to sam from comment #0)

new URL('dweb://example.com/test').origin

Also note that per the URL spec non-special schemes such as dweb should have a null origin.
We don't currently respect that, but we have bug 1553105 to deal with it. But in any case, I expect we want dweb URLs to have a hostname.

Comment 5

[sam]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #4)

If we want dweb to continue having a host, I suggest we add the scheme to NS_NewURI in order to instantiate an nsStandardURL.

Does this mean that each dweb protocol that would like their hostname and origin parsed as per nsStandardURL will need to be explicitly defined at compile time? If so, could you point out where new protocols can be added to NS_NewURI?

Comment 6

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

(In reply to sam from comment #5)

(In reply to Valentin Gosu [:valentin] (he/him) from comment #4)

If we want dweb to continue having a host, I suggest we add the scheme to NS_NewURI in order to instantiate an nsStandardURL.

Does this mean that each dweb protocol that would like their hostname and origin parsed as per nsStandardURL will need to be explicitly defined at compile time? If so, could you point out where new protocols can be added to NS_NewURI?

Yes. I think the code should go here:
https://searchfox.org/mozilla-central/rev/b3b401254229f0a26f7ee625ef5f09c6c31e3949/netwerk/base/nsNetUtil.cpp#1863
Basically the implementation from ExtensionProtocol.js::newURI should be moved to NS_NewURI.
Note that we only need to do this until we get nsSimpleURI (or maybe some other nsIURI implementation based on RustURL) to parse unknown schemes and also have a valid GetHost implementation.

Comment 7

[Marcia Knous [:marcia]]

Moving this out of Untriaged hopefully to a better component.

Comment 8

[Junior [inactive]]

Do we have some document to address this for the extension developers? Or it will be compatible to instantiate an nsStandardURL eventually.

Comment 9

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Attachment: Bug 1559356 - Make sure dweb URLs have a proper host r=kershaw

We want dweb URLs to continue working as before bug 1536744 landed.
So we make sure to instantiate it as an nsStandardURL.
This is not a good long-term solution, as we don't want to hardcode
all the various schemes that we want to behave properly.
The fix would be to add a new spec-compliant nsIURI implementation,
based on RustURL and use it for all unknown schemes.

See bug 1561860 for a more complete solution.

Comment 10

[sam]

As my use case is for the dat protocol, would it be possible to add that to this patch too?

Comment 11

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

How do dat URLs work?
is it dat://key/ or must it be parsed as dat://key (without the slash?)

Comment 12

[sam]

They should be parsed as http(s) URLs. There is an optional non-standard version parameter (see https://github.com/pfrazee/parse-dat-url), however to avoid over-complication at this point I think this can be omitted for the time-being.

Examples:
dat://example.com/test
dat://41f8a987cfeba80a037e51cc8357d513b62514de36f2f9b3d3eeec7a8fb3b5a5/

Comment 13

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

(In reply to sam from comment #12)

They should be parsed as http(s) URLs. There is an optional non-standard version parameter (see https://github.com/pfrazee/parse-dat-url), however to avoid over-complication at this point I think this can be omitted for the time-being.

Examples:
dat://example.com/test
dat://41f8a987cfeba80a037e51cc8357d513b62514de36f2f9b3d3eeec7a8fb3b5a5/

Thanks for the info. Yes, this makes it a candidate to lump together with dweb, but I assume dat didn't have a host before bug 1536744 either, right? Or is this the fix for https://github.com/sammacbeth/dat-fox/issues/18 ?

Comment 14

[sam]

When using the libdweb protocol handler, the protocol registration was previously able to define newURI for that protocol: https://github.com/mozilla/libdweb/blob/master/src/toolkit/components/extensions/ExtensionProtocol.js#L1288 . This was then parsing the registered protocol as a nsIStandardURL.

For extensions using the current protocol_handlers extension manifest key (i.e. dat-fox), custom protocol urls were never parsed correctly.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bee08b082e13
Make sure dweb URLs have a proper host r=kershaw

Comment 16

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/bee08b082e13

Comment 17

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Hi Everyone,

I apologize for jumping on this so late. I am afraid current intermediate solution isn't really a viable as it depends on whitelisting two protocol schemes. This implies it won't work in other cases like ipfs://, ssb://, ....

As a fallout of bug 1536744 pretty much all tests for the following patch https://bugzilla.mozilla.org/attachment.cgi?id=9064203 are failing. It's due to origin becoming null & the fact that Ci.nsIProtocolHandler.URI_IS_POTENTIALLY_TRUSTWORTHY is no longer taken into account for isSecrueContext.

Is there chance protocolFlags of the nsIProtocolHandler could be consulted to decide if url should be parsed as URI or URL ?

Or maybe there could be a some alternative way to register scheme to be parsed as standardURL instead ?

Comment 18

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

:mayhemer :baku you reviewed patches for bug 1536744 so maybe you can provide some insight on the above ?

Comment 19

[Andrea Marchesini [:baku]]

Or maybe there could be a some alternative way to register scheme to be parsed as standardURL instead ?

Since D30742, nsIProtocolHandler has changed. 'newURI' method doesn't exist anymore because the nsIURI creation is now thread-safe \o/

Probably, the best way to support new protocols, implemented by webExtensions, is to have a C++, thread-safe object which contains the list of them. This object can be queried in NS_NewURI(), in order to use NewStandardURI() for those protocols.