Fix potential to miss file download sequences (#217)

* buffer files on a character basis

* Add tests and initial github workflow

* Bump version

* Pull request feedback from @butlerx

* ESlint ignore download.spec.ts file

.github/workflows/test.yml

@@ -0,0 +1,22 @@
+---
+on: [pull_request]
+
+name: Run tests
+jobs:
+  build_and_test:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v1
+      - name: Setup env
+        uses: actions/setup-node@v1
+        with:
+          node-version: 12
+      - run: yarn
+        name: Install dependencies
+      - name: ESLint checks
+        run: yarn lint
+      - run: yarn build
+        name: Compile Typescript
+      - run: yarn test
+        name: Run tests

package.json

@@ -1,6 +1,6 @@
 {
   "name": "wetty",
-  "version": "1.2.1",
+  "version": "1.2.2",
   "description": "WeTTY = Web + TTY. Terminal access in browser over http/https",
   "homepage": "https://github.com/butlerx/wetty",
   "repository": {
@@ -23,7 +23,8 @@
     "dev": "NODE_ENV=development concurrently --kill-others --success first \"babel-node node_modules/.bin/webpack --watch\" \"nodemon .\"",
     "lint": "eslint --ext .ts,.js .",
     "prepublishOnly": "NODE_ENV=production yarn build",
-    "start": "node ."
+    "start": "node .",
+    "test": "mocha -r babel-register-ts src/**/*.spec.ts"
   },
   "husky": {
     "hooks": {
@@ -79,14 +80,18 @@
     "@babel/preset-env": "^7.5.5",
     "@babel/preset-typescript": "^7.3.3",
     "@babel/register": "^7.5.5",
+    "@types/chai": "^4.2.5",
     "@types/compression": "^1.0.1",
     "@types/express": "^4.17.1",
     "@types/fs-extra": "^8.0.0",
     "@types/helmet": "^0.0.44",
+    "@types/jsdom": "^12.2.4",
     "@types/lodash": "^4.14.138",
+    "@types/mocha": "^5.2.7",
     "@types/morgan": "^1.7.37",
     "@types/node": "^12.7.3",
     "@types/serve-favicon": "^2.2.31",
+    "@types/sinon": "^7.5.1",
     "@types/socket.io": "^2.1.2",
     "@types/socket.io-client": "^1.4.32",
     "@types/webpack-env": "^1.14.0",
@@ -96,6 +101,8 @@
     "all-contributors-cli": "^6.9.3",
     "babel-loader": "^8.0.6",
     "babel-plugin-lodash": "^3.3.4",
+    "babel-register-ts": "^7.0.0",
+    "chai": "^4.2.0",
     "concurrently": "^4.1.2",
     "css-loader": "^3.2.0",
     "eslint": "6.1.0",
@@ -106,12 +113,15 @@
     "file-loader": "^4.2.0",
     "git-authors-cli": "^1.0.18",
     "husky": "^3.0.9",
+    "jsdom": "^15.2.1",
     "lint-staged": "~9.2.5",
     "mini-css-extract-plugin": "^0.8.0",
+    "mocha": "^6.2.2",
     "node-sass": "^4.12.0",
     "nodemon": "^1.19.2",
     "prettier": "^1.18.2",
     "sass-loader": "^8.0.0",
+    "sinon": "^7.5.0",
     "style-loader": "^1.0.0",
     "typescript": "~3.6.2",
     "webpack": "^4.39.3",

src/client/download.ts

@@ -1,55 +1,96 @@
-import * as fileType from 'file-type';
+const DEFAULT_FILE_BEGIN = '\u001b[5i';
-import Toastify from 'toastify-js';
+const DEFAULT_FILE_END = '\u001b[4i';
-
+
-export const FILE_BEGIN = '\u001b[5i';
+export class FileDownloader {
-export const FILE_END = '\u001b[4i';
+  constructor(
-export let fileBuffer = [];
+    onCompleteFileCallback: (file: string) => any,
-
+    fileBegin: string = DEFAULT_FILE_BEGIN,
-export function onCompleteFile() {
+    fileEnd: string = DEFAULT_FILE_END
-  let bufferCharacters = fileBuffer.join('');
+  ) {
-  bufferCharacters = bufferCharacters.substring(
+    this.fileBuffer = [];
-    bufferCharacters.lastIndexOf(FILE_BEGIN) + FILE_BEGIN.length,
+    this.onCompleteFileCallback = onCompleteFileCallback;
-    bufferCharacters.lastIndexOf(FILE_END)
+    this.fileBegin = fileBegin;
-  );
+    this.fileEnd = fileEnd;
-
+    this.partialFileBegin = '';
-  // Try to decode it as base64, if it fails we assume it's not base64
+  }
-  try {
+
-    bufferCharacters = window.atob(bufferCharacters);
+  bufferCharacter(character: string): string {
-  } catch (err) {
+    // If we are not currently buffering a file.
-    // Assuming it's not base64...
+    if (this.fileBuffer.length === 0) {
+      // If we are not currently expecting the rest of the fileBegin sequences.
+      if (this.partialFileBegin.length === 0) {
+        // If the character is the first character of fileBegin we know to start
+        // expecting the rest of the fileBegin sequence.
+        if (character === this.fileBegin[0]) {
+          this.partialFileBegin = character;
+          return '';
+        }
+        // Otherwise, we just return the character for printing to the terminal.
+
+        return character;
+      }
+      // We're currently in the state of buffering a beginner marker...
+
+      const nextExpectedCharacter = this.fileBegin[
+        this.partialFileBegin.length
+      ];
+      // If the next character *is* the next character in the fileBegin sequence.
+      if (character === nextExpectedCharacter) {
+        this.partialFileBegin += character;
+        // Do we now have the complete fileBegin sequence.
+        if (this.partialFileBegin === this.fileBegin) {
+          this.partialFileBegin = '';
+          this.fileBuffer = this.fileBuffer.concat(this.fileBegin.split(''));
+          return '';
+        }
+        // Otherwise, we just wait until the next character.
+
+        return '';
+      }
+      // If the next expected character wasn't found for the fileBegin sequence,
+      // we need to return all the data that was bufferd in the partialFileBegin
+      // back to the terminal.
+
+      const dataToReturn = this.partialFileBegin + character;
+      this.partialFileBegin = '';
+      return dataToReturn;
+    }
+    // If we are currently in the state of buffering a file.
+
+    this.fileBuffer.push(character);
+    // If we now have an entire fileEnd marker, we have a complete file!
+    if (
+      this.fileBuffer.length >= this.fileBegin.length + this.fileEnd.length &&
+      this.fileBuffer.slice(-this.fileEnd.length).join('') === this.fileEnd
+    ) {
+      this.onCompleteFile(
+        this.fileBuffer
+          .slice(
+            this.fileBegin.length,
+            this.fileBuffer.length - this.fileEnd.length
+          )
+          .join('')
+      );
+      this.fileBuffer = [];
+    }
+
+    return '';
   }
 
-  const bytes = new Uint8Array(bufferCharacters.length);
+  buffer(data: string): string {
-  for (let i = 0; i < bufferCharacters.length; i += 1) {
+    // This is a optimization to quickly return if we know for
-    bytes[i] = bufferCharacters.charCodeAt(i);
+    // sure we don't need to loop over each character.
+    if (
+      this.fileBuffer.length === 0 &&
+      this.partialFileBegin.length === 0 &&
+      data.indexOf(this.fileBegin[0]) === -1
+    ) {
+      return data;
+    }
+    return data.split('').map(this.bufferCharacter.bind(this)).join('')
   }
 
-  let mimeType = 'application/octet-stream';
+  onCompleteFile(bufferCharacters: string) {
-  let fileExt = '';
+    this.onCompleteFileCallback(bufferCharacters);
-  const typeData = fileType(bytes);
-  if (typeData) {
-    mimeType = typeData.mime;
-    fileExt = typeData.ext;
   }
-  const fileName = `file-${new Date()
-    .toISOString()
-    .split('.')[0]
-    .replace(/-/g, '')
-    .replace('T', '')
-    .replace(/:/g, '')}${fileExt ? `.${fileExt}` : ''}`;
-
-  const blob = new Blob([new Uint8Array(bytes.buffer)], { type: mimeType });
-  const blobUrl = URL.createObjectURL(blob);
-
-  fileBuffer = [];
-
-  Toastify({
-    text: `Download ready: <a href="${blobUrl}" target="_blank" download="${fileName}">${fileName}</a>`,
-    duration: 10000,
-    newWindow: true,
-    gravity: 'bottom',
-    position: 'right',
-    backgroundColor: '#fff',
-    stopOnFocus: true,
-  }).showToast();
 }

src/client/index.ts

@@ -3,9 +3,11 @@ import { isNull } from 'lodash';
 
 import { dom, library } from "@fortawesome/fontawesome-svg-core";
 import { faCogs } from "@fortawesome/free-solid-svg-icons/faCogs";
+import Toastify from 'toastify-js';
+import * as fileType from 'file-type';
 import { socket } from './socket';
 import { overlay, terminal } from './elements';
-import { FILE_BEGIN, FILE_END, fileBuffer, onCompleteFile } from './download';
+import { FileDownloader } from './download';
 import verifyPrompt from './verify';
 import disconnect from './disconnect';
 import mobileKeyboard from './mobile';
@@ -76,6 +78,52 @@ socket.on('connect', () => {
   term.focus();
   mobileKeyboard();
 
+  const fileDownloader = new FileDownloader(function (
+    bufferCharacters: string
+  ) {
+    let fileCharacters = bufferCharacters;
+    // Try to decode it as base64, if it fails we assume it's not base64
+    try {
+      fileCharacters = window.atob(fileCharacters);
+    } catch (err) {
+      // Assuming it's not base64...
+    }
+
+    const bytes = new Uint8Array(fileCharacters.length);
+    for (let i = 0; i < fileCharacters.length; i += 1) {
+      bytes[i] = fileCharacters.charCodeAt(i);
+    }
+
+    let mimeType = 'application/octet-stream';
+    let fileExt = '';
+    const typeData = fileType(bytes);
+    if (typeData) {
+      mimeType = typeData.mime;
+      fileExt = typeData.ext;
+    }
+    const fileName = `file-${new Date()
+      .toISOString()
+      .split('.')[0]
+      .replace(/-/g, '')
+      .replace('T', '')
+      .replace(/:/g, '')}${fileExt ? `.${fileExt}` : ''}`;
+
+    const blob = new Blob([new Uint8Array(bytes.buffer)], {
+      type: mimeType,
+    });
+    const blobUrl = URL.createObjectURL(blob);
+
+    Toastify({
+      text: `Download ready: <a href="${blobUrl}" target="_blank" download="${fileName}">${fileName}</a>`,
+      duration: 10000,
+      newWindow: true,
+      gravity: 'bottom',
+      position: 'right',
+      backgroundColor: '#fff',
+      stopOnFocus: true,
+    }).showToast();
+  });
+
   term.on('data', data => {
     socket.emit('input', data);
   });
@@ -84,30 +132,9 @@ socket.on('connect', () => {
   });
   socket
     .on('data', (data: string) => {
-      const indexOfFileBegin = data.indexOf(FILE_BEGIN);
+      const remainingData = fileDownloader.buffer(data);
-      const indexOfFileEnd = data.indexOf(FILE_END);
+      if (remainingData) {
-
+        term.write(remainingData);
-      // If we've got the entire file in one chunk
-      if (indexOfFileBegin !== -1 && indexOfFileEnd !== -1) {
-        fileBuffer.push(data);
-        onCompleteFile();
-      }
-      // If we've found a beginning marker
-      else if (indexOfFileBegin !== -1) {
-        fileBuffer.push(data);
-      }
-      // If we've found an ending marker
-      else if (indexOfFileEnd !== -1) {
-        fileBuffer.push(data);
-        onCompleteFile();
-      }
-      // If we've found the continuation of a file
-      else if (fileBuffer.length > 0) {
-        fileBuffer.push(data);
-      }
-      // Just treat it as normal data
-      else {
-        term.write(data);
       }
     })
     .on('login', () => {

src/client/specs/download.spec.ts

@@ -0,0 +1,195 @@
+/* eslint-disable */
+
+import { expect } from 'chai';
+import 'mocha';
+import * as sinon from 'sinon';
+
+import { JSDOM } from 'jsdom';
+import { FileDownloader } from '../download';
+
+const { window } = new JSDOM(`...`);
+
+describe('FileDownloader', () => {
+    const FILE_BEGIN = 'BEGIN';
+    const FILE_END = 'END';
+    let fileDownloader: any;
+
+    beforeEach(() => {
+        fileDownloader = new FileDownloader(() => { }, FILE_BEGIN, FILE_END);
+    });
+
+    afterEach(() => {
+        sinon.restore();
+    });
+
+    it('should return data before file markers', () => {
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+        expect(
+            fileDownloader.buffer(`DATA AT THE LEFT${FILE_BEGIN}FILE${FILE_END}`)
+        ).to.equal('DATA AT THE LEFT');
+        expect(onCompleteFileStub.calledOnce).to.be.true;
+        expect(onCompleteFileStub.getCall(0).args[0]).to.equal('FILE');
+    });
+
+    it('should return data after file markers', () => {
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+        expect(
+            fileDownloader.buffer(`${FILE_BEGIN}FILE${FILE_END}DATA AT THE RIGHT`)
+        ).to.equal('DATA AT THE RIGHT');
+        expect(onCompleteFileStub.calledOnce).to.be.true;
+        expect(onCompleteFileStub.getCall(0).args[0]).to.equal('FILE');
+    });
+
+    it('should return data before and after file markers', () => {
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+        expect(
+            fileDownloader.buffer(
+                `DATA AT THE LEFT${FILE_BEGIN}FILE${FILE_END}DATA AT THE RIGHT`
+            )
+        ).to.equal('DATA AT THE LEFTDATA AT THE RIGHT');
+        expect(onCompleteFileStub.calledOnce).to.be.true;
+        expect(onCompleteFileStub.getCall(0).args[0]).to.equal('FILE');
+    });
+
+    it('should return data before a beginning marker found', () => {
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+        expect(fileDownloader.buffer(`DATA AT THE LEFT${FILE_BEGIN}FILE`)).to.equal(
+            'DATA AT THE LEFT'
+        );
+    });
+
+    it('should return data after an ending marker found', () => {
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+        expect(fileDownloader.buffer(`${FILE_BEGIN}FI`)).to.equal('');
+        expect(fileDownloader.buffer(`LE${FILE_END}DATA AT THE RIGHT`)).to.equal(
+            'DATA AT THE RIGHT'
+        );
+        expect(onCompleteFileStub.calledOnce).to.be.true;
+        expect(onCompleteFileStub.getCall(0).args[0]).to.equal('FILE');
+    });
+
+    it('should buffer across incomplete file begin marker sequence on two calls', () => {
+        fileDownloader = new FileDownloader(() => { }, 'BEGIN', 'END');
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+
+        expect(fileDownloader.buffer('BEG')).to.equal('');
+        expect(fileDownloader.buffer('INFILEEND')).to.equal('');
+        expect(onCompleteFileStub.calledOnce).to.be.true;
+        expect(onCompleteFileStub.getCall(0).args[0]).to.equal('FILE');
+    });
+
+    it('should buffer across incomplete file begin marker sequence on n calls', () => {
+        fileDownloader = new FileDownloader(() => { }, 'BEGIN', 'END');
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+
+        expect(fileDownloader.buffer('B')).to.equal('');
+        expect(fileDownloader.buffer('E')).to.equal('');
+        expect(fileDownloader.buffer('G')).to.equal('');
+        expect(fileDownloader.buffer('I')).to.equal('');
+        expect(fileDownloader.buffer('NFILE' + 'END')).to.equal('');
+        expect(onCompleteFileStub.calledOnce).to.be.true;
+        expect(onCompleteFileStub.getCall(0).args[0]).to.equal('FILE');
+    });
+
+    it('should buffer across incomplete file begin marker sequence with data on the left and right on multiple calls', () => {
+        fileDownloader = new FileDownloader(() => { }, 'BEGIN', 'END');
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+
+        expect(fileDownloader.buffer('DATA AT THE LEFT' + 'B')).to.equal(
+            'DATA AT THE LEFT'
+        );
+        expect(fileDownloader.buffer('E')).to.equal('');
+        expect(fileDownloader.buffer('G')).to.equal('');
+        expect(fileDownloader.buffer('I')).to.equal('');
+        expect(fileDownloader.buffer('NFILE' + 'ENDDATA AT THE RIGHT')).to.equal(
+            'DATA AT THE RIGHT'
+        );
+        expect(onCompleteFileStub.calledOnce).to.be.true;
+        expect(onCompleteFileStub.getCall(0).args[0]).to.equal('FILE');
+    });
+
+    it('should buffer across incomplete file begin marker sequence then handle false positive', () => {
+        fileDownloader = new FileDownloader(() => { }, 'BEGIN', 'END');
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+
+        expect(fileDownloader.buffer('DATA AT THE LEFT' + 'B')).to.equal(
+            'DATA AT THE LEFT'
+        );
+        expect(fileDownloader.buffer('E')).to.equal('');
+        expect(fileDownloader.buffer('G')).to.equal('');
+        // This isn't part of the file_begin marker and should trigger the partial
+        // file begin marker to be returned with the normal data
+        expect(fileDownloader.buffer('ZDATA AT THE RIGHT')).to.equal(
+            'BEGZDATA AT THE RIGHT'
+        );
+        expect(onCompleteFileStub.called).to.be.false;
+    });
+
+    it('should buffer across incomplete file end marker sequence on two calls', () => {
+        fileDownloader = new FileDownloader(() => { }, 'BEGIN', 'END');
+        const mockFilePart1 = 'DATA AT THE LEFTBEGINFILEE';
+        const mockFilePart2 = 'NDDATA AT THE RIGHT';
+
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+        expect(fileDownloader.buffer(mockFilePart1)).to.equal('DATA AT THE LEFT');
+        expect(fileDownloader.buffer(mockFilePart2)).to.equal('DATA AT THE RIGHT');
+
+        expect(onCompleteFileStub.calledOnce).to.be.true;
+        expect(onCompleteFileStub.getCall(0).args[0]).to.equal('FILE');
+    });
+
+    it('should buffer across incomplete file end and file begin marker sequence with data on the left and right on multiple calls', () => {
+        fileDownloader = new FileDownloader(() => { }, 'BEGIN', 'END');
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+
+        expect(fileDownloader.buffer('DATA AT THE LEFT' + 'BE')).to.equal(
+            'DATA AT THE LEFT'
+        );
+        expect(fileDownloader.buffer('G')).to.equal('');
+        expect(fileDownloader.buffer('I')).to.equal('');
+        expect(fileDownloader.buffer('NFILEE')).to.equal('');
+        expect(fileDownloader.buffer('N')).to.equal('');
+        expect(fileDownloader.buffer('DDATA AT THE RIGHT')).to.equal(
+            'DATA AT THE RIGHT'
+        );
+        expect(onCompleteFileStub.calledOnce).to.be.true;
+        expect(onCompleteFileStub.getCall(0).args[0]).to.equal('FILE');
+    });
+
+    it('should be able to handle multiple files', () => {
+        fileDownloader = new FileDownloader(() => { }, 'BEGIN', 'END');
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+
+        expect(
+            fileDownloader.buffer(
+                'DATA AT THE LEFT' + 'BEGIN' + 'FILE1' + 'END' + 'SECOND DATA' + 'BEGIN'
+            )
+        ).to.equal('DATA AT THE LEFT' + 'SECOND DATA');
+        expect(onCompleteFileStub.calledOnce).to.be.true;
+        expect(onCompleteFileStub.getCall(0).args[0]).to.equal('FILE1');
+
+        expect(fileDownloader.buffer('FILE2')).to.equal('');
+        expect(fileDownloader.buffer('E')).to.equal('');
+        expect(fileDownloader.buffer('NDRIGHT')).to.equal('RIGHT');
+        expect(onCompleteFileStub.calledTwice).to.be.true;
+        expect(onCompleteFileStub.getCall(1).args[0]).to.equal('FILE2');
+    });
+
+    it('should be able to handle multiple files with an ending marker', () => {
+        fileDownloader = new FileDownloader(() => { }, 'BEGIN', 'END');
+        const onCompleteFileStub = sinon.stub(fileDownloader, 'onCompleteFile');
+
+        expect(
+            fileDownloader.buffer('DATA AT THE LEFT' + 'BEGIN' + 'FILE1' + 'EN')
+        ).to.equal('DATA AT THE LEFT');
+        expect(onCompleteFileStub.calledOnce).to.be.false;
+        expect(
+            fileDownloader.buffer('D' + 'SECOND DATA' + 'BEGIN' + 'FILE2' + 'EN')
+        ).to.equal('SECOND DATA');
+        expect(onCompleteFileStub.calledOnce).to.be.true;
+        expect(onCompleteFileStub.getCall(0).args[0]).to.equal('FILE1');
+        expect(fileDownloader.buffer('D')).to.equal('');
+        expect(onCompleteFileStub.calledTwice).to.be.true;
+        expect(onCompleteFileStub.getCall(1).args[0]).to.equal('FILE2');
+    });
+});