Bug 186441

Summary: http/tests/security/cors-post-redirect-307.html fails with PSON enabled
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Page LoadingAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, commit-queue, dbates, ews-watchlist, ggaren, japhet, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=184142
Bug Depends on:    
Bug Blocks: 186542    
Attachments:
Description Flags
For EWS
none
WIP Patch
none
Patch
none
Patch
none
Patch none

Chris Dumez
Reported 2018-06-08 13:12:56 PDT
http/tests/security/cors-post-redirect-307.html and http/tests/security/cors-post-redirect-308.html fail with PSON enabled: --- /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release/layout-test-results/http/tests/security/cors-post-redirect-307-expected.txt +++ /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release/layout-test-results/http/tests/security/cors-post-redirect-307-actual.txt @@ -1 +1 @@ -There was no origin header +Origin header value: null
Attachments
For EWS (744 bytes, patch)
2018-06-08 14:12 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
WIP Patch (1.97 KB, patch)
2018-06-08 14:24 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (3.90 KB, patch)
2018-06-08 14:30 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (57.78 KB, patch)
2018-06-11 12:32 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (57.36 KB, patch)
2018-06-11 13:50 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2018-06-08 13:15:45 PDT
Looks like a legit bug in our PSON implementation. The Origin header should not be present on cross-origin redirect requests.
Chris Dumez
Comment 2 2018-06-08 13:19:00 PDT
The origin gets cleared in NetworkDataTaskCocoa::willPerformHTTPRedirection() but I suspect we add the header back later on in the case where we decide to do the redirect load in a new process.
Chris Dumez
Comment 3 2018-06-08 13:45:58 PDT
It is added later on by this code: if (origin.isEmpty()) { // If we don't know what origin header to attach, we attach the value // for an empty origin. request.setHTTPOrigin(SecurityOrigin::createUnique()->toString()); return; } in FrameLoader::addHTTPOriginIfNeeded().
Chris Dumez
Comment 4 2018-06-08 14:12:54 PDT
Created attachment 342325 [details] For EWS
Chris Dumez
Comment 5 2018-06-08 14:24:35 PDT
Created attachment 342330 [details] WIP Patch
Chris Dumez
Comment 6 2018-06-08 14:30:06 PDT
Created attachment 342332 [details] Patch
Daniel Bates
Comment 7 2018-06-08 15:20:55 PDT
Comment on attachment 342332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342332&action=review > Source/WebCore/loader/FrameLoader.cpp:-2835 > - if (origin.isEmpty()) { This code was intentionally added and a reason was given in the comment above. Please read through the comments on <https://bugs.webkit.org/show_bug.cgi?id=20792> and ensure we are not breaking anything.
Daniel Bates
Comment 8 2018-06-08 15:23:02 PDT
Obviously think about WebKit legacy.
Chris Dumez
Comment 9 2018-06-08 16:56:11 PDT
(In reply to Daniel Bates from comment #7) > Comment on attachment 342332 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342332&action=review > > > Source/WebCore/loader/FrameLoader.cpp:-2835 > > - if (origin.isEmpty()) { > > This code was intentionally added and a reason was given in the comment > above. Please read through the comments on > <https://bugs.webkit.org/show_bug.cgi?id=20792> and ensure we are not > breaking anything. Yes, I suspected this was not OK. Still uploaded it without review flag before my lab to see which tests would fail. If this is important behavior, it is sad there is no test coverage for it.
Chris Dumez
Comment 10 2018-06-11 09:55:23 PDT
I have a proper fix, will upload shortly.
Chris Dumez
Comment 11 2018-06-11 12:32:52 PDT
Created attachment 342454 [details] Patch
youenn fablet
Comment 12 2018-06-11 13:44:18 PDT
Comment on attachment 342454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342454&action=review > Source/WebCore/loader/FrameLoader.cpp:2839 > + WTFReportBacktrace(); Probably to be removed.
Chris Dumez
Comment 13 2018-06-11 13:49:21 PDT
(In reply to youenn fablet from comment #12) > Comment on attachment 342454 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342454&action=review > > > Source/WebCore/loader/FrameLoader.cpp:2839 > > + WTFReportBacktrace(); > > Probably to be removed. LoL XD
Chris Dumez
Comment 14 2018-06-11 13:50:38 PDT
Created attachment 342461 [details] Patch
Chris Dumez
Comment 15 2018-06-11 13:52:36 PDT
Comment on attachment 342461 [details] Patch Clearing flags on attachment: 342461 Committed r232730: <https://trac.webkit.org/changeset/232730>
Chris Dumez
Comment 16 2018-06-11 13:52:38 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 17 2018-06-11 13:53:24 PDT
<rdar://problem/41018447>
Note You need to log in before you can comment on or make changes to this bug.