1419391 - Firefox address bar spoof using RTL language and references

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

+++ This bug was initially created as a clone of Bug #1395508 +++

In Bug 1395508 we are going to keep spaces encoded when they are adjacent, to avoid possible spoofing attempts.
Though, the real host is still not visible, and thus the result may still be confusing for users.
This follow-up bug is about showing the real host whenever it's needed.

PS: I'm copying sec keywords from bug 1395508, I don't know if the security team wants to re-evaluate once bug 1395508 is fixed.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: always show the host

Comment 2

[:Gijs (he/him)]

Comment on attachment 8930490 [details] [diff] [review]
always show the host

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

Generally this approach looks sane. Some smaller comments below. We may want to land tests separately if this is going to ride the trains.

::: browser/base/content/browser.css
@@ +580,5 @@
> +  direction: ltr;
> +  pointer-events: none;
> +}
> +
> +html|input.urlbar-scheme[textoverflow="start"]:not([focused]):valid {

Why do we need the `:valid` pseudoselector?

::: browser/base/content/urlbarBindings.xml
@@ +27,5 @@
>  
>      <content sizetopopup="pref">
>        <xul:hbox flex="1" class="urlbar-textbox-container">
>          <children includes="image|deck|stack|box"/>
> +        <xul:stack anonid="textbox-input-box"

This is going to be not fun for talos, I suspect. :-(

Can we achieve the same effect without using a stack by using an element with negative margins? Per discussion on IRC, it seems this might be tricky - I would expect that using the same element type in the same container with the same XUL align/pack stuff, sizing CSS etc., it "should" work. Maybe Dão has more specific ideas about what would make this tricky to do...

@@ +156,5 @@
>          this.inputField.removeEventListener("mousemove", this);
>          this.inputField.removeEventListener("mouseout", this);
>          this.inputField.removeEventListener("overflow", this);
>          this.inputField.removeEventListener("underflow", this);
> +        this.inputField.removeEventListener("scroll", this);

As per IRC discussion, we might want to use the  new scrollend listener. Either that or talking to :masayuki or other Editor folks about how platform can help us here.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8930490 [details] [diff] [review]
always show the host

Discussed briefly with Gijs about whether we should continue on this. For now I'm putting the work on pause, the severity of the issue needs to be re-audited now that we fixed the adjacent whitespaces problem in bug 1395508.
Additionally, we may want to move to a completely different "origin-only" representation of the url in the Address Bar that would pretty much resolve this and other future spoof bugs, once and for all, but that requires some infra-team investigation that still has to happen.

Comment 5

[Daniel Veditz [:dveditz]]

Spoofing example from bug 1447954: http://www.gmail.com.xn--ggbla3j.xn--ngbc5azd/%DB%B0.html

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Putting back this bug at the current status, that is currently unassigned.

The next steps would be to discuss and evaluate if we want to go the Safari path, only show the origin by default and show the full url when hovering/tooltip. Gijs, I'm not sure how we should proceed from here, should a product manager take care of this?

Comment 7

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #6)
> Putting back this bug at the current status, that is currently unassigned.
> 
> The next steps would be to discuss and evaluate if we want to go the Safari
> path, only show the origin by default and show the full url when
> hovering/tooltip. Gijs, I'm not sure how we should proceed from here, should
> a product manager take care of this?

Yes. ISTR we talked with :phlsa about this (who I realize is UX and not product, but still), but I don't remember where/when...
Philipp and/or Peter, are you the right people to talk to?

I'll note that this is a pretty serious change for a security issue that IMO is relatively minor. It has all kinds of nasty edgecases, like how do you persist selection state in a hidden input element in a way that is not confusing to users, etc. etc. So not sure if we should go ahead with that... Equally, not sure that we have (m)any other options if we want to address this type of spoofing. Showing punycode feels wrong, too.

Comment 8

[xisigr]

(In reply to Marco Bonardo [::mak] from comment #6)
> Putting back this bug at the current status, that is currently unassigned.
> 
> The next steps would be to discuss and evaluate if we want to go the Safari
> path, only show the origin by default and show the full url when
> hovering/tooltip. Gijs, I'm not sure how we should proceed from here, should
> a product manager take care of this?

Only show the origin doesn't fully resolve these issues. You can still do RTL spoofs with just the origin.

Comment 9

[:Gijs (he/him)]

(In reply to xisigr from comment #8)
> (In reply to Marco Bonardo [::mak] from comment #6)
> > Putting back this bug at the current status, that is currently unassigned.
> > 
> > The next steps would be to discuss and evaluate if we want to go the Safari
> > path, only show the origin by default and show the full url when
> > hovering/tooltip. Gijs, I'm not sure how we should proceed from here, should
> > a product manager take care of this?
> 
> Only show the origin doesn't fully resolve these issues. You can still do
> RTL spoofs with just the origin.

How/why? The original spoof example depends on the RTL characters causing the actual domain to have a slash prepended. That wouldn't happen if just showing the hostname - it would just show as "http://www.gmail.com.اماء.شبكة/" which is just as non-spoofy as "http://www.gmail.com.myfancyowndomain.com/"...

Jonathan, is there some way we can change our text rendering code to avoid rendering the slashes between the domain and its subdomains here?

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to :Gijs (he/him) from comment #9)
> Jonathan, is there some way we can change our text rendering code to avoid
> rendering the slashes between the domain and its subdomains here?

What result would you hope to see instead?

If we have the opportunity to munge the URL before putting it into the urlbar to display, I guess we could insert Unicode directional controls to force a desired outcome -- e.g. I think inserting a Left-To-Right Mark at the end of the domain would change the example (comment 7) from displaying 

    http://www.gmail.com.اماء.شبكة/٠.html

to the (arguably less confusing)

    http://www.gmail.com.اماء.شبكة‎/٠.html

(not sure what the implications would be for a fully RTL url, though, or for a browser running with a RTL UI -- maybe in that case we'd want to insert RLM instead?).

We'd need to be able to filter out such controls if the user copies the text from the urlbar, though, otherwise we're presumably going to break other stuff.

A better way to address this is probably to split the parts of the URL -- scheme, domain, path, [maybe ?query, #fragment] -- into separate elements and then use Unicode directional controls on them (unicode-bidi:isolate, perhaps?), along with any styling to highlight the important parts (I'd suggest that maybe the current black/gray distinction between the domain and the rest of the URL is a bit too subtle; maybe the domain should be highlighted more strongly?)

Comment 11

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

I don't think we really landed on any firm decisions with regards to going the Safari route. Adding Michael Verdi here who has since taken over the URL bar and search work from me.

Summary for Michael:
If I understand it correctly, Gijs is asking whether we have only showing the domain of a site in the URL bar by default (like Safari does) on the roadmap. I remember that it came up once or twice during the search work week, but to my knowledge it's not something we're firmly planning on.

Comment 12

[Verdi [:verdi] Best to slack me]

(In reply to (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #11)
> Summary for Michael:
> If I understand it correctly, Gijs is asking whether we have only showing
> the domain of a site in the URL bar by default (like Safari does) on the
> roadmap. I remember that it came up once or twice during the search work
> week, but to my knowledge it's not something we're firmly planning on.

Right. It's on the list of things to explore this year but we haven't decided to do it or have any other firm plans around it.

Comment 13

[xisigr]

(In reply to :Gijs (he/him) from comment #9)
> (In reply to xisigr from comment #8)
> > (In reply to Marco Bonardo [::mak] from comment #6)
> > > Putting back this bug at the current status, that is currently unassigned.
> > > 
> > > The next steps would be to discuss and evaluate if we want to go the Safari
> > > path, only show the origin by default and show the full url when
> > > hovering/tooltip. Gijs, I'm not sure how we should proceed from here, should
> > > a product manager take care of this?
> > 
> > Only show the origin doesn't fully resolve these issues. You can still do
> > RTL spoofs with just the origin.
> 
> How/why? The original spoof example depends on the RTL characters causing
> the actual domain to have a slash prepended. That wouldn't happen if just
> showing the hostname - it would just show as
> "http://www.gmail.com.اماء.شبكة/" which is just as non-spoofy as
> "http://www.gmail.com.myfancyowndomain.com/"...


In origin the labels shown out-of-order. I give another classic case. Please access http://www.gmail.com.xn--mgb.999.xn--ggbla3j.xn--ngbc5azd in firefox. What will you see in the address bar?

Is it http://www.gmail.com.ا.999.اماء.شبكة/ ?

Comment 14

[:Gijs (he/him)]

(In reply to xisigr from comment #13)
> Please
> access http://www.gmail.com.xn--mgb.999.xn--ggbla3j.xn--ngbc5azd in firefox.
> What will you see in the address bar?
> 
> Is it http://www.gmail.com.ا.999.اماء.شبكة/ ?

Yes, but this seems like a separate issue... Does it actually work with something other than numbers - like latin text - in the seemingly-post-fixed domain segment? Because I expect not, and I expect the cause is that the unit of numbers has weak directionality in the absence of any other strong-directionality text in the same host segment. Put differently, I couldn't get this "spoof" to work once I put an "a" anywhere in the "999" segment.

> In origin the labels shown out-of-order. I give another classic case.

How is this is a "classic case"? Is this already on file elsewhere?

Obviously we could "workaround" by enforcing fully-LTR display of the entire URL. That would presumably not display fully arabic or hebrew IDN domains correctly though, which doesn't seem like a good idea.


(In reply to Jonathan Kew (:jfkthame) from comment #10)
> (In reply to :Gijs (he/him) from comment #9)
> > Jonathan, is there some way we can change our text rendering code to avoid
> > rendering the slashes between the domain and its subdomains here?
> 
> What result would you hope to see instead?
> 
> If we have the opportunity to munge the URL before putting it into the
> urlbar to display, I guess we could insert Unicode directional controls to
> force a desired outcome -- e.g. I think inserting a Left-To-Right Mark at
> the end of the domain would change the example (comment 7) from displaying 
> 
>     http://www.gmail.com.اماء.شبكة/٠.html
> 
> to the (arguably less confusing)
> 
>     http://www.gmail.com.اماء.شبكة‎/٠.html

Yes, for this particular URL I think this would work better. Funnily enough, the same suggestion appears in https://bugzilla.mozilla.org/show_bug.cgi?id=525831 .

> (not sure what the implications would be for a fully RTL url, though, or for
> a browser running with a RTL UI -- maybe in that case we'd want to insert
> RLM instead?).

I don't know the answer to this, though https://bugzilla.mozilla.org/show_bug.cgi?id=525831 implies that we may want to do the same "even" for fully-RTL urls.

> We'd need to be able to filter out such controls if the user copies the text
> from the urlbar, though, otherwise we're presumably going to break other
> stuff.

We can do that - we already do copy-filtering of the URL bar in order to re-prefix "http://" - though we'd need to be quite specific to avoid filtering out used-typed instances of such characters, I guess.

Marco, is this something you have cycles to look into?

Comment 15

[:Gijs (he/him)]

(In reply to Verdi [:verdi] from comment #12)
> (In reply to (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX)
> please use needinfo from comment #11)
> > Summary for Michael:
> > If I understand it correctly, Gijs is asking whether we have only showing
> > the domain of a site in the URL bar by default (like Safari does) on the
> > roadmap. I remember that it came up once or twice during the search work
> > week, but to my knowledge it's not something we're firmly planning on.
> 
> Right. It's on the list of things to explore this year but we haven't
> decided to do it or have any other firm plans around it.

OK, then let's see if we can come up with alternative solutions given that it's not clear we want to do this in the first place.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to :Gijs (he/him) from comment #14)
> Marco, is this something you have cycles to look into?

So the currently proposed solutions are either:
a. https://bugzilla.mozilla.org/show_bug.cgi?id=525831#c12
b. adding a mark at the end of the host
All of this considering various cases: mixed-direction host, rtl host, fully rtl url.
All of this considering two possible ui directions.
It still looks like a little bit under specified, thus it will need experimentation.

Moreover, now we seem to be tracking two different problems here?
1. confusing position of / in mixed alignment urls
2. long urls with RTL aligned host where the host is overflowed and thus not visible

I won't have time shortly, I'm open to experiment a bit with this, but currently there are a couple perf issues I must investigate for other teams that should make 61, and a training at the end of the month. So, probably not before May.

Comment 17

[xisigr]

(In reply to :Gijs (he/him) from comment #14)
> (In reply to xisigr from comment #13)
> > Please
> > access http://www.gmail.com.xn--mgb.999.xn--ggbla3j.xn--ngbc5azd in firefox.
> > What will you see in the address bar?
> > 
> > Is it http://www.gmail.com.ا.999.اماء.شبكة/ ?
> 
> Yes, but this seems like a separate issue... Does it actually work with
> something other than numbers - like latin text - in the seemingly-post-fixed
> domain segment? Because I expect not, and I expect the cause is that the
> unit of numbers has weak directionality in the absence of any other
> strong-directionality text in the same host segment. Put differently, I
> couldn't get this "spoof" to work once I put an "a" anywhere in the "999"
> segment.
> 


http://www.gmail.com.סו.اماء.شبكة/ 

I think U+05D5 (ו )/U+05D7 ( ח )/U+05E1 (ס) should be shown in punycode


===============

In addition, I just submitted another bug 1453861 about RLT spoof. You can spoof all 'io/no' TLD in address bar at least.

Comment 18

[Jonathan Kew [:jfkthame]]

Isn't that basically the same as already discussed in bug 1395854?

Comment 19

[:Gijs (he/him)]

(In reply to Jonathan Kew (:jfkthame) from comment #18)
> Isn't that basically the same as already discussed in bug 1395854?

I agree, marked as a dupe.

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I have a patch that implements the suggestion in comment 10 (that is pretty much the same that was suggested earlier in other RTL bugs) to add a force LTR char at the end of the domain.
It has a few advantages:
* it's much simpler than playing with selection, easy to backout in case of problems too
* it fixes comment 5
* it makes the hidden-on-overflow case moot, because the domain won't slide anymore at the end
It also has a few disadvantages:
* on focus we remove the marker and the domain slides elsewhere
* due to the above, a port number moves from after to before the domain

I think those disadvantages are acceptable compared to the spoof-safe benefit and simplicity, but likely will require some RTL user to try it, I really don't use RTL domains at all.

There are still a few failures to look into (looks like problems with trimurl), but you may try it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=152f02414bfd9a952bcd6fd6309d0d8bb8ac0178

Comment 22

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #21)
> I have a patch that implements the suggestion in comment 10 (that is pretty
> much the same that was suggested earlier in other RTL bugs) to add a force
> LTR char at the end of the domain.
> It has a few advantages:
> * it's much simpler than playing with selection, easy to backout in case of
> problems too
> * it fixes comment 5
> * it makes the hidden-on-overflow case moot, because the domain won't slide
> anymore at the end
> It also has a few disadvantages:
> * on focus we remove the marker and the domain slides elsewhere
> * due to the above, a port number moves from after to before the domain

I think these sets of advantages/disadvantages sound OK, but like you I don't really use RTL domains...

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The problem with the tests is that they don't expect to find the forceLTR char

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to :Gijs (he/him) from comment #22)
> (In reply to Marco Bonardo [::mak] from comment #21)

> > It also has a few disadvantages:
> > * on focus we remove the marker and the domain slides elsewhere
> > * due to the above, a port number moves from after to before the domain

I think I'd find it pretty disturbing if the displayed URL changes on focus/blur. I guess I was imagining that if we use a control like this, it would always be present in the URL bar text that we render (we'd just filter it out when copying or using the URL). But maybe that isn't feasible.

Comment 25

[:Gijs (he/him)]

(In reply to Jonathan Kew (:jfkthame) from comment #24)
> (In reply to :Gijs (he/him) from comment #22)
> > (In reply to Marco Bonardo [::mak] from comment #21)
> 
> > > It also has a few disadvantages:
> > > * on focus we remove the marker and the domain slides elsewhere
> > > * due to the above, a port number moves from after to before the domain
> 
> I think I'd find it pretty disturbing if the displayed URL changes on
> focus/blur. I guess I was imagining that if we use a control like this, it
> would always be present in the URL bar text that we render (we'd just filter
> it out when copying or using the URL). But maybe that isn't feasible.

I guess then we'd need to distinguish us putting one in vs. the user typing one... though perhaps we don't care about that edgecase?

Comment 26

[Jonathan Kew [:jfkthame]]

The one we insert would always be at a known position (the end of the domain), right? So we'd filter out just that one occurrence, and leave any others that happen to exist.

I suppose we'd also need to watch the user's edits, and automagically restore our control character if it ever gets deleted -- which _will_ happen.

Comment 27

[:Gijs (he/him)]

(In reply to Jonathan Kew (:jfkthame) from comment #26)
> The one we insert would always be at a known position (the end of the
> domain), right? So we'd filter out just that one occurrence, and leave any
> others that happen to exist.

Where is the end of the domain? This is especially not-entirely-trivial when it comes to arbitrary user selections that won't necessarily include the protocol, or will have spaces / other chars that make them invalid URLs, or move the "normal" start of the post-domain stuff (e.g. by inserting "/" in the middle of what used to be the domain).

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

After some testing, I can tell this approach is not trivial as well, less fragile than the old approach but has some edge cases.

Tracking the position of our added char is not trivial by itself due to trimURL, and we'd have to track any of the user's edits, what if the user edits a url to a non-url? Then our position is just screwed up.
Cleaning up things on focus/edit reduces the likelihood of regressions and failures.
For these reasons I considered the added char as part of the url formatting we do, also because without url formatting we also lack domain hilighting, so there's no great value in trying to keep the domain visible when we don't even emphasize it. When we hilight we know the thing is a valid url, I can find the domain and add the char.
Also, my initial implementation just removes any forceRTL char (can the user actually insert it manually and is that an acceptable char in an url?). Striong the position is again fragile, the user can do any kind of edit making it pointless.

Even doing that, there is still problem with anything bypassing the urlbar binding, for example DOMWindowUtils.GetSelectionAsPlaintext() (currently used in browser_bug1261299.js, maybe not a big deal).

I'm trying at least to reach a green Try run, then we can look at the patch and find solutions.

Comment 29

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I also just had an alternative idea for my initial proposal.
Let's sum up:

Approach A)
Always scroll the urlbar so that the host is visible. If the scheme becomes invisible and it's not trimmed, put it into a box so it stays visible.
I can now probably implement this in a non-fragile way.
Downside:
It won't solve comment 5. On the other side, domain hilighting makes that case less critical, because we de-emphasize the "spoofed" domain. This should be deduped from the original bug, but I'm not sure what to do about it without modifying the string. Maybe we should just emphasise the domain more, or de-emphasise the rest more.

Approach B)
Add a forceLTR char at the end of the domain.
Downside:
Getting all the hook points to add/remove the char is more complex than it looks, and I suspect it will bring us more headaches long term.

Approach C)
Just replace all "/" with "LRM/"
downside:
Somewhat similar to approach B

Comment 30

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: approach A

Comment 31

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Always show the domain and the scheme (if not http)

At this time I don't feel comfortable with the url modifying approach, because there are too many unknowns about code paths working with the input field value. I'd suggest duping bug 1447954 to bug 525831, and keep that one to investigate url modifications. it's a problem that exists from quite some time and it's not even addressed in other browsers.

This patch works quite well for me, I addressed previous feedback and it doesn't have anymore the fragile layout flush handling of the first version. It also doesn't prevent us from fixing bug 525831 in the future (I added a specific check for that).

Comment 32

[:Gijs (he/him)]

Comment on attachment 8987776 [details] [diff] [review]
Always show the domain and the scheme (if not http)

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

I haven't had a chance to test this, but off-hand it seems workable... I just have some questions, and I would like to test things and review the test.

::: browser/base/content/browser.css
@@ +611,5 @@
> +  pointer-events: none;
> +}
> +
> +/* Visible if the urlbar is not focused and it overflows at the start.
> +   Uses the required-valid trick to check if it contains a value */

Do we need to do something to avoid us ever trying to display the form validation popup for this field?

::: browser/base/content/urlbarBindings.xml
@@ +588,5 @@
> +          this.selectionStart = this.selectionEnd = 0;
> +          window.requestAnimationFrame(() => {
> +            let isDomainRTL = this.DOMWindowUtils.getDirectionFromText(domain);
> +            if (isDomainRTL && value[preDomain.length + domain.length] != "\u200E") {
> +              this.inputField.scrollLeft = this.inputField.scrollLeftMax;

*Why* do we need to check for the forceRTL char (and can you update the comment with that info) ?

Do we need to check if the url bar's focused state (ie focused or not focused) changed since the rAF was called? And/or the domain/value (ie avoid doing work if we know we've re-entered) ?

@@ +1420,5 @@
> +      <method name="updateTextOverflow">
> +        <body><![CDATA[
> +          let side = "end";
> +          if (this._inOverflow) {
> +            side = this.inputField.scrollLeft == this.inputField.scrollLeftMax ? "start" : "end";

This flushes layout, I think, but there isn't a non-flushing alternative. Is it difficult to use promiseDocumentFlushed here?

The other thing I'm wondering here is whether it's ever possible for this code to be called when the cursor is somewhere in the middle of the url bar and thus we essentially overflow on both sides. Do we make the right change here then? Is this not possible for some reason that I'm missing?

Comment 33

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to :Gijs (he/him) from comment #32)
> Do we need to do something to avoid us ever trying to display the form
> validation popup for this field?

Which popup? I didn't notice a thing, but I suppose that is part of form manager, this is a normal input field.
If you have ideas on how I can try to cause something like that, please let me know.

> ::: browser/base/content/urlbarBindings.xml
> @@ +588,5 @@
> > +          this.selectionStart = this.selectionEnd = 0;
> > +          window.requestAnimationFrame(() => {
> > +            let isDomainRTL = this.DOMWindowUtils.getDirectionFromText(domain);
> > +            if (isDomainRTL && value[preDomain.length + domain.length] != "\u200E") {
> > +              this.inputField.scrollLeft = this.inputField.scrollLeftMax;
> 
> *Why* do we need to check for the forceRTL char (and can you update the
> comment with that info) ?

Just because if in the future we add a forceRTL char, I don't want this to regress (as I said at the end of comment 31).
And anyway, I have no idea if that char is usable in a tld, and if it is we don't want to overflow to the wrong side.

> Do we need to check if the url bar's focused state (ie focused or not
> focused) changed since the rAF was called? And/or the domain/value (ie avoid
> doing work if we know we've re-entered) ?

I'll add some checks.

> @@ +1420,5 @@
> > +      <method name="updateTextOverflow">
> > +        <body><![CDATA[
> > +          let side = "end";
> > +          if (this._inOverflow) {
> > +            side = this.inputField.scrollLeft == this.inputField.scrollLeftMax ? "start" : "end";
> 
> This flushes layout, I think, but there isn't a non-flushing alternative. Is
> it difficult to use promiseDocumentFlushed here?

I won't be able to check _inOverflow since the event has gone, but maybe it's not strictly necessary. will check.

> The other thing I'm wondering here is whether it's ever possible for this
> code to be called when the cursor is somewhere in the middle of the url bar
> and thus we essentially overflow on both sides. Do we make the right change
> here then? Is this not possible for some reason that I'm missing?

If the field is not focused we always set selection to 0.
If it's focused we don't show the mask, so the value of textoverflow doesn't matter (but it matters that there's an overflow attribute, for the url tooltip).

Comment 34

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Always show the domain and the scheme (modulo trimURL)

Comment 35

[:Gijs (he/him)]

Comment on attachment 8991548 [details] [diff] [review]
Always show the domain and the scheme (modulo trimURL)

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

Some behavior seems to not be quite right yet. STR:

1. open web console
2. execute to go to testcase from bug 1395508: location.href = "https://xn--ggbla1c4e.xn--ngbc5azd/#"+Array(0x200).join("%20")+"סוֹ.סח"
3. open another tab if this is the only tab, then go back to tab with the dodgy RTL URL
4. drag tab into new window

ER: location bar looks the same
AR: no fixed scheme box anymore, looks like formatValue didn't finish running at all?


This seems to also happen with "regular" URLs... and actually, now that I'm checking, it also seems to happen pre-patch. I'll file a separate bug for this, doesn't need to block landing this.

::: browser/base/content/browser.css
@@ +592,5 @@
> +  position: absolute;
> +  height: 100%;
> +  -moz-appearance: none;
> +  border: none;
> +  padding: 0 1px;

Can you note which properties here are just mirroring .textbox-input ? Is there a reason we shouldn't/can't give it that class to avoid duplicating these (and potentially getting them out of sync)?

::: browser/base/content/test/urlbar/browser_urlOverflow.js
@@ +13,5 @@
> +               "Check the scheme box visibility");
> +
> +  gURLBar.blur();
> +  // We must wait for multiple layout flushes.
> +  await new Promise(resolve => setTimeout(resolve, 500));

Could you use waitForAttribute to avoid the yucky setTimeout? (  https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#1433 )

@@ +40,5 @@
> +function toUnicode(input) {
> +  let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
> +                    .createInstance(Ci.nsIScriptableUnicodeConverter);
> +  converter.charset = "UTF-8";
> +  return converter.ConvertToUnicode(input);

I'm a bit surprised we need this. Can you elaborate on why this is necessary?

Comment 36

[:Gijs (he/him)]

I filed bug 1475589 for the formatting going AWOL.

Comment 37

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to :Gijs (he/him) from comment #35)
> Can you note which properties here are just mirroring .textbox-input ? Is
> there a reason we shouldn't/can't give it that class to avoid duplicating
> these (and potentially getting them out of sync)?

No reason apart from the fact I forgot about it, I added the class and it works well, good catch.

> ::: browser/base/content/test/urlbar/browser_urlOverflow.js
> @@ +13,5 @@
> > +               "Check the scheme box visibility");
> > +
> > +  gURLBar.blur();
> > +  // We must wait for multiple layout flushes.
> > +  await new Promise(resolve => setTimeout(resolve, 500));
> 
> Could you use waitForAttribute to avoid the yucky setTimeout? ( 

Nope, waitForAttribute requires the attribute to change after the action, here it doesn't always change.
Though, I can try to use waitForCondition.

> @@ +40,5 @@
> > +function toUnicode(input) {
> > +  let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
> > +                    .createInstance(Ci.nsIScriptableUnicodeConverter);
> > +  converter.charset = "UTF-8";
> > +  return converter.ConvertToUnicode(input);
> 
> I'm a bit surprised we need this. Can you elaborate on why this is necessary?

It seems to be necessary for the expected behavior and makes easy to write the test. I also used it in browser_urlbarCopying.js
Why do you think it's a problem?

Comment 38

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Let's see how the test behaves now
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eea25d7b779ef46ec8a714bfdb31f9dd3c3c0326

Comment 39

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Always show the domain and the scheme (modulo trimURL)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eea25d7b779ef46ec8a714bfdb31f9dd3c3c0326

Comment 40

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #37)
> > @@ +40,5 @@
> > > +function toUnicode(input) {
> > > +  let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
> > > +                    .createInstance(Ci.nsIScriptableUnicodeConverter);
> > > +  converter.charset = "UTF-8";
> > > +  return converter.ConvertToUnicode(input);
> > 
> > I'm a bit surprised we need this. Can you elaborate on why this is necessary?
> 
> It seems to be necessary for the expected behavior and makes easy to write
> the test. I also used it in browser_urlbarCopying.js
> Why do you think it's a problem?

I guess I think of our files as already being UTF-8 so converting to UTF-8 seems like something that shouldn't be necessary... maybe I misunderstand how you're using it?

Comment 41

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Always show the domain and the scheme (modulo trimURL)

This just uses a plain escaped js string, so we avoid any fancy conversion
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4177df5ddc829f0fe4fed3f4f8ee40c3693e9981

Comment 42

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Always show the domain and the scheme (modulo trimURL)

Added a non-zero check, covering the case where scrollLeft is zero and the placeholder overflows.

Comment 43

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6233f4d899c3da8143461b19201baf69e63595f

https://hg.mozilla.org/mozilla-central/rev/b6233f4d899c

Comment 44

[Julien Cristau [:jcristau]]

Should we uplift this to 62 and/or esr60?

Comment 45

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I don't have an answer yet. The patch in itself is not particularly scary, but there are unknowns, and a regression in bug 1478489 that I didn't have time to look into yet. Surely we want to address that before evaluating an uplift.

Comment 46

[ovidiu boca[:Ovidiu]]

Attachment: the fix from bug 1419391.png

I tested this issue using the link from https://bugzilla.mozilla.org/show_bug.cgi?id=1395508#c1 on Mac OS X 10.12, Windows 10 x64 and Ubuntu 18.04 with the latest Nightly 63.0a1(2018-07-31) but before to confirm the fix I have some questions:

1. Please take a look at the attached print screen and tell me if this is the expected result? To be sure that I'm looking for the right behavior. 
2. If I open the link in full screen and then I resize the window the real host (سماء.شبكة) is not visible, please see this screen recording: https://streamable.com/7iein   This behavior is expected? 
3. When you are trying to select something from the URL, I see a gap. Please see the screen recording: https://streamable.com/srulf   Note: This behavior is happening only with this URL. 

Note: I didn't use RTL builds to test this issue if I need to use them please tell me and I will retest it.

Comment 47

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to ovidiu boca[:Ovidiu] from comment #46)
> 1. Please take a look at the attached print screen and tell me if this is
> the expected result? To be sure that I'm looking for the right behavior. 

Yes, it's the right behavior, the origin should remain visible.

> 2. If I open the link in full screen and then I resize the window the real
> host (سماء.شبكة) is not visible, please see this screen recording:
> https://streamable.com/7iein   This behavior is expected? 

No, this is a not handled case, we should probably handle the "resize" event better.
It's less critical than the original bug here imo, since the user is unlikely to resize the window just after a navigation and before actually noticing the url.
Anyway, it should be filed and fixed in a new bug (imo a normal one)

> 3. When you are trying to select something from the URL, I see a gap. Please
> see the screen recording: https://streamable.com/srulf   Note: This behavior
> is happening only with this URL. 

Is this a regression or did it happen before too? The code I added here, afaict, is completely disabled when the input field is focused.

> Note: I didn't use RTL builds to test this issue if I need to use them
> please tell me and I will retest it.

Yes please, both LTR and RTL builds would be nice.

Comment 48

[Jonathan Kew [:jfkthame]]

(In reply to ovidiu boca[:Ovidiu] from comment #46)

> 2. If I open the link in full screen and then I resize the window the real
> host (سماء.شبكة) is not visible, please see this screen recording:
> https://streamable.com/7iein   This behavior is expected? 

Aside from resizing, this seems to show another issue: at the beginning of the recording (e.g. at time 0.01), before the window is resized, it looks like the right-hand edge of the URL (which is the *beginning* of the host name), is being faded out. That seems a bit unfortunate; it makes it much less obvious that this part of the URL is really the most significant.

> 3. When you are trying to select something from the URL, I see a gap. Please
> see the screen recording: https://streamable.com/srulf   Note: This behavior
> is happening only with this URL.

That's normal when selecting in mixed-direction content (here, the % signs inherit the URL's RTL directionality, but the digits "20" have LTR direction); a single continuous (in the underlying text) selection, may render as multiple visually-discontiguous parts. It's similar to the effect if you drag slowly through "the real host (سماء.شبكة) is not visible", but at the level of each individual "%20" group.

Comment 49

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Jonathan Kew (:jfkthame) from comment #48)
> (In reply to ovidiu boca[:Ovidiu] from comment #46)
> Aside from resizing, this seems to show another issue: at the beginning of
> the recording (e.g. at time 0.01), before the window is resized, it looks
> like the right-hand edge of the URL (which is the *beginning* of the host
> name), is being faded out.

Maybe he already shrinked the window first, it seems to be the same exact resize bug.

Comment 50

[ovidiu boca[:Ovidiu]]

(In reply to Jonathan Kew (:jfkthame) from comment #48)
> (In reply to ovidiu boca[:Ovidiu] from comment #46)

> 
> Aside from resizing, this seems to show another issue: at the beginning of
> the recording (e.g. at time 0.01), before the window is resized, it looks
> like the right-hand edge of the URL (which is the *beginning* of the host
> name), is being faded out. That seems a bit unfortunate; it makes it much
> less obvious that this part of the URL is really the most significant.


Like Marco suggested the window was already shrunk first. 

(In reply to Marco Bonardo [::mak] from comment #47)
> (In reply to ovidiu boca[:Ovidiu] from comment #46)
> > 1. Please take a look at the attached print screen and tell me if this is
> > the expected result? To be sure that I'm looking for the right behavior. 
> 
> Yes, it's the right behavior, the origin should remain visible.

Thanks for clearing this out. 
> 
> > 2. If I open the link in full screen and then I resize the window the real
> > host (سماء.شبكة) is not visible, please see this screen recording:
> > https://streamable.com/7iein   This behavior is expected? 
> 
> No, this is a not handled case, we should probably handle the "resize" event
> better.
> It's less critical than the original bug here imo, since the user is
> unlikely to resize the window just after a navigation and before actually
> noticing the url.
> Anyway, it should be filed and fixed in a new bug (imo a normal one)

I will file a new bug for this issue. 
> 
> > 3. When you are trying to select something from the URL, I see a gap. Please
> > see the screen recording: https://streamable.com/srulf   Note: This behavior
> > is happening only with this URL. 
> 
> Is this a regression or did it happen before too? The code I added here,
> afaict, is completely disabled when the input field is focused.
> 

From comment 48 I understand that this is the right behaviour. 

> > Note: I didn't use RTL builds to test this issue if I need to use them
> > please tell me and I will retest it.
> 
> Yes please, both LTR and RTL builds would be nice.

I used LTR build Nightly 63.0a1 (2018-08-01) and everithing seems to be ok. OSes that I used: Mac 10.12, Ubuntu 18.04, Windows 10.
I also tested the RTL build (Arabic) Nightly 63.0a1(2018-08-01) and I found out an issue. If you open the link from https://bugzilla.mozilla.org/show_bug.cgi?id=1395508#c1 and you un-focus the browse window you will see taht that the text from the URL bar is overlaid, here is a print screen with the issue: https://imgur.com/vPY0kQ0. This issue is reproducible on all OSes.

Comment 51

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to ovidiu boca[:Ovidiu] from comment #50)
> I used LTR build Nightly 63.0a1 (2018-08-01) and everithing seems to be ok.
> OSes that I used: Mac 10.12, Ubuntu 18.04, Windows 10.
> I also tested the RTL build (Arabic) Nightly 63.0a1(2018-08-01) and I found
> out an issue. If you open the link from
> https://bugzilla.mozilla.org/show_bug.cgi?id=1395508#c1 and you un-focus the
> browse window you will see taht that the text from the URL bar is overlaid,
> here is a print screen with the issue: https://imgur.com/vPY0kQ0. This issue
> is reproducible on all OSes.

Please file this bug too.

Comment 52

[ovidiu boca[:Ovidiu]]

Here is the bug for resizing the browser window: bug 1480349.
And also, the bug from comment 51 with the RTL build: bug 1480355

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider backporting or can it ride the trains?

Comment 54

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #53)
> Is this something we should consider backporting or can it ride the trains?

Imo it would be better to just leave it in 63, the security problem is not that critical, and as we saw there were a few edge cases to handle. Now all of them are handled, but it'd still be better to do a full cycle.

If anyone has reasons to think this is more critical to backport, please let me know.

Comment 55

[ovidiu boca[:Ovidiu]]

I will mark this as verified based on the fact that the other issues that were found during the testing for this issue are already filed in separate bugs.