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

Update the UTF-8 percent encoded regex to match only valid encodings … #200

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@mmilford

mmilford commented May 13, 2016

…and to ignore case

assert.deepEqual(Cookies.get(), { c: 'v' }, 'should not throw a URI malformed exception when retrieving all cookies');
Cookies.withConverter(unescape).remove('invalid');
});

This comment has been minimized.

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

Why removing this test?

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

Why removing this test?

@@ -41,6 +41,12 @@ QUnit.test('percent character in cookie value mixed with encoded values', functi
assert.strictEqual(Cookies.get('bad'), 'foo%bar"baz%bax=', 'should read the percent character');
});
QUnit.test('percent character in cookie value mixed with encoded values lower case', function (assert) {
assert.expect(1);
document.cookie = 'mybad=foo%bar%22baz%bax%3d';

This comment has been minimized.

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

In 3d, see here for some context:

Although lowercase and uppercase digits are equivalent per spec, is there any browser where those characters are treated differently after being encoded with encodeURIComponent? Or what is the use case for considering lowercase characters in percent-encoding for the default decoding mechanism, can't this be done sing converters?

Cookies = Cookies.withConverter(acceptLowercaseEncoding);

We use the Strictness Principle for new features in the library, therefore we need at least a reasonable use case that justifies increasing the scope of the behavior of the default decoding mechanism.

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

In 3d, see here for some context:

Although lowercase and uppercase digits are equivalent per spec, is there any browser where those characters are treated differently after being encoded with encodeURIComponent? Or what is the use case for considering lowercase characters in percent-encoding for the default decoding mechanism, can't this be done sing converters?

Cookies = Cookies.withConverter(acceptLowercaseEncoding);

We use the Strictness Principle for new features in the library, therefore we need at least a reasonable use case that justifies increasing the scope of the behavior of the default decoding mechanism.

This comment has been minimized.

@mmilford

mmilford May 14, 2016

No, I'm not aware of any browsers with differing behavior for encodeURIComponent. However, it wasn't clear to me from my reading of your documentation that this library can only be used to read cookies that it creates.

@mmilford

mmilford May 14, 2016

No, I'm not aware of any browsers with differing behavior for encodeURIComponent. However, it wasn't clear to me from my reading of your documentation that this library can only be used to read cookies that it creates.

This comment has been minimized.

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

... it wasn't clear to me from my reading of your documentation that this library can only be used to read cookies that it creates.

Do you want to create a Pull Request that improves the documentation in that sense?

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

... it wasn't clear to me from my reading of your documentation that this library can only be used to read cookies that it creates.

Do you want to create a Pull Request that improves the documentation in that sense?

This comment has been minimized.

@FagnerMartinsBrack

FagnerMartinsBrack May 27, 2016

Member

@mmilford Do you think we can improve the docs about this? Do you have any suggestion? Thanks!

@FagnerMartinsBrack

FagnerMartinsBrack May 27, 2016

Member

@mmilford Do you think we can improve the docs about this? Do you have any suggestion? Thanks!

This comment has been minimized.

@FagnerMartinsBrack

FagnerMartinsBrack Jun 16, 2016

Member

I have created an issue for this, see #211

@FagnerMartinsBrack

FagnerMartinsBrack Jun 16, 2016

Member

I have created an issue for this, see #211

@@ -1,6 +1,6 @@
{
"name": "js-cookie",
"version": "2.1.1",
"version": "2.1.2",

This comment has been minimized.

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

There is no need to increase the version in this commit, this is done when a new version is released.

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

There is no need to increase the version in this commit, this is done when a new version is released.

This comment has been minimized.

@mmilford

mmilford May 14, 2016

Sorry, didn't realize.

@mmilford

mmilford May 14, 2016

Sorry, didn't realize.

// Values U+8000 - U+FFFF are 3 byte encoded. The first byte begins with 1110 (E). The continuation byte begins with 10 ([89AB])
// Values U+10000 - U+FFFFF are 4 byte encoded. The first byte begins with 111100 (F[0-3]). The continuation byte begins with 10 ([89AB])
// Values U+100000 - U+10FFFF are 4 byte encoded. The first byte is 11110100 (F4). The second byte begins with 1000 (8). The continuation byte begins with 10 ([89AB])
var rdecode = /(%[0-7][0-9A-F])|(%[CD][0-9A-F]%[89AB][0-9A-F])|(%E[0-9A-F]%[89AB][0-9A-F]%[89AB][0-9A-F])|(%F3%[89AB][0-9A-F]%[89AB][0-9A-F]%[89AB][0-9A-F])|(%F4%8[0-9A-F]%[89AB][0-9A-F]%[89AB][0-9A-F])+/gi;

This comment has been minimized.

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

The default decoding mechanism for cookie values is meant to be used from js-cookie to js-cookie. Anything different should be done using converters.

Can you please create some tests in the encoding.js file that reproduces the problem this code is trying to solve using js-cookie APIs to set the cookie?

If this problem is not about js-cookie, but an attempt to adapt itself to external cookies that are created in another format, can you please explain where are you getting this cookie so that we can understand what the use case is? Or if possible, create a test simulating the use case.

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

The default decoding mechanism for cookie values is meant to be used from js-cookie to js-cookie. Anything different should be done using converters.

Can you please create some tests in the encoding.js file that reproduces the problem this code is trying to solve using js-cookie APIs to set the cookie?

If this problem is not about js-cookie, but an attempt to adapt itself to external cookies that are created in another format, can you please explain where are you getting this cookie so that we can understand what the use case is? Or if possible, create a test simulating the use case.

This comment has been minimized.

@mmilford

mmilford May 14, 2016

The cookie's being created by an ASP.NET MVC web project, using a standard method (UrlEncode) https://msdn.microsoft.com/en-us/library/system.web.httputility.urlencode(v=vs.110).aspx. Why they chose to lowercase the hex values is unclear, but as you noted above it is spec compliant.

As I mentioned before, it wasn't clear to me from the documentation that I'd have to write custom converters for use cases that weren't strictly js-cookie to js-cookie. However, as a consumer of this library I think that it would make sense for it to be updated to read all spec compliant cookies, as that seems like a standard use case. Custom converters seems appropriate for custom cases, which I would argue this is not.

I can definitely add some unit tests in encoding.js to highlight what are valid and invalid UTF-8 percent encodings.

@mmilford

mmilford May 14, 2016

The cookie's being created by an ASP.NET MVC web project, using a standard method (UrlEncode) https://msdn.microsoft.com/en-us/library/system.web.httputility.urlencode(v=vs.110).aspx. Why they chose to lowercase the hex values is unclear, but as you noted above it is spec compliant.

As I mentioned before, it wasn't clear to me from the documentation that I'd have to write custom converters for use cases that weren't strictly js-cookie to js-cookie. However, as a consumer of this library I think that it would make sense for it to be updated to read all spec compliant cookies, as that seems like a standard use case. Custom converters seems appropriate for custom cases, which I would argue this is not.

I can definitely add some unit tests in encoding.js to highlight what are valid and invalid UTF-8 percent encodings.

This comment has been minimized.

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

The cookie's being created by an ASP.NET MVC web project, using a standard method (UrlEncode) https://msdn.microsoft.com/en-us/library/system.web.httputility.urlencode(v=vs.110).aspx. Why they chose to lowercase the hex values is unclear, but as you noted above it is spec compliant.

I understand. The problem is that the amount of unminifiable content that is added in the code will make it bigger than 900 bytes gzipped, so I am definitely not willing to add such amount of code if the problem can be fixed using converters. Out of the box, js-cookie supports only what it produces, maybe we can update the docs to make that clear.

I don't use ASP.NET, therefore, can you add the peculiarities of ASP.NET in the SERVER_SIDE documentation along with the proper converter to make it handle ASP.NET cookie, just like what was done with the PHP one? That would be much appreciated!

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

The cookie's being created by an ASP.NET MVC web project, using a standard method (UrlEncode) https://msdn.microsoft.com/en-us/library/system.web.httputility.urlencode(v=vs.110).aspx. Why they chose to lowercase the hex values is unclear, but as you noted above it is spec compliant.

I understand. The problem is that the amount of unminifiable content that is added in the code will make it bigger than 900 bytes gzipped, so I am definitely not willing to add such amount of code if the problem can be fixed using converters. Out of the box, js-cookie supports only what it produces, maybe we can update the docs to make that clear.

I don't use ASP.NET, therefore, can you add the peculiarities of ASP.NET in the SERVER_SIDE documentation along with the proper converter to make it handle ASP.NET cookie, just like what was done with the PHP one? That would be much appreciated!

This comment has been minimized.

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

As a sidenote, in order to provide what js-cookie is meant to provide (an API for handling cookie), I would argue that we didn't even needed a standard decoding/encoding mechanism, each use case could use converters. The problem is that many people use only js-cookie and anything else, that is why making it just work by default is a requirement.

@FagnerMartinsBrack

FagnerMartinsBrack May 14, 2016

Member

As a sidenote, in order to provide what js-cookie is meant to provide (an API for handling cookie), I would argue that we didn't even needed a standard decoding/encoding mechanism, each use case could use converters. The problem is that many people use only js-cookie and anything else, that is why making it just work by default is a requirement.

This comment has been minimized.

@FagnerMartinsBrack

FagnerMartinsBrack May 27, 2016

Member

@mmilford Can you provide an example with code that make the cookies work for ASP.NET using converters? I can add that to the SERVER_SIDE integration file.

Thanks!

@FagnerMartinsBrack

FagnerMartinsBrack May 27, 2016

Member

@mmilford Can you provide an example with code that make the cookies work for ASP.NET using converters? I can add that to the SERVER_SIDE integration file.

Thanks!

This comment has been minimized.

@FagnerMartinsBrack

FagnerMartinsBrack Jun 16, 2016

Member

If you want to help us improving the docs I recommend creating a Pull Request. I will be closing this for now because it doesn't seems to have been tracked by the OP anymore.

@FagnerMartinsBrack

FagnerMartinsBrack Jun 16, 2016

Member

If you want to help us improving the docs I recommend creating a Pull Request. I will be closing this for now because it doesn't seems to have been tracked by the OP anymore.

@FagnerMartinsBrack

This comment has been minimized.

Show comment
Hide comment
@FagnerMartinsBrack
Member

FagnerMartinsBrack commented Jun 8, 2016

Ping @mmilford.

@FagnerMartinsBrack

This comment has been minimized.

Show comment
Hide comment
@FagnerMartinsBrack

FagnerMartinsBrack Jun 16, 2016

Member

Closing due to the lack of feedback from the OP. If you want to continue the discussion, please comment here mentioning one of the members.

Member

FagnerMartinsBrack commented Jun 16, 2016

Closing due to the lack of feedback from the OP. If you want to continue the discussion, please comment here mentioning one of the members.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment