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

[Core HTTP] @azure/core-http Failed to set cookies when the domain is localhost after its sub-dependency upgraded #23004

Closed
MaryGao opened this issue Aug 25, 2022 · 4 comments
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. needs-team-triage This issue needs the team to triage.

Comments

@MaryGao
Copy link
Contributor

MaryGao commented Aug 25, 2022

  • Package Name: @azure/core-http
  • Package Version: both @azure/core-http@1 and @azure/core-http@2

Describe the bug
In autorest.typescript repo we have integration tests to set user agent info in cookies code here. In this test we add the @azure/core-http as the dependency.

"@azure/core-http": "^2.0.0"

Everything works fine expect recently we upgrade our dependency to the latest. AND one of the core-http's sub-dependency tough-cookie upgrades from 4.0.0 to 4.1.0.

https://github.com/Azure/autorest.typescript/blob/023dafb4c6c1ffc18daf5619798d231be3aa08c5/common/config/rush/pnpm-lock.yaml#L390.

Our test cases failed with below message link here:

13) Integration tests for User Agents
       should send correct user agent string with custom string for core v1:
     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}.
      at Object.getPublicSuffix (/Users/runner/work/1/s/common/temp/node_modules/.pnpm/tough-cookie@4.1.0/node_modules/tough-cookie/lib/pubsuffix-psl.js:62:11)
      at permuteDomain (/Users/runner/work/1/s/common/temp/node_modules/.pnpm/tough-cookie@4.1.0/node_modules/tough-cookie/lib/permuteDomain.js:38:28)
      at MemoryCookieStore.findCookies (/Users/runner/work/1/s/common/temp/node_modules/.pnpm/tough-cookie@4.1.0/node_modules/tough-cookie/lib/memstore.js:99:21)
      at MemoryCookieStore.findCookies (/Users/runner/work/1/s/common/temp/node_modules/.pnpm/universalify@0.2.0/node_modules/universalify/index.js:5:67)
      at CookieJar.getCookies (/Users/runner/work/1/s/common/temp/node_modules/.pnpm/tough-cookie@4.1.0/node_modules/tough-cookie/lib/cookie.js:1407:11)
      at CookieJar.getCookies (/Users/runner/work/1/s/common/temp/node_modules/.pnpm/universalify@0.2.0/node_modules/universalify/index.js:5:67)
      at CookieJar.getCookieString (/Users/runner/work/1/s/common/temp/node_modules/.pnpm/tough-cookie@4.1.0/node_modules/tough-cookie/lib/cookie.js:1452:21)
      at CookieJar.getCookieString (/Users/runner/work/1/s/common/temp/node_modules/.pnpm/universalify@0.2.0/node_modules/universalify/index.js:5:67)
      at /Users/runner/work/1/s/common/temp/node_modules/.pnpm/@azure+core-http@1.2.6/node_modules/@azure/core-http/src/nodeFetchHttpClient.ts:95:25
      

Expected behavior
Pass all test cases.

@ghost ghost added the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Aug 25, 2022
@MaryGao MaryGao changed the title [Core HTTP] @azure/core-http Failed to set cookies when the domain is localhost when its sub-dependency upgraded [Core HTTP] @azure/core-http Failed to set cookies when the domain is localhost after its sub-dependency upgraded Aug 25, 2022
@azure-sdk azure-sdk added Azure.Core Client This issue points to a problem in the data-plane of the library. needs-team-triage This issue needs the team to triage. labels Aug 25, 2022
@ghost ghost removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Aug 25, 2022
@sheilf-synapse
Copy link

Salesforce is working on it
salesforce/tough-cookie#253

@xirzec
Copy link
Member

xirzec commented Aug 25, 2022

@MaryGao is there any work on core-http here? It looks like we're still depending on ^4.0.0.

I believe you could work around this pulling in 4.1.0 by either depending on ~4.0.0 in your app itself or by using overrides in your package.json: https://docs.npmjs.com/cli/v8/configuring-npm/package-json#overrides

Really having support for cookies in core is degenerate, we don't actually want to do this for any service calls ever. It was removed in core-rest-pipeline for this reason.

@MaryGao
Copy link
Contributor Author

MaryGao commented Aug 26, 2022

@xirzec If this issue could be resolved from the salesforce/tough-cookie lib, I think we don't need any work on core-http.

Thanks for the suggestion, we could lock our tough-cookie on ~4.0.0. I believe this is our test cases existing only in Track 1 and we'll investigate if we really need them in here.

@xirzec
Copy link
Member

xirzec commented Aug 26, 2022

I'm going to close this for now, but please reopen if there are changes you need in core-http to unblock this.

@xirzec xirzec closed this as completed Aug 26, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. needs-team-triage This issue needs the team to triage.
Projects
None yet
Development

No branches or pull requests

4 participants