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

Cookies: Setting the expiration property fails while adding cookies #1025

Closed
Tracked by #6
inancgumus opened this issue Sep 1, 2023 · 0 comments · Fixed by #1031
Closed
Tracked by #6

Cookies: Setting the expiration property fails while adding cookies #1025

inancgumus opened this issue Sep 1, 2023 · 0 comments · Fixed by #1031
Assignees
Labels
bug Something isn't working cookies

Comments

@inancgumus
Copy link
Member

inancgumus commented Sep 1, 2023

There is a problem while converting the expires property (in seconds) to cdproto's cdp.TimeSinceEpoch.

  1. Users provide seconds since Unix time epoch while passing the expires property.
  2. CDP and cdproto require setting an exact date (time since epoch).
  3. We directly pass the property to cdproto while it expects us a time.Time for setting an expiration date for cookies.

xk6-browser version

v1.0.2

Steps to reproduce the problem

Add a cookie with an expiration property (in seconds).

context.addCookies([{
  name: 'foo',
  value: '42',
  domain: '127.0.0.1',
  path: '/',
  expires: 10,  // <---
}]);

Expected behaviour

Cookies should be added with an expiration date.

Actual behaviour

ERRO[0000] Uncaught (in promise) GoError:
cannot recognize cookie values:
could not convert array element [object Object] to []network.CookieParam at 0:
could not convert struct value 10 to *cdp.TimeSinceEpoch for field Expires:
could not convert 10 to cdp.TimeSinceEpoch

Suggestion

The current addCookies API accepts an internal cookie object: cdproto.network.Cookie. This object contains superfluous information and can be dangerous because it lets users set internal CDP details from their scripts. We can use api.Cookie instead of cdproto.network.Cookie to be consistent with our BrowserContext.cookies API. And also prevent possible security problems.

Updates: #6

@inancgumus inancgumus added the bug Something isn't working label Sep 1, 2023
@inancgumus inancgumus self-assigned this Sep 1, 2023
@inancgumus inancgumus changed the title Cannot add cookies with an expiration Fix expiration property while adding cookies Sep 7, 2023
@inancgumus inancgumus changed the title Fix expiration property while adding cookies Fails to set expiration property while adding cookies Sep 19, 2023
@inancgumus inancgumus changed the title Fails to set expiration property while adding cookies Cookies: Setting the expiration property fails while adding cookies Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cookies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant