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

setCookie() can't save object anymore #5642

Closed
jdvegmond opened this issue Nov 8, 2019 · 22 comments · Fixed by #5957 · May be fixed by neo355/grafana#2
Closed

setCookie() can't save object anymore #5642

jdvegmond opened this issue Nov 8, 2019 · 22 comments · Fixed by #5957 · May be fixed by neo355/grafana#2

Comments

@jdvegmond
Copy link

@jdvegmond jdvegmond commented Nov 8, 2019

Since upgrading my Cypress to 3.6.0 I can't set cookies that I need to set for my tests. (The application sets them as well.)
I need a way to set the cookie.

Current behavior:

When using cy.setCookie() I get the following error:

CypressError: cy.setCookie() must be passed an RFC-6265-compliant cookie value. You passed:

{"name1":"value1","name2":"value2","name3":"value3","name4":"value4","name5":"value5","name6":"value6"}

(Values have been changed for privacy reasons.)

Desired behavior:

The cookie should be set. It's the format that is also used by our application.

Steps to reproduce: (app code and test code)

I use the following:

cy.setCookie('cookiename', cookieString, { domain: 'required.domain', });

where

cookieString = {"name1":"value1","name2":"value2","name3":"value3","name4":"value4","name5":"value5","name6":"value6"}

Versions

Cypress 3.6.0

@hsaab

This comment has been minimized.

Copy link

@hsaab hsaab commented Nov 8, 2019

We are having this issue as well!

edit: also seems like this issue is popping up for any value we put whether it is an object, string or bool

@flotwig flotwig self-assigned this Nov 8, 2019
@flotwig

This comment has been minimized.

Copy link
Member

@flotwig flotwig commented Nov 8, 2019

If you look at the cy.setCookie docs, value should be a string: https://docs.cypress.io/api/commands/setcookie.html#Syntax

Can you clarify if you are passing a string?


If you're passing a string and getting this error, read on...

In 3.5.0, strict cookie value validation was added.

Technically, a cookie value containing quotes is invalid, if you see the RFC:

 cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
 cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash

However, it's possible that versions of Cypress < 3.5.0 would encode the value for you, and this may be broken in new releases.

You might be able to just encode your cookie value as a workaround:

cy.setCookie('name', encodeURIComponent('{"name1":"value1","name2":"value2","name3":"value3","name4":"value4","name5":"value5","name6":"value6"}'))
@hsaab

This comment has been minimized.

Copy link

@hsaab hsaab commented Nov 8, 2019

Found a fix where you specifically need to format setting cookies like so. Specifically using the double quotes around the object then single quotes around the key and value. In previous cypress versions the double quotes vs. single quotes format wasn't as strict for setting cookies

cy.setCookie('cookie', "{'viewed':'true'}");

Edit: commented at the same time! Yep, found that workaround. Thank you

@hsaab

This comment has been minimized.

Copy link

@hsaab hsaab commented Nov 8, 2019

Thanks @flotwig for the proposed solution. However, when using encodingURIComponent in the Cypress test, we then need to decodeURIComponent in our code which requires a change of logic to check for what environment we are in (e.g., development, cypress, production etc.).

This extra logic to determine the environment is not ideal. I appreciate the workaround, though I'm curious if the cypress auto encoding will be planned to be re-included in an upcoming release?

@flotwig

This comment has been minimized.

Copy link
Member

@flotwig flotwig commented Nov 8, 2019

@hsaab It appears that you don't need quoting - you were passing an object, now you're passing a string and it works, right?

Passing an object never should have worked, the contract has always expected cy.setCookie(string, string) and as of 3.5.0 cy.setCookie correctly enforces that: https://docs.cypress.io/api/commands/setcookie.html#Syntax

@hsaab

This comment has been minimized.

Copy link

@hsaab hsaab commented Nov 8, 2019

@flotwig First I apologize I did not have this exact issue as the author but it was related. I was not passing in an object. I was passing in a string that represented an object to the cy.setCookie.

So cy.setCookie('cookie', '{"viewed":"true"}');

Because of the stricter cookie setting enforcement, I had to change the cookie value toencodeURIComponent('{"viewed":"true"}') (per your suggestion - thank you)

Doing this requires extra handling in the code to decode the encoded cookie value when the tests run. So instead of just

function parseCookie(unparsedCookie) {
  return JSON.parse(unparsedCookie);
}

we need --

function parseCookie(environment, unparsedCookie) {
  if (environment === LOCAL_ENVIRONMENT.CYPRESS) {
    return JSON.parse(decodeURIComponent(unparsedCookie));
  }
  return JSON.parse(unparsedCookie);
}

Its not a huge deal to be honest, but just wondering if this extra logic can be avoided

@flotwig

This comment has been minimized.

Copy link
Member

@flotwig flotwig commented Nov 9, 2019

Yeah, I wonder if previously, cy.setCookie would eventually escape cy.setCookie('name', 'value: "value"') so that "value: \"value\"" is the actual cookie sent, and now it doesn't because it rejects too early. I will look in to the differences between 3.6.1 and 3.4.1.

@jdvegmond

This comment has been minimized.

Copy link
Author

@jdvegmond jdvegmond commented Nov 11, 2019

Found a fix where you specifically need to format setting cookies like so. Specifically using the double quotes around the object then single quotes around the key and value. In previous cypress versions the double quotes vs. single quotes format wasn't as strict for setting cookies

