Skip to content

Conversation

@p1gp1g
Copy link

@p1gp1g p1gp1g commented Dec 30, 2024

This is a first step for #32

@CLAassistant
Copy link

CLAassistant commented Dec 30, 2024

CLA assistant check
All committers have signed the CLA.

@rfc2822
Copy link
Member

rfc2822 commented Dec 30, 2024

For curiosity: Is it for real use? With which server/service?

Faking security doesn't sound like a good idea to me. Would it be an option to change the server so that it doesn't always enforce encrypted payload?

@p1gp1g
Copy link
Author

p1gp1g commented Dec 30, 2024

Sunup uses mozilla's webpush server which enforce encryption. It will be advertised as the goto distributor, since it doesn't need any user login, nor to change any setting, the default server doesn't have rate limits and it follows the last specifications.

So, this is a workaround for a real use, until notifications are properly encrypted (following webpush specs)

@rfc2822
Copy link
Member

rfc2822 commented Dec 30, 2024

I still think it's a security issue to mark non-encrypted data as encrypted. Let's assume the service is hacked and data leaks – then they can and will say "we made sure that all data is encrypted". If however DAVx5 or Nextcloud had marked unencrypted data as encrypted, DAVx5 or this extension will then rightly be blamed for the data leak.

If it's that important, we should implement encryption instead.

Or is there anything I have missed?

@p1gp1g
Copy link
Author

p1gp1g commented Dec 30, 2024

I agree, and this is definitely a workaround until encryption is properly setup.

But there 2 things to keep in mind:

  • Push notifications don't contain any sensitive information (It sends only the collection id)
  • This extension is properly marked as unstable and not ready for production (in the README) :

This extension is in an early stage of development. It is for demonstration and testing purposes only. Don't use it on production systems!

So, I think this is OK to do the hack as a first step for a transparent transition to the encrypted notifications

@rfc2822
Copy link
Member

rfc2822 commented Dec 31, 2024

Push notifications don't contain any sensitive information (It sends only the collection id)

Plus the sync token. Hopefully servers won't use sensitive information for the topic or sync-token.

This extension is properly marked as unstable and not ready for production (in the README) :

Good point. I suggest that we also add the hint that it marks unencrypted data as encrypted into the README, directly after the sentence above.

@p1gp1g
Copy link
Author

p1gp1g commented Jan 2, 2025

It makes sense, I've pushed a change for the README

@p1gp1g
Copy link
Author

p1gp1g commented Feb 1, 2025

I've seen in the 0.0.2 release note "Improved compatibility with WebPush/UnifiedPush servers", so I thought this PR was merged. Is it an oversight ?

@JonathanTreffler
Copy link
Collaborator

I've seen in the 0.0.2 release note "Improved compatibility with WebPush/UnifiedPush servers", so I thought this PR was merged. Is it an oversight ?

The PR was not merged, this is about other changes that improve compatibility.
We currently plan to implement proper encryption in the next few weeks. So thank you for your contribution, but I don't think we need to merge and release this workaround just to replace it with the full implementation in a few weeks :)

@p1gp1g
Copy link
Author

p1gp1g commented Feb 22, 2025

That's great news ! We can close that PR then :)

@JonathanTreffler
Copy link
Collaborator

VAPID and message encryption was just implemented in 642b9c5 , closing :)

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.

4 participants