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

feat: add WebView::cookies and WebView::cookies_for_url #1394

Merged
merged 17 commits into from
Oct 21, 2024
Merged

Conversation

amrbashir
Copy link
Member

@amrbashir amrbashir commented Oct 16, 2024

@amrbashir amrbashir requested a review from a team as a code owner October 16, 2024 23:36
Copy link
Contributor

github-actions bot commented Oct 16, 2024

Package Changes Through f307f71

There are 1 changes which include wry with patch

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
wry 0.46.3 0.46.4

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

Comment on lines 871 to 886
// domain is the same
cookie.domain() == url.domain()
// path is the same
&& cookie.path() == Some(url.path())
/// and one of
&& (
// cookie is secure and url is https
(cookie.secure() && url.scheme() == "https") ||
// or cookie is secure and is localhost
(
cookie.secure() && url.scheme() == "http" &&
(url.domain() == Some("localhost") || url.domain().and_then(|d| Ipv4Addr::from_str(d).ok()).map(|ip| ip.is_loopback()).unwrap_or(false))
) ||
// or cookie is not secure
(!cookie.secure())
)
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure if this is too restrictive, tried to check how webkit2gtk cookies_for_url is implementation but I think their implementation is a bit more relaxed ? https://github.com/WebKit/WebKit/blob/c305a86965236fd8052623faaf8804b1424e7f6b/Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp#L69

Copy link
Member

Choose a reason for hiding this comment

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

when in doubt just ask @tweidinger though I think we should just match the behavior from other platforms if possible

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do match too strict on the path depending on the cookie policy.
For example / in the cookie would not match a subpath cookie but it should match any path.

Copy link
Contributor

@tweidinger tweidinger Oct 17, 2024

Choose a reason for hiding this comment

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

Just checking the domain match and checking if secure cookie or not sounds acceptable if we don't want to implement the cookie specs fully 😁 .

Copy link
Member Author

Choose a reason for hiding this comment

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

alright, pushed a change to match on domain and secure or not. (basically just removed matching the path)

src/webkitgtk/mod.rs Outdated Show resolved Hide resolved
src/webkitgtk/mod.rs Outdated Show resolved Hide resolved
@lucasfernog lucasfernog merged commit c1b26b9 into dev Oct 21, 2024
14 checks passed
@lucasfernog lucasfernog deleted the feat/cookies branch October 21, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

want an API can get webview cookie
3 participants