Conversation
package.json
Outdated
| "lint:js": "eslint .", | ||
| "start": "node server/server", | ||
| "test": "mocha test/unit && mocha test/server", | ||
| "test-browser": "watchify test/frontend/frontend.bundle.js -o test/frontend/bundle.js -d", |
There was a problem hiding this comment.
if "test/frontend/bundle.js" is compiled/browserified, we may want to add it to .gitignore and nuke the file below.
There was a problem hiding this comment.
Ahh good idea. Did not mean to push that.
test/frontend/frontend.bundle.js
Outdated
| constructor(name, data, opt) { | ||
| super(data, opt); | ||
| this.name = name; | ||
| } |
There was a problem hiding this comment.
Indenting looks weird in this file (4 space indent). We may want to confirm that ESLint and prettier are linting these files. I would have almost expected CircleCI linting to fail on these.
There was a problem hiding this comment.
This is the line in package.json that you may want to tweak:
"format": "prettier '{frontend/src/,scripts/,server/,test/}*.js' 'public/*.css' --single-quote --write",Instead of "test/", we'll need "test/**/" due to the nested files in the test directory.
Although, I think if I currently run npm run format on the codebase it may break linting, so I'll shut up now.
test/frontend/frontend.test.js
Outdated
| }) | ||
| }) | ||
|
|
||
| it('Should catch tampered checksums', function(done) { |
There was a problem hiding this comment.
I think "tampered" isn't the best word for this. It took me a while to understand what you meant since tampering can happen at other times besides creation. Phony, bogus, or fraudulent better imply it's the creator that's being a jerk.
| { | ||
| name: 'AES-GCM', | ||
| iv: hexToArray(encryptedIV), | ||
| additionalData: fileHash, |
There was a problem hiding this comment.
Add a comment here, this is the meat of this whole test, lets make it obvious that fileHash is the phony checksum
There was a problem hiding this comment.
Gotcha, will do.
frontend/src/fileReceiver.js
Outdated
| }), | ||
| new Promise((resolve, reject) => { | ||
| resolve(hexToArray(fdata.aad)); | ||
| }) |
There was a problem hiding this comment.
For these two promises, I don't think you need a Promise wrapper since you're doing a Promise.all() above. IIRC, you can just return fdata.filename and hexToArray(fdata.aad) and it should still work the same.
frontend/src/fileReceiver.js
Outdated
| }), | ||
| new Promise((resolve, reject) => { | ||
| resolve(fname); | ||
| }) |
There was a problem hiding this comment.
Same here. I think you can just do return Promise.all([decrypted, fname]); or possibly even Promise.resolve([decrypted, fname]);.
circle.yml
Outdated
| pre: | ||
| - npm i -g get-firefox | ||
| - get-firefox --platform linux --extract --target /home/ubuntu/send | ||
| - npm i -g geckodriver |
There was a problem hiding this comment.
I bet you can add this global install to the line 10 npm i -g get-firefox geckodriver and install them both at the same time.
Not sure if we'd want to add these as devDependencies if we'd ever need to run them locally. 🤷♀️
There was a problem hiding this comment.
@dannycoates What are your thoughts on adding these as devDependencies?
test/frontend/frontend.test.html
Outdated
| </script> | ||
|
|
||
| <script> | ||
| </script> |
There was a problem hiding this comment.
nit: Probably don't need the empty <script></script> tag here.
3 participants
Fixes #181