Skip to content

fix: allow special use domains by default#249

Merged
awaterma merged 1 commit into
masterfrom
fix/246_allow_special_use_domains_by_default
Aug 24, 2022
Merged

fix: allow special use domains by default#249
awaterma merged 1 commit into
masterfrom
fix/246_allow_special_use_domains_by_default

Conversation

@colincasey
Copy link
Copy Markdown
Contributor

@colincasey colincasey commented Aug 24, 2022

To avoid breaking behavior the allowSpecialUseDomain option should have been set to true by default.

This PR also adds tests that cover when a default CookieStore is created it does allow cookies with special use domains.

closes #246

@colincasey
fix: allow special use domains by default
0564a6f 
To avoid breaking behavior the `allowSpecialUseDomain` option should have been set to `true` by default.

This PR also adds tests that cover when a default `CookieStore` is created it does allow cookies with special use domains.

closes #246
@colincasey colincasey added the patch We expect this work to be a patch level change label Aug 24, 2022
@colincasey colincasey requested a review from awaterma August 24, 2022 03:15
@colincasey colincasey self-assigned this Aug 24, 2022
halivert
halivert approved these changes Aug 24, 2022
View reviewed changes
Copy link
Copy Markdown

@halivert halivert left a comment

Choose a reason for hiding this comment

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

Thanks @colincasey, great work 🎉

Comment thread lib/cookie.js
@awaterma
Copy link
Copy Markdown
Member

awaterma commented Aug 24, 2022

Looks good to me as well @colincasey; nice to see a fix that makes a change and adds a test. :)

I've also hand-checked straight localhost creation:

cookie@d8759915569c:~$ node 
Welcome to Node.js v16.17.0.
Type ".help" for more information.
> const Cookie = require(".")
undefined
> jar = new Cookie.CookieJar();
CookieJar {
  rejectPublicSuffixes: true,
  enableLooseMode: false,
  allowSpecialUseDomain: true,
  store: { idx: {} },
  prefixSecurity: 'silent',
  _cloneSync: [Function (anonymous)],
  _importCookiesSync: [Function (anonymous)],
  getCookiesSync: [Function (anonymous)],
  getCookieStringSync: [Function (anonymous)],
  getSetCookieStringsSync: [Function (anonymous)],
  removeAllCookiesSync: [Function (anonymous)],
  setCookieSync: [Function (anonymous)],
  serializeSync: [Function (anonymous)]
}
> await(jar.setCookie('settingThisShouldPass=true,Domain=localhost; Path="/;"','http://localhost'))
Cookie="settingThisShouldPass=true,Domain=localhost; Path=/; "; hostOnly=true; aAge=1ms; cAge=1ms"

awaterma
awaterma approved these changes Aug 24, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@awaterma awaterma left a comment

Choose a reason for hiding this comment

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

Approved

@awaterma
Copy link
Copy Markdown
Member

awaterma commented Aug 24, 2022

Merging with single approval and discussion with @colincasey. Will work on our 4.1.1 release next.

@awaterma awaterma merged commit d4ac580 into master Aug 24, 2022
@awaterma awaterma deleted the fix/246_allow_special_use_domains_by_default branch August 24, 2022 18:16
@CSchulz
Copy link
Copy Markdown

CSchulz commented Aug 24, 2022 •
edited
Loading

This doesn't close #246 completly, because there is still the breaking change mentioned in #248 contained.
If you are using localhost as test environment url it is not enough, you have to change it to something else like localhost.local.

@Sourav-techlur
Copy link
Copy Markdown

Sourav-techlur commented Aug 25, 2022

Hey @awaterma I am still having the same issue with v4.1.1.

@Silverium Silverium mentioned this pull request Aug 26, 2023
Open
@botanical botanical mentioned this pull request May 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@awaterma awaterma awaterma approved these changes

+2 more reviewers

@halivert halivert halivert approved these changes

@bartgriffith bartgriffith bartgriffith approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@colincasey colincasey

Labels

patch We expect this work to be a patch level change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4.1 introduced breaking changes

6 participants

@colincasey @awaterma @CSchulz @Sourav-techlur @halivert @bartgriffith