fix: bound push responses and reject interrupted streams

README.md

@@ -48,6 +48,39 @@ const pushSubscription = {
 webpush.sendNotification(pushSubscription, 'Your Push Payload Text');
 ```
 
+## Security considerations
+
+A `PushSubscription` usually comes from the browser, so treat its `endpoint` as
+untrusted input. `sendNotification()` makes an HTTPS POST to whatever URL it is
+given, which means an attacker who can register a subscription can point your
+server at internal hosts (SSRF) or at a server that never finishes responding.
+
+- Validate the endpoint before storing it, e.g. only accept `https:` URLs on the
+default port whose hostname belongs to a known push service:
+
+```javascript
+const PUSH_HOSTS = [
+  /^fcm\.googleapis\.com$/,
+  /^updates\.push\.services\.mozilla\.com$/,
+  /\.push\.apple\.com$/,
+  /\.notify\.windows\.com$/
+];
+
+function isPushEndpoint(endpoint) {
+  try {
+    const url = new URL(endpoint);
+    return url.protocol === 'https:' && url.port === ''
+      && PUSH_HOSTS.some((re) => re.test(url.hostname));
+  } catch {
+    return false;
+  }
+}
+```
+
+- Set the `timeout` option. There is no timeout by default, and a socket timeout
+does not bound the total time of a slow response, so consider an overall
+deadline as well.
+
 ## Using VAPID Key for applicationServerKey
 
 When subscribing to push messages, you'll need to pass your VAPID key,
@@ -200,13 +233,17 @@ or an "options" object with more specific properties.
 A promise that resolves if the notification was sent successfully
 with details of the request, otherwise it rejects.
 
-In both cases, resolving or rejecting, you'll be able to access the following
-values on the returned object or error.
+For a complete HTTP response, the returned object or HTTP error contains the
+following values:
 
 - *statusCode*, the status code of the response from the push service;
 - *headers*, the headers of the response from the push service;
 - *body*, the body of the response from the push service.
 
+Responses larger than 64 KiB are rejected and the request is destroyed.
+Interrupted responses and socket timeouts also reject the promise. These
+transport errors do not include the complete HTTP response fields above.
+
 <hr />
 
 ## generateVAPIDKeys()

src/web-push-lib.js

@@ -10,6 +10,10 @@ import * as urlBase64Helper from './urlsafe-base64-helper.js';
 // Default TTL is four weeks.
 const DEFAULT_TTL = 2419200;
 
+// Push services reply with small bodies. Anything larger is not buffered, so an
+// untrusted subscription endpoint can't make the server hold an unbounded response.
+const MAX_RESPONSE_BYTES = 64 * 1024;
+
 let gcmAPIKey = '';
 let vapidDetails;
 
@@ -362,8 +366,19 @@ export class WebPushLib {
 
       const pushRequest = https.request(httpsOptions, function(pushResponse) {
         let responseText = '';
+        let responseBytes = 0;
+
+        // A truncated response errors on the response stream, not necessarily
+        // on the request. Reject instead of leaving the notification pending.
+        pushResponse.on('error', reject);
 
         pushResponse.on('data', function(chunk) {
+          responseBytes += chunk.length;
+          if (responseBytes > MAX_RESPONSE_BYTES) {
+            pushRequest.destroy(new Error('Push service response exceeded '
+            + MAX_RESPONSE_BYTES + ' bytes'));
+            return;
+          }
           responseText += chunk;
         });
 

test/testSendNotification.js

@@ -81,7 +81,23 @@ suite('sendNotification', function() {
       req.on('end', function() {
         requestDetails = req;
 
-        if (req.url.indexOf('statusCode=404') !== -1) {
+        if (req.url.indexOf('abortedResponse') !== -1) {
+          res.writeHead(201, { 'Content-Length': 100 });
+          res.write('partial');
+          setTimeout(function() {
+            res.destroy();
+          }, 20);
+        } else if (req.url.indexOf('stalledResponse') !== -1) {
+          res.writeHead(201, { 'Content-Length': 100 });
+          res.write('partial');
+        } else if (req.url.indexOf('responseSize=') !== -1) {
+          const size = Number(new URL(req.url, 'https://localhost').searchParams.get('responseSize'));
+          res.writeHead(201);
+          res.end(Buffer.alloc(size, 'a'));
+        } else if (req.url.indexOf('largeResponse') !== -1) {
+          res.writeHead(201);
+          res.end(Buffer.alloc(1024 * 1024, 'a'));
+        } else if (req.url.indexOf('statusCode=404') !== -1) {
           res.writeHead(404);
           res.end();
         } else {
@@ -832,6 +848,44 @@ suite('sendNotification', function() {
     });
   });
 
+  [undefined, 100].forEach(function(timeout) {
+    test('rejects an interrupted response with timeout ' + timeout, function() {
+      return assert.rejects(sendNotification({
+        endpoint: 'https://127.0.0.1:' + serverPort + '?abortedResponse'
+      }, null, { timeout }), { code: 'ECONNRESET' });
+    });
+  });
+
+  test('rejects a stalled response on socket timeout', function() {
+    return assert.rejects(sendNotification({
+      endpoint: 'https://127.0.0.1:' + serverPort + '?stalledResponse'
+    }, null, { timeout: 50 }), /Socket timeout/);
+  });
+
+  test('accepts a response at the size limit', async function() {
+    const response = await sendNotification({
+      endpoint: 'https://127.0.0.1:' + serverPort + '?responseSize=65536'
+    });
+    assert.equal(response.body.length, 65536);
+  });
+
+  test('rejects a response one byte over the size limit', function() {
+    return assert.rejects(sendNotification({
+      endpoint: 'https://127.0.0.1:' + serverPort + '?responseSize=65537'
+    }), /Push service response exceeded 65536 bytes/);
+  });
+
+  test('rejects when the push service response is too large', function() {
+    return sendNotification({
+      endpoint: 'https://127.0.0.1:' + serverPort + '?largeResponse'
+    })
+    .then(function() {
+      throw new Error('sendNotification should have rejected due to a large response');
+    }, function(err) {
+      assert(/exceeded/.test(err.message), err.message);
+    });
+  });
+
   test('rejects when it can\'t connect to the server', function() {
     const currentServerPort = serverPort;
     return closeServer()