From 8cfc2e2ec0cc95d822da936e193baf5e22880a0a Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Tue, 27 Jun 2017 20:14:41 +0300 Subject: [PATCH] refactor(aio): unify error messages for invalid requests to upload-server Previously, there was a distinction between GET requests to invalid URLs and all other requests. This was mainly because the upload-server only accepts GET requests, but that is not a hard limitation and may change in the future. Thus, it makes sense to return a 404 response for requests to invalid URLs regardless of the method used. --- .../upload-server/upload-server-factory.ts | 3 +- .../lib/verify-setup/upload-server.e2e.ts | 28 +++++------- .../upload-server-factory.spec.ts | 43 ++++++++----------- 3 files changed, 28 insertions(+), 46 deletions(-) diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/upload-server-factory.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/upload-server-factory.ts index 3fd771f295..c13a177330 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/upload-server-factory.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/upload-server-factory.ts @@ -105,8 +105,7 @@ class UploadServerFactory { } }); middleware.get(/^\/health-check\/?$/, (_req, res) => res.sendStatus(200)); - middleware.get('*', req => this.throwRequestError(404, 'Unknown resource', req)); - middleware.all('*', req => this.throwRequestError(405, 'Unsupported method', req)); + middleware.all('*', req => this.throwRequestError(404, 'Unknown resource', req)); middleware.use((err: any, _req: any, res: express.Response, _next: any) => this.respondWithError(res, err)); return middleware; diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/upload-server.e2e.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/upload-server.e2e.ts index 2b59df69c5..ab058a3303 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/upload-server.e2e.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/verify-setup/upload-server.e2e.ts @@ -26,13 +26,13 @@ describe('upload-server (on HTTP)', () => { it('should disallow non-GET requests', done => { const url = `http://${host}/create-build/${pr}/${sha9}`; - const bodyRegex = /^Unsupported method/; + const bodyRegex = /^Unknown resource/; Promise.all([ - h.runCmd(`curl -iLX PUT ${url}`).then(h.verifyResponse(405, bodyRegex)), - h.runCmd(`curl -iLX POST ${url}`).then(h.verifyResponse(405, bodyRegex)), - h.runCmd(`curl -iLX PATCH ${url}`).then(h.verifyResponse(405, bodyRegex)), - h.runCmd(`curl -iLX DELETE ${url}`).then(h.verifyResponse(405, bodyRegex)), + h.runCmd(`curl -iLX PUT ${url}`).then(h.verifyResponse(404, bodyRegex)), + h.runCmd(`curl -iLX POST ${url}`).then(h.verifyResponse(404, bodyRegex)), + h.runCmd(`curl -iLX PATCH ${url}`).then(h.verifyResponse(404, bodyRegex)), + h.runCmd(`curl -iLX DELETE ${url}`).then(h.verifyResponse(404, bodyRegex)), ]).then(done); }); @@ -376,25 +376,17 @@ describe('upload-server (on HTTP)', () => { describe(`${host}/*`, () => { - it('should respond with 404 for GET requests to unknown URLs', done => { + it('should respond with 404 for requests to unknown URLs', done => { const bodyRegex = /^Unknown resource/; Promise.all([ h.runCmd(`curl -iL http://${host}/index.html`).then(h.verifyResponse(404, bodyRegex)), h.runCmd(`curl -iL http://${host}/`).then(h.verifyResponse(404, bodyRegex)), h.runCmd(`curl -iL http://${host}`).then(h.verifyResponse(404, bodyRegex)), - ]).then(done); - }); - - - it('should respond with 405 for non-GET requests to any URL', done => { - const bodyRegex = /^Unsupported method/; - - Promise.all([ - h.runCmd(`curl -iLX PUT http://${host}`).then(h.verifyResponse(405, bodyRegex)), - h.runCmd(`curl -iLX POST http://${host}`).then(h.verifyResponse(405, bodyRegex)), - h.runCmd(`curl -iLX PATCH http://${host}`).then(h.verifyResponse(405, bodyRegex)), - h.runCmd(`curl -iLX DELETE http://${host}`).then(h.verifyResponse(405, bodyRegex)), + h.runCmd(`curl -iLX PUT http://${host}`).then(h.verifyResponse(404, bodyRegex)), + h.runCmd(`curl -iLX POST http://${host}`).then(h.verifyResponse(404, bodyRegex)), + h.runCmd(`curl -iLX PATCH http://${host}`).then(h.verifyResponse(404, bodyRegex)), + h.runCmd(`curl -iLX DELETE http://${host}`).then(h.verifyResponse(404, bodyRegex)), ]).then(done); }); diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/upload-server-factory.spec.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/upload-server-factory.spec.ts index c7041797cf..2e0c9eae99 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/upload-server-factory.spec.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/upload-server-factory.spec.ts @@ -258,12 +258,12 @@ describe('uploadServerFactory', () => { }); - it('should respond with 405 for non-GET requests', done => { + it('should respond with 404 for non-GET requests', done => { verifyRequests([ - agent.put(`/create-build/${pr}/${sha}`).expect(405), - agent.post(`/create-build/${pr}/${sha}`).expect(405), - agent.patch(`/create-build/${pr}/${sha}`).expect(405), - agent.delete(`/create-build/${pr}/${sha}`).expect(405), + agent.put(`/create-build/${pr}/${sha}`).expect(404), + agent.post(`/create-build/${pr}/${sha}`).expect(404), + agent.patch(`/create-build/${pr}/${sha}`).expect(404), + agent.delete(`/create-build/${pr}/${sha}`).expect(404), ], done); }); @@ -418,12 +418,12 @@ describe('uploadServerFactory', () => { }); - it('should respond with 405 for non-GET requests', done => { + it('should respond with 404 for non-GET requests', done => { verifyRequests([ - agent.put('/health-check').expect(405), - agent.post('/health-check').expect(405), - agent.patch('/health-check').expect(405), - agent.delete('/health-check').expect(405), + agent.put('/health-check').expect(404), + agent.post('/health-check').expect(404), + agent.patch('/health-check').expect(404), + agent.delete('/health-check').expect(404), ], done); }); @@ -442,26 +442,17 @@ describe('uploadServerFactory', () => { }); - describe('GET *', () => { - - it('should respond with 404', done => { - const responseBody = 'Unknown resource in request: GET /some/url'; - verifyRequests([agent.get('/some/url').expect(404, responseBody)], done); - }); - - }); - - describe('ALL *', () => { - it('should respond with 405', done => { - const responseFor = (method: string) => `Unsupported method in request: ${method.toUpperCase()} /some/url`; + it('should respond with 404', done => { + const responseFor = (method: string) => `Unknown resource in request: ${method.toUpperCase()} /some/url`; verifyRequests([ - agent.put('/some/url').expect(405, responseFor('put')), - agent.post('/some/url').expect(405, responseFor('post')), - agent.patch('/some/url').expect(405, responseFor('patch')), - agent.delete('/some/url').expect(405, responseFor('delete')), + agent.get('/some/url').expect(404, responseFor('get')), + agent.put('/some/url').expect(404, responseFor('put')), + agent.post('/some/url').expect(404, responseFor('post')), + agent.patch('/some/url').expect(404, responseFor('patch')), + agent.delete('/some/url').expect(404, responseFor('delete')), ], done); });