Repository: salesforce/tough-cookie (ts)

Issue:
Can url-parse library be dropped?
Hello!

According to url-parse README file (https://www.npmjs.com/package/url-parse):

> url-parse was created in 2014 when the WHATWG URL API was not available in Node.js and the URL interface was supported only in some browsers. Today this is no longer true. The URL interface is available in all supported Node.js release lines and basically all browsers. Consider using it for better security and accuracy.

I wonder if url-parse is still necessary in this library, or it can be abandoned and URL be used instead. Is there any particular reason not to do this?
If this is ok, I'd be available to submit a Pull Request that does the work

Pull request description:
Remove url-parse and punycode
We originally added url-parse to facilitate using the library with react native. However, there's a [URL polyfill](https://github.com/charpeni/react-native-url-polyfill) that react native users can use, and we don't otherwise support the runtime, so we've decided to remove the dependency (fixes #278).

I also realized that `new URL(str)` also does the same thing as the `punycode` library, so I removed that as well.

Patch:
diff --git a/lib/cookie/canonicalDomain.ts b/lib/cookie/canonicalDomain.ts
index fe9de57..2b7830e 100644
--- a/lib/cookie/canonicalDomain.ts
+++ b/lib/cookie/canonicalDomain.ts
@@ -1,4 +1,3 @@
-import { toASCII } from 'punycode/punycode.js'
 import { IP_V6_REGEX_OBJECT } from './constants'
 import type { Nullable } from '../utils'
 
@@ -39,17 +38,24 @@ export function canonicalDomain(
   if (domainName == null) {
     return undefined
   }
-  let _str = domainName.trim().replace(/^\./, '') // S4.1.2.3 & S5.2.3: ignore leading .
+  let str = domainName.trim().replace(/^\./, '') // S4.1.2.3 & S5.2.3: ignore leading .
 
-  if (IP_V6_REGEX_OBJECT.test(_str)) {
-    _str = _str.replace('[', '').replace(']', '')
+  if (IP_V6_REGEX_OBJECT.test(str)) {
+    if (!str.startsWith('[')) {
+      str = '[' + str
+    }
+    if (!str.endsWith(']')) {
+      str = str + ']'
+    }
+    return new URL(`http://${str}`).hostname.slice(1, -1) // remove [ and ]
   }
 
   // convert to IDN if any non-ASCII characters
   // eslint-disable-next-line no-control-regex
-  if (/[^\u0001-\u007f]/.test(_str)) {
-    _str = toASCII(_str)
+  if (/[^\u0001-\u007f]/.test(str)) {
+    return new URL(`http://${str}`).hostname
   }
 
-  return _str.toLowerCase()
+  // ASCII-only domain - not canonicalized with new URL() because it may be a malformed URL
+  return str.toLowerCase()
 }
diff --git a/lib/cookie/cookieJar.ts b/lib/cookie/cookieJar.ts
index 8dddf41..b0de567 100644
--- a/lib/cookie/cookieJar.ts
+++ b/lib/cookie/cookieJar.ts
@@ -1,5 +1,3 @@
-import urlParse from 'url-parse'
-
 import { getPublicSuffix } from '../getPublicSuffix'
 import * as validators from '../validators'
 import { ParameterError } from '../validators'
@@ -190,14 +188,14 @@ export interface CreateCookieJarOptions {
 const SAME_SITE_CONTEXT_VAL_ERR =
   'Invalid sameSiteContext option for getCookies(); expected one of "strict", "lax", or "none"'
 
-function getCookieContext(url: unknown): URL | urlParse<string> {
+function getCookieContext(url: unknown): URL {
   if (url instanceof URL) {
     return url
   } else if (typeof url === 'string') {
     try {
-      return urlParse(decodeURI(url))
+      return new URL(decodeURI(url))
     } catch {
-      return urlParse(url)
+      return new URL(url)
     }
   } else {
     throw new ParameterError('`url` argument is not a string or URL.')
@@ -437,7 +435,7 @@ export class CookieJar {
     }
     const promiseCallback = createPromiseCallback(callback)
     const cb = promiseCallback.callback
-    let context
+    let context: URL
 
     try {
       if (typeof url === 'string') {
@@ -475,7 +473,6 @@ export class CookieJar {
     const host = canonicalDomain(context.hostname) ?? null
     const loose = options?.loose || this.enableLooseMode
 
-    let err
     let sameSiteContext = null
     if (options?.sameSiteContext) {
       sameSiteContext = checkSameSiteContext(options.sameSiteContext)
@@ -488,7 +485,7 @@ export class CookieJar {
     if (typeof cookie === 'string' || cookie instanceof String) {
       const parsedCookie = Cookie.parse(cookie.toString(), { loose: loose })
       if (!parsedCookie) {
-        err = new Error('Cookie failed to parse')
+        const err = new Error('Cookie failed to parse')
         return options?.ignoreError
           ? promiseCallback.resolve(undefined)
           : promiseCallback.reject(err)
@@ -497,7 +494,7 @@ export class CookieJar {
     } else if (!(cookie instanceof Cookie)) {
       // If you're seeing this error, and are passing in a Cookie object,
       // it *might* be a Cookie object from another loaded version of tough-cookie.
-      err = new Error(
+      const err = new Error(
         'First argument to setCookie must be a Cookie object or string',
       )
 
@@ -526,7 +523,7 @@ export class CookieJar {
             : null
         if (suffix == null && !IP_V6_REGEX_OBJECT.test(cookie.domain)) {
           // e.g. "com"
-          err = new Error('Cookie has domain set to a public suffix')
+          const err = new Error('Cookie has domain set to a public suffix')
 
           return options?.ignoreError
             ? promiseCallback.resolve(undefined)
@@ -549,7 +546,7 @@ export class CookieJar {
       if (
         !domainMatch(host ?? undefined, cookie.cdomain() ?? undefined, false)
       ) {
-        err = new Error(
+        const err = new Error(
           `Cookie not in this host's domain. Cookie:${
             cookie.cdomain() ?? 'null'
           } Request:${host ?? 'null'}`,
@@ -581,7 +578,7 @@ export class CookieJar {
 
     // S5.3 step 10
     if (options?.http === false && cookie.httpOnly) {
-      err = new Error("Cookie is HttpOnly and this isn't an HTTP API")
+      const err = new Error("Cookie is HttpOnly and this isn't an HTTP API")
       return options.ignoreError
         ? promiseCallback.resolve(undefined)
         : promiseCallback.reject(err)
@@ -598,7 +595,9 @@ export class CookieJar {
       //  exact match for request-uri's host's registered domain, then
       //  abort these steps and ignore the newly created cookie entirely."
       if (sameSiteContext === 'none') {
-        err = new Error('Cookie is SameSite but this is a cross-origin request')
+        const err = new Error(
+          'Cookie is SameSite but this is a cross-origin request',
+        )
         return options?.ignoreError
           ? promiseCallback.resolve(undefined)
           : promiseCallback.reject(err)
@@ -820,7 +819,7 @@ export class CookieJar {
     }
     const promiseCallback = createPromiseCallback(callback)
     const cb = promiseCallback.callback
-    let context
+    let context: URL
 
     try {
       if (typeof url === 'string') {
diff --git a/package-lock.json b/package-lock.json
index 9d6a587..3ce0aa5 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -9,9 +9,7 @@
       "version": "5.0.0-rc.3",
       "license": "BSD-3-Clause",
       "dependencies": {
-        "punycode": "^2.3.1",
-        "tldts": "^6.1.28",
-        "url-parse": "^1.5.10"
+        "tldts": "^6.1.28"
       },
       "devDependencies": {
         "@eslint/js": "^9.5.0",
@@ -19,8 +17,6 @@
         "@microsoft/api-extractor": "^7.47.0",
         "@types/jest": "^29.5.12",
         "@types/node": "^14.18.63",
-        "@types/punycode": "^2.1.4",
-        "@types/url-parse": "^1.4.11",
         "async": "3.2.5",
         "eslint": "^8.57.0",
         "eslint-config-prettier": "^9.1.0",
@@ -1594,24 +1590,12 @@
       "integrity": "sha512-fAtCfv4jJg+ExtXhvCkCqUKZ+4ok/JQk01qDKhL5BDDoS3AxKXhV5/MAVUZyQnSEd2GT92fkgZl0pz0Q0AzcIQ==",
       "dev": true
     },
-    "node_modules/@types/punycode": {
-      "version": "2.1.4",
-      "resolved": "https://registry.npmjs.org/@types/punycode/-/punycode-2.1.4.tgz",
-      "integrity": "sha512-trzh6NzBnq8yw5e35f8xe8VTYjqM3NE7bohBtvDVf/dtUer3zYTLK1Ka3DG3p7bdtoaOHZucma6FfVKlQ134pQ==",
-      "dev": true
-    },
     "node_modules/@types/stack-utils": {
       "version": "2.0.3",
       "resolved": "https://registry.npmjs.org/@types/stack-utils/-/stack-utils-2.0.3.tgz",
       "integrity": "sha512-9aEbYZ3TbYMznPdcdr3SmIrLXwC/AKZXQeCf9Pgao5CKb8CyHuEX5jzWPTkvregvhRJHcpRO6BFoGW9ycaOkYw==",
       "dev": true
     },
-    "node_modules/@types/url-parse": {
-      "version": "1.4.11",
-      "resolved": "https://registry.npmjs.org/@types/url-parse/-/url-parse-1.4.11.tgz",
-      "integrity": "sha512-FKvKIqRaykZtd4n47LbK/W/5fhQQ1X7cxxzG9A48h0BGN+S04NH7ervcCjM8tyR0lyGru83FAHSmw2ObgKoESg==",
-      "dev": true
-    },
     "node_modules/@types/yargs": {
       "version": "17.0.32",
       "resolved": "https://registry.npmjs.org/@types/yargs/-/yargs-17.0.32.tgz",
@@ -4783,6 +4767,7 @@
       "version": "2.3.1",
       "resolved": "https://registry.npmjs.org/punycode/-/punycode-2.3.1.tgz",
       "integrity": "sha512-vYt7UD1U9Wg6138shLtLOvdAu+8DsC/ilFtEVHcH+wydcSpNE20AfSOduf6MkRFahL5FY7X1oU7nKVZFtfq8Fg==",
+      "dev": true,
       "engines": {
         "node": ">=6"
       }
@@ -4803,11 +4788,6 @@
         }
       ]
     },
-    "node_modules/querystringify": {
-      "version": "2.2.0",
-      "resolved": "https://registry.npmjs.org/querystringify/-/querystringify-2.2.0.tgz",
-      "integrity": "sha512-FIqgj2EUvTa7R50u0rGsyTftzjYmv/a3hO345bZNrqabNqjtgiDMgmo4mkUjd+nzU5oF3dClKqFIPUKybUyqoQ=="
-    },
     "node_modules/queue-microtask": {
       "version": "1.2.3",
       "resolved": "https://registry.npmjs.org/queue-microtask/-/queue-microtask-1.2.3.tgz",
@@ -4852,11 +4832,6 @@
         "node": ">=0.10.0"
       }
     },
-    "node_modules/requires-port": {
-      "version": "1.0.0",
-      "resolved": "https://registry.npmjs.org/requires-port/-/requires-port-1.0.0.tgz",
-      "integrity": "sha512-KigOCHcocU3XODJxsu8i/j8T9tzT4adHiecwORRQ0ZZFcp7ahwXuRU1m+yuO90C5ZUyGeGfocHDI14M3L3yDAQ=="
-    },
     "node_modules/resolve": {
       "version": "1.22.8",
       "resolved": "https://registry.npmjs.org/resolve/-/resolve-1.22.8.tgz",
@@ -5495,15 +5470,6 @@
         "punycode": "^2.1.0"
       }
     },
-    "node_modules/url-parse": {
-      "version": "1.5.10",
-      "resolved": "https://registry.npmjs.org/url-parse/-/url-parse-1.5.10.tgz",
-      "integrity": "sha512-WypcfiRhfeUP9vvF0j6rw0J3hrWrw6iZv3+22h6iRMJ/8z1Tj6XfLP4DsUix5MhMPnXpiHDoKyoZ/bdCkwBCiQ==",
-      "dependencies": {
-        "querystringify": "^2.1.1",
-        "requires-port": "^1.0.0"
-      }
-    },
     "node_modules/v8-compile-cache-lib": {
       "version": "3.0.1",
       "resolved": "https://registry.npmjs.org/v8-compile-cache-lib/-/v8-compile-cache-lib-3.0.1.tgz",
diff --git a/package.json b/package.json
index 324727d..813de74 100644
--- a/package.json
+++ b/package.json
@@ -117,8 +117,6 @@
     "@microsoft/api-extractor": "^7.47.0",
     "@types/jest": "^29.5.12",
     "@types/node": "^14.18.63",
-    "@types/punycode": "^2.1.4",
-    "@types/url-parse": "^1.4.11",
     "async": "3.2.5",
     "eslint": "^8.57.0",
     "eslint-config-prettier": "^9.1.0",
@@ -134,8 +132,6 @@
     "vows": "^0.8.3"
   },
   "dependencies": {
-    "punycode": "^2.3.1",
-    "tldts": "^6.1.28",
-    "url-parse": "^1.5.10"
+    "tldts": "^6.1.28"
   }
 }


Test patch:
diff --git a/lib/__tests__/cookieJar.spec.ts b/lib/__tests__/cookieJar.spec.ts
index 3d2e95d..4a3f5e0 100644
--- a/lib/__tests__/cookieJar.spec.ts
+++ b/lib/__tests__/cookieJar.spec.ts
@@ -1068,7 +1068,7 @@ describe('setCookie errors', () => {
   it('should throw an error if domain is set to a public suffix', async () => {
     const cookieJar = new CookieJar()
     await expect(
-      cookieJar.setCookie('i=9; Domain=kyoto.jp; Path=/', 'kyoto.jp'),
+      cookieJar.setCookie('i=9; Domain=kyoto.jp; Path=/', 'http://kyoto.jp'),
     ).rejects.toThrowError('Cookie has domain set to a public suffix')
   })
 
@@ -1104,6 +1104,13 @@ describe('setCookie errors', () => {
     })
     expect(cookies).toEqual([httpCookie])
   })
+
+  it('should throw when URL is missing protocol', async () => {
+    const cookieJar = new CookieJar()
+    await expect(
+      cookieJar.setCookie('L=12; Domain=example.ch; Path=/', 'example.ch'),
+    ).rejects.toThrow(new TypeError('Invalid URL'))
+  })
 })
 
 describe('loose mode', () => {
@@ -1489,7 +1496,7 @@ describe('Synchronous API on async CookieJar', () => {
 })
 
 describe('validation errors invoke callbacks', () => {
-  it('getCookies', () => {
+  it('getCookies', (done) => {
     const invalidUrl = {}
     const cookieJar = new CookieJar()
     // @ts-expect-error deliberately trigger validation error
@@ -1497,10 +1504,11 @@ describe('validation errors invoke callbacks', () => {
       expect(err).toMatchObject({
         message: '`url` argument is not a string or URL.',
       })
+      done()
     })
   })
 
-  it('setCookie', () => {
+  it('setCookie', (done) => {
     const invalidUrl = {}
     const cookieJar = new CookieJar()
     // @ts-expect-error deliberately trigger validation error
@@ -1508,6 +1516,7 @@ describe('validation errors invoke callbacks', () => {
       expect(err).toMatchObject({
         message: '`url` argument is not a string or URL.',
       })
+      done()
     })
   })
 })
diff --git a/lib/__tests__/domainMatch.spec.ts b/lib/__tests__/domainMatch.spec.ts
index edb0075..8ef9b92 100644
--- a/lib/__tests__/domainMatch.spec.ts
+++ b/lib/__tests__/domainMatch.spec.ts
@@ -69,6 +69,11 @@ describe('domainMatch', () => {
     ['com', 'net', false], // same len, non-match
     ['com', 'com', true], // "are identical" rule
     ['NOTATLD', 'notaTLD', true], // "are identical" rule (after canonicalization)
+
+    // non-ASCII hostnames
+    ['🫠.com', 'xn--129h.com', true], // Emoji!
+    ['ρһіѕһ.info', 'xn--2xa01ac71bc.info', true], // Greek + Cyrillic characters
+    ['猫.cat', 'xn--z7x.cat', true], // Japanese characters
   ])('domainMatch(%s, %s) => %s', (string, domain, expectedValue) => {
     expect(domainMatch(string, domain)).toBe(expectedValue)
   })
diff --git a/test/cookie_jar_test.js b/test/cookie_jar_test.js
index 7899eb6..1544df1 100644
--- a/test/cookie_jar_test.js
+++ b/test/cookie_jar_test.js
@@ -531,7 +531,7 @@ vows
           const cj = new CookieJar();
           cj.setCookie(
             "i=9; Domain=kyoto.jp; Path=/",
-            "kyoto.jp",
+            "http://kyoto.jp",
             this.callback
           );
         },