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

400 Bad Request when using Windows Push Notification Services (WNS) endpoint #162

Open
meewash-p opened this issue Mar 4, 2024 · 8 comments · May be fixed by #163
Open

400 Bad Request when using Windows Push Notification Services (WNS) endpoint #162

meewash-p opened this issue Mar 4, 2024 · 8 comments · May be fixed by #163

Comments

@meewash-p
Copy link

meewash-p commented Mar 4, 2024

Hi,

I've encountered an issue when sending a push to the endpoint from WNS (MS Edge browser). Their service is responding with 400 Bad Request with no body.

After quick debugging, there was an error reason in response headers:

{
"Content-Length": "0",
"mise-correlation-id": "927f3b90-c2cf-4649-97a9-94330058c16d",
"X-WNS-ERROR-DESCRIPTION": "Ttl value conflicts with X-WNS-Cache-Policy.",
"X-WNS-STATUS": "dropped",
"X-WNS-NOTIFICATIONSTATUS": "dropped",
"Date": "Mon, 04 Mar 2024 12:24:23 GMT"
}

Turns out, WNS requires X-WNS-Cache-Policy request header set to either cache or no-cache depending on a ttl value.

Adding the header helps. Also, I'd suggest including headers in the exception message. :)

If you have that issue with WNS add x-wns-cache-policy header (if using ttl than x-wns-cache-policy: cache) in your code, e.g.:

webpush(
    subscription_info=push_subscription,
    data=payload_data,
    vapid_private_key=vapid_private_key,
    vapid_claims=vapid_claims,
    headers={'x-wns-cache-policy': 'no-cache'}
)

I'd do sth like this:

From a5442bd95eeb82835d3e7ecf62f11616d1cca8bd Mon Sep 17 00:00:00 2001
From: mp <[email protected]>
Date: Mon, 4 Mar 2024 13:40:55 +0100
Subject: [PATCH] Add headers.update with X-WNS-Cache-Policy and tests

---diff
 pywebpush/__init__.py           | 12 ++++++++++--
 pywebpush/tests/test_webpush.py | 21 +++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

--- a/pywebpush/__init__.py
+++ b/pywebpush/__init__.py
@@ -326,6 +326,14 @@ class WebPusher:
             headers.update({
                 'content-encoding': content_encoding,
             })
+            if ttl == 0:
+                headers.update({
+                    'x-wns-cache-policy': 'no-cache',
+                })
+            else:
+                headers.update({
+                    'x-wns-cache-policy': 'cache',
+                })
         if gcm_key:
             # guess if it is a legacy GCM project key or actual FCM key
             # gcm keys are all about 40 chars (use 100 for confidence),
@@ -494,7 +502,7 @@ def webpush(subscription_info,
         timeout=timeout,
     )
     if not curl and response.status_code > 202:
