Bug 157396 – REGRESSION (r199313): ICBC app: text field In the webview is not tappable

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Jiewen Tan]

I'm trying to login to my account for first time

The bank makes me do 2 step auth by sending me a text message

I get the text message with the code but I can't enter it back can't tap on the text field...

[Jiewen Tan]

<rdar://problem/25931513>

[Jiewen Tan]

Created attachment 278203 [details]
Patch

[Brady Eidson]

Comment on attachment 278203 [details]
Patch

Was the original security fix important?

If it's important to have the recc. behavior in the browser, there are ways we can have diff behavior in browser vs in app.

[Jiewen Tan]

(In reply to comment #3)
> Comment on attachment 278203 [details]
> Patch
> 
> Was the original security fix important?
> 
> If it's important to have the recc. behavior in the browser, there are ways
> we can have diff behavior in browser vs in app.

I don't think so as I can't come out a way to exploit it, but it doesn't mean others can't. I wonder if you could tell me that how I could make a change such that it only affect the behavior of apps. I will really appreciate that.

[Chris Dumez]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 278203 [details]
> > Patch
> > 
> > Was the original security fix important?
> > 
> > If it's important to have the recc. behavior in the browser, there are ways
> > we can have diff behavior in browser vs in app.
> 
> I don't think so as I can't come out a way to exploit it, but it doesn't
> mean others can't. I wonder if you could tell me that how I could make a
> change such that it only affect the behavior of apps. I will really
> appreciate that.

See RuntimeApplicationChecks.h

[Jiewen Tan]

Created attachment 278301 [details]
Patch

[Darin Adler]

Comment on attachment 278301 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278301&action=review

> Source/WebCore/ChangeLog:17
> +        Even though the HTML spec suggests that we should navigate to about:blank
> +        for subframes/iframes, of which the src cannot be resolved to an valid URL,
> +        many iOS WebKit1 apps are relying on invalid URLs to bridge their WebView
> +        codes and their native Objective-C codes. Therefore, a quirk is added to
> +        escape the URL validation.

What about Mac WebKit 1 apps? They aren’t doing this?

> Source/WebCore/ChangeLog:21
> +        The assertion is actually uesless as latter on in void URL::parse(const char*, const String*),
> +        we have an early return for the same check.

I believe this change is incorrect. The assertion is still valuable even though URL::parse will also detect the invalid case. I suspect the real problem is that somewhere we are treating a URL string as already parsed, when it’s not. I’d need to see the backtrace of the firing assertion to figure out where the real problem lies.

> Source/WebCore/loader/SubframeLoader.cpp:98
> +#if PLATFORM(IOS)
> +    // FIXME: remove this quirk once iOS WebKit1 apps become almost extinct.
> +    if (IOSApplication::isWebProcess() && !url.isValid())
> +        url = blankURL();
> +#else
>      if (!url.isValid())
>          url = blankURL();
> +#endif

We don’t want to replicate the code to add a quirk. Instead we should have a function or a local variable that tells whether to apply the quirk. It will be a constant in the non-quirky case.

    if (!shouldNotConvertInvalidURLsToBlank() && !url.isValid())
        url = blankURL();

I am not sure that IOSApplication::isWebProcess() is quite the right way to detect all iOS WebKit 1 use. And also, for quirks like this we normally use "linked on or after" checks so that anyone who recompiles their app has to stop relying on the quirk.

I think the FIXME is a little strange. It’s going to be a long, long time before iOS WebKit 1 apps are "almost extinct". I am not sure we should even have that comment.

[Jiewen Tan]

Comment on attachment 278301 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278301&action=review

Thanks for reviewing my code!

>> Source/WebCore/ChangeLog:17
>> +        escape the URL validation.
> 
> What about Mac WebKit 1 apps? They aren’t doing this?

Fixed. Mac WebKit1 apps that uses Cordova are significantly less than iOS. Since there are a few (~30 known apps), I add a quirk for them as well.

>> Source/WebCore/ChangeLog:21
>> +        we have an early return for the same check.
> 
> I believe this change is incorrect. The assertion is still valuable even though URL::parse will also detect the invalid case. I suspect the real problem is that somewhere we are treating a URL string as already parsed, when it’s not. I’d need to see the backtrace of the firing assertion to figure out where the real problem lies.

Removed. I agree that this assertion can reveal some problems. However, it doesn't mean it always holds. There are exceptions that a parsed invalid URL will not follow the assertion that it starts with a scheme. For example, a relative URL that is based on data URL will result be an invalid URL that with the relative URL's string, which could start with any characters.

>> Source/WebCore/loader/SubframeLoader.cpp:98
>> +#endif
> 
> We don’t want to replicate the code to add a quirk. Instead we should have a function or a local variable that tells whether to apply the quirk. It will be a constant in the non-quirky case.
> 
>     if (!shouldNotConvertInvalidURLsToBlank() && !url.isValid())
>         url = blankURL();
> 
> I am not sure that IOSApplication::isWebProcess() is quite the right way to detect all iOS WebKit 1 use. And also, for quirks like this we normally use "linked on or after" checks so that anyone who recompiles their app has to stop relying on the quirk.
> 
> I think the FIXME is a little strange. It’s going to be a long, long time before iOS WebKit 1 apps are "almost extinct". I am not sure we should even have that comment.

FIXED.

[Jiewen Tan]

Created attachment 278564 [details]
Patch

[Jiewen Tan]

Created attachment 278638 [details]
Patch

[Jiewen Tan]

Created attachment 278640 [details]
Patch

[Jiewen Tan]

Created attachment 278662 [details]
Patch

[Build Bot]

Comment on attachment 278662 [details]
Patch

Attachment 278662 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1305538

New failing tests:
fast/loader/iframe-src-invalid-url.html

[Build Bot]

Created attachment 278666 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Jiewen Tan]

Created attachment 278668 [details]
Patch

[Jiewen Tan]

Created attachment 278683 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 278683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278683&action=review

r=me with the fix for non-iOS/non-OSX clients to be secure by default.

> Source/WebKit/mac/WebView/WebView.mm:885
> +    static bool shouldConvertInvalidURLsToBlank = false;

This should be true because we want to be secure by default.

[Chris Dumez]

Comment on attachment 278683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278683&action=review

>> Source/WebKit/mac/WebView/WebView.mm:885
>> +    static bool shouldConvertInvalidURLsToBlank = false;
> 
> This should be true because we want to be secure by default.

Could we make these const?

[Jiewen Tan]

Comment on attachment 278683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278683&action=review

>>> Source/WebKit/mac/WebView/WebView.mm:885
>>> +    static bool shouldConvertInvalidURLsToBlank = false;
>> 
>> This should be true because we want to be secure by default.
> 
> Could we make these const?

I don't know if it is legitimate, but most of the existing codes use static only.

[Jiewen Tan]

Created attachment 278744 [details]
Patch for landing

[Jiewen Tan]

Committed r200799: <http://trac.webkit.org/changeset/200799>

[Jiewen Tan]

Committed r200800: <http://trac.webkit.org/changeset/200800> for build fix.