Skip to content

Commit d5cfabb

Browse files
camreevesclaude
andcommitted
test(saml): the "accepts signed assertion" case was passing vacuously
It asserted only that Location did not contain "/auth/failure". A rejection returns 400 with NO Location, so the expression evaluated to `nil.should_not be_true` — which passes. The case reported green while the assertion was in fact being refused, which is exactly backwards for a test whose job is to prove acceptance. Rewritten to assert the thing that matters: a UserAuthLookup is created. That correctly FAILS today, so it is marked pending against two independent crystal-saml bugs, both confirmed by experiment rather than inspection: 1. validate_signature serialises SignedInfo via XML::Node#to_xml, losing the namespace it INHERITS when the IdP declares xmldsig as a DEFAULT namespace on <Signature> — which is what real IdPs emit (mock-saml, Azure AD, ADFS). The canonical bytes then differ from the signer's and every real-world signature fails. Fix written and verified locally (false -> true against a live mock-saml assertion); awaiting push access. 2. sign_document writes its `uuid` ARGUMENT into Reference URI instead of the signed element's ID, so verify_digest can never resolve the referenced element. The library's own sign->verify round trip has never worked — verified against unmodified code, so it predates (1). Marking pending keeps the intent recorded without reporting a false green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 4e1d709 commit d5cfabb

1 file changed

Lines changed: 26 additions & 4 deletions

File tree

spec/controllers/saml_callbacks_spec.cr

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -218,15 +218,37 @@ module PlaceOS::Auth
218218
lookup.should be_nil
219219
}
220220

221-
it "accepts an assertion correctly signed by the configured IdP cert" do
221+
# PENDING until the `crystal-saml` shard can verify a signature.
222+
#
223+
# This case previously asserted only that `Location` did not contain
224+
# "/auth/failure". A rejection returns 400 with NO Location at all, so
225+
# the expression was `nil.should_not be_true` — which PASSES. It was
226+
# reporting green while the assertion was actually being refused.
227+
#
228+
# Rewritten to assert the thing that matters (an identity is
229+
# established), which correctly fails today because of two independent
230+
# bugs in `crystal-saml`, both confirmed by experiment:
231+
#
232+
# 1. `validate_signature` serialises `SignedInfo` with
233+
# `XML::Node#to_xml`, losing the namespace it INHERITS when the IdP
234+
# declares xmldsig as a DEFAULT namespace on <Signature> (what real
235+
# IdPs emit). The canonical bytes then differ from the signer's.
236+
# Fix written, pending push access to spider-gazelle/crystal-saml.
237+
# 2. `sign_document` writes its `uuid` ARGUMENT into `Reference URI`
238+
# rather than the signed element's actual ID, so `verify_digest`
239+
# can never resolve the referenced element — the library's own
240+
# sign->verify round trip has never worked.
241+
#
242+
# Un-pend once the shard is fixed; the assertion below is what we want.
243+
pending "accepts an assertion correctly signed by the configured IdP cert" do
222244
strat = signed_strat.call(true)
223245
email = "saml-ok-#{Random.rand(99999)}@localhost"
224246
xml = Spec::SamlFixtures.signed(build.call(strat, email))
225247

226-
result = post_assertion.call(strat, Spec::SamlFixtures.encode(xml))
248+
post_assertion.call(strat, Spec::SamlFixtures.encode(xml))
227249

228-
# a genuine assertion must NOT be bounced to the failure page
229-
result.headers["Location"]?.try(&.includes?("/auth/failure")).should_not be_true
250+
# positive assertion: a genuine assertion must establish an identity
251+
::PlaceOS::Model::UserAuthLookup.where(uid: email, provider: "adfs").first?.should_not be_nil
230252
ensure
231253
strat.try &.destroy
232254
end

0 commit comments

Comments
 (0)