-        raise WebPushException("Push failed: {} {}\nResponse body:{}".format(
-            response.status_code, response.reason, response.text),
+        raise WebPushException("Push failed: {} {}\nResponse body:{}\nResponse headers: {}".format(
+            response.status_code, response.reason, response.text, str(response.headers)),
             response=response)
     return response
diff --git a/pywebpush/tests/test_webpush.py b/pywebpush/tests/test_webpush.py
index 93dc51a..938d9d1 100644
--- a/pywebpush/tests/test_webpush.py
+++ b/pywebpush/tests/test_webpush.py
@@ -142,6 +142,23 @@ class WebpushTestCase(unittest.TestCase):
         ckey = pheaders.get('crypto-key')
         assert 'pre-existing' in ckey
         assert pheaders.get('content-encoding') == 'aes128gcm'
+        assert pheaders.get('x-wns-cache-policy') == 'no-cache'
+
+    @patch("requests.post")
+    def test_send_ttl(self, mock_post):
+        subscription_info = self._gen_subscription_info()
+        headers = {"Crypto-Key": "pre-existing",
+                   "Authentication": "bearer vapid"}
+        data = "Mary had a little lamb"
+        WebPusher(subscription_info).send(data, headers, ttl=10)
+        assert subscription_info.get('endpoint') == mock_post.call_args[0][0]
+        pheaders = mock_post.call_args[1].get('headers')
+        assert pheaders.get('ttl') == '10'
+        assert pheaders.get('AUTHENTICATION') == headers.get('Authentication')
+        ckey = pheaders.get('crypto-key')
+        assert 'pre-existing' in ckey
+        assert pheaders.get('content-encoding') == 'aes128gcm'
+        assert pheaders.get('x-wns-cache-policy') == 'cache'
 
     @patch("requests.post")
     def test_send_vapid(self, mock_post):
@@ -173,6 +190,7 @@ class WebpushTestCase(unittest.TestCase):
         ckey = pheaders.get('crypto-key')
         assert 'dh=' in ckey
         assert pheaders.get('content-encoding') == 'aesgcm'
+        assert pheaders.get('x-wns-cache-policy') == 'no-cache'
         assert pheaders.get('test-header') == 'test-value'
 
     @patch.object(WebPusher, "send")
@@ -288,6 +306,7 @@ class WebpushTestCase(unittest.TestCase):
         pheaders = mock_post.call_args[1].get('headers')
         assert pheaders.get('ttl') == '0'
         assert pheaders.get('content-encoding') == 'aes128gcm'
+        assert pheaders.get('x-wns-cache-policy') == 'no-cache'
 
     @patch("pywebpush.open")
     def test_as_curl(self, opener):
@@ -334,6 +353,7 @@ class WebpushTestCase(unittest.TestCase):
         assert pdata["registration_ids"][0] == "regid123"
         assert pheaders.get("authorization") == "key=gcm_key_value"
         assert pheaders.get("content-type") == "application/json"
+        assert pheaders.get('x-wns-cache-policy') == 'no-cache'
 
     @patch("requests.post")
     def test_timeout(self, mock_post):
@@ -360,6 +380,7 @@ class WebpushTestCase(unittest.TestCase):
         ckey = pheaders.get('crypto-key')
         assert 'pre-existing' in ckey
         assert pheaders.get('content-encoding') == 'aes128gcm'
+        assert pheaders.get('x-wns-cache-policy') == 'no-cache'
 
 
 class WebpushExceptionTestCase(unittest.TestCase):
-- 
2.34.1
@jrconlin
Copy link
Member

jrconlin commented Mar 4, 2024

Ah, looks like there are several headers that they want.
I'm half tempted to put these under an option flag just in case they might mess up Apple or Google for some stupid reason.

@jrconlin
Copy link
Member

jrconlin commented Mar 4, 2024

(I'll also note that pywebpush exposes a parameter for headers for things like this. It does mean that the calling code needs to know the TTL is 0, so it applies the correct caching header, but it's a reasonable work-around)

jrconlin added a commit that referenced this issue Mar 4, 2024
Microsoft has introduced [extra
header](https://learn.microsoft.com/en-us/windows/apps/design/shell/tiles-and-notifications/push-request-response-headers#request-parameters)
requirements for incoming push messages. I kind of want to avoid adding
a lot of system specific smarts to pywebpush, mostly because that's an
endless road of feature creep. The preferred way to handle this would be
to include the extra, call specific headers in the `webpush(...,
headers=dict(...))` argument.

Closes #162
@jrconlin jrconlin linked a pull request Mar 4, 2024 that will close this issue
@martinthomas
Copy link

I have been trying to make this work (web push to Edge from a Flask app using this library) and was about to give up on Edge until I successfully sent a web push to Edge using the Node web-push library from the command line. After seeing the notes in the issue, I took a look in the Node library code for wns headers and found none. Just FYI

@jrconlin
Copy link
Member

Ah, thanks for the bump on this.

I was waiting for a review. Apparently from Godot.
I'm going to update it to include an error to check for the X-WNS-Type header if --wns is specified as a flag.

@martinthomas
Copy link

My code now works after adding only a ttl value after seeing the npm module has a large default value.
Looking at the MS doc mentioned earlier many of the headers are optional.

@jrconlin
Copy link
Member

Right, X-WNS-Type is marked as a required field (as either tile, toast, badge or raw, with the Content-Type set to text/xml except if raw where it's set to application/octet-stream.)

I can do some simple checks to make sure folk are sending the right format, but this is all fairly non-standard and a lot of extra work. I'm also just tempted to add some content to the README talking about how non-standard this is and what extra stuff you need to do to send notification messages to Windows computers.

@jrconlin
Copy link
Member

jrconlin commented Apr 19, 2024

Ok, so here's the deal.

I'm not going to add any special handling for WNS. In fact, I'm going to take this opportunity to drop the special handling we were doing for GCM/FCM, since that's going away in less than 2 months and has been obsolete for nearly a year now.

If you want to use this library with WNS, you have to add the headers yourself, since they're subject to change anyway. I've added a note in the README for what you'll need to do.
If you were using this to talk to GCM/FCM directly and not using Web Push, this is your wake-up call that you really, really need to stop doing that.

@martinthomas
Copy link

martinthomas commented Apr 20, 2024 via email

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 a pull request may close this issue.

3 participants