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...
<rdar://problem/25931513>
Created attachment 278203 [details] Patch
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.
(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.
(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
Created attachment 278301 [details] Patch
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.
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.
Created attachment 278564 [details] Patch
Created attachment 278638 [details] Patch
Created attachment 278640 [details] Patch
Created attachment 278662 [details] Patch
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
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
Created attachment 278668 [details] Patch
Created attachment 278683 [details] Patch
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.
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?
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.
Created attachment 278744 [details] Patch for landing
Committed r200799: <http://trac.webkit.org/changeset/200799>
Committed r200800: <http://trac.webkit.org/changeset/200800> for build fix.