Max cookie size assert #159
Conversation
addon/services/cookies.js
Outdated
| @@ -76,6 +77,8 @@ export default Service.extend({ | |||
| assert('Cookies cannot be set to be HTTP-only as those cookies would not be accessible by the Ember.js application itself when running in the browser!', !options.httpOnly); | |||
| assert("Cookies cannot be set as signed as signed cookies would not be modifyable in the browser as it has no knowledge of the express server's signing key!", !options.signed); | |||
| assert('Cookies cannot be set with both maxAge and an explicit expiration time!', isEmpty(options.expires) || isEmpty(options.maxAge)); | |||
| assert(`Cookies larger than ${MAX_COOKIE_BYTE_LENGTH} bytes are not supported by most browsers!`, this._isCookieSizeAcceptable(value)); | |||
There was a problem hiding this comment.
I'd move this to after value is assigned in line 82 and run the check on the actual value we're also writing then.
addon/services/cookies.js
Outdated
|
|
||
| _getByteCount(value) { | ||
| return typeof(value) === 'string' ? encodeURI(value).split(/%(?:u[0-9A-F]{2})?[0-9A-F]{2}|./).length - 1 : 0; | ||
| } |
There was a problem hiding this comment.
I guess these 2 methods could also be combined.
addon/services/cookies.js
Outdated
| }, | ||
|
|
||
| _getByteCount(value) { | ||
| return typeof(value) === 'string' ? encodeURI(value).split(/%(?:u[0-9A-F]{2})?[0-9A-F]{2}|./).length - 1 : 0; |
There was a problem hiding this comment.
When the value assigned in line 82 is passed here, the typeof check would no longer be necessary.
Also it would be good to add a comment explaining what the split does.
|
@marcoow Thanks for the review! I've made the changes and added a unit test Now, I wasn't sure whether you prefer the MAX_COOKIE_SIZE_LENGTH to be used by the unit test, or whether to decouple the test from the unit entirely. So I've gone for the latter option, the |
addon/services/cookies.js
Outdated
| // This snippet counts the bytes in the value | ||
| // about to be stored as the cookie: | ||
| // See https://stackoverflow.com/a/25994411/6657064 | ||
| let _countUtf8Bytes = function (s){ |
There was a problem hiding this comment.
It seems the inner function isn't actually needed here?
tests/unit/services/cookies-test.js
Outdated
| it('throws when a larger than allowed cookie is set', function() { | ||
| expect(() => { | ||
| let value = ""; | ||
| for(let i = 0; i < 400; i++) { value += Math.random().toString(36).substring(2) } |
There was a problem hiding this comment.
I guess we could just use a static value here that we know has more than 4096 bytes?
addon/services/cookies.js
Outdated
|
|
||
| assert(`Cookies larger than ${MAX_COOKIE_BYTE_LENGTH} bytes are not supported by most browsers!`, this._isCookieSizeAcceptable(value)); | ||
|
|
||
|
|
There was a problem hiding this comment.
double newline - this should ideally get caught be ESLint actually…
|
I restarted the build - seems like the failures were unrelated actually. |
|
@omairvaiyani: tests run now but there are some ESLint issues… |
|
I fixed the linter errors. This should be good to go after #168 is merged and this is rebased. |
Replaced byte counter func with more accurate snippet Added comment on byte counter func Removed unnecessary type check
marcoow
force-pushed
the
cookie-size-assert
branch
from
e292381
to
4a41f69
Compare
|
Hey @marcoow thanks for sorting this! |
In relation to mainmatter/ember-simple-auth#1534 (comment)