Closed Bug 1261289 Opened 4 years ago Closed 2 years ago

Allow webextensions to open view-source: links

Categories

(WebExtensions :: General, enhancement, P3)

enhancement

Tracking

(firefox57 fixed)

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: dveditz, Assigned: wisniewskit)

References

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(3 files, 1 obsolete file)

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.
Priority: -- → P4
Whiteboard: triaged
Component: WebExtensions: Untriaged → WebExtensions: General
Priority: P4 → P3
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.
Duplicate of this bug: 1356648
I'm trying to convert my addon to a Webextension and need this to work.

Could this please be fixed sooner than later?
(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!
(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??????????????????
BTW, this may not only affect view-source: URLs but also all the "about:..." URLs and maybe others.
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.
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.
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 ?
> 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.
(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.
If someone has the time to work on it and needs a mentor, we'll gladly try and find one.
The steps in comment 12 proved to be enough to get this working; here's a patch.
Attachment #8879576 - Flags: review?(kmaglione+bmo)
Attachment #8879576 - Flags: review?(bugs)
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 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.
Attachment #8879576 - Flags: review?(kmaglione+bmo)
Attachment #8879576 - Flags: review?(bugs)
(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
A quick fix you can try is keeping URI_DANGEROUS_TO_LOAD in GetProtocolFlags along with URI_LOADABLE_BY_EXTENSIONS.
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.
Assignee: nobody → wisniewskit
Indeed, that did it. Here's an updated patch which passes try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b439151cb148ac9a2ca966d59372d84bf0f7d503
Attachment #8879576 - Attachment is obsolete: true
Attachment #8879730 - Flags: review?(kmaglione+bmo)
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.
Attachment #8879730 - Flags: review?(bugs)
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.
Attachment #8879730 - Flags: review?(bugs) → review+
(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
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!
kmag, do you feel you'll have time to review this anytime soon, or know someone who can do so in your stead? Thanks!
Flags: needinfo?(kmaglione+bmo)
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?
Flags: needinfo?(kmaglione+bmo)
Attachment #8879730 - Flags: review?(mixedpuppy)
Attachment #8879730 - Flags: review?(kmaglione+bmo)
Attachment #8879730 - Flags: feedback+
Attachment #8879730 - Flags: review?(mixedpuppy) → review+
Thanks, guys! The patch still cleanly applies (just one hunk offset a bit), so I'm requesting check-in.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a2e09bc461f
Allow webextensions to open view-source links. r=mixedpuppy, r=smaug
Keywords: checkin-needed
You haven't addressed :smaug's comment 24.
Good point, thanks ntim. I'll mark this leave-open so I can land a follow-up comment fix before it's closed.
Keywords: leave-open
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.
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3dd3442dc847
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Does the fix also allow about: URLs?

Is it possible to uplift the fix to Firefox 56?
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.
Keywords: dev-doc-needed
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.
Markus,Daniel can you please add a link to your webextensions in order to try to do some QA around this issue in 57?
Flags: needinfo?(dveditz)
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.
(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
Marking bug as verified.
Status: RESOLVED → VERIFIED
(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!
Flags: needinfo?(dveditz)
Depends on: CVE-2018-5134
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.