Repository navigation
fix(client): Reject push requests with a missing or invalid signature - #836
Merged
untitaker merged 1 commit intoOct 29, 2026
Merged
Conversation
RequestSignatureServerInterceptor passed a failed signature check from the request deserializer to the servicer through a thread-local. grpcio's sync server deserializes on its polling thread and runs the servicer on a pool thread, so the servicer never saw the flag and calls with a missing or wrong sentry-signature got OK with an empty request. Return a marker from the deserializer instead and abort with UNAUTHENTICATED when the servicer gets it. The new test sends signed and unsigned calls through a real grpcio server. Signed-off-by: MinJun Choi <mj04300017@gmail.com>
untitaker
approved these changes
Oct 29, 2026
Member
|
ref STREAM-1099 was the same thing (reported by AI), but we were unable to produce a fix with low effort at the time, so it disappeared in the backlog. I'll just get this merged. ty for reporting! |
This file contains hidden or 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
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.
The push worker's
RequestSignatureServerInterceptorchecks the signature inside the request deserializer and hands the result to the servicer through a thread-local. That only works if both run on the same thread, and on grpcio's sync server they don't: the deserializer runs on the server's polling thread and the servicer on a pool thread. So the servicer never sees a failed check, and aPushTaskwith a missing or wrongsentry-signaturegets OK and goes through with an empty request.Now the deserializer returns a marker object when the check fails, and the servicer aborts with
UNAUTHENTICATEDwhen it gets one. The result travels with the request, so the threading no longer matters.The new test starts a real grpcio server with the interceptor and sends four calls: no signature, the wrong secret, a signature for a different body, and a valid one. On main the first three come back OK. With this change they get
UNAUTHENTICATEDand only the valid call reaches the handler.I reported this to security@sentry.io first, and they said they'd be happy to review a PR with the fix and a test.