The big refactor/fix branch #20
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AppState changes: * Construct an "OK" response with `x-vercel-verify` header on start-up, and store it in the `AppState`, so it can be cloned as needed. This avoids needing an `unwrap()` in the ingest handler, that could be triggered with a configuration error but only after the first request. Ingest handler fixes: * Respond to ingest requests without a signature with HTTP 200 OK and `x-vercel-verify` header, to match Vercel not sending this header, even when you set a custom secret during setup. Added a test case for this and a request with a valid signature but empty payload. * Verify the request payload before trying to decode it as UTF-8. * Return Unauthorized error whenever `x-vercel-signature` header is present, and is not in the expected format or doesn't pass HMAC checks, and don't send a `x-vercel-verify` header in those responses. * Avoid copying the payload by using `str::from_utf8()` on a `&[u8]`, rather than `::to_vec()` and passing it into a `String`. * Use `IntoResponse::into_response()` to construct responses with empty bodies and no special header. * Return Unprocessable Entity error whenever it doesn't parse correctly, rather than returning OK. * Removed many `unwrap()` calls which could crash the binary with a crafted request. * Added test cases for the expected response codes, and also for some requests that could crash because of `unwrap()`. Other handlers: * Make `root()` return a `StatusCode` directly, like `health_check()`. * Fix needless-returns. Tests: * Use `?` operator rather than `unwrap()` where possible. * Use arrays for constants rather than `vec!`. * Use `&str` and `b""` for constants where possible.
dacbd
requested changes
Sep 4, 2024
* Remove comment about signature handling, because that's in the README. * Add explicit `return` where a function also has a `return` elsewhere. * Move `hmac::Key` construction into `AppState::new`, which now accepts a key as `&[u8]`. * Add `AppState::sign_request_for_test_only` for signing requests, which is only for test cases. This means that all the HMAC code is in one place.
dacbd
approved these changes
Sep 4, 2024
There was a problem hiding this comment.
This is great, Thanks a bunch! If you have some spare time I'd love it if you reached out, I'd like to ask you a few rust questions sometime. (dabarnes2b@gmail.com)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Discussed in #8; this has a bunch of refactoring and fixes that are difficult to untangle.
AppState changes:
Construct an "OK" response with
x-vercel-verifyheader on start-up, and store it in theAppState, so it can be cloned as needed.This avoids needing an
unwrap()in the ingest handler, that could be triggered with a configuration error but only after the first request.Accept
vercel_secretas a&[u8], and move all the HMAC handling in there.Ingest handler:
Respond to ingest requests without a signature with HTTP 200 OK and
x-vercel-verifyheader, to match Vercel not sending this header, even when you set a custom secret during setup.Added a test case for this and a request with a valid signature but empty payload (in case they fix this).
Verify the request payload's HMAC before trying to decode it as UTF-8.
Return Unauthorized error whenever
x-vercel-signatureheader is present, and is not in the expected format or doesn't pass HMAC checks, and don't send ax-vercel-verifyheader in those responses.Avoid copying the payload by using
str::from_utf8()on a&[u8], rather than::to_vec()and passing it into aString.Use
IntoResponse::into_response()to construct responses with empty bodies and no special header.Return Unprocessable Entity error whenever it doesn't parse correctly, rather than returning OK.
Removed many
unwrap()calls which could crash the binary with a crafted request.Added test cases for the expected response codes, and also for some requests that could crash because of
unwrap().Other handlers:
Make
root()return aStatusCodedirectly, likehealth_check().Fix needless-returns.
Tests:
Use
?operator rather thanunwrap()where possible.Use arrays for constants rather than
vec!.Use
&strandb""for constants where possible.