relayState should be cleaned to prevent bogus redirects
Description
Environment
Activity
Scott Cantor June 23, 2009 at 12:46 PM
Closing after releases.
André Cruz August 18, 2008 at 10:10 AM
Great. Thanks.
I'm still using IDP 1.3.3 (and I think I will for some time) but I wouldn't want to display an error page.
A redirect to the SP HomeURL is perfect.
Scott Cantor August 18, 2008 at 10:04 AM
Scott Cantor August 18, 2008 at 9:59 AM
This came up because the cookie-based state mechanism changed to fix a problem with frames, and the RelayState value is now cookie:key instead of just cookie. I didn't code for that and handle the old case. I can fix that, but by the time it shows up, I expect most of the bookmarks will be gone anyway.
Scott Cantor August 18, 2008 at 9:46 AM
If there are cases where it's not being cleared, that may be intentional, because it generally does get cleared unless it can't be for some reason. It should never be redirecting to "cookie" anyway. I think it's misinterpreting the value in some cases, that's probably the real bug.
This really needs to be prevented by the IdP anyway. That's why we supply a time parameter. The old IdP has this feature built-in, and can block stale requests if you tell it to, and display a nice error message. I don't know if the new IdP can, in which case that would be a bug there I think.
Your fix won't work, though, it cannot be cleared in every case.
I started to notice errors like this:
http://blogs.sapo.pt/Shibboleth.sso/SAML/cookie
Shibboleth handler invoked at an unconfigured location.
After some detective work it seems that some of our users have bookmarked the IDP login URL with TARGET=cookie. What this means is that after they login and the Artifact is resolved by the SP, the cookie that should be present at the SP to send the browser on their way is not there. The logic at shibsp/handler/impl/AbstractHandler.cpp:recoverRelayState() should clear invalid values otherwise people will be redirected to invalid URLs.
Maybe this will suffice:
// Look for StorageService-backed state of the form "ss:SSID:key".
const char* state = relayState.c_str();
+ relayState.erase();