Changing owner to rohit@ since justincohen is OOO.
Rohit, could you PTAL.

re-assigning to justincohen@ per original report.  (He has touched the relevant code.)

I can't tell what the bug is here.  When !editing we force everything LTR.  When editing we allow RTL.

All four of these URLS appear with 127.0.0.1 shown as the leftmost host when !editing, which is correct.

All four of these URLs appear as RTL when editing (with the attack domain first), since that's allowed.

Sorry for the late reply.

It's really complicated and took me a long time of staring at the video to figure out what's going on. This is a guess; it's hard to tell what the exact strings are. I've broken the video down second-by-second:

CASE 1
Begin with a tab open at the address: http://127.0.0.1/%D8%A7%D8%A7%D8%A7%D8%A7%D8%A7%D8%A7/http%3A//evil.com
0:02: Open that tab
Displays (non-focus) as:
‎127.0.0.1/اااااا/http://evil.com
(i.e., as a URL)
0:03: Focus Omnibox.
Displays (focus) as:
‫127.0.0.1/اااااا/http://evil.com‬
(i.e., as plain text)

CASE 2
0:15: Edit "evil" to "gmail" and navigate.
Displays (non-focus) as:
‎127.0.0.1/اااااا/http://gmail.com
(i.e., as a URL)
Displays (focus) as:
‫127.0.0.1/اااااا/http://gmail.com‬
(i.e., as plain text)
(same as previous case).

CASE 3
0:23: Select-all. Paste the following string from clipboard: "127.0.0.1/ا/http://attack.com"
This corresponds to the URL: http://127.0.0.1/%D8%A7/http%3A//attack.com

Displays (focus) as:
‎127.0.0.1/ا/http://attack.com
(i.e., as a URL; *inconsistent* with previously seen behaviour)
Also it looks like the suggested URL is being displayed as plain text, not as a URL.
0:25: Navigate to de-focus the Omnibox.
Displays (non-focus) as:
‎127.0.0.1/ا/http://attack.com
(i.e., as a URL)
0:26: Focus omnibox.
Displays (focus) as:
‫127.0.0.1/ا/http://attack.com‬
(i.e., as plain text, for a fraction of a second)
0:27: Not sure if tapped, or just waited, but the display changes.
Displays (focus) as:
127.0.0.1/ا/http://attack.com
(i.e., as a URL)

As I wrote previously, it's not clear whether editing should be displayed as a URL or as plain text (depends if you are able to detect when it's a valid URL and when it isn't). But there is some inconsistency seen here in Case 3. Are you able to reproduce this if you carefully execute the steps mentioned above?

I can't reproduce this.  What I can reproduce is the alignment is different when looking at the pre-edit label (when everything is selected on initial omnibox focus) and actually editing the text (tapping on the pre-edit label).

I'm still not sure what is correct.  rohitrao@ do you have any preference here?

We briefly played with this on an iPhone and couldn't repro either.

rayyanh: Are you able to give us any further details about this? Such as what iOS and Chrome version you are running in this video?

The bug is working fine on iOS 10.3 and Chrome 59;

Let me *try* to make it simple for you.

After executing the link "http://127.0.0.1/ا/http://evil.com" when you double tap the link (to edit it), the link still shows "http://127.0.0.1/ا/http://evil.com" (which is fine)

However, the inconsistency is showed when you use multiple arabic letters in the link such as "http://127.0.0.1/اااااا/http://evil.com"; after executing it, when you double tap the link, the link is showed as "‫127.0.0.1/اااااا/http://evil.com‬
"‬

> The bug is working fine on iOS 10.3 and Chrome 59;

What do you mean by "working fine"? Is the bug exhibited on those versions, or not?

Yes, the bug is exhibited on those versions.

I can reproduce this, but I see the text is also laying out RTL.

My understanding is we do not do anything special when *editing* text in the omnibox -- we just follow normal Apple layout for the text.  Unless we want to force LTR when editing, which I do not think we want to do, this seems to be working correctly.  What would we do differently?  Force LTR layout?  Show RTL layout, but somehow flip the text?

It's fine for the text to lay out RTL while being edited. The problem is that in some cases, it's being laid out with an LTR top-level while being edited (from Comment #5, Case 3 shows this inconsistent behaviour).

Are you able to reproduce the behaviour shown in Case 3? Is that Apple's renderer doing this? Or has an LRM snuck into the editable text field perhaps?

mgiuca@ I don't know if this is the issue being described, but I think I see an inconsistency with how the pre-edit label shows. 

For case #3, 127.0.0.1/ا/http://attack.com, I see 127 first when editing and not editing, but I see attack first for the pre-edit label (after focusing the omnibox)

Is that issue?

#14 I don't know what "pre-edit label" is. Can you be more descriptive? Also when you say "I see attack first" do you mean the string shows as "http://attack.com/<l>/127.0.0.1" (where <l> is the Arabic character)?

When you focus the omnibox we show a blue selection box, but we do not show the left/right handlebars, or the cut/copy/share buttons.  If you tap a second time you get the normal iOS selection.  In this first, fake, selection, I see, "http://attack.com/<l>/127.0.0.1".  When the omnibox is unfocused, focused and not showing the fake selection, I see "127.0.0.1/ا/http://attack.com" 

I think it's an easy fix -- I just want to make sure this is the issue described.

#16 I see, the 0:26 in the video is the "fake selection" whereas 0:27 is real selection with handlebars.

What fix are you proposing?

What I still don't understand is the focused behaviour earlier in the video, at 0:03 and 0:15. In those two cases, the focused text is shown as plain text, whereas at 0:27 it is shown as a URL. Are you able to reproduce those?

I'm not sure what you mean 'shown as plain text' vs 'shown as URL'.  So far the only issue I can see is that the preedit label doesn't override LTR.  This is an easy change and shouldn't be controversial.  How about try to we make this change and then see if there's still an issue?

See: https://chromium-review.googlesource.com/c/chromium/src/+/641658

#18 Yes, "shown as URL" just means embedded in an LTR paragraph context.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e86b68d914f37845d5193099d83aafd1b4788496

commit e86b68d914f37845d5193099d83aafd1b4788496
Author: Justin Cohen <justincohen@google.com>
Date: Wed Aug 30 14:23:18 2017

[ios] Override paragraph style as LTR for omnibox pre-edit label.

Pre-edit label is always left-to-right, and needs the same paragraph style
override to compensate for unicode RLM/LRM codes.

Bug:  717868 
Change-Id: I0796237be3f598e6b222c02307b33d1cef27ec23
Reviewed-on: https://chromium-review.googlesource.com/641658
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498455}
[modify] https://crrev.com/e86b68d914f37845d5193099d83aafd1b4788496/ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm

Checked on chrome beta version 62.0.3202.39 on iPhone 6s plus with iOS 10.3.3, 11.0.  

The 127.0.0.1/اااااا/http://evil.com does not change to 
‫127.0.0.1/اااااا/http://evil.com‬, it remains same 127.0.0.1/اااااا/http://evil.com

on tapping or focusing on the omnibox second time changes the URL to 
‫127.0.0.1/اااااا/http://evil.com‬.


Video link:

https://drive.google.com/a/google.com/file/d/0Bz2uwV55gGwDMVRHUGtXZEhpSlE/view?usp=sharing