Closed Bug 1552141 Opened 6 years ago Closed 5 years ago

Quantum Bar ignores keywords pointing to javascript by default

Categories

(Firefox :: Address Bar, defect, P1)

68 Branch
defect
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 70
Iteration:
70.1 - Jul 8 - 21
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 69+ verified
firefox67 --- unaffected
firefox68 - wontfix
firefox69 + verified
firefox70 --- verified

People

(Reporter: janmoesen_=-bugzilla-=+spamtrap, Assigned: mak)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(3 files)

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

Steps to reproduce:

In a new profile (with browser.urlbar.quantumbar set to true, by default in newer nightlies):

  1. Create a bookmark with location “javascript:alert(%s)” and keyword “ex”.
  2. Type “ex 1<Enter>” in the address bar.
  3. Type “ex 2<Alt+Enter>” in the address bar.

Actual results:

  1. The default search engine is used to search for whatever suggestion has come up first, e.g. “ex 1.1 class 10”.
  2. The default search engine is used to search for whatever suggestion has come up first, e.g. “ex 2.3 class 10” in a new tab.

Expected results:

  1. A message box with the text “1” should have popped up in the current tab.
  2. A message box with the text “2” should have popped up in a new tab.

This behaviour makes it impossible to use bookmarklets (bookmarks with the location “javascript:<some code>”). It still seems to work for “normal” HTTP/HTTPS bookmarks.

The workaround (apart from disabling the Quantum Bar) I have found so far is to get rid of the autosuggest dropdown by pressing <Escape> before pressing <Enter> or <Alt+Enter>.

mozregression narrowed it down to bug 1548031, which simply enables the Quantum Bar, and so does not tell us the original patch that caused this. Sorry.

I did not find a dupe, but I did come across these FIXED/DUPLICATE issues: bug 1523952, bug 1549320, bug 1544380 and bug 1539483. No dice.

Has STR: --- → yes
Component: Untriaged → Address Bar

:janmoesen_=-bugzilla-=+spamtrap, if you think that's a regression, then could you try to find a regression range in using for example mozregression?

Didn't try to reproduce yet, but seems like worth investigating, maybe we are filtering out too much, we do have filters for javascript urls, in particular browser.urlbar.filter.javascript, it's possible the filtering doesn't consider the heuristic result for keywords properly.

How is your browser.urlbar.filter.javascript pref set in about:config? Does flipping that make any difference?

Flags: needinfo?(janmoesen_=-bugzilla-=+spamtrap)
Status: UNCONFIRMED → NEW
Points: --- → 2
Ever confirmed: true
Priority: -- → P2

Tested in a new profile: browser.urlbar.filter.javascript is true by default. When I set it to false, the behaviour is interesting. The first suggestion when I type “ex 1” in the list now appears to be the bookmarklet. If I then press <Enter> or <Alt>+<Enter>, the address bar contains “javascript:alert(1)”, but no alert is issued, so I guess the code never ran.

Manually entering javascript:alert(123) does not run, either. Am I right to guess that setting browser.urlbar.filter.javascript to false solves this bug/problem, and the non-execution is another thing?

Flags: needinfo?(janmoesen_=-bugzilla-=+spamtrap)

For the original issue, it should actually not be necessary to set browser.urlbar.filter.javascript to false, I just wanted to check if my supposition was correct. It is still a bug that I can handle here.

The other bug is actually bug 1506100, it broke 6 months ago and there's no consensus about what to do. For now you can use the devtools console for immediate execution.

Assignee: nobody → mak77
Status: NEW → ASSIGNED

Though, re-reading your comment again, even if I fix the code to return the js keyword, it won't work due to bug 1506100.
So this is not actually actionable, returning a non-working entry wouldn't be better than returning no entry :(

We'll have to figure out what to do with js urls, so this may not be fixed in time for v68.

Assignee: mak77 → nobody
No longer blocks: quantumbar-release
Status: ASSIGNED → NEW
Depends on: 1506100
Priority: P2 → P3

Thanks for the detailed follow-up. I get the reasoning behind filtering JavaScript URLs, and I understand that my use case is rather exotic. It's just that I have a whole collection of highly useful (in my opinion) bookmarklets that I use all the time: http://janmoesen.github.io/bookmarklets/

The basic idea behind most of them is that they work like “normal” keyworded bookmarks, but try to be smart about parameters. If there is no parameter (no %s), it looks at the currently selected text on the page, or the currently active INPUT element, etc. So I can type “enw mozilla” to open up the Mozilla article on the English Wikipedia, or I can select some text and type “enw” to look that up. (And if no text is selected, it prompts for one.)

