Closed Bug 1559356 Opened 5 months ago Closed 5 months ago

Regression: URL class does not parse registered custom protocols correctly

Categories

(Core :: Networking, defect, P2)

69 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- fixed

People

(Reporter: sam, Assigned: valentin)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: regression, Whiteboard: [necko-triaged])

Attachments

(1 file)

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) => { ... });
  1. 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

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.

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

Valentin, can you confirm?

Flags: needinfo?(valentin.gosu)

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.

Flags: needinfo?(valentin.gosu)

(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?

(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.

Moving this out of Untriaged hopefully to a better component.

Status: UNCONFIRMED → NEW
Component: Untriaged → Networking
Ever confirmed: true
Product: Firefox → Core

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

Flags: needinfo?(valentin.gosu)
Blocks: 1561860

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.

Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Priority: -- → P2
Regressed by: 1536744
Whiteboard: [necko-triaged]

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

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

Flags: needinfo?(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/

Flags: needinfo?(sam)

(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 ?

Flags: needinfo?(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.

Flags: needinfo?(sam)
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bee08b082e13
Make sure dweb URLs have a proper host r=kershaw
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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 ?

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

Flags: needinfo?(honzab.moz)
Flags: needinfo?(amarchesini)

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.

Flags: needinfo?(amarchesini)
Flags: needinfo?(honzab.moz)
You need to log in before you can comment on or make changes to this bug.