Repository navigation
test(ui): check a signed SAML redirect request by its query signature - #7562
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
gustavosbarreto
force-pushed
the
fix/e2e-saml-redirect-signature
branch
from
October 16, 2026 17:57
296b63a to
e3b7c9c
Compare
Member
Author
|
/review |
This comment has been minimized.
This comment has been minimized.
gustavosbarreto
force-pushed
the
fix/e2e-saml-redirect-signature
branch
from
October 16, 2026 18:03
e3b7c9c to
f494080
Compare
Member
Author
|
/review |
This comment has been minimized.
This comment has been minimized.
shellhub-io/cloud#2667 moved the signature of an HTTP-Redirect AuthnRequest from inside the XML to the query string, as the binding requires (saml-bindings-2.0, 3.4.4.1): the request carries no XML signature, and SigAlg and Signature sign SAMLRequest, RelayState and SigAlg as sent. The spec that turns request signing on still looked for an XML signature and has failed on every run since. The spec now asserts the request went to the redirect endpoint and verifies its query signature against the SP certificate, over the values still encoded as they came in the URL, rebuilt in the binding's order; decoding them first would hide an escaping mismatch between what was signed and what was sent. Only RSA-SHA256 is accepted, the one algorithm the SP signs with, so a change to the algorithm fails the spec instead of passing it.
Member
Author
|
/review |
gustavosbarreto
force-pushed
the
fix/e2e-saml-redirect-signature
branch
from
October 16, 2026 18:09
f494080 to
03e5287
Compare
|
Claude finished @gustavosbarreto's task in 1m 32s —— View job Code Review CompleteReviewed 2 files on two axes: Standards (repo rules, language conventions, tests, code smells) and Correctness (security and runtime defects). No new issues found. The code looks good as-is. The Spec axis did not run: this PR closes no issue and its commit has no |
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
1 participant
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 enterprise e2e fails on every run since shellhub-io/cloud#2667 merged:
saml.spec.ts"ShellHub signs its authentication requests when asked to" stops withexpected a signed SAML request.cloud#2667 fixed how ShellHub signs a request sent over the HTTP-Redirect binding. The request no longer carries an XML signature:
SigAlgandSignaturein the query signSAMLRequest,RelayStateandSigAlgas sent, which is what the binding requires (saml-bindings-2.0, 3.4.4.1). The spec still looked for the XML signature the old code embedded.The spec now:
The master runs since then went red on this test only (38938597194 and 38938705956). The run on 72502e7 shows green because its e2e job was skipped, not because the test passed.