Skip to content

[webview_flutter] Platform implementations for getCookies #11037#11386

Merged
auto-submit[bot] merged 22 commits intoflutter:mainfrom
khaled-0:implementations
Apr 29, 2026
Merged

[webview_flutter] Platform implementations for getCookies #11037#11386
auto-submit[bot] merged 22 commits intoflutter:mainfrom
khaled-0:implementations

Conversation

@khaled-0
Copy link
Copy Markdown
Contributor

List which issues are fixed by this PR. You must list at least one issue.
#10833 #11037

Pre-Review Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request implements the getCookies method for PlatformWebViewCookieManager in both the Android and WKWebView packages. The changes include updated Pigeon definitions, native implementations for cookie retrieval, and updates to the example application to utilize the new API. Feedback identifies a bug in Android cookie parsing when values contain equals signs, incorrect domain matching logic in the WKWebView implementation that violates RFC 6265, and a missing nullability annotation in the Android native code. Typographical errors in the changelogs were also noted.

Comment on lines +100 to +107
final List<String> cookieValue = cookie.split('=');
webViewCookies.add(
WebViewCookie(
name: cookieValue.first,
value: cookieValue.last,
domain: url.toString(),
),
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

The current cookie parsing logic is incorrect if a cookie's value contains an equals sign (=). Using cookie.split('=') and taking cookieValue.last will only capture the part of the value after the last =.

To correctly parse the cookie, you should take everything after the first = as the value.

Suggested change
final List<String> cookieValue = cookie.split('=');
webViewCookies.add(
WebViewCookie(
name: cookieValue.first,
value: cookieValue.last,
domain: url.toString(),
),
);
final List<String> cookieParts = cookie.split('=');
webViewCookies.add(
WebViewCookie(
name: cookieParts.first,
value: cookieParts.skip(1).join('='),
domain: url.toString(),
),
);

Comment thread packages/webview_flutter/webview_flutter_android/CHANGELOG.md Outdated
Comment thread packages/webview_flutter/webview_flutter_wkwebview/CHANGELOG.md Outdated
@stuartmorgan-g stuartmorgan-g added triage-ios Should be looked at in iOS triage triage-android Should be looked at in Android triage federated: partial_changes PR that contains changes for only a single package of a federated plugin change labels Mar 31, 2026
Copy link
Copy Markdown
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

The webkit implementation LGTM with a few nits / questions

final Uri? domain = Uri.tryParse(
(await webViewController.currentUrl()) ?? '',
);
late final List<WebViewCookie> cookies;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Is the late necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

apparently it's not.

}

return Dictionary(uniqueKeysWithValues: keyValueTuples)
// Use newest value in case of duplicates
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this change needed? The original collection was a Dictionary so there won't be duplicated keys unless the code above mapped different keys to the same key (it seems to be possible with the default case, but do we care about the .unknown key?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why is this change needed? The original collection was a Dictionary so there won't be duplicated keys unless the code above mapped different keys to the same key (it seems to be possible with the default case, but do we care about the .unknown key?)

we don't care but swift crashes when it encounters duplicate keys
Swift/NativeDictionary.swift:792: Fatal error: Duplicate values for key: 'unknown'

cookieDomain = cookieDomain.substring(1);
}

if (url.host != cookieDomain && !url.host.endsWith('.$cookieDomain')) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The API documentation says

If the domain does not start with a dot, then the cookie is only sent to the exact host specified by the domain. If the domain does start with a dot, then the cookie is sent to other hosts in that domain as well, subject to certain restrictions. See RFC 6265 for more detail.

It looks like the implementation treats both cases as the wildcard case (as if there was always a leading .)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because that's how WebKit behaves too

image image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting. So the API documentation is wrong / outdated. Thank you for verifying this!

…TPCookieStoreProxyAPITests. Remove unnecessary late in _onListCookies
Copy link
Copy Markdown
Contributor

@mboetger mboetger left a comment

Choose a reason for hiding this comment

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

LGTM for Android

@camsim99 camsim99 added the CICD Run CI/CD label Apr 14, 2026
@flutter-dashboard
Copy link
Copy Markdown

This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled.

@camsim99
Copy link
Copy Markdown
Contributor

@khaled-0 Can you rebase onto main and resolve the merge conflicts so that we can take a look? Thanks!

@github-actions github-actions Bot removed the CICD Run CI/CD label Apr 14, 2026
@khaled-0
Copy link
Copy Markdown
Contributor Author

@khaled-0 Can you rebase onto main and resolve the merge conflicts so that we can take a look? Thanks!

done. please have a look

@gmackall gmackall removed the triage-android Should be looked at in Android triage label Apr 21, 2026
@bparrishMines bparrishMines added the CICD Run CI/CD label Apr 27, 2026
@github-actions github-actions Bot removed the CICD Run CI/CD label Apr 28, 2026
@bparrishMines bparrishMines added the CICD Run CI/CD label Apr 28, 2026
@github-actions github-actions Bot removed the CICD Run CI/CD label Apr 28, 2026
@bparrishMines bparrishMines added the CICD Run CI/CD label Apr 28, 2026
@github-actions github-actions Bot removed the CICD Run CI/CD label Apr 28, 2026
@bparrishMines bparrishMines added the CICD Run CI/CD label Apr 28, 2026
Copy link
Copy Markdown
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM

I went ahead and merged in main and handled the conflicts. Sorry for the delay.

@bparrishMines bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 29, 2026
@auto-submit auto-submit Bot merged commit ff0189e into flutter:main Apr 29, 2026
83 checks passed
pull Bot pushed a commit to Klomgor/flutter that referenced this pull request May 1, 2026
…r#185897)

flutter/packages@cde5b36...daf30f8

2026-04-30 mdebbar@google.com [url_launcher_web] Re-enable flaky test
(not flaky anymore) (flutter/packages#11478)
2026-04-30 stuartmorgan@google.com [in_app_purchase] Switch to Kotlin
Pigeon (flutter/packages#11608)
2026-04-29 katelovett@google.com [two_dimensional_scrollables] Fix mouse
event loop when calling setState in TableSpan.onEnter
(flutter/packages#11606)
2026-04-29 katelovett@google.com [two_dimensional_scrollables] trailing
pinned row/col for TableView (flutter/packages#11519)
2026-04-29 10687576+bparrishMines@users.noreply.github.com
[webview_flutter_wkwebview] Tear down ProxyAPIRegistrar in
`applicationWillTerminate` (flutter/packages#11567)
2026-04-29 stuartmorgan@google.com [google_maps_flutter] Replace use of
zIndex in examples and tests (flutter/packages#11572)
2026-04-29 stuartmorgan@google.com [tool] Remove --against-pub flag
(flutter/packages#11550)
2026-04-29 47866232+chunhtai@users.noreply.github.com [ci] Update branch
management for batch release (flutter/packages#11575)
2026-04-29 spkhalad@gmail.com [webview_flutter] Platform implementations
for getCookies flutter#11037 (flutter/packages#11386)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD federated: partial_changes PR that contains changes for only a single package of a federated plugin change p: webview_flutter platform-android platform-ios platform-macos triage-ios Should be looked at in iOS triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants