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

4.1 introduced breaking changes #246

Closed
Twipped opened this issue Aug 22, 2022 · 41 comments · Fixed by #249
Closed

4.1 introduced breaking changes #246

Twipped opened this issue Aug 22, 2022 · 41 comments · Fixed by #249
Assignees
Labels
patch We expect this work to be a patch level change

Comments

@Twipped
Copy link

Twipped commented Aug 22, 2022

Our jest environment just started throwing errors because allowSpecialUseDomain is now being more enforced in this new release and jest-environment-jsdom doesn't set it. I tried to correct for it via Jest's testEnvironmentOptions config passthru, but that produced other problems. You may want to revert that release, it's gonna break a WHOLE LOT of builds.

@gbettencourt
Copy link

Also breaking our jest tests. Don't see any breaking changes mentioned in the release notes.

@arpowers
Copy link

arpowers commented Aug 22, 2022

Same here! Happy that this isn't a bug on our end, was about to throw the computer out the window.

More info:
Adding cookies via JSDom adds them to localhost domain ... this now errors, cost me about 2 hours

@arpowers
Copy link

arpowers commented Aug 22, 2022

@awaterma after reviewing a few things, this is definitely a breaking change and probably causing lots of headaches right now. Recommend publishing a rollback minor release and pushing v5 with any updates. For now, we are manually overriding the dep via PNPM as we can't update JSDom ourselves and there provide no easy way of patching this (e.g. to provide the options recommended in the error)... \

Screen Shot 2022-08-22 at 3 44 12 PM

@Jhzl2
Copy link

Jhzl2 commented Aug 22, 2022

Our jest environment also failed in our tests. We work on a react app but we don't use tough-cookie. We assume that it is a dependency of some installed library. Could you give me a hand?
image
Locally it does not give us an error, but in github actions it does appear

@awaterma
Copy link
Member

awaterma commented Aug 22, 2022

I'm sorry this is causing you all some issues! For some background on how to use localhost as a special use domain, please see the discussion and tests in issue #215.

I believe this is coming up for v4.1.0 as @ShivanKaul implemented all of the "special-use" domains per RFC 6761 in the following p/r: https://github.com/salesforce/tough-cookie/pull/203/files, which we included in this release. We view this as continued work for the RFC that we started in v4.0.0.

In regards to working with cookies in localhost please see: https://www.rfc-editor.org/rfc/rfc6761.html#section-6.3

I will reach out to the team in Slack and see if we can get some discussion going. Sorry to cause issues for those that depend upon us; we are working to improve tough-cookie and keep up to date with the relevant RFCs to support cookies correctly into the future!

@colincasey

@arpowers
Copy link

arpowers commented Aug 22, 2022

Most of us can certainly empathize with these type of challenges so no worries there. We appreciate the work on keeping things compliant.

Al final, i think the best course of action is to move this to version 5; as a practice versions in NPM should live and die with Semver since things are automatically updated and hard to override (using standard NPM client)... By that, I mean RFC targets shouldn't have any relation to version numbers being used.

@lyz810
Copy link

lyz810 commented Aug 23, 2022

Our jest environment just started throwing errors because allowSpecialUseDomain is now being more enforced in this new release and jest-environment-jsdom doesn't set it. I tried to correct for it via Jest's testEnvironmentOptions config passthru, but that produced other problems. You may want to revert that release, it's gonna break a WHOLE LOT of builds.

If you are using jest@28.x add

testEnvironmentOptions: {
   url: 'https://jestjs.io'
}

to jest config file
If you are using earlier version of jest, add

testURL: 'https://jestjs.io'

to jest config file

You can change https://jestjs.io to your url
It works on me

https://jestjs.io/docs/upgrading-to-jest28#testurl

@privateOmega
Copy link

privateOmega commented Aug 23, 2022

Broke my environment as well. Is there a simple fix for it, other than to pinpoint to the last version?

@MedAmineAyadi96
Copy link

MedAmineAyadi96 commented Aug 23, 2022

Faced this problem this morning, jest test failing, quick walkaround is to add
"tough-cookie": "4.0.0"
to package.json file

