[commerce-sdk-react] Bug Fix Cookie expires time (@W-16626227@) #1994
Conversation
| beforeEach(() => { | ||
| storage = new CookieStorage() | ||
| jest.useFakeTimers() | ||
| Date.now = jest.fn(() => new Date('2024-09-03T21:00:00Z').getTime()) |
There was a problem hiding this comment.
prototype pollution! Date.now is changed for the entire node process for the jest tests. I think ya may need to revert the implementation of Date.now after these tests are done
Co-authored-by: Kevin He <[email protected]> Signed-off-by: Adam Raya <[email protected]>
| const cookieOptions = { | ||
| ...defaultAttributes, | ||
| ...options, | ||
| expires: this.convertSecondsToDate(options?.expires) | ||
| } |
There was a problem hiding this comment.
I don't think that this is the right place to making tis fix. Expires is a date and we are using seconds (ttl). We should be converting the ttl in seconds to a date before calling set.
// auth/indx.ts line 381
this.set(refreshTokenKey, res.refresh_token, {
expires: res.refresh_token_expires_in // This needs to be updated here.
})
| if (typeof seconds === 'number') { | ||
| return new Date(Date.now() + seconds * 1000) | ||
| } | ||
| return undefined |
There was a problem hiding this comment.
What are your thoughts on throwing if the parameter isn't a number instead of returning a different type object?
There was a problem hiding this comment.
Theundefined value is valid for expires cookie attribute in js-cookie. Creates a session cookie that expires when the browser is closed if the expires is undefined.
https://github.com/js-cookie/js-cookie/tree/latest?tab=readme-ov-file#expires
There was a problem hiding this comment.
I see.. I figured that was the case. Maybe I'm just being a little nit picky but the name of your function implies that you are going to return a date hence the comment. I suppose you can return undefined, just make sure you describe this functions behaviour in a js doc comment so others will understand how it works.
adamraya
changed the base branch from
release-commerce-sdk-react-3.0.1
to
release-3.7.x
| const expiresDate = res.refresh_token_expires_in | ||
| ? this.convertSecondsToDate(res.refresh_token_expires_in) | ||
| : undefined |
There was a problem hiding this comment.
As you mentioned because our tests aren't all properly passing tokens it might be too much work at the moment to fix them, so we are just falling back to allowing the passing of undefined to the set function. We should look at fixing this in the future and we need to create a ticket for this to place in the backlog.
… (#1996) * Fix Cookie expires converting seconds to Date * Update CHANGELOG.md * Add tests * PR Feedback * Update packages/commerce-sdk-react/CHANGELOG.md * PR Feedback convert seconds to date in auth * Bump version * Bump retail-react-app * Update convertSecondsToDate logic * Support expires not defined --------- Signed-off-by: Adam Raya <[email protected]> Co-authored-by: Kevin He <[email protected]>
Description
The PR fixes the
commerce-sdk-reactbug #1991.Types of Changes
Changes
How to Test-Drive This PR
fix-cookie-expires-timeand runnpm cinpm startcc-nx-g_RefArchGlobalcookie is 30 days.Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization