Fix validation of upload-length header in patch handler (#303)

mitjap · Murderlon · web-flow · commit 63b733089f9b · 2022-10-03T15:25:41.000-03:00
Co-authored-by: Merlijn Vos &lt;merlijn@soverin.net&gt;
diff --git a/lib/constants.js b/lib/constants.js
@@ -64,6 +64,10 @@ const ERRORS = {
         status_code: 501,
         body: 'Concatenation extension is not (yet) supported. Disable parallel uploads in the tus client.\n',
     },
+    UNSUPPORTED_CREATION_DEFER_LENGTH_EXTENSION: {
+        status_code: 501,
+        body: 'creation-defer-length extension is not (yet) supported.\n',
+    },
 };
 
 const EVENT_ENDPOINT_CREATED = 'EVENT_ENDPOINT_CREATED';
diff --git a/lib/handlers/PatchHandler.js b/lib/handlers/PatchHandler.js
@@ -32,14 +32,6 @@ class PatchHandler extends BaseHandler {
             throw ERRORS.INVALID_CONTENT_TYPE;
         }
 
-        // The request MUST validate upload-length related headers
-        const upload_length = req.headers['upload-length'];
-        const upload_defer_length = req.headers['upload-defer-length'];
-
-        if ((upload_length === undefined) === (upload_defer_length === undefined)) {
-            throw ERRORS.INVALID_LENGTH;
-        }
-
         const file = await this.store.getOffset(file_id);
 
         if (file.size !== offset) {
@@ -48,22 +40,29 @@ class PatchHandler extends BaseHandler {
             throw ERRORS.INVALID_OFFSET;
         }
 
-        // Throw error is upload-length header does not match current upload-length if it is set.
-        if (upload_length !== undefined && file.upload_length !== undefined && upload_length !== file.upload_length) {
-            throw ERRORS.INVALID_LENGTH;
-        }
-        // Throw error is upload-defer-length header is present while upload-length is already known
-        else if (upload_defer_length !== undefined && file.upload_length !== undefined) {
-            throw ERRORS.INVALID_LENGTH;
-        }
+        // The request MUST validate upload-length related headers
+        const upload_length = req.headers['upload-length'];
+        if (upload_length !== undefined) {
+            // Throw error if extension is not supported
+            if (!this.store.hasExtension('creation-defer-length')) {
+                throw ERRORS.UNSUPPORTED_CREATION_DEFER_LENGTH_EXTENSION;
+            }
+
+            // Throw error if upload-length is already set.
+            if (file.upload_length !== undefined) {
+                throw ERRORS.INVALID_LENGTH;
+            }
+
+            if (parseInt(upload_length, 10) < file.size) {
+                throw ERRORS.INVALID_LENGTH;
+            }
 
-        // Declare upload-length if it is not already known
-        if (upload_length !== undefined && file.upload_length === undefined) {
             await this.store.declareUploadLength(file_id, upload_length);
+            file.upload_length = upload_length;
         }
 
         const new_offset = await this.store.write(req, file_id, offset);
-        if (new_offset === parseInt(upload_length, 10)) {
+        if (new_offset === parseInt(file.upload_length, 10)) {
             this.emit(EVENTS.EVENT_UPLOAD_COMPLETE, { file: new File(file_id, file.upload_length, file.upload_defer_length, file.upload_metadata) });
         }
 
diff --git a/lib/handlers/PostHandler.js b/lib/handlers/PostHandler.js
@@ -37,6 +37,13 @@ class PostHandler extends BaseHandler {
         const upload_defer_length = req.headers['upload-defer-length'];
         const upload_metadata = req.headers['upload-metadata'];
 
+        if (upload_defer_length !== undefined) {
+            // Throw error if extension is not supported
+            if (!this.store.hasExtension('creation-defer-length')) {
+                throw ERRORS.UNSUPPORTED_CREATION_DEFER_LENGTH_EXTENSION;
+            }
+        }
+
         if ((upload_length === undefined) === (upload_defer_length === undefined)) {
             throw ERRORS.INVALID_LENGTH;
         }
diff --git a/lib/stores/DataStore.js b/lib/stores/DataStore.js
@@ -85,7 +85,7 @@ class DataStore extends EventEmitter {
      * Called in PATCH requests when upload length is known after being defered.
      *
      * @param {string} file_id Name of file
-     * @param {number} upload_length Declared upload length
+     * @param {string} upload_length Declared upload length
      * @return {Promise}
      */
     async declareUploadLength(file_id, upload_length) {
diff --git a/test/Test-EndToEnd.js b/test/Test-EndToEnd.js
@@ -219,7 +219,6 @@ describe('EndToEnd', () => {
                 const write_stream = agent.patch(`${STORE_PATH}/${file_id}`)
                     .set('Tus-Resumable', TUS_RESUMABLE)
                     .set('Upload-Offset', 0)
-                    .set('Upload-Length', TEST_FILE_SIZE)
                     .set('Content-Type', 'application/offset+octet-stream');
 
                 write_stream.on('response', (res) => {
@@ -471,7 +470,6 @@ describe('EndToEnd', () => {
                 const write_stream = agent.patch(`${STORE_PATH}/${file_id}`)
                     .set('Tus-Resumable', TUS_RESUMABLE)
                     .set('Upload-Offset', 0)
-                    .set('Upload-Length', `${TEST_FILE_SIZE}`)
                     .set('Content-Type', 'application/offset+octet-stream');
 
                 write_stream.on('response', (res) => {
diff --git a/test/Test-PatchHandler.js b/test/Test-PatchHandler.js
@@ -65,19 +65,6 @@ describe('PatchHandler', () => {
             return assert.rejects(() => handler.send(req, res), { status_code: 403 });
         });
 
-        it('should 400 if upload-defer-length is present when it upload-length is known', () => {
-            req.headers = {
-                'upload-offset': '0',
-                'upload-defer-length': '1',
-                'content-type': 'application/offset+octet-stream',
-            };
-            req.url = `${path}/file`;
-
-            store.getOffset.resolves({ size: 0, upload_length: '512' });
-
-            return assert.rejects(() => handler.send(req, res), { status_code: 400 });
-        });
-
         it('should declare upload-length once it is send', async () => {
             req.headers = {
                 'upload-offset': '0',
@@ -86,6 +73,7 @@ describe('PatchHandler', () => {
             };
             req.url = `${path}/file`;
 
+            store.hasExtension.withArgs('creation-defer-length').returns(true);
             store.getOffset.resolves({ size: 0, upload_defer_length: '1' });
             store.write.resolves(5);
             store.declareUploadLength.resolves();
@@ -95,7 +83,7 @@ describe('PatchHandler', () => {
             assert.equal(store.declareUploadLength.calledOnceWith('file', '10'), true);
         });
         
-        it('should 400 if upload-length does not match', () => {
+        it('should 400 if upload-length is already set', () => {
             req.headers = {
                 'upload-offset': '0',
                 'upload-length': '10',
@@ -104,6 +92,7 @@ describe('PatchHandler', () => {
             req.url = `${path}/file`;
 
             store.getOffset.resolves({ size: 0, upload_length: '20' });
+            store.hasExtension.withArgs('creation-defer-length').returns(true);
 
             return assert.rejects(() => handler.send(req, res), { status_code: 400 });
         });
@@ -134,7 +123,6 @@ describe('PatchHandler', () => {
         it('must acknowledge successful PATCH requests with the 204', async () => {
             req.headers = {
                 'upload-offset': '0',
-                'upload-length': '1024',
                 'content-type': 'application/offset+octet-stream',
             };
             req.url = `${path}/1234`;
diff --git a/test/Test-PostHandler.js b/test/Test-PostHandler.js
@@ -21,6 +21,7 @@ describe('PostHandler', () => {
     let res = null;
 
     const fake_store = sinon.createStubInstance(DataStore);
+    fake_store.hasExtension.withArgs('creation-defer-length').returns(true);
 
     beforeEach((done) => {
         req = { headers: {}, url: '/files', host: 'localhost:3000' };
@@ -169,6 +170,7 @@ describe('PostHandler', () => {
                 
                 fake_store.create.resolvesArg(0);
                 fake_store.write.resolves(upload_length);
+                fake_store.hasExtension.withArgs('creation-defer-length').returns(true);
 
                 const handler = new PostHandler(fake_store, { path: '/test/output' });
                 handler.on(EVENTS.EVENT_UPLOAD_COMPLETE, (obj) => {
diff --git a/test/Test-Server.js b/test/Test-Server.js
@@ -305,7 +305,6 @@ describe('Server', () => {
                         .send('test')
                         .set('Tus-Resumable', TUS_RESUMABLE)
                         .set('Upload-Offset', 0)
-                        .set('Upload-Length', Buffer.byteLength('test', 'utf8'))
                         .set('Content-Type', 'application/offset+octet-stream')
                         .end((err) => { if (err) done(err) });
                 })

Original file line number	Diff line number	Diff line change
`@@ -85,7 +85,7 @@ class DataStore extends EventEmitter {`
`85`	`85`	`* Called in PATCH requests when upload length is known after being defered.`
`86`	`86`	`*`
`87`	`87`	`* @param {string} file_id Name of file`
`88`		`- * @param {number} upload_length Declared upload length`
	`88`	`+ * @param {string} upload_length Declared upload length`
`89`	`89`	`* @return {Promise}`
`90`	`90`	`*/`
`91`	`91`	`async declareUploadLength(file_id, upload_length) {`