@danymarques
Copy link

You can also add as a workaround in your pacakge.json : "overrides": { "jsdom": { "tough-cookie": "4.0.0" } },

@antonvialibri1
Copy link

@lyz810 Adding a valid fully qualified domain name for testEnvironmentOptions.url worked for us.

jtoar added a commit to redwoodjs/redwood that referenced this issue Aug 23, 2022
@AbhaySBhosale
Copy link

New version introduced following error
Cookie has domain set to the public suffix "localhost" which is a special use domain. To allow this, configure your CookieJar with {allowSpecialUseDomain:true, rejectPublicSuffixes: false}.

@CSchulz
Copy link

CSchulz commented Aug 23, 2022

Another workaround is setting your own CookieJar for the jsdom environment:

testEnvironmentOptions: {
  url: 'http://localhost.local/',
  cookieJar: new jsdom.CookieJar(undefined, {
    allowSpecialUseDomain: true,
  }),
},

You have to use a FQDN even for localhost otherwise you will run into #248 bug.

@Twipped
Copy link
Author

Twipped commented Aug 23, 2022

@CSchulz when I've tried that, I get an error that this._cookieJar.getCookieStringSync is not a function

@archer56
Copy link

I have raised a PR to fix the version of tough-cookie to v4.0.0 which should fix the immediate problem

jsdom/jsdom#3420

@CSchulz
Copy link

CSchulz commented Aug 23, 2022

@CSchulz when I've tried that, I get an error that this._cookieJar.getCookieStringSync is not a function

Do you have a more detailled stacktrace?

@embeddedt
Copy link

embeddedt commented Aug 23, 2022

Does anyone have a working override config for pnpm?

I've tried:

{
  "pnpm": {
    "overrides": {
      "tough-cookie": "4.0.0"
    }
  }
}

It does not seem to work, as pnpm why tough-cookie shows 4.1.0 still being used.

EDIT: It seems that this works after installing tough-cookie@4.0.0 as a dev dependency and running pnpm update --depth Infinity.

@matthew-dean
Copy link

Ohhh THIS is why all our tests are breaking. 🤦‍♂️

@CSchulz
Copy link

CSchulz commented Aug 23, 2022

Related #248

@puneetmakkar
Copy link

puneetmakkar commented Aug 23, 2022

failing for me on npm v6. tried both in package.json
"tough-cookie": "4.0.0"
"overrides": { "jsdom": { "tough-cookie": "4.0.0" } }

@privateOmega
Copy link

privateOmega commented Aug 23, 2022

@puneetmakkar

This solution totally depends on which package-manager you are using,

for npm v8+, add overrides key
for yarn, add it under resolutions key
for older npm versions, add a npm-shrinkwrap.json file. or define resolutions and use something like https://www.npmjs.com/package/npm-force-resolutions.

@Ltchoc221
Copy link

Ltchoc221 commented Aug 24, 2022

I am also still seeing this issue using 4.1.1:

|-- jest@27.5.1
|--- @jest/core@27.5.1
|---- jest-config@27.5.1
|----- jest-environment-jsdom@27.5.1
|------ jsdom@16.7.0
|-------tough-cookie@4.1.1

Did the patch completely revert the change that caused this, or was there a new patch laid over the original change in an attempt to fix?

@Uninen
Copy link

Uninen commented Aug 24, 2022

4.1.1 doesn't solve the issue for Vitest users because (at least to my knowledge, please correct me if I'm wrong) there's no way to configure jsdom -- our tests are still failing. (The workaround is to override tough-cookie version to 4.0.0)

vitest 0.22.1
└─┬ jsdom 20.0.0 peer
  └── tough-cookie 4.1.1

@gauravshah27
Copy link

4.1.1 - this does not solve the issue.

@gbettencourt
Copy link

4.1.1 not working for me either

@LiuJinYang9527
Copy link

4.1.1 still not working

