feat: checksum extension #347

manohar27 · 2022-04-27T06:33:09Z

Adds Checksum Extension support.

This is work in progress.
Uses SubtleCrypto on browser and crypto library on nodejs

Acconut · 2022-07-25T12:23:07Z

Thank you very much for this PR! It looks interesting. Let me know once it is ready for a review!

manohar27 · 2022-10-26T07:18:07Z

Thank you very much for this PR! It looks interesting. Let me know once it is ready for a review!

Sorry this took so long.

Need some help with code reviews and testing this

Acconut · 2022-10-29T21:17:44Z

Thank you! I will try to review this in the next two weeks!

partially copied from tus#347

Acconut

Thank you again for this incredible work. I left some comments to your code :)

lib/browser/extensions/checksum.js

lib/node/extensions/checksum.js

lib/upload.js

Acconut · 2023-03-26T23:13:33Z

lib/upload.js

+            Checksum = require('./browser/extensions/checksum').default
+          }
+          this._checksum = new Checksum(
+            Checksum.supportedAlogrithms.find((supportedAlgo) => supportedAlgorithms.includes(supportedAlgo)),


What does this check do? The combination of find and includes looks suspicious.

I was trying to fetch the supported algorithms from the server and see if one of them matches one of the supported algorithms on the client. If we're saying client and server have agreed upon an algorithm from the get go, then we can skip this OPTIONS call and let client just configure the algorithm in options.

lib/upload.js

Acconut · 2023-03-26T23:15:16Z

lib/upload.js

-        if (value === null) {
+        if (this.options.enableChecksum) {
+          if (this._checksum) {
+            value.arrayBuffer().then((chunkBuffer) => {


Could this entire if block be merged with the last else block? They seem very similar and would avoid some duplication.

Not sure how this might reduce the duplication as one is async. I've removed one outer if condition. Is that okay?

test/spec/test-browser-specific.js

manohar27 · 2023-04-12T09:06:07Z

Checksum header being added in the demo browser page

manohar27 · 2023-04-12T09:07:29Z

Thank you for the review. Please have a look @Acconut, I see there are conflicts. I'll try to address them, might need help

manohar27 · 2023-05-04T09:58:14Z

@Acconut please have another look at this

mahyarmirrashed · 2023-12-19T02:57:27Z

Can I work on this? I may have a need to have this working in the future.
@Acconut

Acconut · 2023-12-19T12:35:39Z

Apologies, I wasn't aware that @manohar27 performed changes after my last review!

@mahyarmirrashed Feel free to continue, but did you have anything specific in mind that needs to be changed here?

mahyarmirrashed · 2023-12-19T15:03:33Z

@Acconut I have not taken a close look at the PR. Just wondering if there is a way that I can contribute/continue if needed. I might have a need for the checksum extension in about a month.

Acconut

Again, thanks very much for the work and apologies for losing track of this! I left a few comments if you or anybody else is interested in continuing this. It's on a very good track and we just need a few adjustments to get this ready.

Acconut · 2023-12-22T08:33:58Z

.babelrc

+			"forceAllTransforms": true
+		} ]
+	],
+	"plugins": ["@babel/transform-runtime"]


Could you explain why this plugin is necessary?

Acconut · 2023-12-22T08:37:34Z

demos/browser/demo.js

@@ -66,6 +66,7 @@ function startUpload() {
    endpoint,
    chunkSize,
    retryDelays: [0, 1000, 3000, 5000],
+    checksumAlgo: "SHA-256",


The tus protocol itself uses a naming conventional like sha1, so I would prefer if we could also use sha256 here.

Acconut · 2023-12-22T08:38:24Z

lib/browser/extensions/checksum.js

+class Checksum {
+  static supportedAlgorithms = ['SHA-1', 'SHA-256', 'SHA-384', 'SHA-512'];
+
+  constructor (algo = 'SHA-256') {


I don't think we need a default value here. If no algorithm or an unsupported one is supplied, we can just error out.

Acconut · 2023-12-22T08:41:43Z

lib/index.d.ts

@@ -129,3 +131,7 @@ export class DetailedError extends Error {
  originalResponse: HttpResponse
  causingError: Error
 }
+
+export interface Checksum {
+  getHexDigest(): string;


The Upload-Checksum encodes the checksum in base64, instead of hexadecimal. So we need to adjust the interface itself as well as the implementations. I am also wondering if the checksum provider should just return a byte array and let the main tus-js-client logic take care of encoding it into the right format.

Acconut · 2023-12-22T08:42:02Z

lib/index.d.ts

@@ -129,3 +131,7 @@ export class DetailedError extends Error {
  originalResponse: HttpResponse
  causingError: Error
 }
+
+export interface Checksum {
+  getHexDigest(): string;


The inferface definition is also missing the data argument.

Acconut · 2023-12-22T08:46:33Z

lib/index.d.ts


  urlStorage?: UrlStorage
  fileReader?: FileReader
  httpStack?: HttpStack
+  checksum?: Checksum;


I would prefer a name that aligns with fileReader for the checksum as well. Maybe we can call this option checksumProvider and the classes also ChecksumGenerator.

"Digest" would also likely be a better fit than checksum, although the protocol specification does not use that term, unfortunately.

Acconut · 2023-12-22T08:47:55Z

lib/upload.js

+              })
+              .catch((err) => {
+                log(err)
+                throw err


For error handling, please use _emitError.

Acconut · 2023-12-22T09:06:18Z

test/spec/test-browser-specific.js

@@ -787,4 +787,24 @@ describe('tus', () => {
      await assertUrlStorage(tus.defaultOptions.urlStorage)
    })
  })
+
+  describe('#Checksum', () => {


We would also need a full test to ensure the checksum header is added to the actual requests. Such a test should be placed in test-common, which includes tests shared between browsers and Node.js.

Let me know if you need help with getting this running :)

Acconut · 2023-12-22T09:07:00Z

lib/index.d.ts

@@ -129,3 +131,7 @@ export class DetailedError extends Error {
  originalResponse: HttpResponse
  causingError: Error
 }
+
+export interface Checksum {


Once we have finalized the interface and options, we should also add documentation. But that can wait for now.

mahyarmirrashed · 2023-12-30T22:40:49Z

I'll try to do some work on this in the new year. By the way @Acconut , should there be any documentation changes with this? Something to let people know how to use this feature?

Acconut · 2024-01-08T15:38:54Z

should there be any documentation changes with this? Something to let people know how to use this feature?

Yes, once we have settled on how the feature's API should work, we can also address the documentation.

I'll try to do some work on this in the new year.

Great! I am looking forward to your contributions :)

Manohar Srinivasa and others added 4 commits April 27, 2022 11:59

feat: checksum extension, wip

262d44f

test: added tests for checksum

9c0526c

fix: formatting

eb51375

Merge branch 'master' into feat/extension-checksum

f887195

manohar27 marked this pull request as draft May 30, 2022 05:34

Manohar Srinivasa added 2 commits October 26, 2022 12:31

feat - take algo from server response header

5e18e6f

Merge branch 'master' into feat/extension-checksum

a036309

manohar27 marked this pull request as ready for review October 26, 2022 07:17

Delete package-lock.json

05adba7

1token added a commit to cargomail-org/cargomail-tus-js-client that referenced this pull request Jan 8, 2023

client-side checksum -not finished yet

91656ac

partially copied from tus#347

Acconut requested changes Mar 26, 2023

View reviewed changes

Acconut reviewed Mar 26, 2023

View reviewed changes

test/spec/test-browser-specific.js Outdated Show resolved Hide resolved

feat - reverted OPTIONS call

5d353ea

manohar27 requested a review from Acconut April 12, 2023 09:03

merge - updated with latest main

bfc6179

manohar27 changed the title ~~Work In Progress - feat: checksum extension~~ feat: checksum extension Apr 26, 2023

Acconut requested changes Dec 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: checksum extension #347

feat: checksum extension #347

manohar27 commented Apr 27, 2022

Acconut commented Jul 25, 2022

manohar27 commented Oct 26, 2022

Acconut commented Oct 29, 2022

Acconut left a comment

Acconut Mar 26, 2023

manohar27 Apr 12, 2023

Acconut Mar 26, 2023

manohar27 Apr 12, 2023

manohar27 commented Apr 12, 2023

manohar27 commented Apr 12, 2023

manohar27 commented May 4, 2023

mahyarmirrashed commented Dec 19, 2023 •

edited

Acconut commented Dec 19, 2023

mahyarmirrashed commented Dec 19, 2023

Acconut left a comment

Acconut Dec 22, 2023

Acconut Dec 22, 2023

Acconut Dec 22, 2023

Acconut Dec 22, 2023

Acconut Dec 22, 2023

Acconut Dec 22, 2023

Acconut Dec 22, 2023

Acconut Dec 22, 2023

Acconut Dec 22, 2023

mahyarmirrashed commented Dec 30, 2023

Acconut commented Jan 8, 2024

feat: checksum extension #347

Are you sure you want to change the base?

feat: checksum extension #347

Conversation

manohar27 commented Apr 27, 2022

Acconut commented Jul 25, 2022

manohar27 commented Oct 26, 2022

Acconut commented Oct 29, 2022

Acconut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manohar27 commented Apr 12, 2023

manohar27 commented Apr 12, 2023

manohar27 commented May 4, 2023

mahyarmirrashed commented Dec 19, 2023 • edited

Acconut commented Dec 19, 2023

mahyarmirrashed commented Dec 19, 2023

Acconut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahyarmirrashed commented Dec 30, 2023

Acconut commented Jan 8, 2024

mahyarmirrashed commented Dec 19, 2023 •

edited