[cpp-sp] branch main updated: POST recovery and NoPassive error handling cleanup.
Scott Cantor
cantor.2 at osu.edu
Tue Sep 30 02:07:57 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=e6cbbe5a4b13e5e077b7801c43abd0da4c17afd1
The following commit(s) were added to refs/heads/main by this push:
new e6cbbe5a POST recovery and NoPassive error handling cleanup.
e6cbbe5a is described below
commit e6cbbe5a4b13e5e077b7801c43abd0da4c17afd1
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Mon Sep 29 22:07:49 2025 -0400
POST recovery and NoPassive error handling cleanup.
---
shibsp/AbstractSPRequest.cpp | 6 +-
shibsp/exceptions.h | 26 ++++++
shibsp/handler/impl/AbstractHandler.cpp | 14 ++-
shibsp/handler/impl/TokenConsumer.cpp | 106 +++++++++++++++--------
shibsp/remoting/impl/AbstractRemotingService.cpp | 36 +++++---
5 files changed, 141 insertions(+), 47 deletions(-)
diff --git a/shibsp/AbstractSPRequest.cpp b/shibsp/AbstractSPRequest.cpp
index 5350a889..0587b519 100644
--- a/shibsp/AbstractSPRequest.cpp
+++ b/shibsp/AbstractSPRequest.cpp
@@ -132,8 +132,9 @@ string AbstractSPRequest::getRemoteAddr() const
const char* AbstractSPRequest::getParameter(const char* name) const
{
- if (!m_parser.get())
+ if (!m_parser) {
m_parser.reset(new CGIParser(*this));
+ }
pair<CGIParser::walker,CGIParser::walker> bounds = m_parser->getParameters(name);
return (bounds.first==bounds.second) ? nullptr : bounds.first->second;
@@ -141,8 +142,9 @@ const char* AbstractSPRequest::getParameter(const char* name) const
vector<const char*>::size_type AbstractSPRequest::getParameters(const char* name, vector<const char*>& values) const
{
- if (!m_parser.get())
+ if (!m_parser) {
m_parser.reset(new CGIParser(*this));
+ }
pair<CGIParser::walker,CGIParser::walker> bounds = m_parser->getParameters(name);
while (bounds.first != bounds.second) {
diff --git a/shibsp/exceptions.h b/shibsp/exceptions.h
index 5a6c59d5..71bcf880 100644
--- a/shibsp/exceptions.h
+++ b/shibsp/exceptions.h
@@ -23,6 +23,7 @@
#include <shibsp/base.h>
#include <shibsp/logging/Priority.h>
+#include <shibsp/remoting/ddf.h>
#include <exception>
#include <string>
@@ -161,6 +162,31 @@ namespace shibsp {
DECL_SHIBSP_EXCEPTION(SessionException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::AgentException);
DECL_SHIBSP_EXCEPTION(SessionValidationException,SHIBSP_EXCEPTIONAPI(SHIBSP_API),shibsp::SessionException);
+ /**
+ * Specialized OperationException that propagates back a managed copy of the wrapped
+ * HTTP response from the hub for POST preservation on passive SSO.
+ */
+ class SHIBSP_EXCEPTIONAPI(SHIBSP_API) NoPassiveException : public OperationException {
+ public:
+ NoPassiveException(DDF http) : OperationException(nullptr), m_http(http.remove()) {}
+ virtual ~NoPassiveException() noexcept {
+ m_http.destroy();
+ }
+
+ /**
+ * Return wrapped HTTP response object.
+ *
+ * @return wrapped HTTP response
+ */
+ DDF getHTTPObject() const {
+ return m_http;
+ }
+
+ private:
+ DDF m_http;
+ };
+
+
#if defined (_MSC_VER)
#pragma warning( pop )
#endif
diff --git a/shibsp/handler/impl/AbstractHandler.cpp b/shibsp/handler/impl/AbstractHandler.cpp
index b740d4c7..1f4d87e7 100644
--- a/shibsp/handler/impl/AbstractHandler.cpp
+++ b/shibsp/handler/impl/AbstractHandler.cpp
@@ -128,7 +128,19 @@ DDF AbstractHandler::wrapRequest(const SPRequest& request, const set<string>& he
in.addmember("port").integer(request.getPort());
in.addmember("content_type").string(request.getContentType().c_str());
if (sendBody) {
- in.addmember("body").unsafe_string(request.getRequestBody());
+ if (request.getContentType().find("application/x-www-form-urlencoded") != string::npos) {
+ unsigned int postLimit =
+ getUnsignedInt("postLimit", request, 1024 * 1024, HANDLER_PROPERTY_FIXED | HANDLER_PROPERTY_MAP);
+ if (postLimit == 0 || request.getContentLength() <= postLimit) {
+ in.addmember("body").unsafe_string(request.getRequestBody());
+ }
+ else {
+ request.warn("POST limit exceeded, ignoring posted data");
+ }
+ }
+ else {
+ request.warn("Content type not supported, ignoring posted data");
+ }
}
in.addmember("content_length").longinteger(request.getContentLength());
in.addmember("remote_user").string(request.getRemoteUser().c_str());
diff --git a/shibsp/handler/impl/TokenConsumer.cpp b/shibsp/handler/impl/TokenConsumer.cpp
index 1d287bf3..f15f2b86 100644
--- a/shibsp/handler/impl/TokenConsumer.cpp
+++ b/shibsp/handler/impl/TokenConsumer.cpp
@@ -27,6 +27,7 @@
#include "logging/Category.h"
#include "session/SessionCache.h"
#include "remoting/RemotingService.h"
+#include "util/CGIParser.h"
#include "util/Misc.h"
#include "util/URLEncoder.h"
@@ -73,10 +74,31 @@ TokenConsumer::TokenConsumer(const ptree& pt, const char* path)
pair<bool,long> TokenConsumer::run(SPRequest& request, bool isHandler) const
{
- // TODO: check for session hook return to break loop.
-
string target;
+ // Check for a message back to the handler from a session hook.
+ if (request.getQueryString() && strstr(request.getQueryString(), "shibsp_hook=1")) {
+ // Parse the query string only, to preserve any POST data in case this is
+ // *not* a hook roundtrip but an actual token response that has that parameter
+ // for whatever odd reason.
+ CGIParser cgi(request, true);
+ pair<CGIParser::walker,CGIParser::walker> param = cgi.getParameters("shibsp_hook");
+ if (param.first != param.second && param.first->second && !strcmp(param.first->second, "1")) {
+ // This is a hook return, so we extract the target parameter and redirect to it.
+ param = cgi.getParameters("target");
+ if (param.first != param.second && param.first->second) {
+ target = param.first->second;
+ }
+ else {
+ target = getString("homeURL", request, "/", HANDLER_PROPERTY_FIXED | HANDLER_PROPERTY_MAP);
+ }
+ request.limitRedirect(target.c_str());
+ return make_pair(true, request.sendRedirect(target.c_str()));
+ }
+ }
+
+ // Not a hook response, so process as a token-consumer operation.
+
try {
DDF input("token-consumer");
DDFJanitor inputJanitor(input);
@@ -95,14 +117,27 @@ pair<bool,long> TokenConsumer::run(SPRequest& request, bool isHandler) const
if (s) {
target = s;
}
-
+ else if (!output.getmember("http.response.data").string()) {
+ // Shouldn't happen, but we can route ourselves to homeURL or /
+ target = getString("homeURL", request, "/", HANDLER_PROPERTY_FIXED | HANDLER_PROPERTY_MAP);
+ 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.
SessionCache* cache = request.getAgent().getSessionCache();
DDF sessionData = output["session"];
- // Ownership of sessionData transfers on input to create call.
+ // Ownership of sessionData transfers on input to create call (will be detached from output).
cache->create(request, sessionData);
const char* sessionHook = request.getRequestSettings().first->getString(RequestMapper::SESSION_HOOK_PROP_NAME);
+
+ if (target.empty() && sessionHook) {
+ request.warn("response contained recovered POST data, ignoring configured sessionHook");
+ sessionHook = nullptr;
+ }
+
if (sessionHook) {
string hook(sessionHook);
request.absolutize(hook);
@@ -111,7 +146,7 @@ pair<bool,long> TokenConsumer::run(SPRequest& request, bool isHandler) const
// The target also must be included.
const URLEncoder& encoder = AgentConfig::getConfig().getURLEncoder();
string returnURL = request.getRequestURL();
- returnURL = returnURL.substr(0, returnURL.find('?')) + "?hook=1";
+ returnURL = returnURL.substr(0, returnURL.find('?')) + "?shibsp_hook=1";
string encodedTarget;
if (!target.empty()) {
@@ -134,43 +169,46 @@ pair<bool,long> TokenConsumer::run(SPRequest& request, bool isHandler) const
// Overrwrite the original redirection target and issue.
// This is necessary to ensure any Set-Cookie headers placed by the hub will reach the client.
output.addmember("http.redirect").unsafe_string(hook.c_str());
- return unwrapResponse(request, output);
+ }
+
+ return unwrapResponse(request, output);
+ }
+ catch (NoPassiveException& ex) {
+ ex.addProperty(AgentException::HANDLER_TYPE_PROP_NAME, TOKEN_CONSUMER_HANDLER);
+
+ // Check for a wrapped POST recovery.
+ if (ex.getHTTPObject()["response.data"].string()) {
+ ex.log(request, Priority::SHIB_WARN);
+ DDF http = ex.getHTTPObject();
+ return unwrapResponse(request, http);
}
- // TODO: POST restoration...
+ // We do more probing in a GET recovery to prevent loops.
- return unwrapResponse(request, output);
+ const char* error_target = ex.getHTTPObject()["redirect"].string();
+ if (!error_target) {
+ error_target = target.empty() ? ex.getProperty(AgentException::TARGET_PROP_NAME) : target.c_str();
+ }
+
+ if (error_target) {
+ 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 {
+ ex.log(request, Priority::SHIB_WARN);
+ request.info("trapping TokenConsumer failure and returning to target location for passive request");
+ return make_pair(true, request.sendRedirect(error_target));
+ }
+ }
+
+ throw;
}
catch (exception& ex) {
AgentException* agent_ex = dynamic_cast<AgentException*>(&ex);
if (agent_ex) {
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
- // to the original resource.
-
- const char* event = agent_ex ? agent_ex->getProperty(AgentException::EVENT_PROP_NAME) : nullptr;
- if (event && !strcmp(event, "NoPassive")) {
- 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?
-
- 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 for passive request");
- return make_pair(true, request.sendRedirect(error_target));
- }
- }
- else {
- request.warn("TokenConsumer caught NoPassive error but had no target to redirect to");
- }
- }
+ }
throw;
}
}
diff --git a/shibsp/remoting/impl/AbstractRemotingService.cpp b/shibsp/remoting/impl/AbstractRemotingService.cpp
index f53170da..8a56eb97 100644
--- a/shibsp/remoting/impl/AbstractRemotingService.cpp
+++ b/shibsp/remoting/impl/AbstractRemotingService.cpp
@@ -46,18 +46,34 @@ DDF AbstractRemotingService::send(const DDF& in) const
const char* event = output.getmember("event").string();
if (event && strcmp(event, "success")) {
- OperationException ex("Remote operation was unsuccessful.");
- ex.addProperty(AgentException::EVENT_PROP_NAME, event);
- if (in.name()) {
- ex.addProperty("operation", in.name());
- }
- const char* target = output.getmember("target").string();
- if (target) {
- ex.addProperty(AgentException::TARGET_PROP_NAME, target);
+ if (!strcmp(event, "NoPassive")) {
+ NoPassiveException ex(output.getmember("http"));
+ ex.addProperty(AgentException::EVENT_PROP_NAME, event);
+ if (in.name()) {
+ ex.addProperty("operation", in.name());
+ }
+ const char* target = output.getmember("target").string();
+ if (target) {
+ ex.addProperty(AgentException::TARGET_PROP_NAME, target);
+ }
+
+ output.destroy();
+ throw ex;
}
+ else {
+ OperationException ex("Remote operation was unsuccessful.");
+ ex.addProperty(AgentException::EVENT_PROP_NAME, event);
+ if (in.name()) {
+ ex.addProperty("operation", in.name());
+ }
+ const char* target = output.getmember("target").string();
+ if (target) {
+ ex.addProperty(AgentException::TARGET_PROP_NAME, target);
+ }
- output.destroy();
- throw ex;
+ output.destroy();
+ throw ex;
+ }
}
return output;
}
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list