In case the behaviour these bookmarklets depend on is not coming back, I guess I could somehow convert them into simple add-ons that listen to the address bar input, or some such. To be honest, I have not looked at that yet because of the convenience, low overhead, and portability of bookmarklets.

It's not that exotic for developers: I, too, have a large collection of bookmarklets. Some of the non-parameterized ones I guess I can launch from the bookmark menus, but this is horribly slow and inefficient compared to Ctrl-L <keyword>. The parameterized ones won't work at all. Yes, bookmarklets are a kind of hack, but they're over 20 years old and work reliably across browsers (at least the non-replacement ones). Firefox's keywords are a true win.

An alternative might be to create a first-class stored macro functionality in DevTools, but that seems even less likely to happen than fixing bug 1506100

[Tracking Requested - why for this release]: Functionality regression likely to bite developers.

Though, re-reading your comment again, even if I fix the code to return the js keyword, it won't work due to bug 1506100.

I'm not quite following this. Disabling quantumbar makes javascript: keyword bits work fine for me... Is the issue that quantumbar treats bookmark keywords like user-typed javascript: and not like bookmarks, while non-quantumbar treats them as bookmarks?

For the rest, I also have javascript: bookmarklets that are broken now; I hadn't noticed until today because I had quantumbar turned off due to bug 1550025. :(

Flags: needinfo?(mak77)
Keywords: regression
Regressed by: 1548031

@bzbarsky

Does toggling the "browser.urlbar.quantumbar" to false make things work right for you?

yes, thank you for the tip

Is the issue that quantumbar treats bookmark keywords like user-typed javascript: and not like bookmarks

I remember, a long time ago, urlbar entered javascript urls were blocked, while javascript bookmarklets were intentionally allowed.

That bug 1506100 exists, suggests the ban on entered javascript urls was lifted at some point? And then regressed?

since javascript bookmarklets were intended to work regardless of whether entered js urls are allowed, this shouldn't depended on bug 1506100

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #9)

I'm not quite following this. Disabling quantumbar makes javascript: keyword bits work fine for me... Is the issue that quantumbar treats bookmark keywords like user-typed javascript: and not like bookmarks, while non-quantumbar treats them as bookmarks?

Probably we are filtering javascript urls regardless of their origin, while previously we allowed them for bookmarks.

I'll have a look, but it's too late for 68, imo.

Flags: needinfo?(mak77)
Assignee: nobody → mak77
Status: NEW → ASSIGNED

At this stage for 68 the only thing we can realistically do is disable quantumbar, which doesn't seem warranted for this particular bug.

Can we at least release note this?

Flags: needinfo?(jcristau)

Definitely. Can you suggest some text? I'm afraid I'm not familiar with bookmarklets.

Flags: needinfo?(jcristau)

Maybe something something like:

There is a known issue with triggering javascript: bookmarklets via bookmark keywords in Firefox 68 due to a new, much faster, URL bar implementation that does not handle them correctly yet. See https://bugzilla.mozilla.org/show_bug.cgi?id=1552141.

