Prevent XSS in the cookie attributes

[FagnerMartinsBrack]

Honestly, I'm not happy with this +19 bytes gzipped:

X-Ref: #396

[FagnerMartinsBrack]

#370 (comment) might help to recover these bytes, though...

[carhartl]

We are able to save more bytes... with the proposed change we get +6 gzipped.

[carhartl]

We could write

stringifiedAttributes += '=' + attributes[attributeName].split(';')[0];

and save some more bytes.

[carhartl]

[carhartl]

Well, unless attributes[attributeName] were needed afterwards for some other processing... but it doesn't look like it.

[FagnerMartinsBrack]

Good idea! Done.

[carhartl]

Looking through the code, I found at least one opportunity to reduce bytes, there might be more:

key = encodeURIComponent(String(key));
key = key.replace(/%(23|24|26|2B|5E|60|7C)/g, decodeURIComponent);
key = key.replace(/[\(\)]/g, escape);

=>

key = encodeURIComponent(String(key))
    .replace(/%(23|24|26|2B|5E|60|7C)/g, decodeURIComponent)
    .replace(/[\(\)]/g, escape);

I'm going to prepare a PR for it.

[FagnerMartinsBrack]

I'm going to prepare a PR for it.

X-Ref: #401

[FagnerMartinsBrack]

Bytes after 99e379b:

I guess the byte reduction from #401 negates this one :D

[carhartl]

Haha, so I'll have to find a way to get rid of 1 more byte...

[carhartl]

We have 22 on the plus side now...

[qiqingfu]

hi, I look at the code in the process, encountered a question, but also ask edify ing on the . I can understand this line of code in this function, but I don't understand why you're splitting up.

stringifiedAttributes += '=' + attributes[attributeName].split(';')[0]

attributes are an option object that sets cookies, but the value value of this option object contains ';'' .

For example, I write like this.

const attributes = {
  path: '/',
  expires: 7,
}
Cookie.set('name', 'value', attributes)

The value values of the attributes are '/' and 7, respectively, and do not contain ';'' the symbol, why split it. this makes me a little confused, I hope the big guy to tell you what you think.

Thank you very much.

[FagnerMartinsBrack]

@qiqingfu If you remove those lines, run the tests and debug, you'll find out why they're necessary. If no test fails, then that means that code is not necessary because we can (and we do) cover 100% of the use cases of js-cookie.

There's also a comment in the code in the diff from this PR explaining why that piece eas necessary.

[qiqingfu]

@FagnerMartinsBrack

Thank you very much for your answer, I found the reason in the test case.

QUnit.test(
  'sanitization of attributes to prevent XSS from untrusted input',
  function (assert) {
    assert.expect(1)
    assert.strictEqual(
      Cookies.set('c', 'v', {
        path: '/;domain=sub.domain.com',
        domain: 'site.com;remove_this',
        customAttribute: 'value;;remove_this'
      }),
      'c=v; path=/; domain=site.com; customAttribute=value',
      'should not allow semicolon in a cookie attribute'
    )
  }
)