-
-
Notifications
You must be signed in to change notification settings - Fork 598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(middleware/etag): generate etag hash value from all chunks #3604
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3604 +/- ##
==========================================
- Coverage 94.71% 89.80% -4.92%
==========================================
Files 158 159 +1
Lines 9555 10120 +565
Branches 2813 2932 +119
==========================================
+ Hits 9050 9088 +38
- Misses 505 1032 +527 ☔ View full report in Codecov by Sentry. |
@@ -8,7 +8,7 @@ type Algorithm = { | |||
alias: string | |||
} | |||
|
|||
type Data = string | boolean | number | object | ArrayBufferView | ArrayBuffer | ReadableStream | |||
type Data = string | boolean | number | object | ArrayBufferView | ArrayBuffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type Data = string | boolean | number | object | ArrayBufferView | ArrayBuffer | |
type Data = string | boolean | number | object |
I think we can remove ArrayBuffer
, ArrayBufferView
, etc. since they are subtypes of object
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out! I see.
It's true that it allows any object
(including those that cannot be JSON.stringify
), not just ArrayBufferView
or ArrayBuffer
.
Semantically, I think the following changes are appropriate.
diff --git a/src/utils/crypto.ts b/src/utils/crypto.ts
index 9ffb8310..68fcf8ca 100644
--- a/src/utils/crypto.ts
+++ b/src/utils/crypto.ts
@@ -3,12 +3,14 @@
* Crypto utility.
*/
+import type { JSONValue } from './types'
+
type Algorithm = {
name: string
alias: string
}
-type Data = string | boolean | number | object | ArrayBufferView | ArrayBuffer
+type Data = string | boolean | number | JSONValue | ArrayBufferView | ArrayBuffer
export const sha256 = async (data: Data): Promise<string | null> => {
const algorithm: Algorithm = { name: 'SHA-256', alias: 'sha256' }
However, there is a slight possibility (I think it's very slight) that it could affect compatibility, so I think it would be better to consider it in a separate pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'll try it.
Co-authored-by: Yusuke Wada <[email protected]>
Looks good to me! I'll merge this. Thanks! |
…s#3604) * fix(middleware/etag): generate etag hash value from all chunks * refactor(middleware/etag): returns immediately if digest cannot be generated. * refactor(utils/crypto): remove ReadableStream support * test(middleware/etag): add tests for empty stream and null body Co-authored-by: Yusuke Wada <[email protected]> --------- Co-authored-by: Yusuke Wada <[email protected]>
Fix #3536
There are several ways to generate digests from ReadableStream, but here we have used a method that includes the previous hash value for each read, so that unique digests are generated for the same continuous data.
Even if the overall data is the same, if the number of chunks read is different, a different digest will be generated, but since it is unlikely that the same architecture (server) will have different data divisions, I think it will work effectively in practice.
It uses only the web standard
crypto.subtle
, and is characterized by its low memory load even for large-sized files.Other approaches
Self-implemented incremental SHA1 generator
We can write our own incremental sha1 generator in TypeScript, and using this it is possible to generate the correct sha1 for the entire data. However, I did not adopt this because an exact sha1 is not necessary for etags, and it is also slow.
incremental_sha1.ts
benchmark
Using "node:crypto"
“node:crypto” can be used in runtimes other than node, so we can use it to incrementally add data and generate sha1.
https://nodejs.org/api/crypto.html
However, I didn't adopt it because I thought that ‘node:*’ should not be used where
crypto.subtle
is sufficient.The author should do the following, if applicable
bun run format:fix && bun run lint:fix
to format the code