catch JSON.parse and return 403 properly (#6614)

Co-authored-by: Gordon Sun <gordon.sun@pipe17.com>

Author: sunshineo

spec/Middlewares.spec.js

@@ -12,7 +12,7 @@ describe('middlewares', () => {
         _ApplicationId: 'FakeAppId',
       },
       headers: {},
-      get: key => {
+      get: (key) => {
         return fakeReq.headers[key.toLowerCase()];
       },
     };
@@ -24,7 +24,7 @@ describe('middlewares', () => {
     AppCache.del(fakeReq.body._ApplicationId);
   });
 
-  it('should use _ContentType if provided', done => {
+  it('should use _ContentType if provided', (done) => {
     expect(fakeReq.headers['content-type']).toEqual(undefined);
     const contentType = 'image/jpeg';
     fakeReq.body._ContentType = contentType;
@@ -64,7 +64,7 @@ describe('middlewares', () => {
     expect(fakeRes.status).toHaveBeenCalledWith(403);
   });
 
-  it('should succeed when any one of the configured keys supplied', done => {
+  it('should succeed when any one of the configured keys supplied', (done) => {
     AppCache.put(fakeReq.body._ApplicationId, {
       clientKey: 'clientKey',
       masterKey: 'masterKey',
@@ -77,7 +77,7 @@ describe('middlewares', () => {
     });
   });
 
-  it('should succeed when client key supplied but empty', done => {
+  it('should succeed when client key supplied but empty', (done) => {
     AppCache.put(fakeReq.body._ApplicationId, {
       clientKey: '',
       masterKey: 'masterKey',
@@ -90,7 +90,7 @@ describe('middlewares', () => {
     });
   });
 
-  it('should succeed when no keys are configured and none supplied', done => {
+  it('should succeed when no keys are configured and none supplied', (done) => {
     AppCache.put(fakeReq.body._ApplicationId, {
       masterKey: 'masterKey',
     });
@@ -110,22 +110,22 @@ describe('middlewares', () => {
 
   const BodyKeys = Object.keys(BodyParams);
 
-  BodyKeys.forEach(infoKey => {
+  BodyKeys.forEach((infoKey) => {
     const bodyKey = BodyParams[infoKey];
     const keyValue = 'Fake' + bodyKey;
     // javascriptKey is the only one that gets defaulted,
     const otherKeys = BodyKeys.filter(
-      otherKey => otherKey !== infoKey && otherKey !== 'javascriptKey'
+      (otherKey) => otherKey !== infoKey && otherKey !== 'javascriptKey'
     );
 
-    it(`it should pull ${bodyKey} into req.info`, done => {
+    it(`it should pull ${bodyKey} into req.info`, (done) => {
       fakeReq.body[bodyKey] = keyValue;
 
       middlewares.handleParseHeaders(fakeReq, fakeRes, () => {
         expect(fakeReq.body[bodyKey]).toEqual(undefined);
         expect(fakeReq.info[infoKey]).toEqual(keyValue);
 
-        otherKeys.forEach(otherKey => {
+        otherKeys.forEach((otherKey) => {
           expect(fakeReq.info[otherKey]).toEqual(undefined);
         });
 
@@ -145,7 +145,7 @@ describe('middlewares', () => {
     expect(fakeRes.status).toHaveBeenCalledWith(403);
   });
 
-  it('should succeed if the ip does belong to masterKeyIps list', done => {
+  it('should succeed if the ip does belong to masterKeyIps list', (done) => {
     AppCache.put(fakeReq.body._ApplicationId, {
       masterKey: 'masterKey',
       masterKeyIps: ['ip1', 'ip2'],
@@ -169,7 +169,7 @@ describe('middlewares', () => {
     expect(fakeRes.status).toHaveBeenCalledWith(403);
   });
 
-  it('should succeed if the connection.remoteAddress does belong to masterKeyIps list', done => {
+  it('should succeed if the connection.remoteAddress does belong to masterKeyIps list', (done) => {
     AppCache.put(fakeReq.body._ApplicationId, {
       masterKey: 'masterKey',
       masterKeyIps: ['ip1', 'ip2'],
@@ -193,7 +193,7 @@ describe('middlewares', () => {
     expect(fakeRes.status).toHaveBeenCalledWith(403);
   });
 
-  it('should succeed if the socket.remoteAddress does belong to masterKeyIps list', done => {
+  it('should succeed if the socket.remoteAddress does belong to masterKeyIps list', (done) => {
     AppCache.put(fakeReq.body._ApplicationId, {
       masterKey: 'masterKey',
       masterKeyIps: ['ip1', 'ip2'],
@@ -217,7 +217,7 @@ describe('middlewares', () => {
     expect(fakeRes.status).toHaveBeenCalledWith(403);
   });
 
-  it('should succeed if the connection.socket.remoteAddress does belong to masterKeyIps list', done => {
+  it('should succeed if the connection.socket.remoteAddress does belong to masterKeyIps list', (done) => {
     AppCache.put(fakeReq.body._ApplicationId, {
       masterKey: 'masterKey',
       masterKeyIps: ['ip1', 'ip2'],
@@ -230,7 +230,7 @@ describe('middlewares', () => {
     });
   });
 
-  it('should allow any ip to use masterKey if masterKeyIps is empty', done => {
+  it('should allow any ip to use masterKey if masterKeyIps is empty', (done) => {
     AppCache.put(fakeReq.body._ApplicationId, {
       masterKey: 'masterKey',
       masterKeyIps: [],
@@ -243,7 +243,7 @@ describe('middlewares', () => {
     });
   });
 
-  it('should succeed if xff header does belong to masterKeyIps', done => {
+  it('should succeed if xff header does belong to masterKeyIps', (done) => {
     AppCache.put(fakeReq.body._ApplicationId, {
       masterKey: 'masterKey',
       masterKeyIps: ['ip1'],
@@ -256,7 +256,7 @@ describe('middlewares', () => {
     });
   });
 
-  it('should succeed if xff header with one ip does belong to masterKeyIps', done => {
+  it('should succeed if xff header with one ip does belong to masterKeyIps', (done) => {
     AppCache.put(fakeReq.body._ApplicationId, {
       masterKey: 'masterKey',
       masterKeyIps: ['ip1'],
@@ -357,7 +357,7 @@ describe('middlewares', () => {
     );
   });
 
-  it('should use user provided on field userFromJWT', done => {
+  it('should use user provided on field userFromJWT', (done) => {
     AppCache.put(fakeReq.body._ApplicationId, {
       masterKey: 'masterKey',
     });
@@ -367,4 +367,17 @@ describe('middlewares', () => {
       done();
     });
   });
+
+  it('should give invalid response when upload file without x-parse-application-id in header', () => {
+    AppCache.put(fakeReq.body._ApplicationId, {
+      masterKey: 'masterKey',
+    });
+    fakeReq.body = Buffer.from('fake-file');
+    console.log('fakeReq.body.constructor');
+    console.log(fakeReq.body.constructor);
+    console.log('fakeReq.body instanceof Buffer');
+    console.log(fakeReq.body instanceof Buffer);
+    middlewares.handleParseHeaders(fakeReq, fakeRes);
+    expect(fakeRes.status).toHaveBeenCalledWith(403);
+  });
 });

src/middlewares.js

@@ -8,7 +8,7 @@ import defaultLogger from './logger';
 export const DEFAULT_ALLOWED_HEADERS =
   'X-Parse-Master-Key, X-Parse-REST-API-Key, X-Parse-Javascript-Key, X-Parse-Application-Id, X-Parse-Client-Version, X-Parse-Session-Token, X-Requested-With, X-Parse-Revocable-Session, Content-Type, Pragma, Cache-Control';
 
-const getMountForRequest = function(req) {
+const getMountForRequest = function (req) {
   const mountPathLength = req.originalUrl.length - req.url.length;
   const mountPath = req.originalUrl.slice(0, mountPathLength);
   return req.protocol + '://' + req.get('host') + mountPath;
@@ -59,7 +59,14 @@ export function handleParseHeaders(req, res, next) {
     if (req.body instanceof Buffer) {
       // The only chance to find the app id is if this is a file
       // upload that actually is a JSON body. So try to parse it.
-      req.body = JSON.parse(req.body);
+      // https://github.com/parse-community/parse-server/issues/6589
+      // It is also possible that the client is trying to upload a file but forgot
+      // to provide x-parse-app-id in header and parse a binary file will fail
+      try {
+        req.body = JSON.parse(req.body);
+      } catch (e) {
+        return invalidRequest(req, res);
+      }
       fileViaJSON = true;
     }
 
@@ -168,10 +175,10 @@ export function handleParseHeaders(req, res, next) {
   // Client keys are not required in parse-server, but if any have been configured in the server, validate them
   //  to preserve original behavior.
   const keys = ['clientKey', 'javascriptKey', 'dotNetKey', 'restAPIKey'];
-  const oneKeyConfigured = keys.some(function(key) {
+  const oneKeyConfigured = keys.some(function (key) {
     return req.config[key] !== undefined;
   });
-  const oneKeyMatches = keys.some(function(key) {
+  const oneKeyMatches = keys.some(function (key) {
     return req.config[key] !== undefined && info[key] === req.config[key];
   });
 
@@ -225,13 +232,13 @@ export function handleParseHeaders(req, res, next) {
         });
       }
     })
-    .then(auth => {
+    .then((auth) => {
       if (auth) {
         req.auth = auth;
         next();
       }
     })
-    .catch(error => {
+    .catch((error) => {
       if (error instanceof Parse.Error) {
         next(error);
         return;