Refine SLO RelayState validation#245
Conversation
d837597 to
b86c0cc
Compare
|
@fh1ch @bufferoverflow Hi 👋 Can you please have a look at this PR. |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances security for SAML Single Logout (SLO) by introducing a configurable RelayState validator. The default validator restricts RelayState values to relative paths starting with /, preventing open redirect vulnerabilities from absolute URLs, protocol-relative URLs, and other potentially unsafe values.
- Adds
:slo_relay_state_validatoroption with a secure default that validates RelayState parameters - Refactors RelayState handling logic to use the validator before accepting user-provided values
- Provides comprehensive test coverage for various RelayState validation scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/omniauth/strategies/saml.rb | Implements the default RelayState validator and refactors slo_relay_state method to validate input before use |
| spec/omniauth/strategies/saml_spec.rb | Adds extensive test coverage for RelayState validation scenarios including edge cases and custom validators |
| README.md | Documents the new :slo_relay_state_validator option with usage examples |
Comments suppressed due to low confidence (1)
lib/omniauth/strategies/saml.rb:187
- This assignment to arity is useless, since its value is never read.
arity = validator.arity
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* Provide a default validator that accepts only relative paths (starting with /) and rejects absolute URLs, protocol-relative, invalid URIs, and other schemes. * Add :slo_relay_state_validator option to override or change acceptance semantics. * Update documentation.
7be460d to
286a297
Compare
|
I rebased with the latest @fh1ch Let me know if you have other concerns that I should address. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
286a297 to
be0f3be
Compare
Simplified documentation for the `slo_relay_state_validator`.
fh1ch
left a comment
There was a problem hiding this comment.
@gerardo-navarro thanks a lot for the rework here, all good now from my end as well 💚
LGTM 👍
This pull request introduces a new option for validating SAML Single Logout (SLO) RelayState parameters, improving the security and configurability of logout flows in the OmniAuth SAML strategy. The changes include the addition of a default validator, new configuration options, and comprehensive test coverage for various RelayState scenarios.
Addresses #240 (comment)
SLO RelayState validation enhancements
:slo_relay_state_validatoroption to the OmniAuth SAML strategy, allowing custom validation of RelayState values during Single Logout flows. The default validator only accepts relative paths starting with/and rejects absolute URLs, protocol-relative URLs, and other potentially unsafe values. [1] [2]slo_relay_stateand related methods to use the validator for checking if a provided RelayState is acceptable, falling back to a default value if not.Test suite improvements
Documentation updates
README.mdto document the new:slo_relay_state_validatoroption, its default behavior, and usage examples, helping developers understand how to customize RelayState validation.Dependency update
uriinsaml.rbto support robust URI parsing for RelayState validation.Testing
bundle exec rspec