Use cookie package again

[blakeembrey]

Reverting the change to use cookie from #260. As far as I can tell these two packages are in sync, and I recently merged some security and performance updates you might want to apply here. Overall parsing should be ~10% faster and serialization no longer allows string fields (such as value, domain, and path) to escape their bounds by using a ;.

I didn't see any benchmarks relevant to parsing.

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[blakeembrey]

I'm not too familiar with why reaction was to fork, and it looks like that historical issue was resolved ~2 weeks after the code was copied over to this repo, so let me know if I'm missing something. I'll definitely try to maintain the cookie repo and merge changes faster moving forward.

[Uzlopak]

I dont see any reason at all to go back to that dependency. Just because they fixed something after years, is not a reason.

[blakeembrey]

Just because they fixed something after years, is not a reason.

What was broken? As far as I can read there was a feature that took 3 weeks to merge, is there something I’m missing?

[mcollina]

I'm not too familiar with why reaction was to fork

Given the impossibility of releasing fixes in any timely manner for the original repos of jshttp, we have resorted to immediate forking as there was no chance of anything happening. They had been abandoned. Moreover, the module support for the EOL release made contributing incredibly hard. After being burned a few times, I decided to not try again.

I think the new express TSC should have a chance. I'm in favor of restoring our dependency with cookie, as it will remove us from security reporting chain of that piece of code.

Found something.

[Uzlopak]

Because you deleted cookie-module.test.js I was curious, how it is ensured, that we dont have a breaking change.

I checked it and the module is somehow deviating from our vendored code.

test('unencoded', (t) => {
  t.plan(2)
  t.same(cookie.serialize('cat', '+ ', {
    encode: function (value) { return value }
  }), 'cat=+ ')

  t.throws(cookie.serialize.bind(cookie, 'cat', '+ \n', {
    encode: function (value) { return value }
  }), /argument val is invalid/)
  t.end()
})

results in:

# Subtest: unencoded
    1..2
    not ok 1 - argument val is invalid
      ---
      stack: |
        Object.serialize (node_modules/cookie/index.js:198:11)
        Test.<anonymous> (test/cookie-module.test.js:197:17)
      at:
        fileName: node_modules/cookie/index.js
        lineNumber: 198
        columnNumber: 11
        typeName: Object
        methodName: serialize
        functionName: Object.serialize
      type: TypeError
      tapCaught: testFunctionThrow
      source: |2
      
          if (!cookieValueRegExp.test(value)) {
            throw new TypeError('argument val is invalid');
        ----------^
          }
      ...
    
    not ok 2 - test count(1) != plan(2)
      ---
      at:
        fileName: test/cookie-module.test.js
        lineNumber: 196
        columnNumber: 5
        typeName: Test
      source: |
      
        test('unencoded', (t) => {
          t.plan(2)
        ----^
          t.same(cookie.serialize('cat', '+ ', {
            encode: function (value) { return value }
      diff: |
        --- expected
        +++ actual
        @@ -1,1 +1,1 @@
        -2
        +1
      ...
    
not ok 21 - unencoded # time=31.884ms
  ---
  at:
    fileName: test/cookie-module.test.js
    lineNumber: 195
    columnNumber: 1
    typeName: Object
  source: |
    })
  
    test('unencoded', (t) => {
    ^
      t.plan(2)
      t.same(cookie.serialize('cat', '+ ', {
  ...

[blakeembrey]

I checked it and the module is somehow deviating from our vendored code.

That is correct, I should have highlighted it more strongly.

The package is following the RFC spec properly, so some fields that didn't error before could error now, if they contain values that are invalid. The spec does not allow whitespace in the cookie value. It could be added back if you think it's important, but the default encoding would translate the space to %20.

It also deviates in that you have a bug in your package that's trimming whitespace within " during parsing.

Edit: Also, just to be clear with that test, it produces cat=+ . When sent to a browser, that trailing whitespace is getting trimmed, so it'd be cat=+. That's another good reason to more closely follow the spec, which results in the cookie meeting expectations.

[gurgunday]

Alright, seems like the package is more active these days

@blakeembrey I'm also down to moving back after v1 is released

I also see some perf gains jshttp/cookie#172

[Uzlopak]

Its not only using v 1.0.0 but switching to semver

[blakeembrey]

Released and bumped 1.0, there are some breaking changes: https://github.com/jshttp/cookie/releases/tag/v1.0.0

Most notable are JS version used (should be fine since this package uses the same features), null prototype object returned in parse, strict and priority strings, and maxAge integer requirement.

If users are overwriting decode the try..catch is also relevant, but without overwriting the behavior is the same.

[gurgunday]

Already released v1? Awesome!

The upstream is in a great shape right now

[mcollina]

lgtm

I would consider bumping the major for this

[gurgunday]

Yeah, we'll combine it with #304

.