Conversation
🦋 Changeset detectedLatest commit: 15775a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
SHession
left a comment
SHession
left a comment
There was a problem hiding this comment.
Some small comments to consider, hopefully not too pedantic
.changeset/angry-camels-tie.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@guardian/pan-domain-node": patch | |||
There was a problem hiding this comment.
For Semver this should technically be a minor change:
MINOR version when you add functionality in a backward compatible manner
test/panda.test.ts
Outdated
| jest.setSystemTime(100); | ||
|
|
||
| const panda = new PanDomainAuthentication('rightcookiename', 'region', 'bucket', 'keyfile', guardianValidation); | ||
| // There is a valid Panda cookie in here, but it's under the wrong name |
There was a problem hiding this comment.
I'm not quite sure what this comment means, I think it can just be removed?
There was a problem hiding this comment.
Sorry for this irrelevant comment - I've removed it.
src/panda.ts
Outdated
| if (!requestCookies) { | ||
| return { | ||
| success: false, | ||
| reason: 'no-cookie' | ||
| }; | ||
| } |
There was a problem hiding this comment.
We could avoid this early return by passing an empty string to cookie.parse i.e. cookie.parse(requestCookies || ""). This way we could avoid any duplicate logic from the verify function.
There was a problem hiding this comment.
Good idea! I've made the change and I think using nullish coalescing operator (??) here might make the code clearer.
2 participants
What does this change?
We noticed that the quiz builder crashed when there were no cookie value. The quiz builder passed in an undefined value as the cookie and the
pan-domain-nodelibrary threw aTypeErrorwhen it attempted to parse it withcookie.parse(requestCookies).The library expects a string as the cookie value in its
verifymethod, but the quiz builder was built in Javascript and it bypassed the typechecking.The pull request changes the function signature of
PanDomainAuthentication.verifyto accommodate an undefined value in the cookie value and return failed status with no-cookie as the reason. It is the same response it returns when the specific cookiegutoolsAuth-assymis missing.How to test
Added an unit test on this case.
I've also tested the quiz builder locally which pulls the local pan-domain-node. I could open the quiz builder successfully after I manually deleted all its cookies through the browser's developer tool.
How can we measure success?
Avoid app crash due to missing cookie values.
Have we considered potential risks?
Should be minimal.