205979 2020-01-08 17:49:04 -0800 REGRESSION (r253662): Large Data URLs are not being handled properly 2020-01-10 09:29:55 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 203825 1 bfulgham bfulgham achristensen bfulgham cdumez cgarcia commit-queue darin simon.fraser webkit-bug-importer youennf zalan oldest_to_newest 1604423 0 bfulgham 2020-01-08 17:49:04 -0800 The changes in Bug 203825 restricted the size of URLs too much. We need to allow reasonably sized data and other URLs. 1604424 1 bfulgham 2020-01-08 17:56:48 -0800 <rdar://problem/58346124> 1604426 2 387173 bfulgham 2020-01-08 17:58:51 -0800 Created attachment 387173 Patch 1604429 3 387173 simon.fraser 2020-01-08 18:00:26 -0800 Comment on attachment 387173 Patch How do we know what a "reasonable" data URI size is? 1604430 4 simon.fraser 2020-01-08 18:00:55 -0800 Also having values in hex isn't very helpful. 1604434 5 bfulgham 2020-01-08 18:05:37 -0800 According to <https://bugs.chromium.org/p/chromium/issues/detail?id=44820#c1>, Chrome limits data URI's to 2 MB. This patch uses 64 MB, which if anything might be a bit large. 1604435 6 bfulgham 2020-01-08 18:06:53 -0800 @Alex Christensen: Does the URL Parser process all of the contents of a data URL? Could I just exclude data URIs from the test? 1604436 7 387173 bfulgham 2020-01-08 18:08:07 -0800 Comment on attachment 387173 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387173&action=review > Source/WebCore/page/SecurityOrigin.cpp:49 > +constexpr unsigned maximumURLSize = 0x04000000; This changes from 32 KB to 64 MB, which might be too big for memory-constrained devices. Might be good to keep the limit and just skip the check for data URIs? Or bump the standard URL limit, but have a much larger limit for data URIs only? 1604667 8 bfulgham 2020-01-09 09:20:36 -0800 (In reply to Brent Fulgham from comment #6) > @Alex Christensen: Does the URL Parser process all of the contents of a data > URL? Could I just exclude data URIs from the test? I created a test case, and confirmed that data URLs are processed the same way, and can run up against this problem too. 1604680 9 387173 youennf 2020-01-09 09:53:57 -0800 Comment on attachment 387173 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387173&action=review >> Source/WebCore/page/SecurityOrigin.cpp:49 >> +constexpr unsigned maximumURLSize = 0x04000000; > > This changes from 32 KB to 64 MB, which might be too big for memory-constrained devices. Might be good to keep the limit and just skip the check for data URIs? Should we add a 32KB+1 data URL test? > Or bump the standard URL limit, but have a much larger limit for data URIs only? I think it might be best to bump to 64 MB. We do not know what happens for custom schemes for instance. Also, javascript scheme should probably have the same constraint has data URL. We could also decide to decrease specific schemes like HTTP/HTTPS/FTP URLs back to 0x8000 if that is adding some kind of protection. 1604745 10 387249 bfulgham 2020-01-09 11:33:59 -0800 Created attachment 387249 Patch for landing 1604758 11 achristensen 2020-01-09 12:04:04 -0800 (In reply to Brent Fulgham from comment #6) > @Alex Christensen: Does the URL Parser process all of the contents of a data URL? It does. 1604811 12 commit-queue 2020-01-09 13:22:58 -0800 The commit-queue encountered the following flaky tests while processing attachment 387249: storage/indexeddb/detached-iframe.html bug 206028 (author: achristensen@apple.com) The commit-queue is continuing to process your patch. 1604814 13 commit-queue 2020-01-09 13:23:04 -0800 The commit-queue encountered the following flaky tests while processing attachment 387249: imported/w3c/web-platform-tests/content-security-policy/script-src/scriptnonce-changed-1.html bug 206029 (author: psaavedra@igalia.com) The commit-queue is continuing to process your patch. 1604870 14 387249 commit-queue 2020-01-09 14:41:36 -0800 Comment on attachment 387249 Patch for landing Clearing flags on attachment: 387249 Committed r254301: <https://trac.webkit.org/changeset/254301> 1604871 15 commit-queue 2020-01-09 14:41:38 -0800 All reviewed patches have been landed. Closing bug. 1604936 16 387249 darin 2020-01-09 17:17:42 -0800 Comment on attachment 387249 Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=387249&action=review > Source/WebCore/page/SecurityOrigin.cpp:49 > +constexpr unsigned maximumURLSize = 0x04000000; Could we put a comment here explaining the rationale behind this maximum? For example, I know it needs to be "big enough for reasonable data URI sizes" but not how we decided what is reasonable. And I don’t know why it shouldn’t be even bigger. We should just "show our work" here so someone later can make a smart decision about changing this or not changing it. 1605154 17 bfulgham 2020-01-10 09:29:55 -0800 (In reply to Darin Adler from comment #16) > Comment on attachment 387249 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=387249&action=review > > > Source/WebCore/page/SecurityOrigin.cpp:49 > > +constexpr unsigned maximumURLSize = 0x04000000; > > Could we put a comment here explaining the rationale behind this maximum? > For example, I know it needs to be "big enough for reasonable data URI > sizes" but not how we decided what is reasonable. And I don’t know why it > shouldn’t be even bigger. We should just "show our work" here so someone > later can make a smart decision about changing this or not changing it. Sure -- I'll add a comment in a follow-up. 387173 2020-01-08 17:58:51 -0800 2020-01-09 11:33:58 -0800 Patch bug-205979-20200108175850.patch text/plain 1323 bfulgham U3VidmVyc2lvbiBSZXZpc2lvbjogMjU0MjA5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDIxMTBiODhkODQwZDFj NDgwMTljMmRjZDJhMzcyOWI5NWFkMDg2Yy4uN2UyYTI0N2Q2YTAwMGIyZmE5NmY0NWIxMDRkZDA3 NWM5NmVkZmM4NyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDIwLTAxLTA4ICBCcmVu dCBGdWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFJFR1JFU1NJT04gKHIy NTM2NjIpOiBMYXJnZSBEYXRhIFVSTHMgYXJlIG5vdCBiZWluZyBoYW5kbGVkIHByb3Blcmx5Cisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDU5NzkKKyAg ICAgICAgPHJkYXI6Ly9wcm9ibGVtLzU4MzQ2MTI0PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBVUkwgc2l6ZSBsaW1pdGF0aW9uIGFkZGVkIGlu IHIyNTM2NjIgd2FzIHRvbyBsb3cuIFdlIHNob3VsZCBidW1wIGl0IHRvIGhhbmRsZQorICAgICAg ICByZWFzb25hYmxlIGRhdGEgVVJJIHNpemVzLgorCisgICAgICAgICogcGFnZS9TZWN1cml0eU9y aWdpbi5jcHA6CisKIDIwMjAtMDEtMDggIEFuZHJlcyBHb256YWxleiAgPGFuZHJlc2dfMjJAYXBw bGUuY29tPgogCiAgICAgICAgIEZpeCBmb3IgcGFyYW1ldGVyaXplZCBhdHRyaWJ1dGUgVGV4dE1h cmtlclJhbmdlRm9yVUlFbGVtZW50IHdoaWNoIHNob3VsZCBydW4gaW9uIG1haW4gdGhyZWFkLgpk aWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGFnZS9TZWN1cml0eU9yaWdpbi5jcHAgYi9Tb3Vy Y2UvV2ViQ29yZS9wYWdlL1NlY3VyaXR5T3JpZ2luLmNwcAppbmRleCBiMTI5YTgyNDk5NDVlNzdk NjljZjU2MzAwNzQxYjFkZjljODAyNTI3Li4xYjI5YWRkMzBmM2E5MGJiM2UwZDAwODFjYzhlZjdh NzljZjdmODlkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wYWdlL1NlY3VyaXR5T3JpZ2lu LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL1NlY3VyaXR5T3JpZ2luLmNwcApAQCAtNDYs NyArNDYsNyBAQAogCiBuYW1lc3BhY2UgV2ViQ29yZSB7CiAKLWNvbnN0ZXhwciB1bnNpZ25lZCBt YXhpbXVtVVJMU2l6ZSA9IDB4ODAwMDsKK2NvbnN0ZXhwciB1bnNpZ25lZCBtYXhpbXVtVVJMU2l6 ZSA9IDB4MDQwMDAwMDA7CiAKIHN0YXRpYyBib29sIHNjaGVtZVJlcXVpcmVzSG9zdChjb25zdCBV UkwmIHVybCkKIHsK 387249 2020-01-09 11:33:59 -0800 2020-01-09 14:41:36 -0800 Patch for landing bug-205979-20200109113359.patch text/plain 3455 bfulgham U3VidmVyc2lvbiBSZXZpc2lvbjogMjU0MjA5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDIxMTBiODhkODQwZDFj NDgwMTljMmRjZDJhMzcyOWI5NWFkMDg2Yy4uMTYxZGNkOWMxYjZmN2NlYTI4MDE0NDBkYWNlM2Y3 ZGQ4ZTBkY2NkYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDIwLTAxLTA4ICBCcmVu dCBGdWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFJFR1JFU1NJT04gKHIy NTM2NjIpOiBMYXJnZSBEYXRhIFVSTHMgYXJlIG5vdCBiZWluZyBoYW5kbGVkIHByb3Blcmx5Cisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDU5NzkKKyAg ICAgICAgPHJkYXI6Ly9wcm9ibGVtLzU4MzQ2MTI0PgorCisgICAgICAgIFJldmlld2VkIGJ5IFlv dWVubiBGYWJsZXQuCisKKyAgICAgICAgVGhlIFVSTCBzaXplIGxpbWl0YXRpb24gYWRkZWQgaW4g cjI1MzY2MiB3YXMgdG9vIGxvdy4gV2Ugc2hvdWxkIGJ1bXAgaXQgdG8gaGFuZGxlCisgICAgICAg IHJlYXNvbmFibGUgZGF0YSBVUkkgc2l6ZXMuCisKKyAgICAgICAgVGVzdDogZmFzdC91cmwvZGF0 YS11cmwtbGFyZ2UuaHRtbC4KKworICAgICAgICAqIHBhZ2UvU2VjdXJpdHlPcmlnaW4uY3BwOgor CiAyMDIwLTAxLTA4ICBBbmRyZXMgR29uemFsZXogIDxhbmRyZXNnXzIyQGFwcGxlLmNvbT4KIAog ICAgICAgICBGaXggZm9yIHBhcmFtZXRlcml6ZWQgYXR0cmlidXRlIFRleHRNYXJrZXJSYW5nZUZv clVJRWxlbWVudCB3aGljaCBzaG91bGQgcnVuIGlvbiBtYWluIHRocmVhZC4KZGlmZiAtLWdpdCBh L1NvdXJjZS9XZWJDb3JlL3BhZ2UvU2VjdXJpdHlPcmlnaW4uY3BwIGIvU291cmNlL1dlYkNvcmUv cGFnZS9TZWN1cml0eU9yaWdpbi5jcHAKaW5kZXggYjEyOWE4MjQ5OTQ1ZTc3ZDY5Y2Y1NjMwMDc0 MWIxZGY5YzgwMjUyNy4uMWIyOWFkZDMwZjNhOTBiYjNlMGQwMDgxY2M4ZWY3YTc5Y2Y3Zjg5ZCAx MDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGFnZS9TZWN1cml0eU9yaWdpbi5jcHAKKysrIGIv U291cmNlL1dlYkNvcmUvcGFnZS9TZWN1cml0eU9yaWdpbi5jcHAKQEAgLTQ2LDcgKzQ2LDcgQEAK IAogbmFtZXNwYWNlIFdlYkNvcmUgewogCi1jb25zdGV4cHIgdW5zaWduZWQgbWF4aW11bVVSTFNp emUgPSAweDgwMDA7Citjb25zdGV4cHIgdW5zaWduZWQgbWF4aW11bVVSTFNpemUgPSAweDA0MDAw MDAwOwogCiBzdGF0aWMgYm9vbCBzY2hlbWVSZXF1aXJlc0hvc3QoY29uc3QgVVJMJiB1cmwpCiB7 CmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKaW5kZXggYzNjZWM5MjM3MmQ5YWJlNzY1YTUxZTRlODk0YzdkY2RjYjNjN2M3Yy4uMzdlNWNi ODlmN2I1ZWRhYTkyYjYxZDQxOWU5MDYyYmQwZDAzNWQxYSAxMDA2NDQKLS0tIGEvTGF5b3V0VGVz dHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3RzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBA CisyMDIwLTAxLTA5ICBCcmVudCBGdWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAg ICAgIFJFR1JFU1NJT04gKHIyNTM2NjIpOiBMYXJnZSBEYXRhIFVSTHMgYXJlIG5vdCBiZWluZyBo YW5kbGVkIHByb3Blcmx5CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0yMDU5NzkKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzU4MzQ2MTI0PgorCisgICAg ICAgIFJldmlld2VkIGJ5IFlvdWVubiBGYWJsZXQuCisKKyAgICAgICAgKiBmYXN0L3VybC9kYXRh LXVybC1sYXJnZS1leHBlY3RlZC50eHQ6IEFkZGVkLgorICAgICAgICAqIGZhc3QvdXJsL2RhdGEt dXJsLWxhcmdlLmh0bWw6IEFkZGVkLgorCiAyMDIwLTAxLTA4ICBEaWVnbyBQaW5vIEdhcmNpYSAg PGRwaW5vQGlnYWxpYS5jb20+CiAKICAgICAgICAgW0dUS11bV1BFXSBVbnJldmlld2VkIGdhcmRl bmluZyBmb3IgdW5leHBlY3RlZCBwYXNzZXMKZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL2Zhc3Qv dXJsL2RhdGEtdXJsLWxhcmdlLWV4cGVjdGVkLnR4dCBiL0xheW91dFRlc3RzL2Zhc3QvdXJsL2Rh dGEtdXJsLWxhcmdlLWV4cGVjdGVkLnR4dApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi44ZGI0OTM0MmYxNjYyMmE0ZWFj ODgwNWMzMTNkMTA5MmI4OWQ4MjcwCi0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvZmFz dC91cmwvZGF0YS11cmwtbGFyZ2UtZXhwZWN0ZWQudHh0CkBAIC0wLDAgKzEsNSBAQAorUEFTUyBM YXJnZSBVUkwgaGFuZGxlZCBwcm9wZXJseS4KK1BBU1Mgc3VjY2Vzc2Z1bGx5UGFyc2VkIGlzIHRy dWUKKworVEVTVCBDT01QTEVURQorCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9mYXN0L3VybC9k YXRhLXVybC1sYXJnZS5odG1sIGIvTGF5b3V0VGVzdHMvZmFzdC91cmwvZGF0YS11cmwtbGFyZ2Uu aHRtbApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwLi41NDAwZWVhMDJhOTQ4NTk4NmMzNzkwYzQxOGMzNGE0OWM2YWMwZDM4 Ci0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvZmFzdC91cmwvZGF0YS11cmwtbGFyZ2Uu aHRtbApAQCAtMCwwICsxLDIyIEBACis8IURPQ1RZUEUgaHRtbD4KKzxodG1sPgorPGJvZHk+Cis8 c2NyaXB0IHNyYz0iLi4vLi4vcmVzb3VyY2VzL2pzLXRlc3QuanMiPjwvc2NyaXB0PgorPHNjcmlw dD4KK2pzVGVzdElzQXN5bmMgPSB0cnVlOworY29uc3QgdXJsID0gImRhdGE6YXVkaW8vbXBlZzti YXNlNjQsLytNWXhBQUFBQU5JQVVBQUFBU0VFQi9qd09GTS8wTU0vOTBiLytSaFNULy93NE5Gd09q Zi8vL1BadS8vLy85bG5zNUdGRHYvL2w5R2xVSUVFSUFBQWdJZzhJci9KR3EzLytNWXhEc0xJajVR TVljb0FQMGR2OUhJalVjSC8veVlTZytDSWJrR1AvLzh3MGJMVmpVUC8vLzNaMHg1UUNBdi95TGp3 dEdLVEVGTlJUTXVPVGVxcXFxcXFxcXFxcXFxLytNWXhFa05tZEprVVljNEFLcSIgKyAicSIucmVw ZWF0KDMyNTM1KTsKKworY29uc3QgeGhyID0gbmV3IFhNTEh0dHBSZXF1ZXN0KCk7Cit4aHIub25s b2FkID0gZnVuY3Rpb24oKSB7CisgICAgdGVzdFBhc3NlZCgiTGFyZ2UgVVJMIGhhbmRsZWQgcHJv cGVybHkuIik7CisgICAgZmluaXNoSlNUZXN0KCk7Cit9Cit4aHIub25lcnJvciA9IGZ1bmN0aW9u KCkgeworICAgIHRlc3RGYWlsZWQoIkxhcmdlIFVSTCB3YXMgaW1wcm9wZXJseSByZWplY3RlZC4i KTsKKyAgICBmaW5pc2hKU1Rlc3QoKTsKK30KK3hoci5vcGVuKCJHRVQiLCB1cmwpOworeGhyLnNl bmQoKTsKKzwvc2NyaXB0PgorPC9ib2R5PgorPC9odG1sPgo=