Skip to content
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

Use dapp identity key with aud, per spec #40

Merged

Conversation

devceline
Copy link
Collaborator

@devceline devceline commented Aug 22, 2023

Per spec: https://specs.walletconnect.com/2.0/specs/clients/notify/notify-authentication#notify-subscription

We were using dapp url as aud in all JWTs, when it should be dapp identity key.

The reason this worked so far is because the aud field was not validated yet

@devceline devceline force-pushed the refactor/notify-client-symkey-delete branch from 74e5dfd to 1addad2 Compare August 22, 2023 08:29
@devceline devceline changed the title Remove symkey deletion Use dapp identity key with aud, per spec Aug 22, 2023
@devceline devceline requested a review from bkrem August 22, 2023 15:12
Copy link
Member

@bkrem bkrem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch thank you 🙏 not quite ready to merge yet

@@ -237,7 +237,7 @@ export class NotifyClient extends INotifyClient {
if (existingSubExists) return;

this.messages.set(subTopic, { topic: subTopic, messages: [] });
this.core.crypto.setSymKey(subscription.symKey).then(() => {
this.core.crypto.setSymKey(subscription.symKey, subTopic).then(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related/still wanted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - doesn't hurt to have it

ksu: this.client.keyserverUrl,
scp,
act: "notify_subscription",
app: metadata.url,
};

console.log(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug log

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping logs for now as they're needed for testing

@@ -429,8 +437,19 @@ export class NotifyEngine extends INotifyEngine {
async (event: RelayerTypes.MessageEvent) => {
const { topic, message, publishedAt } = event;

console.log(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug log

const payload = await this.client.core.crypto.decode(topic, message);

console.log("Decoded: ", payload);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug log

packages/notify-client/src/controllers/engine.ts Outdated Show resolved Hide resolved
Co-authored-by: Ben Kremer <ben@walletconnect.com>
Copy link
Member

@bkrem bkrem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build is failing bc the method callsites need to be updated still to the proper naming

@devceline devceline merged commit d947212 into refactor/notify-client Aug 23, 2023
1 check passed
@devceline devceline deleted the refactor/notify-client-symkey-delete branch August 23, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants