Closed
Bug 1261289
Opened 9 years ago
Closed 7 years ago
Allow webextensions to open view-source: links
Categories
(WebExtensions :: General, enhancement, P3)
WebExtensions
General
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)
(deleted),
patch
|
mixedpuppy
:
review+
smaug
:
review+
rpl
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
application/x-zip-compressed
|
Details |
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.
Updated•9 years ago
|
Priority: -- → P4
Whiteboard: triaged
Updated•8 years ago
|
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.
Comment 3•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
BTW, this may not only affect view-source: URLs but also all the "about:..." URLs and maybe others.
Comment 7•8 years ago
|
||
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 hidden (advocacy) |
Comment 9•8 years ago
|
||
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 hidden (advocacy) |
Comment hidden (abuse-reviewed) |
Comment 12•8 years ago
|
||
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•8 years ago
|
||
> 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•8 years ago
|
||
(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•8 years ago
|
||
If someone has the time to work on it and needs a mentor, we'll gladly try and find one.
Assignee | ||
Comment 16•7 years ago
|
||
The steps in comment 12 proved to be enough to get this working; here's a patch.
Attachment #8879576 -
Flags: review?(kmaglione+bmo)
Updated•7 years ago
|
Attachment #8879576 -
Flags: review?(bugs)
Comment 17•7 years ago
|
||
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.
Assignee | ||
Comment 18•7 years ago
|
||
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)
Comment 19•7 years ago
|
||
(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•7 years ago
|
||
A quick fix you can try is keeping URI_DANGEROUS_TO_LOAD in GetProtocolFlags along with URI_LOADABLE_BY_EXTENSIONS.
Assignee | ||
Comment 21•7 years ago
|
||
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
Assignee | ||
Comment 22•7 years ago
|
||
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 23•7 years ago
|
||
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 24•7 years ago
|
||
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+
Comment 25•7 years ago
|
||
(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•7 years ago
|
||
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!
Assignee | ||
Comment 27•7 years ago
|
||
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 28•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8879730 -
Flags: review?(mixedpuppy) → review+
Assignee | ||
Comment 29•7 years ago
|
||
Thanks, guys! The patch still cleanly applies (just one hunk offset a bit), so I'm requesting check-in.
Keywords: checkin-needed
Comment 30•7 years ago
|
||
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
Comment 31•7 years ago
|
||
You haven't addressed :smaug's comment 24.
Assignee | ||
Comment 32•7 years ago
|
||
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
Comment 33•7 years ago
|
||
bugherder |
Assignee | ||
Comment 34•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open → checkin-needed
Comment 35•7 years ago
|
||
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
Comment 36•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 37•7 years ago
|
||
Does the fix also allow about: URLs?
Is it possible to uplift the fix to Firefox 56?
Assignee | ||
Comment 38•7 years ago
|
||
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.
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 39•7 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
Comment 40•7 years ago
|
||
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)
Comment 41•7 years ago
|
||
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•7 years ago
|
||
Comment 43•7 years ago
|
||
(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
Reporter | ||
Comment 45•7 years ago
|
||
(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)
Reporter | ||
Updated•7 years ago
|
Depends on: CVE-2018-5134
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•