220019 2020-12-18 10:43:57 -0800 Login flows on sites with ITP quirks no longer work with ITP disabled 2020-12-18 16:12:08 -0800 1 1 1 Unclassified WebKit WebKit Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 katherine_cheney katherine_cheney achristensen bfulgham darin webkit-bug-importer wilander oldest_to_newest 1715750 0 katherine_cheney 2020-12-18 10:43:57 -0800 We should add a check for ITP enabled before attempting to apply quirks, otherwise the quirks will commandeer the login process, fail (because no ITP), and thus result in inability to login when ITP is disabled. 1715751 1 webkit-bug-importer 2020-12-18 10:44:51 -0800 <rdar://problem/72472858> 1715752 2 416529 katherine_cheney 2020-12-18 10:50:09 -0800 Created attachment 416529 Patch 1715757 3 416529 darin 2020-12-18 10:59:23 -0800 Comment on attachment 416529 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416529&action=review > Source/WebCore/ChangeLog:14 > + No new tests, this will fix login flows on sites with ITP quirks when > + ITP is turned off. I confirmed these cases manually. Disappointing there’s no way to test this. A test has value even in a simple case like this, so if you do think of a way to validate it, that would be welcome. 1715818 4 katherine_cheney 2020-12-18 14:54:05 -0800 (In reply to Darin Adler from comment #3) > Comment on attachment 416529 [details] > Patch Thank you for the review! > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416529&action=review > > > Source/WebCore/ChangeLog:14 > > + No new tests, this will fix login flows on sites with ITP quirks when > > + ITP is turned off. I confirmed these cases manually. > > Disappointing there’s no way to test this. A test has value even in a simple > case like this, so if you do think of a way to validate it, that would be > welcome. I have been trying to think of a good way to test this. We’d need to test that we do not trigger the quirks when ITP is disabled. I don’t believe our test infrastructure supports loading arbitrary domains, so this eliminates testing an actual quirk’d site. One idea is I could add a new simple quirk in this function for localhost and then create a layout test for it to make sure that the quirk does not work with ITP disabled. But, I am not sure it is a good idea to add code in WebCore that is only triggered for a test case. 1715827 5 darin 2020-12-18 15:37:45 -0800 (In reply to katherine_cheney from comment #4) > I have been trying to think of a good way to test this. We’d need to test > that we do not trigger the quirks when ITP is disabled. I don’t believe our > test infrastructure supports loading arbitrary domains, so this eliminates > testing an actual quirk’d site. I think we should consider adding a way to "spoof" which site something is coming from in WebKitTestRunner, to test site-specific hacks. That could be used to make tests for bugs like this one, without requiring we add something specific to this quirk. Worth talking to some other WebKit people about how easy that would be. 1715843 6 katherine_cheney 2020-12-18 16:08:08 -0800 (In reply to Darin Adler from comment #5) > (In reply to katherine_cheney from comment #4) > > I have been trying to think of a good way to test this. We’d need to test > > that we do not trigger the quirks when ITP is disabled. I don’t believe our > > test infrastructure supports loading arbitrary domains, so this eliminates > > testing an actual quirk’d site. > > I think we should consider adding a way to "spoof" which site something is > coming from in WebKitTestRunner, to test site-specific hacks. That could be > used to make tests for bugs like this one, without requiring we add > something specific to this quirk. Worth talking to some other WebKit people > about how easy that would be. Ok, I like that idea. I filed rdar://72484249 and will talk with some others about the best way to do this. 1715848 7 ews-feeder 2020-12-18 16:12:07 -0800 Committed r270995: <https://trac.webkit.org/changeset/270995> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416529. 416529 2020-12-18 10:50:09 -0800 2020-12-18 16:12:08 -0800 Patch bug-220019-20201218105008.patch text/plain 2158 katherine_cheney U3VidmVyc2lvbiBSZXZpc2lvbjogMjcwOTY4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjY1MWMyZmQzOTg2OTJj YWIyZDQ3YWM3ZDUzYTEzY2UxN2Y3MDZhYS4uZWFlNzgxZjliZDgxZTMzMTQ1ZWVmMGNiZTc1NzRk NDA4MGQzNGQ5MyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDIwLTEyLTE4ICBLYXRl IENoZW5leSAgPGthdGhlcmluZV9jaGVuZXlAYXBwbGUuY29tPgorCisgICAgICAgIExvZ2luIGZs b3dzIG9uIHNpdGVzIHdpdGggSVRQIHF1aXJrcyBubyBsb25nZXIgd29yayB3aXRoIElUUCBkaXNh YmxlZAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjIw MDE5CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS83MjQ3Mjg1OD4KKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXZSBzaG91bGQgYWRkIGEgY2hlY2sgZm9y IElUUCBlbmFibGVkIGJlZm9yZSBhdHRlbXB0aW5nIHRvIGFwcGx5IHF1aXJrcywKKyAgICAgICAg b3RoZXJ3aXNlIHRoZSBxdWlya3Mgd2lsbCBjb21tYW5kZWVyIHRoZSBsb2dpbiBwcm9jZXNzLCBm YWlsIChiZWNhdXNlCisgICAgICAgIG5vIElUUCksIGFuZCB0aHVzIHJlc3VsdCBpbiBhbiBpbmFi aWxpdHkgdG8gbG9naW4gd2hlbiBJVFAgaXMgZGlzYWJsZWQuCisKKyAgICAgICAgTm8gbmV3IHRl c3RzLCB0aGlzIHdpbGwgZml4IGxvZ2luIGZsb3dzIG9uIHNpdGVzIHdpdGggSVRQIHF1aXJrcyB3 aGVuCisgICAgICAgIElUUCBpcyB0dXJuZWQgb2ZmLiBJIGNvbmZpcm1lZCB0aGVzZSBjYXNlcyBt YW51YWxseS4KKworICAgICAgICAqIHBhZ2UvUXVpcmtzLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OlF1aXJrczo6dHJpZ2dlck9wdGlvbmFsU3RvcmFnZUFjY2Vzc1F1aXJrIGNvbnN0KToKKwogMjAy MC0xMi0xOCAgTGF1cm8gTW91cmEgIDxsbW91cmFAaWdhbGlhLmNvbT4KIAogICAgICAgICBbV1BF XSBVbnJldmlld2VkIGJ1aWxkZml4IGFmdGVyIHIyNzA5NjQKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3BhZ2UvUXVpcmtzLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BhZ2UvUXVpcmtzLmNwcApp bmRleCA1NGU2ZGUwMzYyZjNjMDQwNjgwZjBhNjcwODJhNTI3ZDlkZWJlODUzLi5mMjZlNGM2NTQx OTMyNTEyY2ExMDg0NzQ4YmJjNTc1Y2Q2OGNlYzQ2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9wYWdlL1F1aXJrcy5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGFnZS9RdWlya3MuY3BwCkBA IC0yOSw2ICsyOSw3IEBACiAjaW5jbHVkZSAiQXR0ci5oIgogI2luY2x1ZGUgIkRPTVRva2VuTGlz dC5oIgogI2luY2x1ZGUgIkRPTVdpbmRvdy5oIgorI2luY2x1ZGUgIkRlcHJlY2F0ZWRHbG9iYWxT ZXR0aW5ncy5oIgogI2luY2x1ZGUgIkRvY3VtZW50LmgiCiAjaW5jbHVkZSAiRG9jdW1lbnRMb2Fk ZXIuaCIKICNpbmNsdWRlICJEb2N1bWVudFN0b3JhZ2VBY2Nlc3MuaCIKQEAgLTEwMDYsNiArMTAw Nyw5IEBAIGJvb2wgUXVpcmtzOjpoYXNTdG9yYWdlQWNjZXNzRm9yQWxsTG9naW5Eb21haW5zKGNv bnN0IEhhc2hTZXQ8UmVnaXN0cmFibGVEb21haW4+CiAKIFF1aXJrczo6U3RvcmFnZUFjY2Vzc1Jl c3VsdCBRdWlya3M6OnRyaWdnZXJPcHRpb25hbFN0b3JhZ2VBY2Nlc3NRdWlyayhFbGVtZW50JiBl bGVtZW50LCBjb25zdCBQbGF0Zm9ybU1vdXNlRXZlbnQmIHBsYXRmb3JtRXZlbnQsIGNvbnN0IEF0 b21TdHJpbmcmIGV2ZW50VHlwZSwgaW50IGRldGFpbCwgRWxlbWVudCogcmVsYXRlZFRhcmdldCkg Y29uc3QKIHsKKyAgICBpZiAoIURlcHJlY2F0ZWRHbG9iYWxTZXR0aW5nczo6cmVzb3VyY2VMb2Fk U3RhdGlzdGljc0VuYWJsZWQoKSkKKyAgICAgICAgcmV0dXJuIFF1aXJrczo6U3RvcmFnZUFjY2Vz c1Jlc3VsdDo6U2hvdWxkTm90Q2FuY2VsRXZlbnQ7CisKICNpZiBFTkFCTEUoUkVTT1VSQ0VfTE9B RF9TVEFUSVNUSUNTKQogICAgIGlmICghbmVlZHNRdWlya3MoKSkKICAgICAgICAgcmV0dXJuIFF1 aXJrczo6U3RvcmFnZUFjY2Vzc1Jlc3VsdDo6U2hvdWxkTm90Q2FuY2VsRXZlbnQ7Cg==