jasmine-q added a commit to buildkite/kitesocial-js that referenced this issue Aug 25, 2022
avoiding breaking changes mentioned here salesforce/tough-cookie#246
which no longer allows localhost domains
@dolevoper
Copy link

having the same issue with 4.1.1

colincasey added a commit that referenced this issue Aug 25, 2022
Adding more tests to cover the breaking use cases noted in #246.

e.g.;.
* `new CookieJar().setCookieSync("settingThisShouldPass=true; Domain=localhost; Path=/;", "http://localhost")`

Also modifies the assertion for a test introduced in #221 that may be incorrect.
colincasey added a commit that referenced this issue Aug 25, 2022
Adding more tests to cover the breaking use cases noted in #246.

e.g.;.
* `new CookieJar().setCookieSync("settingThisShouldPass=true; Domain=localhost; Path=/;", "http://localhost")`

Also modifies the assertion for a test introduced in #221 that may be incorrect.
@colincasey colincasey reopened this Aug 25, 2022
colincasey added a commit that referenced this issue Aug 25, 2022
Adding more tests to cover the breaking use cases noted in #246.

e.g.;.
* `new CookieJar().setCookieSync("settingThisShouldPass=true; Domain=localhost; Path=/;", "http://localhost")`

Also modifies the assertion for a test introduced in #221 that may be incorrect.
awaterma pushed a commit that referenced this issue Aug 25, 2022
* fix: allow set cookies with localhost

Adding more tests to cover the breaking use cases noted in #246.

e.g.;.
* `new CookieJar().setCookieSync("settingThisShouldPass=true; Domain=localhost; Path=/;", "http://localhost")`

Also modifies the assertion for a test introduced in #221 that may be incorrect.

* fix: allow set cookies with localhost

Adding more tests to cover the breaking use cases noted in #246.

e.g.;.
* `new CookieJar().setCookieSync("settingThisShouldPass=true; Domain=localhost; Path=/;", "http://localhost")`

Also modifies the assertion for a test introduced in #221 that may be incorrect.

* fix: allow set cookies with localhost

Adding more tests to cover the breaking use cases noted in #246.

e.g.;.
* `new CookieJar().setCookieSync("settingThisShouldPass=true; Domain=localhost; Path=/;", "http://localhost")`

Also modifies the assertion for a test introduced in #221 that may be incorrect.

* fix: allow set cookies with localhost

updated CHANGELOG.md to point to the releases page since changelogs are auto-generated now.

* Release v4.1.2
@awaterma
Copy link
Member

We have a new patch for this issue. We believe that we have now resolved a bug for how we treated single word special use domains (localhost and invalid) that should resolve this issue for everyone.

@gauravshah27
Copy link

@awaterma - I can confirm that 4.1.2 is working for us. Really appreciate the ownership on the issue and swiftness with which the patch was released. Thanks a lot.

@SevenOutman
Copy link

Also confirm 4.1.2 is working here. For users who had trouble with jsdom/jest, you can add overrides to your package.json and run npm update tough-cookie to bump it to the desired version.

{
  "overrides": {
    "tough-cookie": "4.1.2"
  }
}

Reference: https://stackoverflow.com/a/64273186

alan-agius4 added a commit to alan-agius4/universal that referenced this issue Aug 26, 2022
`tough-cookie` which is a dependency of JSDOM did a breaking change in version 4.1 which requires special handling for cookies in localhost.

See: salesforce/tough-cookie#246
alan-agius4 added a commit to angular/universal that referenced this issue Aug 26, 2022
`tough-cookie` which is a dependency of JSDOM did a breaking change in version 4.1 which requires special handling for cookies in localhost.

See: salesforce/tough-cookie#246
alan-agius4 added a commit to angular/universal that referenced this issue Aug 26, 2022
`tough-cookie` which is a dependency of JSDOM did a breaking change in version 4.1 which requires special handling for cookies in localhost.

See: salesforce/tough-cookie#246
(cherry picked from commit 6dcce85)
@dolevoper
Copy link

Also here to confirm the new version works for us :)
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch We expect this work to be a patch level change
Projects
None yet
Development

Successfully merging a pull request may close this issue.