[cpp-sp] branch main updated: Refactor redirect limiting code to be internal.
Codeberg
noreply at shibboleth.net
Fri Jun 5 16:41:19 UTC 2026
This is an automated email from the git hooks/post-receive script.
codeberg pushed a commit to branch main
in repository cpp-sp.
View the commit online:
https://codeberg.org/Shibboleth/cpp-sp/commit/5f03e55323b946a68a054cc24fbe9448cf545b66
The following commit(s) were added to refs/heads/main by this push:
new 5f03e553 Refactor redirect limiting code to be internal.
5f03e553 is described below
commit 5f03e55323b946a68a054cc24fbe9448cf545b66
Author: Scott Cantor <scott at restingparrotsoftware.com>
AuthorDate: Fri Jun 5 12:40:59 2026 -0400
Refactor redirect limiting code to be internal.
---
apache/mod_shib4.cpp | 3 +-
fastcgi/shibauthorizer.cpp | 4 +-
fastcgi/shibresponder.cpp | 4 +-
iis/IIS7Request.cpp | 4 +-
shibsp/AbstractSPRequest.cpp | 53 +++++++++++++++----------
shibsp/AbstractSPRequest.h | 11 +++--
shibsp/SPRequest.h | 9 -----
shibsp/handler/impl/AbstractHandler.cpp | 5 +--
shibsp/handler/impl/AttributeCheckerHandler.cpp | 7 +---
shibsp/handler/impl/LogoutConsumer.cpp | 3 +-
shibsp/handler/impl/LogoutInitiator.cpp | 3 +-
shibsp/handler/impl/SessionInitiator.cpp | 3 +-
shibsp/handler/impl/TokenConsumer.cpp | 13 +-----
shibsp/io/HTTPResponse.h | 9 ++++-
tests/DummyRequest.h | 2 +-
15 files changed, 58 insertions(+), 75 deletions(-)
diff --git a/apache/mod_shib4.cpp b/apache/mod_shib4.cpp
index 6797c670..5e2fd7ba 100644
--- a/apache/mod_shib4.cpp
+++ b/apache/mod_shib4.cpp
@@ -498,8 +498,7 @@ public:
return status;
return DONE;
}
- long sendRedirect(const char* url) {
- url = sanitizeURL(url);
+ long doRedirect(const char* url) {
apr_table_set(m_req->headers_out, "Location", url);
if (getRequestSettings().first->getBool(
diff --git a/fastcgi/shibauthorizer.cpp b/fastcgi/shibauthorizer.cpp
index 3db6b4bc..a6c57c29 100644
--- a/fastcgi/shibauthorizer.cpp
+++ b/fastcgi/shibauthorizer.cpp
@@ -228,9 +228,7 @@ public:
return SHIB_RETURN_DONE;
}
- long sendRedirect(const char* url) {
- url = sanitizeURL(url);
-
+ long doRedirect(const char* url) {
string hdr=string("Status: 302 Please Wait\r\nLocation: ") + url + "\r\n"
"Content-Type: text/html\r\n"
"Content-Length: 40\r\n";
diff --git a/fastcgi/shibresponder.cpp b/fastcgi/shibresponder.cpp
index 0276e87f..5fb1bdf5 100644
--- a/fastcgi/shibresponder.cpp
+++ b/fastcgi/shibresponder.cpp
@@ -189,9 +189,7 @@ public:
return SHIB_RETURN_DONE;
}
- long sendRedirect(const char* url) {
- url = sanitizeURL(url);
-
+ long doRedirect(const char* url) {
string hdr=string("Status: 302 Please Wait\r\nLocation: ") + url + "\r\n"
"Content-Type: text/html\r\n"
"Content-Length: 40\r\n";
diff --git a/iis/IIS7Request.cpp b/iis/IIS7Request.cpp
index a9c5c530..02716ed0 100644
--- a/iis/IIS7Request.cpp
+++ b/iis/IIS7Request.cpp
@@ -464,10 +464,8 @@ void IIS7Request::setResponseHeader(const char* name, const char* value, bool re
}
}
-long IIS7Request::sendRedirect(const char* url)
+long IIS7Request::doRedirect(const char* url)
{
- url = sanitizeURL(url);
-
if (getRequestSettings().first->getBool(
RequestMapper::EXPIRE_REDIRECTS_PROP_NAME, RequestMapper::EXPIRE_REDIRECTS_PROP_DEFAULT)) {
setResponseHeader("Expires", "Wed, 01 Jan 1997 12:00:00 GMT", true);
diff --git a/shibsp/AbstractSPRequest.cpp b/shibsp/AbstractSPRequest.cpp
index 39d4235f..46c566d8 100644
--- a/shibsp/AbstractSPRequest.cpp
+++ b/shibsp/AbstractSPRequest.cpp
@@ -538,7 +538,7 @@ bool AbstractSPRequest::isPriorityEnabled(Priority::Value level) const
return m_log.isPriorityEnabled(level);
}
-const char* AbstractSPRequest::sanitizeURL(const char* url)
+long AbstractSPRequest::sendRedirect(const char* url, bool limit)
{
if (!url) {
throw domain_error("URL was null");
@@ -551,32 +551,41 @@ const char* AbstractSPRequest::sanitizeURL(const char* url)
}
}
- ch = strchr(url, ':');
- if (!ch) {
- throw domain_error("URL is missing a colon where expected; improper URL encoding?");
+ string holder;
+ if (*url == '/') {
+ // Compute a URL to the root of the site and prefix the relative path with it.
+ const char* scheme = getScheme();
+ holder = string(scheme) + "://" + getHostname();
+ if (!isDefaultPort()) {
+ holder += ":" + boost::lexical_cast<string>(getPort());
+ }
+ holder += url;
+ // Reset parameter to point to internal copy.
+ url = holder.c_str();
}
- string s(url, ch - url);
-
- for (const string& scheme : getAllowedSchemes()) {
- if (strcasecmp(s.c_str(), scheme.c_str()) == 0) {
- // Checks out, but absolutize if necessary.
- if (*url == '/') {
- // Compute a URL to the root of the site.
- const char* scheme = getScheme();
- m_absoluteHolder = string(scheme) + "://" + getHostname();
- if (!isDefaultPort()) {
- m_absoluteHolder += ":" + boost::lexical_cast<string>(getPort());
- }
- m_absoluteHolder += url;
- return m_absoluteHolder.c_str();
- }
- else {
- return url;
+ else {
+ // Absolute now, so check for acceptable scheme.
+ ch = strchr(url, ':');
+ if (!ch) {
+ throw domain_error("URL is missing a colon where expected; improper URL encoding?");
+ }
+ bool valid = false;
+ string s(url, ch - url);
+ for (const string& scheme : getAllowedSchemes()) {
+ if (strcasecmp(s.c_str(), scheme.c_str()) == 0) {
+ valid = true;
}
}
+ if (!valid) {
+ throw domain_error("URL contains invalid scheme.");
+ }
+ }
+
+ if (limit) {
+ limitRedirect(url);
}
- throw domain_error("URL contains invalid scheme.");
+ return doRedirect(url);
}
void SPRequest::debug(const string& msg) const
diff --git a/shibsp/AbstractSPRequest.h b/shibsp/AbstractSPRequest.h
index 85bf5d97..137d1be1 100644
--- a/shibsp/AbstractSPRequest.h
+++ b/shibsp/AbstractSPRequest.h
@@ -84,10 +84,14 @@ namespace shibsp {
const std::map<std::string,std::string>& getCookies() const;
const char* getHandlerURL(const char* resource=nullptr) const;
std::string getNotificationURL(unsigned int index) const;
- void limitRedirect(const char* url) const;
std::string getSecureHeader(const char* name) const;
void setAuthType(const char* authtype);
+
+ // Calls doRedirect to perform the actual operation after
+ // sanitzing the URL as required.
+ long sendRedirect(const char* url, bool limit=false);
+
void log(Priority::Value level, const std::exception& ex) const;
void log(Priority::Value level, const std::string& msg) const;
void log(Priority::Value level, const char* formatString, va_list args) const;
@@ -102,7 +106,7 @@ namespace shibsp {
*
* @return sanitized and possibly altered URL
*/
- const char* sanitizeURL(const char* url);
+ virtual long doRedirect(const char* url)=0;
/**
* Gets the transformed header name constructed from a raw input name by transforming
@@ -120,6 +124,7 @@ namespace shibsp {
private:
static std::vector<std::string> m_allowedSchemes;
+ void limitRedirect(const char* url) const;
Category& m_log;
Agent& m_agent;
@@ -130,8 +135,6 @@ namespace shibsp {
mutable std::string m_handlerURL;
mutable std::unique_ptr<CGIParser> m_parser;
mutable std::map<std::string,std::string> m_cookieMap;
- // Holds URL when promoted to absolute.
- std::string m_absoluteHolder;
};
#if defined (_MSC_VER)
diff --git a/shibsp/SPRequest.h b/shibsp/SPRequest.h
index 8b8f6980..d7800c99 100644
--- a/shibsp/SPRequest.h
+++ b/shibsp/SPRequest.h
@@ -118,15 +118,6 @@ namespace shibsp {
*/
virtual std::string getNotificationURL(unsigned int index) const=0;
- /**
- * Checks a proposed redirect URL against policy settings for legal redirects,
- * such as same-host restrictions or allowed domains, and raises an exception
- * in the event of a violation.
- *
- * @param url an absolute URL to validate
- */
- virtual void limitRedirect(const char* url) const=0;
-
/**
* Returns a non-spoofable request header value, if possible.
* Platforms that support environment export can redirect header
diff --git a/shibsp/handler/impl/AbstractHandler.cpp b/shibsp/handler/impl/AbstractHandler.cpp
index 61b82979..7ae09e6e 100644
--- a/shibsp/handler/impl/AbstractHandler.cpp
+++ b/shibsp/handler/impl/AbstractHandler.cpp
@@ -174,10 +174,7 @@ pair<bool,long> AbstractHandler::unwrapResponse(SPRequest& request, DDF& wrapped
h = http["redirect"];
if (h.isstring()) {
- if (limitRedirect) {
- request.limitRedirect(h.string());
- }
- return make_pair(true, request.sendRedirect(h.string()));
+ return make_pair(true, request.sendRedirect(h.string(), limitRedirect));
}
h = http["response"];
diff --git a/shibsp/handler/impl/AttributeCheckerHandler.cpp b/shibsp/handler/impl/AttributeCheckerHandler.cpp
index a8a9acc8..a5b21518 100644
--- a/shibsp/handler/impl/AttributeCheckerHandler.cpp
+++ b/shibsp/handler/impl/AttributeCheckerHandler.cpp
@@ -113,10 +113,7 @@ pair<bool,long> AttributeCheckerHandler::run(SPRequest& request, bool isHandler)
if (!returnURL) {
returnURL = request.getParameter("target");
}
- if (returnURL) {
- request.limitRedirect(returnURL);
- }
- else {
+ if (!returnURL) {
returnURL = request.getRequestSettings().first->getString(
RequestMapper::HOME_URL_PROP_NAME, RequestMapper::HOME_URL_PROP_DEFAULT);
}
@@ -154,7 +151,7 @@ pair<bool,long> AttributeCheckerHandler::run(SPRequest& request, bool isHandler)
}
if (checked) {
- return make_pair(true, request.sendRedirect(returnURL));
+ return make_pair(true, request.sendRedirect(returnURL, true));
}
if (m_flushSession && session) {
diff --git a/shibsp/handler/impl/LogoutConsumer.cpp b/shibsp/handler/impl/LogoutConsumer.cpp
index 86756b01..621fbb2c 100644
--- a/shibsp/handler/impl/LogoutConsumer.cpp
+++ b/shibsp/handler/impl/LogoutConsumer.cpp
@@ -236,8 +236,7 @@ pair <bool,long> LogoutConsumer::completeLogout(SPRequest& request, bool removeS
dest = getHomeURL(request);
}
- request.limitRedirect(dest);
- return make_pair(true, request.sendRedirect(dest));
+ return make_pair(true, request.sendRedirect(dest, true));
}
const char* LogoutConsumer::getHomeURL(SPRequest& request) const
diff --git a/shibsp/handler/impl/LogoutInitiator.cpp b/shibsp/handler/impl/LogoutInitiator.cpp
index a5824f3b..a2759328 100644
--- a/shibsp/handler/impl/LogoutInitiator.cpp
+++ b/shibsp/handler/impl/LogoutInitiator.cpp
@@ -132,6 +132,5 @@ pair<bool,long> LogoutInitiator::run(SPRequest& request, bool isHandler) const
}
}
- request.limitRedirect(dest);
- return make_pair(true, request.sendRedirect(dest));
+ return make_pair(true, request.sendRedirect(dest, true));
}
diff --git a/shibsp/handler/impl/SessionInitiator.cpp b/shibsp/handler/impl/SessionInitiator.cpp
index a6efb4c0..b8f48709 100644
--- a/shibsp/handler/impl/SessionInitiator.cpp
+++ b/shibsp/handler/impl/SessionInitiator.cpp
@@ -229,8 +229,7 @@ pair<bool,long> SessionInitiator::run(SPRequest& request, bool isHandler) const
// Make sure the target isn't the same as this handler, to avoid a loop.
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));
+ return make_pair(true, request.sendRedirect(error_target, true));
}
}
}
diff --git a/shibsp/handler/impl/TokenConsumer.cpp b/shibsp/handler/impl/TokenConsumer.cpp
index 2418e599..707877ce 100644
--- a/shibsp/handler/impl/TokenConsumer.cpp
+++ b/shibsp/handler/impl/TokenConsumer.cpp
@@ -93,8 +93,7 @@ pair<bool,long> TokenConsumer::run(SPRequest& request, bool isHandler) const
target = getString(RequestMapper::HOME_URL_PROP_NAME, request,
RequestMapper::HOME_URL_PROP_DEFAULT, HANDLER_PROPERTY_MAP);
}
- request.limitRedirect(target.c_str());
- return make_pair(true, request.sendRedirect(target.c_str()));
+ return make_pair(true, request.sendRedirect(target.c_str(), true));
}
}
@@ -153,13 +152,6 @@ pair<bool,long> TokenConsumer::run(SPRequest& request, bool isHandler) const
output.addmember("http.redirect").unsafe_string(target.c_str());
}
- // If target is still empty, then this is a POST recovery attempt with the reesource
- // buried in the form action. Assuming it's non-empty, we must sanitize it.
- // TODO: we have to have some way to sanitize it anyway...
- if (!target.empty()) {
- request.limitRedirect(target.c_str());
- }
-
SessionCache* cache = request.getAgent().getSessionCache();
DDF sessionData = output["session"];
// Ownership of sessionData transfers on input to create call (will be detached from output).
@@ -231,13 +223,12 @@ pair<bool,long> TokenConsumer::run(SPRequest& request, bool isHandler) const
const char* error_target = target.empty() ? agent_ex->getProperty(AgentException::TARGET_PROP_NAME) : target.c_str();
if (error_target) {
agent_ex->log(request, Priority::SHIB_WARN);
- request.limitRedirect(error_target);
// Make sure the target isn't a prefix of this handler, to avoid a loop.
if (boost::starts_with(error_target, request.getRequestURL())) {
request.warn("TokenConsumer target location matched handler, not trapping passive request error");
} else {
request.info("trapping TokenConsumer failure and returning to target location of passive request");
- return make_pair(true, request.sendRedirect(error_target));
+ return make_pair(true, request.sendRedirect(error_target, true));
}
}
else {
diff --git a/shibsp/io/HTTPResponse.h b/shibsp/io/HTTPResponse.h
index af664dc3..16c02ac1 100644
--- a/shibsp/io/HTTPResponse.h
+++ b/shibsp/io/HTTPResponse.h
@@ -89,12 +89,17 @@ namespace shibsp {
/**
* Redirect the client to the specified URL and complete the response.
*
- * <p>Any headers previously set will be sent ahead of the redirect.
+ * <p>Any headers previously set will be sent ahead of the redirect.</p>
+ *
+ * <p>The flag, which defaults to false, controls whether the redirect should
+ * be permitted blindly or if true, policed by local redirect-limiting policy.</p>
*
* @param url location to redirect client
+ * @param limit true iff the redirect should be limited and reviewed against policy
+ *
* @return a result code to return
*/
- virtual long sendRedirect(const char* url)=0;
+ virtual long sendRedirect(const char* url, bool limit=false)=0;
/** Some common HTTP status codes. */
enum status_t {
diff --git a/tests/DummyRequest.h b/tests/DummyRequest.h
index c0b1dad8..6c706c27 100644
--- a/tests/DummyRequest.h
+++ b/tests/DummyRequest.h
@@ -46,7 +46,7 @@ namespace shibsp {
std::string getRemoteAddr() const { return m_addr; }
std::string getLocalAddr() const { return ""; }
std::string getAuthType() const { return ""; }
- long sendRedirect(const char* url) { return SHIBSP_HTTP_STATUS_MOVED; }
+ long doRedirect(const char* url) { return SHIBSP_HTTP_STATUS_MOVED; }
long sendResponse(std::istream&, long status) { return status; }
void clearHeader(const char* name) {}
void setHeader(const char* name, const char* value) {}
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list