cy.setCookie('delivery-beta-modal', "{'viewed':'true'}");

Edit: commented at the same time! Yep, found that workaround. Thank you

I've tried this, but the double quotes are only part of the problem. Commas are also not allowed.

And using encodeURIComponent clearly creates a cookie that my app can't use.

If the RFC-6265 is needed, please make it also possible not to use it, in case an app uses non-compliant cookies.

@hsaab

This comment has been minimized.

Copy link

@hsaab hsaab commented Nov 11, 2019

Gotcha, thank you @flotwig !

@ItsRobbAllen

This comment has been minimized.

Copy link

@ItsRobbAllen ItsRobbAllen commented Nov 15, 2019

I would like to chime in that we're having the same basic issue. Existing app (so I can't change how it is working) sets cookies with colons in them as well as full JSON objects. But because Cypress complains that they are not RFC-6265 compliant, it won't set them. So I have to UriEncode them AND then change all the back end code to UriUnencode them.

I would like to request an {"EnforceRFC6265": false} option when setting a cookie as well.

@flotwig

This comment has been minimized.

Copy link
Member

@flotwig flotwig commented Nov 15, 2019

Workaround

You can suppress this error by using cy.stub to stub out the Cypress error function.

Add the following to your specs or to your support file:

beforeEach(() => {
  cy.stub(Cypress.utils, "throwErrByPath")
  .callThrough() // still throw other types of errors
  .withArgs("setCookie.invalid_value").returns() // suppress invalid value errors
})
@filiphric

This comment has been minimized.

Copy link

@filiphric filiphric commented Nov 21, 2019

there is another workaround for this:

cy
  .document()
  .then( document => {
    document.cookie = 'cookieName={object: [with, commas]}; expires=Fri, 19 Jun 2020 20:47:11 UTC; path=/'
  });

with this method you can set your cookie in any way you need since this bypasses validations done by Cypress.

@krvajal

This comment has been minimized.

Copy link

@krvajal krvajal commented Dec 4, 2019

Thanks for the provided workaround. But still I don't understand how we ended up on this path. Why is Cypress checking the cookie values?

@flotwig

This comment has been minimized.

Copy link
Member

@flotwig flotwig commented Dec 5, 2019

Thanks for the provided workaround. But still I don't understand how we ended up on this path. Why is Cypress checking the cookie values?

Invalid cookie values can cause the browser automation APIs themselves to fail. Chrome fails with a generic error that just says "setting the cookie failed", but gives you no context why. Firefox fails with no error at all.

So if we validate the cookies before sending them to the automation layer, we can give the user a better error, that tells them where to look. But clearly, there's a difference in what the browsers accept and what Cypress thinks is acceptable.


Perhaps the best path forward is to just blindly accept any string value for name or value, but then if there is an error from the browser, we can apply the strict cookie parsing to try and help users find the source of the error. Any thoughts?

@TomDobbelaere

This comment has been minimized.

Copy link

@TomDobbelaere TomDobbelaere commented Dec 13, 2019

Our end to end tests are completely broken because of this, suppressing the error doesn't seem to be working as expected

Please just put it back the way it was, it works fine in the browser. We may have to downgrade to a lower version of cypress until this is resolved

@flotwig

This comment has been minimized.

Copy link
Member

@flotwig flotwig commented Dec 13, 2019

@TomDobbelaere If you put that beforeEach in your supportFile, it will suppress any invalid value errors, there is no reason why it wouldn't work. What error are you getting?

@flotwig

This comment has been minimized.

Copy link
Member

@flotwig flotwig commented Dec 13, 2019

I've opened a PR here to remove the validation, now cookie validation will be left up to the browser automation APIs: #5957

@bahmutov

This comment has been minimized.

Copy link
Contributor

@bahmutov bahmutov commented Dec 13, 2019

I feel this could be a warning we do - since browsers really don't give you an actionable error message if setting a cookie fails

@flotwig

This comment has been minimized.

Copy link
Member

@flotwig flotwig commented Dec 13, 2019

@bahmutov I was thinking that if the browser fails to set the cookie, we could append a hint from Cypress if we detect that the name or value is potentially problematic via strict-cookie-parser. But maybe we should wait and see how common those types of issues are before implementing an experience around it.

@cypress-bot

This comment has been minimized.

Copy link

@cypress-bot cypress-bot bot commented Dec 16, 2019

The code for this is done in cypress-io/cypress#5957, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@TomDobbelaere

This comment has been minimized.

Copy link

@TomDobbelaere TomDobbelaere commented Dec 20, 2019

Neither workaround was working. After a lot of searching it turns out it was due to a different change in 3.5.0, namely:
image

Due to this, the cookie domains were no longer .acme.localhost like in 3.4.1, but now acme.localhost in 3.5.0. I had to solve it by explicitly specifying the domain. This behaviour broke the application because the api and front-end have to share cookies, and they do that by being on the same domain (e.g. www.acme.localhost and api.acme.localhost).

The stubbing workaround works fine to suppress the error, thank you.

@cypress-bot

This comment has been minimized.

Copy link

@cypress-bot cypress-bot bot commented Dec 26, 2019

Released in 3.8.1.

@cypress-io cypress-io locked as resolved and limited conversation to collaborators Dec 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants
You can’t perform that action at this time.