webpush modifies claims

[Seyphaton]

Hey,

the webpush function modifies the vapid_claim dict. Similar issue has been reported here and was fixed by creating a copy first: https://github.com/web-push-libs/vapid/pull/67/commits/f32719dc58f7d2cf396c1c814eb677e55750ca99

[GladOSkar]

I just discovered the same thing, it really tripped me up when the push service was like Invalid Audience Specified because the claims dict still had another service's aud set in it, naturally only in prod and not on my dev machine -.-

[jrconlin]

I understand the frustration, but again, values are passed by reference in Python and therefore are mutable. Heck, values can be mutated by decorators, and I've gotten hit by that one a few times myself. I'm fine with adding warnings into the documents. Likewise, I'm also fine with folk not using the "helpful" .webpush() function and doing the calls themselves if they're doing anything other than a simple, disposable push call.

[GladOSkar]

Oh okay i didn't know that that's a thing to be expected. I thought given the commit @Seyphaton mentioned and since this issue was still open this was something that you wanted to get fixed?

Otherwise maybe close this as wontfix ?

[jrconlin]

Yeah, sorry. That was my fault. I've been busy with a bunch of stuff and these got buried.

Thank you for submitting the PR, but yeah, wontfix.

[zehawki]

I had a hard time figuring out why (during testing) when sending first to Chrome and then to FF one after the other, the FF call would keep failing with:

Message: 'Error in webpush'
Arguments: ("WebPushException('Push failed: 401 Unauthorized')",)

And thankfully found comment by Alexerson with the mention of copy, which solved the issue. So two things:

1. If I'd set the "aud" field explicitly in the vapid_claims, then I guess this problem wont occur. But then its a headache to pass different vapid_claims per browser.
2. Conversely, why not just perform the copy inside the .webpush() foo? (Its quite easy to miss that little line in the readme "In both cases, the passed dict will be mutated after the call.")

[jrconlin]

The bigger problem is that aud should be different for each push service endpoint. (e.g. you shouldn't be using the same VAPID key with, say, Google and Mozilla, so you shouldn't be using the same VAPID header block either.)

The next thing is that the .webpush() function is more just a handy thing to show how you should call it. It's not super efficient for a whole host of reasons, including the fact that it's constantly generating new VAPID headers. You don't need to do that. You could just generate one, with a sufficiently large TTL (say 35 days, and regenerate it every 30 days) and just use that cached header when you make the .send() call.

Remember, the VAPID header is there to identify yourself and to provide the key to validate the endpoint you signed when you created the subscription. You're expected to reuse it (within reason).

[zehawki]

@jrconlin Ah OK, thank you so much for the detailed reply.