Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Curl] Add Timing-Allow-Origin checks for Curl port #5802

Merged
merged 1 commit into from Oct 27, 2022

Conversation

kshukuwa
Copy link
Contributor

@kshukuwa kshukuwa commented Oct 26, 2022

3f6d008

[Curl] Add Timing-Allow-Origin checks for Curl port
https://bugs.webkit.org/show_bug.cgi?id=247048

Reviewed by Fujii Hironori.

Add missing TAO checks to Curl backend.

* LayoutTests/platform/wincairo/TestExpectations:
* Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:
(WebCore::CurlResourceHandleDelegate::curlDidReceiveResponse):
(WebCore::CurlResourceHandleDelegate::curlDidComplete):
(WebCore::CurlResourceHandleDelegate::updateNetworkLoadMetrics):
* Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.h:
* Source/WebKit/NetworkProcess/NetworkDataTask.cpp:
(WebKit::NetworkDataTask::create):
* Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:
(WebKit::NetworkDataTaskCurl::NetworkDataTaskCurl):
(WebKit::NetworkDataTaskCurl::curlDidReceiveResponse):
(WebKit::NetworkDataTaskCurl::curlDidComplete):
(WebKit::NetworkDataTaskCurl::updateNetworkLoadMetrics):
* Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h:

Canonical link: https://commits.webkit.org/256044@main

95d6949

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios   πŸ§ͺ api-mac ❌ πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk1
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch-sim βœ… πŸ§ͺ mac-wk2-stress

@kshukuwa kshukuwa requested a review from cdumez as a code owner October 26, 2022 06:54
@@ -222,6 +226,8 @@ void NetworkDataTaskCurl::curlDidComplete(CurlRequest&, NetworkLoadMetrics&& net
return;
}

updateNetworkLoadMetrics(networkLoadMetrics);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cocoa port is calling checkTAO in willPerformHTTPRedirection and didReceiveResponse.
On the other hand, NetworkDataTaskCurl::updateNetworkLoadMetrics is called in curlDidComplete and curlDidReceiveResponse.
Is this right?

Copy link
Contributor Author

@kshukuwa kshukuwa Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is right. Cocoa port store NetworkLoadMetrics in a member variable of the NetworkDataTaskCocoa class.

WebCore::NetworkLoadMetrics m_networkLoadMetrics;

On the other hand, Curl port does not store it and a new object is spawned every time when called CurlHandle::getNetworkLoadMetrics.

NetworkLoadMetrics networkLoadMetrics;

m_response.networkLoadMetrics = networkLoadMetrics();

auto metrics = networkLoadMetrics();

The generated objects are then notified to curlDidReceiveResponse and curlDidComplete. Therefore, NetworkDataTaskCurl::updateNetworkLoadMetrics must be called in both curlDidReceiveResponse and curlDidComplete to update failsTAOCheck in NetworkLoadMetrics.

FileSystem::PlatformFileHandle m_downloadDestinationFile { FileSystem::invalidPlatformFileHandle };

bool m_blockingCookies { false };

WebCore::ShouldRelaxThirdPartyCookieBlocking m_shouldRelaxThirdPartyCookieBlocking { WebCore::ShouldRelaxThirdPartyCookieBlocking::No };
bool m_failsTAOCheck { false };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why only curl port adds m_failsTAOCheck? Cocoa and Soup ports are using m_networkLoadMetrics.failsTAOCheck.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The answer is similar to the previous question. Curl port does not store NetworkLoadMetrics like Cocoa and SOUP. Therefor, curl port adds m_failsTAOCheck to store results of failsTAOCheck.

Copy link
Contributor

@fujii fujii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fujii fujii added the merge-queue Applied to send a pull request to merge-queue label Oct 27, 2022
https://bugs.webkit.org/show_bug.cgi?id=247048

Reviewed by Fujii Hironori.

Add missing TAO checks to Curl backend.

* LayoutTests/platform/wincairo/TestExpectations:
* Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:
(WebCore::CurlResourceHandleDelegate::curlDidReceiveResponse):
(WebCore::CurlResourceHandleDelegate::curlDidComplete):
(WebCore::CurlResourceHandleDelegate::updateNetworkLoadMetrics):
* Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.h:
* Source/WebKit/NetworkProcess/NetworkDataTask.cpp:
(WebKit::NetworkDataTask::create):
* Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:
(WebKit::NetworkDataTaskCurl::NetworkDataTaskCurl):
(WebKit::NetworkDataTaskCurl::curlDidReceiveResponse):
(WebKit::NetworkDataTaskCurl::curlDidComplete):
(WebKit::NetworkDataTaskCurl::updateNetworkLoadMetrics):
* Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h:

Canonical link: https://commits.webkit.org/256044@main
@webkit-commit-queue
Copy link
Collaborator

Committed 256044@main (3f6d008): https://commits.webkit.org/256044@main

Reviewed commits have been landed. Closing PR #5802 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 3f6d008 into WebKit:main Oct 27, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 27, 2022
@kshukuwa kshukuwa deleted the eng/bug247048 branch October 27, 2022 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants