-
Notifications
You must be signed in to change notification settings - Fork 23
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
Prevent perpetual use of expired auth signature; remove superfluous API functions #567
Changes from all commits
443922f
be3af16
09437cc
35019f3
959f40e
c53ea31
61176ca
236f296
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,7 +52,13 @@ export class EIP4361AuthProvider { | |
// If we have a signature in localStorage, return it | ||
const maybeSignature = this.storage.getAuthSignature(storageKey); | ||
if (maybeSignature) { | ||
return maybeSignature; | ||
// check whether older than node freshness requirement | ||
if (this.isMessageExpired(maybeSignature.typedData)) { | ||
// clear signature so that it will be recreated and stored | ||
this.storage.clear(storageKey); | ||
} else { | ||
return maybeSignature; | ||
} | ||
} | ||
|
||
// If at this point we didn't return, we need to create a new message | ||
|
@@ -61,6 +67,19 @@ export class EIP4361AuthProvider { | |
return authMessage; | ||
} | ||
|
||
private isMessageExpired(message: string): boolean { | ||
const siweMessage = new SiweMessage(message); | ||
if (!siweMessage.issuedAt) { | ||
// don't expect to ever happen; but just in case | ||
return false; | ||
} | ||
Comment on lines
+72
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a sentinel against There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. From the EIP4361 spec, However, in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a divergence of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I can tell, yes, it is a divergence. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I take that back. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it's not the constructor but the SiweMessage struct - so still a deviation - https://github.com/spruceid/siwe/blob/main/packages/siwe/lib/client.ts#L49 Without the check, the compiler returns the following:
I'll file an issue for the (Update: filed spruceid/siwe#211) |
||
|
||
const twoHourWindow = new Date(siweMessage.issuedAt); | ||
twoHourWindow.setHours(twoHourWindow.getHours() + 2); | ||
const now = new Date(); | ||
return twoHourWindow < now; | ||
} | ||
|
||
private async createSIWEAuthMessage(): Promise<AuthSignature> { | ||
const address = await this.signer.getAddress(); | ||
const { domain, uri } = this.providerParams; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still "checking contract compatibility" even when removing this from the Testnet examples that are run every hour?
https://github.com/nucypher/taco-web/actions/workflows/lynx.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, I don't think
isAuthorized
is useful as part of the exposedtaco
API, hence the removal here. If others feel that it is useful I can put it back.It's unclear to me how comprehensive this check is as it pertains to "checking compatibility" based on one function. Do you have any additional insight there, perhaps I'm missing something? Perhaps testing the
DkgCoordinatorAgent
withlynx
Coordinator
contract via a separate workflow could be a better solution to CI checking compatibility...maybe - not sure what that entails or if it is feasible?Additionally,
Coordinator.isEncryptionAuthorized()
which is called byisAuthorized()
, is actually deprecated in the latestCoordinator
contract, and will be removed down the road. Instead, the underlyingacccessController
for the ritual should be called directly for that check.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't have a clear idea about this check beyond that we can check that this specific method on the Coordinator contract has not been modified.
So yes, I agree we can delete this.
My concern came after I read the commented lines in the code talking about "checking contract compatibility": I was wondering if I'm missing something here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally! I was hoping someone had a better picture of it than I did. Definitely a good conversation to have.
It was added as part of #543 (comment), 😅 , but I don't remember why that is - maybe a side discussion with Piotr. But the problem it is trying to solve seems like a stretch.
Perhaps others may weigh in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Including a compatibility check as part of an example is not a good idea (although I understand that it stems from the example doubling as the CI smoke test). But the original question still stands, and I'm afraid I can't help. I don't understand why it was added originally.