1261289 - Allow webextensions to open view-source: links

Status: VERIFIED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Daniel Veditz [:dveditz]]

As a debugging aide I tried to create a simple webextension that had the ability to open links as view-source:<link>. Unfortunately as of bug 1172165 these were made URI_DANGEROUS_TO_LOAD and tab.create() now fails.

In the context of an extension under user's control these are not dangerous and should be allowed.

Comment 1

[Taku0]

I also want this to port my pie menu extension to WebExtensions: https://addons.mozilla.org/en-US/firefox/addon/compass_menu/.
Other authors of mouse gesture extensions may want this.

Comment 3

[Markus Popp]

I'm trying to convert my addon to a Webextension and need this to work.

Could this please be fixed sooner than later?

Comment 4

[Blake Winton (:bwinton) (:☕️)]

(In reply to Markus Popp from comment #3)
> I'm trying to convert my addon to a Webextension and need this to work.
> 
> Could this please be fixed sooner than later?

Hi Markus,

given that this bug is a P3, and seems very important to you, I'll suggest that the quickest way to get it fixed would be for you to submit a patch.  I suspect Kris would be happy to help you out if you wanted to try making the necessary changes, and I would also give you a hand where I could!

Comment 5

[Markus Popp]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #4)
> given that this bug is a P3, and seems very important to you, I'll suggest
> that the quickest way to get it fixed would be for you to submit a patch.  I
> suspect Kris would be happy to help you out if you wanted to try making the
> necessary changes, and I would also give you a hand where I could!

Now you're not only asking me to rewrite my addons as Webextensions but even to fix Firefox that I can do so?

Seriously???

FUCKING SERIOUSLY??????????????????

Comment 6

[Markus Popp]

BTW, this may not only affect view-source: URLs but also all the "about:..." URLs and maybe others.

Comment 7

[Blake Winton (:bwinton) (:☕️)]

I'm not asking you to do anything, I'm suggesting a way you can get what you want, on the timeline you want, without waiting for other people.  If you choose not to, that's totally fine by me, but I will ask you to refrain from using profanity here in the future.  Bugzilla is a place to get work done, not a place for people to vent their anger.

Comment 8

[Markus Popp]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #7)
> I'm not asking you to do anything, I'm suggesting a way you can get what you
> want, on the timeline you want, without waiting for other people.  If you
> choose not to, that's totally fine by me, but I will ask you to refrain from
> using profanity here in the future.  Bugzilla is a place to get work done,
> not a place for people to vent their anger.

Then get the work done.

I've been hearing that addon developers will get great support to migrate their addons to Webextensions.

I can't see any of that.

If even a trivial thing like this fails and such a minor request hasn't been fixed in a year, how many troubles will developers of more sophisticated addons face?

This Webextension thing (or more so: the killing of all other addons) is well on its way to turn into a huge disaster.

Comment 9

[Mike Hoye [:mhoye]]

Markus, bugzilla is our professional working environment as much as it is our bug tracker, and our engineers have a great deal of discretion about how to spend their time and effort. Comments like these are unacceptable, and the fact that we value and encourage community involvement in Firefox development does not give anyone the right to patronize or demean our colleagues or entitle anyone to our engineers' time. 

If you want to meaningfully advance this bug, I encourage you to start by reading our etiquette and contributor guidelines, linked below the comment box. If you can't find a way to participate productively in this bug without following those guidelines, then do not. If you decide to do so anyway, my next step will be deactivating your bugzilla account. 

Have a good weekend.

Comment 10

[Markus Popp]

This does it. I will NOT convert my addons to Webextensions or do any Firefox addon development for that matter in the future.

That's how you destroy your community. Goodbye!

Comment 11

[Markus Popp]

* I reported a bug
* I explained why I need this to continue converting my addon to a Webextension
* The reply I got was: go fix it yourself

Now you tell ME to behave?

That is outrageous and insulting!

Comment 12

[Tim Nguyen :ntim]

I suspect this should be as easy as:
* Introducing a new flag named URI_LOADABLE_BY_EXTENSIONS
* Adding something like this:

// Check for webextension
rv = NS_URIChainHasFlags(aTargetBaseURI,
                         nsIProtocolHandler::URI_LOADABLE_BY_EXTENSIONS,
                         &hasFlags);
NS_ENSURE_SUCCESS(rv, rv);

if (hasFlags) {
  nsString addonId;
  aPrincipal->GetAddonId(addonId);

  if (!addonId.isEmpty()) {
    return NS_OK;
  }
}

in nsScriptSecurityManager::CheckLoadURIWithPrincipal() (caps/nsScriptSecurityManager.cpp)

* Then change: URI_DANGEROUS_TO_LOAD to URI_LOADABLE_BY_EXTENSIONS

in GetProtocolFlags (netwerk/protocol/viewsource/nsViewSourceHandler.cpp)



I don't have time to work on this, but perhaps this could be a good first bug ?

Comment 13

[Andy McKay]

> I don't have time to work on this, but perhaps this could be a good first
> bug ?

This seems a bit harder than the average good first bug, which we try to keep simple.

Comment 14

[Tim Nguyen :ntim]

(In reply to Andy McKay [:andym] from comment #13)
> > I don't have time to work on this, but perhaps this could be a good first
> > bug ?
> 
> This seems a bit harder than the average good first bug, which we try to
> keep simple.

Mentored bug then? Not sure who can be a suitable mentor here though.

Comment 15

[Andy McKay]

If someone has the time to work on it and needs a mentor, we'll gladly try and find one.

Comment 16

[Thomas Wisniewski]

Attachment: 1261289-allow_webextensions_to_open_view-source_links.diff

The steps in comment 12 proved to be enough to get this working; here's a patch.

Comment 17

[Tomislav Jovanovic :zombie]

Comment on attachment 8879576 [details] [diff] [review]
1261289-allow_webextensions_to_open_view-source_links.diff

Review of attachment 8879576 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/viewsource/nsViewSourceHandler.cpp
@@ +37,5 @@
>  
>  NS_IMETHODIMP
>  nsViewSourceHandler::GetProtocolFlags(uint32_t *result)
>  {
> +    *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_EXTENSIONS |

Removing URI_DANGEROUS_TO_LOAD here doesn't seem safe, since that flag is used in many sensitive places, and it has wider implications that just web extensions code.

Comment 18

[Thomas Wisniewski]

Comment on attachment 8879576 [details] [diff] [review]
1261289-allow_webextensions_to_open_view-source_links.diff

Hmm, a try run shows that it's making tests fail, though: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ade16e18d14474b9882221ff1eb5b460a0cbfcc

Seems a review-request was premature; I'll cancel for now and take a look at the failures.

Comment 19

[Tim Nguyen :ntim]

(In reply to Thomas Wisniewski from comment #18)
> Comment on attachment 8879576 [details] [diff] [review]
> 1261289-allow_webextensions_to_open_view-source_links.diff
> 
> Hmm, a try run shows that it's making tests fail, though:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5ade16e18d14474b9882221ff1eb5b460a0cbfcc
Seems like the patch accidently allows web content to link to view source files

Comment 20

[Tim Nguyen :ntim]

A quick fix you can try is keeping URI_DANGEROUS_TO_LOAD in GetProtocolFlags along with URI_LOADABLE_BY_EXTENSIONS.

Comment 21

[Thomas Wisniewski]

Yes, that seems to have been it, thanks Tim. I'm just waiting for a try-run with that change to finish now before I request a fresh review.

Comment 22

[Thomas Wisniewski]

Attachment: 1261289-allow_webextensions_to_open_view-source_links.diff

Indeed, that did it. Here's an updated patch which passes try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b439151cb148ac9a2ca966d59372d84bf0f7d503

Comment 23

[Tim Nguyen :ntim]

Comment on attachment 8879730 [details] [diff] [review]
1261289-allow_webextensions_to_open_view-source_links.diff

:smaug or someone else should review the c++ changes.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8879730 [details] [diff] [review]
1261289-allow_webextensions_to_open_view-source_links.diff

Please ensure we have tests that view-source: can't be loaded by non-extensions.


"The URIs for this protocol can be loaded only by extensions."
'only' is wrong there. Just drop it.

Comment 25

[Tim Nguyen :ntim]

(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8879730 [details] [diff] [review]
> 1261289-allow_webextensions_to_open_view-source_links.diff
> 
> Please ensure we have tests that view-source: can't be loaded by
> non-extensions.
We do: https://hg.mozilla.org/integration/fx-team/rev/d59fd186550c#l8.3

Comment 26

[Markus Popp]

I would like to finish migrating my addon to a WebExtension which requires this bug to be fixed.

Could somebody please get this done? Thanks!

Comment 27

[Thomas Wisniewski]

kmag, do you feel you'll have time to review this anytime soon, or know someone who can do so in your stead? Thanks!

Comment 28

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8879730 [details] [diff] [review]
1261289-allow_webextensions_to_open_view-source_links.diff

Hi Shane,
I took a brief look at this patch:

the C++ bits have been already reviewed by smaug, and the only change to the WebExtensions internals is related to the additional test case added to the browser_ext_tabs_create.js, which looks fine to me.

Can you take a look at it for the final sign-off related to the changes to the WebExtensions internals bits described above?

Comment 29

[Thomas Wisniewski]

Thanks, guys! The patch still cleanly applies (just one hunk offset a bit), so I'm requesting check-in.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a2e09bc461f
Allow webextensions to open view-source links. r=mixedpuppy, r=smaug

Comment 31

[Tim Nguyen :ntim]

You haven't addressed :smaug's comment 24.

Comment 32

[Thomas Wisniewski]

Good point, thanks ntim. I'll mark this leave-open so I can land a follow-up comment fix before it's closed.

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a2e09bc461f

Comment 34

[Thomas Wisniewski]

Attachment: 1261289-remove_stray_word_from_comment.diff

Here's the follow-up patch to just remove the word "only" from a comment. I don't suspect that a comment change requested in a prior review needs additional review, so I'll just set checkin-needed.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd3442dc847
Follow-up: remove the word 'only' from a comment so it is more accurate. r=ntim

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3dd3442dc847

Comment 37

[Markus Popp]

Does the fix also allow about: URLs?

Is it possible to uplift the fix to Firefox 56?

Comment 38

[Thomas Wisniewski]

No, I'm afraid that this fix is specific to view-source URLs (I believe about pages are covered by bug 1371793).

I'm also not sure if it's worth considering this for an uplift, given that bug 1379345 is still unresolved.

Comment 39

[Will Bamberg [:wbamberg]]

I've noted this limitation of Firefox < 57 in the compat data: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create#Browser_compatibility.

Marking dev-doc-complete, but let me know if we need anything else here.

Comment 40

[Victor Carciu]

Markus,Daniel can you please add a link to your webextensions in order to try to do some QA around this issue in 57?

Comment 41

[Victor Carciu]

Also, I tried to verify this using https://addons.mozilla.org/en-US/firefox/addon/compass_menu/ , and I observed that view-source: links can be opened in a new tab in 57, but not in 56. Please see attached video with my results and let me know if any other testing is needed.

Comment 42

[Victor Carciu]

Attachment: Results using https://addons.mozilla.org/en-US/firefox/addon/compass_menu/

Comment 43

[Markus Popp]

(In reply to Victor Carciu from comment #40)
> Markus,Daniel can you please add a link to your webextensions in order to
> try to do some QA around this issue in 57?

Version 2.0.0b4 of my addon creates a View Source toolbar icon, which works fine starting Firefox 57.0 but not in earlier versions:

https://addons.mozilla.org/en-US/firefox/addon/view-page-source-button/versions/beta?page=1#version-2.0.0b4

Comment 44

[Victor Carciu]

Marking bug as verified.

Comment 45

[Daniel Veditz [:dveditz]]

(In reply to Victor Carciu from comment #40)
> Markus,Daniel can you please add a link to your webextensions in order to
> try to do some QA around this issue in 57?

https://addons.mozilla.org/en-US/firefox/addon/open-selected-links/

Right-click on a link to show the "View link source" menu item. It's now working for me. Thanks Thomas!