[cpp-sp] branch main updated: Handling of discovery return URL, add some constants.
Scott Cantor
cantor.2 at osu.edu
Mon Jun 30 16:59:15 UTC 2025
This is an automated email from the git hooks/post-receive script.
scantor pushed a commit to branch main
in repository cpp-sp.
View the commit online:
http://git.shibboleth.net/view/?p=cpp-sp.git;a=commit;h=7fc7d282ac94dc8f61a2be3e1187557b6b96148a
The following commit(s) were added to refs/heads/main by this push:
new 7fc7d282 Handling of discovery return URL, add some constants.
7fc7d282 is described below
commit 7fc7d282ac94dc8f61a2be3e1187557b6b96148a
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Mon Jun 30 12:58:57 2025 -0400
Handling of discovery return URL, add some constants.
---
configure.ac | 16 +-------
shibsp/exceptions.cpp | 4 ++
shibsp/exceptions.h | 5 +++
shibsp/handler/impl/SessionInitiator.cpp | 69 +++++++++++++++++++++++++++-----
shibsp/handler/impl/TokenConsumer.cpp | 6 +--
5 files changed, 71 insertions(+), 29 deletions(-)
diff --git a/configure.ac b/configure.ac
index 61717ccc..1cd599c3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1,4 +1,4 @@
-AC_PREREQ([2.50])
+AC_PREREQ([2.72])
AC_INIT([shibboleth],[4.0.0],[https://shibboleth.atlassian.net/jira],[shibboleth-sp])
AC_CONFIG_SRCDIR(shibsp)
AC_CONFIG_AUX_DIR(build-aux)
@@ -50,20 +50,6 @@ AC_SEARCH_LIBS([dlopen],[dl],,[AC_MSG_ERROR([cannot find dlopen() function])])
AC_SUBST([dlopen_LIBS],[$LIBS])
AX_RESTORE_FLAGS
-
-AC_CACHE_CHECK([for SOCK_CLOEXEC support], [shib_cv_sock_cloexec],
-[AC_TRY_RUN([
-#include <sys/types.h>
-#include <sys/socket.h>
-int main()
-{
-return socket(AF_INET, SOCK_STREAM|SOCK_CLOEXEC, 0) == -1;
-}], [shib_cv_sock_cloexec=yes], [shib_cv_sock_cloexec=no], [shib_cv_sock_cloexec=no])])
-
-if test "$shib_cv_sock_cloexec" = "yes"; then
- AC_DEFINE([HAVE_SOCK_CLOEXEC], 1, [Define if the SOCK_CLOEXEC flag is supported])
-fi
-
AX_PTHREAD(,[AC_MSG_ERROR([unable to find pthreads, currently this is required])])
AC_LANG([C++])
diff --git a/shibsp/exceptions.cpp b/shibsp/exceptions.cpp
index 71fcee24..da709463 100644
--- a/shibsp/exceptions.cpp
+++ b/shibsp/exceptions.cpp
@@ -31,6 +31,10 @@
using namespace shibsp;
using namespace std;
+const char AgentException::HANDLER_TYPE_PROP_NAME[] = "handlerType";
+const char AgentException::PASSIVE_PROP_NAME[] = "passive";
+const char AgentException::TARGET_PROP_NAME[] = "target";
+
AgentException::AgentException(const char* msg) : m_status(HTTPResponse::SHIBSP_HTTP_STATUS_ERROR)
{
if (msg)
diff --git a/shibsp/exceptions.h b/shibsp/exceptions.h
index 6c6276c8..0a1d607c 100644
--- a/shibsp/exceptions.h
+++ b/shibsp/exceptions.h
@@ -142,6 +142,11 @@ namespace shibsp {
*/
void log(const SPRequest& request, Priority::Value priority=Priority::SHIB_ERROR) const;
+ // Defined properties.
+ static const char HANDLER_TYPE_PROP_NAME[];
+ static const char PASSIVE_PROP_NAME[];
+ static const char TARGET_PROP_NAME[];
+
private:
int m_status;
std::string m_msg;
diff --git a/shibsp/handler/impl/SessionInitiator.cpp b/shibsp/handler/impl/SessionInitiator.cpp
index 7460a14c..13f84b83 100644
--- a/shibsp/handler/impl/SessionInitiator.cpp
+++ b/shibsp/handler/impl/SessionInitiator.cpp
@@ -43,7 +43,11 @@ namespace {
pair<bool,long> run(SPRequest& request, bool isHandler) const;
private:
+ // This sets up the proper query string for discovery to work properly.
+ void populateDiscoveryReturnURL(SPRequest& request, bool isHandler, string& returnURL) const;
+
string m_path;
+ bool m_discoveryEnabled;
vector<string> m_remotedHeaders;
vector<string> m_requestMapperSettings;
vector<string> m_querySettings;
@@ -57,9 +61,17 @@ namespace shibsp {
};
SessionInitiator::SessionInitiator(const ptree& pt, const char* path)
- : AbstractHandler(pt), m_path(path), m_remotedHeaders({ "Cookie" })
+ : AbstractHandler(pt), m_path(path), m_discoveryEnabled(true), m_remotedHeaders({ "Cookie" })
{
- const char* settings = getString("requestMapperSettings");
+ static const char DISCOVERY_ENABLED_PROP_NAME[] = "discoveryEnabled";
+ static const char REQUEST_MAPPER_SETTINGS_PROP_NAME[] = "requestMapperSettings";
+ static const char QUERY_SETTINGS_PROP_NAME[] = "querySettings";
+
+ static bool DISCOVERY_ENABLED_PROP_DEFAULT = true;
+
+ m_discoveryEnabled = getBool(DISCOVERY_ENABLED_PROP_NAME, DISCOVERY_ENABLED_PROP_DEFAULT);
+
+ const char* settings = getString(REQUEST_MAPPER_SETTINGS_PROP_NAME);
if (settings) {
split_to_container(m_requestMapperSettings, settings);
}
@@ -78,7 +90,7 @@ SessionInitiator::SessionInitiator(const ptree& pt, const char* path)
};
}
- settings = getString("querySettings");
+ settings = getString(QUERY_SETTINGS_PROP_NAME);
if (settings) {
split_to_container(m_querySettings, settings);
}
@@ -115,9 +127,8 @@ pair<bool,long> SessionInitiator::run(SPRequest& request, bool isHandler) const
}
}
else {
- // target will come from query string, map, or handler or fall back to this request.
- // TODO: shouldm't this fall back to homeURL?
- target = getString("target", request, request.getRequestURL());
+ // target will come from query string, map, or handler or fall back to homeURL.
+ target = getString("target", request, request.getRequestSettings().first->getString("homeURL", "/"));
// handler is derived from the target resource.
handlerBaseURL = request.getHandlerURL(target.c_str());
@@ -147,10 +158,9 @@ pair<bool,long> SessionInitiator::run(SPRequest& request, bool isHandler) const
RequestMapper::APPLICATION_ID_PROP_NAME, RequestMapper::APPLICATION_ID_PROP_DEFAULT));
// Will be set unless discovery was already attempted.
- if (!handler.empty()) {
+ if (m_discoveryEnabled && !handler.empty()) {
// Decorate the handler URL with the signal parameter and then any recognized/allowed custom parameters.
- handler += "?DS=1";
- // TODO: the other parameters
+ populateDiscoveryReturnURL(request, isHandler, handler);
input.addmember("disco_return_url").string(AgentConfig::getConfig().getURLEncoder().encode(handler.c_str()));
}
@@ -197,7 +207,7 @@ pair<bool,long> SessionInitiator::run(SPRequest& request, bool isHandler) const
catch (exception& ex) {
AgentException* agent_ex = dynamic_cast<AgentException*>(&ex);
if (agent_ex) {
- agent_ex->addProperty("handlerType", SESSION_INITIATOR_HANDLER);
+ agent_ex->addProperty(AgentException::HANDLER_TYPE_PROP_NAME, SESSION_INITIATOR_HANDLER);
}
// If it's a handler operation, and isPassive is used or returnOnError is set, we trap the error.
@@ -211,7 +221,7 @@ pair<bool,long> SessionInitiator::run(SPRequest& request, bool isHandler) const
request.warn(ex.what());
const char* error_target = agent_ex ? agent_ex->getProperty("target") : nullptr;
// Make sure the target isn't the same as this handler, to avoid a loop.
- if (error_target && strcmp(error_target, handler.c_str())) {
+ if (error_target && strstr(error_target, handlerBaseURL) != error_target) {
request.info("trapping SessionInitiator failure and returning to target location");
request.limitRedirect(error_target);
return make_pair(true, request.sendRedirect(error_target));
@@ -221,3 +231,40 @@ pair<bool,long> SessionInitiator::run(SPRequest& request, bool isHandler) const
throw;
}
}
+
+void SessionInitiator::populateDiscoveryReturnURL(SPRequest& request, bool isHandler, string& returnURL) const
+{
+ returnURL += "?DS=1";
+
+ const URLEncoder& encoder = AgentConfig::getConfig().getURLEncoder();
+
+ // In the old SP, we did this by conducting surgery on the query string to elide the target parameter.
+ // This is not only nasty, but not even correct since the name of the parameter could be encoded inside
+ // the query string to defeat that check, so the new code is going to build up a new query string by
+ // hand and re-encode all the parrameters it decides to allow.
+
+ // We have parameter name sets defined for both the query parameter case (isHandler is true) and the
+ // RequestMap case (isHandler is false). That case is necessary so that settings applicable to the
+ // original target parameter get preserved even though the handler is the subsequently "active" URL
+ // mapped into the configuration.
+
+ if (isHandler) {
+ for (const string& opt : m_querySettings) {
+ const char* optval = request.getParameter(opt.c_str());
+ if (optval) {
+ returnURL = returnURL + '&' + opt + '=' + encoder.encode(optval);
+ }
+ }
+ }
+ else {
+ // Preserve designated request settings on the URL.
+ const PropertySet* props = request.getRequestSettings().first;
+ for (const string& opt : m_requestMapperSettings) {
+ const char* optval = props->getString(opt.c_str());
+ if (optval) {
+ returnURL = returnURL + '&' + opt + '=' + encoder.encode(optval);
+ }
+ }
+ }
+
+}
diff --git a/shibsp/handler/impl/TokenConsumer.cpp b/shibsp/handler/impl/TokenConsumer.cpp
index c80ae356..c2fbba67 100644
--- a/shibsp/handler/impl/TokenConsumer.cpp
+++ b/shibsp/handler/impl/TokenConsumer.cpp
@@ -136,7 +136,7 @@ pair<bool,long> TokenConsumer::run(SPRequest& request, bool isHandler) const
catch (exception& ex) {
AgentException* agent_ex = dynamic_cast<AgentException*>(&ex);
if (agent_ex) {
- agent_ex->addProperty("handlerType", TOKEN_CONSUMER_HANDLER);
+ agent_ex->addProperty(AgentException::HANDLER_TYPE_PROP_NAME, TOKEN_CONSUMER_HANDLER);
}
// THis is a mess to allow for "ignoring" errors during passive SSO and routing back
@@ -146,10 +146,10 @@ pair<bool,long> TokenConsumer::run(SPRequest& request, bool isHandler) const
// When the cache throws, the error typically would not carry that information but the
// output would have.
- const char* passive = agent_ex ? agent_ex->getProperty("passive") : nullptr;
+ const char* passive = agent_ex ? agent_ex->getProperty(AgentException::PASSIVE_PROP_NAME) : nullptr;
if (wasPassive || (passive && !strcmp(passive, "1"))) {
agent_ex->log(request, Priority::SHIB_WARN);
- const char* error_target = target.empty() ? agent_ex->getProperty("target") : target.c_str();
+ const char* error_target = target.empty() ? agent_ex->getProperty(AgentException::TARGET_PROP_NAME) : target.c_str();
// TODO: either recover POST data or clean up recovery state?
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list