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.

Sign up for GitHub

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

Jump to bottom

Cookies: Fix Goja conversions #1039

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Cookies: Fix Goja conversions #1039

merged 3 commits into from
Sep 19, 2023

Conversation

inancgumus
Copy link
Member

@inancgumus inancgumus commented Sep 19, 2023 •
edited
Loading

What?

Adds js struct tags to api.Cookie to allow Goja to convert api.Cookie values.

Why?

Goja cannot convert httpOnly and sameSite fields while sending and retrieving cookies from the JavaScript side.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Updates: #6

@inancgumus
Fix cookie conversions to/from Goja
a50f4d2 
Goja cannot convert a JavaScript value provided by the user into an
api.Cookie type. While converting, Goja misses the api.CookieSameSite
and httpOnly. The underlying reason is the missing `js` struct tag.
@inancgumus
Improve cookie JS example for robust E2E testing
8f25933 
Checks the goja-incompatible fields to ensure we're setting them while
adding and retriving cookies.  We do this only for the cookie to not to
conflate the example.
@inancgumus inancgumus self-assigned this Sep 19, 2023
@inancgumus inancgumus added the bug Something isn't working label Sep 19, 2023
@inancgumus inancgumus mentioned this pull request Sep 19, 2023
Closed
@inancgumus inancgumus changed the title Fix cookie goja conversions Fix Goja cookie conversions Sep 19, 2023
ankur22
ankur22 previously approved these changes Sep 19, 2023
View reviewed changes
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

Good catch! LGTM 🙂

api/browser_context.go Outdated Show resolved Hide resolved
ka3de
ka3de approved these changes Sep 19, 2023
View reviewed changes
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

LGTM.

@inancgumus inancgumus changed the title Fix Goja cookie conversions Cookies: Fix Goja conversions Sep 19, 2023
@inancgumus inancgumus merged commit 90fae32 into main Sep 19, 2023
12 checks passed
@inancgumus inancgumus deleted the fix/cookie-goja-conversion branch September 19, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankur22 ankur22 ankur22 approved these changes

@ka3de ka3de ka3de approved these changes

Assignees

@inancgumus inancgumus

Labels
bug Something isn't working cookies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@inancgumus @ankur22 @ka3de