I don't know whether we should mention the turn-off pref for people who rely on javascript: bookmarklets triggered via keyword for their daily work; I feel like we should, but then we'll get people stuck with quantumbar turned off like I did. :(

Fwiw, I largely agree with comment 16 in terms of shipping with functionality regressions, unless we have really good data that the new implementation is much better for most users. This is somewhat mitigated by the fact that this really is an intersection of two somewhat-niche features (bookmark keywords and bookmarklets). But I'm pretty surprised the functionality regressions were not tracked to start with and the apparent lack of explicit decision to ship with them apart from "well, its too late now"...

It's not just about having data that the new implementation is better (and it is), we have a pile of critical work on top of it, for which we put enormous efforts into being able to release in 68. That's why I don't think it was fair for comment 16 to ask disabling a whole feature for a partial regression of a niche feature, it's also offending towards release managers, that make a great work at finding the right balance.

This was not tracked because until yesterday nobody knew that this was a QB regression, we thought it regressed with bug 1506100, that doesn't have a clear solution yet.

This was not tracked because until yesterday nobody knew that this was a QB regression,

I'm sorry, but comment 0 quite explicitly says it's a QB regression. I have no idea what comment 1 was about (apart from the bot generally being buggy, I guess), since comment 0 already says mozregression was used and found bug 1548031...

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #19)

I'm sorry, but comment 0 quite explicitly says it's a QB regression. I have no idea what comment 1 was about (apart from the bot generally being buggy, I guess), since comment 0 already says mozregression was used and found bug 1548031...

Yes, and as stated in comment 3 even disabling the filter (browser.urlbar.filter.javascript) the js code didn't run... so while filtering is broken by QB, if the js doesn't run there was no point in fixing the filter.

Indeed I have problems confirming the feature works with QB off, I added a javascript:alert(%s) keyword, with QB disabled is still doesn't work for me, it puts the js address in the urlbar, but doesn't execute it. Could you please provide me a working example or STR?

Flags: needinfo?(bzbarsky)
Summary: Quantum Bar ignores bookmark keywords (bookmarklets) by default → Quantum Bar ignores keywords pointing to javascript by default

I just tried these STR in a 2019-07-05 nightly on Mac, with a clean profile:

  1. Bookmarks > Show All Bookmarks, to open the bookmark library window.
  2. Ctrl+click on "Other Bookmarks" in the left pane.
  3. Select "New Bookmark..."
  4. Type javascript:alert("%s") for the location (note the " there!).
  5. Type testme for the keyword
  6. Click "Add" and close the bookmark library window.
  7. Load about:blank in a tab (to ensure that there is no CSP weirdness involved).
  8. Type "testme xyz" in the URL bar and hit enter. This results in the browser loading https://www.google.com/search?client=firefox-b-d&q=testm (note that part of the string was lost, btw). That said, the exact behavior here varies; I just tried it again and this time I got https://www.google.com/search?client=firefox-b-d&q=test+me+zgjedhje+anglisht.
  9. Load about:config
  10. Filter for "quantumbar"
  11. Toggle it to false.
  12. Load about:blank in a tab (to ensure that there is no CSP weirdness involved).
  13. Type "testme xyz" in the URL bar and hit enter. This results in an alert.

In terms of user-visible behavior before hitting enter, at step 8 the dropdown below the URL bar contains a bunch of search suggestions with the first one selected; the actual suggestions shown seem to vary in time quite a bit. At step 13 the dropdown below the URL bar contains a single item listing itself as moz-action:keyword,{"url":"javascript:alert("xyz")", "keyword":"testme", "input":"testme xyz"}: xyz and that item is selected; hitting enter presumably commits to that item.

Do those steps work for you?

Flags: needinfo?(bzbarsky) → needinfo?(mak77)

Oh, and if I do the above steps but set browser.urlbar.filter.javascript to false but leave quantumbar enabled, then before I hit enter the dropdown before the URL bar shows a single entry with a star, then a globe, then the text ": xyz" and hitting enter has the behavior described in comment 3: places the URL in the URL bar but does not run it. That part may well be bug 1506100.

If I set browser.urlbar.filter.javascript to false and disable quantumbar, then typing "testme xyz" in the URL bar shows the moz-action thing in the dropdown, and hitting enter shows the alert. So it sounds like browser.urlbar.filter.javascript does affect what things quantumbar shows in the dropdown and preselects, but doesn't affect whether it shows/selects the moz-action thing...

Thanks, that's extremely useful.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #22)

Oh, and if I do the above steps but set browser.urlbar.filter.javascript to false but leave quantumbar enabled, then before I hit enter the dropdown before the URL bar shows a single entry with a star, then a globe, then the text ": xyz" and hitting enter has the behavior described in comment 3: places the URL in the URL bar but does not run it. That part may well be bug 1506100.

the ": xyz" is because the url doesn't have a host, we usually show the host before ":", so that's another bug, we should fallback to something else.

If I set browser.urlbar.filter.javascript to false and disable quantumbar, then typing "testme xyz" in the URL bar shows the moz-action thing in the dropdown, and hitting enter shows the alert.

yes, moz-action was supposed to never appear, so that is a bug of the old implementation.
So many bugs around this thing!

Flags: needinfo?(mak77)

I have a patch, I still have to write a test for it. It's simple enough that we may evaluate a dot release, if one should be necessary. Surely it can be uplifted to ESR and Beta.
There's no need for further information, thank you for the help.

Priority: P3 → P1
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/e1fdd812bd79 The address bar is filtering bookmarklets and not executing them. r=dao

Thank you for fixing this!

failing on UNIX, not Windows :( Will look at it tomorrow.

Flags: needinfo?(mak77)
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/7aa396100a76 The address bar is filtering bookmarklets and not executing them. r=dao
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

Comment on attachment 9076478 [details]
Bug 1552141 - The address bar is filtering bookmarklets and not executing them. r=dao

Beta/Release Uplift Approval Request

  • User impact if declined: Bookmarklets not working
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment 21
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): porting a few lines of code that were already in the old urlbar code
  • String changes made/needed: none

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: breaking a feature used by technical users or web developers
  • User impact if declined: Bookmarklets not working
  • Fix Landed on Version: 69
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): porting a few lines of code that were already in the old urlbar code
  • String or UUID changes made by this patch: none
Attachment #9076478 - Flags: approval-mozilla-esr68?
Attachment #9076478 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Flags: in-testsuite+

Comment on attachment 9076478 [details]
Bug 1552141 - The address bar is filtering bookmarklets and not executing them. r=dao

Fixes a QuantumBar regression causing bookmarklets to not work. Includes new automated tests. Approved for 69.0b4.

Attachment #9076478 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Verified as fixed on the latest Firefox Nightly 70.0a1 (2019-07-11) on Windows 10 x64, Mac OS X 10.15, Mac OS X 10.14 and on Ubuntu 18.04 x64.

Backed out changeset 2c36222570a9 for causing bc failures in browser_keywordBookmarklets.js

Backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/ed2ad6dc85a7a2d1b631cefa577acc0232c4ea0f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&selectedJob=255944696&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&searchStr=%28bc&fromchange=66c851b1788288a0e0553728651e34a4a6cddffe

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=255961525&repo=mozilla-beta&lineNumber=5831

[task 2019-07-11T15:24:57.034Z] 15:24:57 INFO - TEST-START | browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js
[task 2019-07-11T15:24:58.357Z] 15:24:58 INFO - TEST-INFO | started process screentopng
[task 2019-07-11T15:24:58.756Z] 15:24:58 INFO - TEST-INFO | screentopng: exit 0
[task 2019-07-11T15:24:58.758Z] 15:24:58 INFO - Buffered messages logged at 15:24:57
[task 2019-07-11T15:24:58.759Z] 15:24:58 INFO - Entering test bound setup
[task 2019-07-11T15:24:58.760Z] 15:24:58 INFO - Console message: [JavaScript Error: "uncaught exception: Object" {file: "resource://testing-common/PromiseTestUtils.jsm" line: 112}]
[task 2019-07-11T15:24:58.761Z] 15:24:58 INFO - Type keyword and immediately press enter
[task 2019-07-11T15:24:58.762Z] 15:24:58 INFO - Awaiting pageshow event
[task 2019-07-11T15:24:58.763Z] 15:24:58 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js | "javascript:''%20" == "javascript:''%20" -
[task 2019-07-11T15:24:58.765Z] 15:24:58 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js | content has the same principal - "moz-nullprincipal:{1f9f585d-ece5-4a2e-ad32-3c860c891a3a}" == "moz-nullprincipal:{1f9f585d-ece5-4a2e-ad32-3c860c891a3a}" -
[task 2019-07-11T15:24:58.766Z] 15:24:58 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js | both principals should be null principals in the parent - true == true -
[task 2019-07-11T15:24:58.767Z] 15:24:58 INFO - Type keyword with searchstring and immediately press enter
[task 2019-07-11T15:24:58.768Z] 15:24:58 INFO - Awaiting pageshow event
[task 2019-07-11T15:24:58.768Z] 15:24:58 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js | "javascript:'a'%20" == "javascript:'a'%20" -
[task 2019-07-11T15:24:58.769Z] 15:24:58 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js | content has the same principal - "moz-nullprincipal:{bb220e16-65e6-4fe8-9a26-e04933f53746}" == "moz-nullprincipal:{bb220e16-65e6-4fe8-9a26-e04933f53746}" -
[task 2019-07-11T15:24:58.771Z] 15:24:58 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js | both principals should be null principals in the parent - true == true -
[task 2019-07-11T15:24:58.771Z] 15:24:58 INFO - Buffered messages logged at 15:24:58
[task 2019-07-11T15:24:58.772Z] 15:24:58 INFO - Search keyword, then press enter
[task 2019-07-11T15:24:58.773Z] 15:24:58 INFO - Buffered messages finished
[task 2019-07-11T15:24:58.773Z] 15:24:58 INFO - TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js | Check title - "" == "javascript:'' " - JS frame :: chrome://mochitests/content/browser/browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js :: setup/testFns< :: line 36
[task 2019-07-11T15:24:58.775Z] 15:24:58 INFO - Stack trace:
[task 2019-07-11T15:24:58.775Z] 15:24:58 INFO - chrome://mochitests/content/browser/browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js:setup/testFns<:36
[task 2019-07-11T15:24:58.776Z] 15:24:58 INFO - Awaiting pageshow event
[task 2019-07-11T15:24:58.776Z] 15:24:58 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js | "javascript:''%20" == "javascript:''%20" -
[task 2019-07-11T15:24:58.778Z] 15:24:58 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js | content has the same principal - "moz-nullprincipal:{5ad9991e-2ab9-4d56-a112-3e3a0394a7f2}" == "moz-nullprincipal:{5ad9991e-2ab9-4d56-a112-3e3a0394a7f2}" -
[task 2019-07-11T15:24:58.780Z] 15:24:58 INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_keywordBookmarklets.js | both principals should be null principals in the parent - true == true -
[task 2019-07-11T15:24:58.781Z] 15:24:58 INFO - Search keyword with searchstring, then press enter
[task 2019-07-11T15:24:59.018Z] 15:24:59 INFO - Not taking screenshot here: see the one that was previously logged

Flags: needinfo?(mak77)

it's a rebasing problem, on Beta we are running the test also in non-quantumbar mode, but that's not supported (nightly is in qb-only mode). I'll post a beta patch removing the test in non-qb mode.

Flags: needinfo?(mak77)
Attached patch Beta patch (deleted) — Splinter Review

The beta patch is identical, but the test is removed from browser/components/urlbar/tests/legacy/browser.ini

Whiteboard: [checkin-needed-beta]

For ESR68 we may need a further patch due to prettier.

esr68 is due to get the prettier treatment next week so you may want to hold off on that. :)

Verified as fixed on Firefox 69.0b5 on Windows 10 x64, Mac OS X 10.15, Mac OS X 10.14 and on Ubuntu 18.04 x64.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Iteration: --- → 70.1 - Jul 8 - 21

Looks like ESR68 will need a rebased patch even post-prettification.

Flags: needinfo?(mak77)

Did you try to apply the beta patch to it?

Flags: needinfo?(mak77) → needinfo?(ryanvm)

Yes, it was the Beta uplift which fails to graft.

Flags: needinfo?(ryanvm)
Flags: needinfo?(mak77)

It doesn't apply because of the changes from bug 1557793.

Flags: needinfo?(mak77)
Attached patch ESR Patch (deleted) — Splinter Review
Attachment #9079851 - Flags: checkin?(ryanvm)
Attachment #9076478 - Flags: approval-mozilla-esr68?
Comment on attachment 9079851 [details] [diff] [review] ESR Patch Thanks for providing the rebased patch for ESR68. Approved for 68.1esr also.
Attachment #9079851 - Flags: checkin?(ryanvm) → approval-mozilla-esr68+

Verified as fixed on Firefox 68.1.0esr on Windows 10 x 64, Mac OS X 10.14 and on Ubuntu 18.04 x64.
Firefox 68.1.0esr build was taken from here: https://tools.taskcluster.net/index/gecko.v2.mozilla-esr68.latest.firefox

There's another 68 bookmarklet regression. With the quantumbar off, bookmarklets fail to execute from a fx windows inital tab.

  • win7, fx 68.0.1, new profile
  • disable quantumbar
  • add a bookmarklet: javascript:void alert('bookmarklet');
  • invoke it via the keyword on the window's inital tab
  • it does not run and the following is logged in the browser console: Content Security Policy: The page’s settings blocked the loading of a resource at inline (“script-src”).
  • open a new tab and invoke the bookmarklet there, it runs, although there's still something wrong, the tab's content is cleared and it's put in a permanent loading state.
  • same thing in a new window

here's another 68 bookmarklet regression. With the quantumbar off, bookmarklets fail to execute from a fx window inital tab.

This appears to be fixed in 69

but this

open a new tab and invoke the bookmarklet there, it runs, although there's still something wrong, the tab's content is cleared and it's put in a permanent loading state.

is not, so created a new issue for it: Bug 1573047

it does not run and the following is logged in the browser console: Content Security Policy: The page’s settings blocked the loading of a resource at inline (“script-src”).

This is unrelated to quantumbar or this bug, and is covered by bug 1478037.

Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: