diff --git a/changelog.md b/changelog.md index 282d78f..bbe3fc7 100644 --- a/changelog.md +++ b/changelog.md @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Namespace the SAML request cache key by application name. `samlRequestCacheName` exists so the pending + AuthnRequest IDs can live in a distributed cache behind a load balancer, but such a cache is shared by + every application pointed at it, so two applications on one region accepted each other's request IDs as + pending. Surfaced on a multi-tenant deployment serving the same code under a different `this.name` per + tenant. + ## [3.0.0] - 2026-08-25 ### Added diff --git a/models/SAMLRequestTracker.cfc b/models/SAMLRequestTracker.cfc index 5935c1b..9e9380a 100644 --- a/models/SAMLRequestTracker.cfc +++ b/models/SAMLRequestTracker.cfc @@ -7,7 +7,8 @@ component singleton { variables.cacheKeyPrefix = "cbsso:saml-request:"; public void function onDIComplete(){ - variables.requestCache = variables.cachebox.getCache( variables.cacheName ); + variables.requestCache = variables.cachebox.getCache( variables.cacheName ); + variables.cacheKeyPrefix = buildCacheKeyPrefix(); } public void function remember( required string requestId ){ @@ -35,4 +36,30 @@ component singleton { return variables.cacheKeyPrefix & arguments.requestId; } + /** + * Namespaced by application. A distributed cache - which is what `samlRequestCacheName` is for, and + * what the documentation asks for behind a load balancer - is shared by every application pointed at + * it, and CacheBox regions in separate applications resolve to one keyspace there. Two applications + * on one Redis region therefore accepted each other's request IDs as pending. + * + * That is not a way in on its own: the response still has to carry an assertion satisfying this + * provider's expected issuer, audience and recipient. It is the difference between "this application + * issued this AuthnRequest" and "some application sharing this cache did", which is the whole point + * of tracking them. + * + * The application name rather than a ColdBox setting, because several applications commonly share one + * ColdBox configuration - a multi-tenant deployment serving the same code under a different + * `this.name` per tenant is the case that surfaced this - and `AppName` would be identical across + * every one of them. + */ + private string function buildCacheKeyPrefix(){ + var applicationName = getApplicationMetadata().name ?: ""; + + if ( !len( trim( applicationName ) ) ) { + return "cbsso:saml-request:"; + } + + return "cbsso:" & trim( applicationName ) & ":saml-request:"; + } + } diff --git a/test-harness/tests/specs/SAMLRequestTrackerSpec.cfc b/test-harness/tests/specs/SAMLRequestTrackerSpec.cfc index e7e1d47..32393e3 100644 --- a/test-harness/tests/specs/SAMLRequestTrackerSpec.cfc +++ b/test-harness/tests/specs/SAMLRequestTrackerSpec.cfc @@ -21,6 +21,22 @@ component extends="coldbox.system.testing.BaseTestCase" { expect( variables.tracker.isPending( variables.requestId ) ).toBeTrue(); } ); + it( "namespaces the cache key by application", function(){ + // a distributed cache is shared by every application pointed at it, so an unnamespaced key + // lets one application's AuthnRequest ID read as pending in another + variables.tracker.remember( variables.requestId ); + + var applicationName = getApplicationMetadata().name; + var cache = getInstance( "cachebox" ).getCache( + getInstance( dsl = "coldbox:setting:samlRequestCacheName@cbsso" ) + ); + + expect( cache.lookupQuiet( "cbsso:#applicationName#:saml-request:#variables.requestId#" ) ).toBeTrue(); + expect( cache.lookupQuiet( "cbsso:saml-request:#variables.requestId#" ) ).toBeFalse( + "an unnamespaced key would collide with every other application on a shared cache" + ); + } ); + it( "consumes a request ID only once", function(){ variables.tracker.remember( variables.requestId );