Uh oh!
There was an error while loading. Please reload this page.
Sync upstream samly v1.4.0 + fix SAML XML crashes and XXE - #8
Merged
Conversation
Handle the possibility that the RelayState is nil
* Manages the takeover of the project by dropbox * updates license copyright * removes unused deps * removes overly restrictive semantic versioning * updates documentation * updates to * experimental depedencies - used for testing and waiting for hex.pm control of project * updated esaml * ran formatter * finalize versioning to merge into main * update esaml target * update required elixir version since esaml requires >=OTP 21 Co-authored-by: KMC <kencross@dropbox.com>
Allow metadata to be specified inline rather than as file
dependency updates // bumps version in prep for release
Added Google to list of IdPs
errors were found at the given arguments:\n\n * 1st argument: the table identifier does not refer to an existing ETS table\n"} lib/samly/sp_data.ex:67: Samly.SpData.load_provider/1 is being returned due to absence of ETS table in esaml
…oding fix: remove double url encoding
Fix Esaml ETS initialisation issue
…turns sessions that are not expired.
Get attestation from ETS or Session now checks for expiry.
* add debug mode with raw response * debug page formatting cleanup
Previously, dialyzer objected to the use of data from these fields in a call to update the record. Luckily, :esaml provides types for these records.
hodakforce-pushed
the
sync-dropbox-upstream-CU-86cb5018f
branch
from
August 20, 2026 10:58
ce77d08 to
613e53eComparehodakforce-pushed
the
sync-dropbox-upstream-CU-86cb5018f
branch
from
August 20, 2026 11:05
613e53e to
b3cbde4Comparesiepet
approved these changes
Aug 20, 2026
hodakforce-pushed
the
sync-dropbox-upstream-CU-86cb5018f
branch
from
August 20, 2026 11:28
b3cbde4 to
f21d478CompareThe upstream v1.4.0 merge resolution flipped metadata/metadata_file in IdpData's defstruct and @type t. Field order has no behavior meaning; restored to the pre-merge order so diffs against our history read clean. debug_mode and the :esaml.idp_metadata()/:esaml.sp() typespec fixes from upstream are kept.
The opts() private helper predates the upstream v1.4.0 merge and was lost in the merge resolution; upstream inlines the same Application.get_env(:samly, Samly.Provider, []) call in both init/1 and refresh_providers/0. No behavior change.
redirect/3 must pass its destination to the location header verbatim: the URL from :esaml_binding.encode_http_redirect is already fully percent-encoded (uri_string:compose_query), so wrapping it in URI.encode/1 turns every % into %25 and corrupts SAMLRequest/RelayState at the IdP. Upstream removed exactly that double encoding in v1.4.0; these tests keep it from being reintroduced by someone assuming encoding is the safer default. Covers redirect/3 byte-preservation directly and a full HTTP-Redirect send_saml_request roundtrip with a reserved-character RelayState.
The ETS and Session stores each carried a byte-identical private validate_assertion_expiry/1. Hoisted into Samly.State.Store - the behaviour module both stores already implement - as a public documented function, so custom store implementations can reuse the same expiry check. Semantics unchanged: missing or unparsable notonorafter still yields nil.
dialyxir 1.4.3's warning formatter crashes with Protocol.UndefinedError on pattern_match_cov warnings (jeremyjh/dialyxir#574, fixed in 1.4.7 via the erlex bump). Lock-only bump: the mix.exs requirement stays ~> 1.0 (dev-only dep, the committed lock governs installs). With the formatter fixed, dialyzer reports two provably unreachable catch-all clauses; both are removed rather than ignored: - Samly.Helper.decode_idp_auth_resp: the else block was an identity clause plus a dead fallback - every reachable non-match is {:error, reason}, which a with without else returns unchanged. - Samly.SPHandler.consume_signin_response: the trailing _ -> 403 clause can never match; only {:error, _} and {:halted, _} reach the else. The clauses were meant as guards against esaml someday returning an unexpected shape, but the security decision is enforced by the positive patterns: anything unexpected fails them and raises WithClauseError - fail closed and loud instead of masking a broken contract as a routine 403. Dialyzer re-derives esaml's return types on every dep upgrade, so a shape change is caught at upgrade time, not runtime. Also covers the surviving non-debug 403 branch with a test (only the debug-mode branch was tested).
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
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.
What
Two things:
5f4c507) via a merge commit — brings in the uri double-encoding fix, esaml init fix, session-expiry enforcement (CVE-2024-25718), and the debug-mode raw-response view.:exit, which the existingrescuecould not catch, so a badSAMLResponsetook the request down. Decoding now catches:exitand returns{:error, {:invalid_response, ...}}. The catch is scoped only to payload decode, so unrelated exits are not swallowed.allow_entities: falseis set explicitly at every parse of attacker-influenced XML — both samly's own parse entry points and, via the pinned esaml dependency, the response/assertion/metadata parses inside esaml — so it holds regardless of the OTP xmerl default.Dependency and runtime
{:esaml, github: "recruitee/esaml", ref: "ef0cde2"}(mix.lock pins the full commit).allow_entitiesexists from OTP 26.0.1, somix.exsrequireselixir: "~> 1.19"(which needs OTP 26+), turning a too-old runtime into a compile error instead of a runtime failure. Struct-typing matches were added where Elixir 1.19's checker now requires them.Debug mode
The debug raw-response view HTML-escapes both the inspected error reason and the raw attacker-controlled response before rendering, so it can't be an injection vector.
Verification
Format clean,
mix compile --warnings-as-errorsclean, full suite green (53 tests, 0 failures, 2 skipped).