[cpp-sp] branch main updated: Revert "POST recovery and NoPassive error handling cleanup."
Scott Cantor
cantor.2 at osu.edu
Tue Sep 30 12:19:35 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=c316e2d6b94cb1598c83ccba54ddfae78e3d1f77
The following commit(s) were added to refs/heads/main by this push:
new c316e2d6 Revert "POST recovery and NoPassive error handling cleanup."
c316e2d6 is described below
commit c316e2d6b94cb1598c83ccba54ddfae78e3d1f77
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Tue Sep 30 08:19:02 2025 -0400
Revert "POST recovery and NoPassive error handling cleanup."
This reverts commit e6cbbe5a4b13e5e077b7801c43abd0da4c17afd1.
POST recovery and IsPassive are actually mutually exclusive use cases.
---
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, 47 insertions(+), 141 deletions(-)
diff --git a/shibsp/AbstractSPRequest.cpp b/shibsp/AbstractSPRequest.cpp
index 0587b519..5350a889 100644
--- a/shibsp/AbstractSPRequest.cpp
+++ b/shibsp/AbstractSPRequest.cpp
@@ -132,9 +132,8 @@ string AbstractSPRequest::getRemoteAddr() const
const char* AbstractSPRequest::getParameter(const char* name) const
{
- if (!m_parser) {
+ if (!m_parser.get())
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;
@@ -142,9 +141,8 @@ 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) {
+ if (!m_parser.get())
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 71bcf880..5a6c59d5 100644
--- a/shibsp/exceptions.h
+++ b/shibsp/exceptions.h
@@ -23,7 +23,6 @@
#include <shibsp/base.h>
#include <shibsp/logging/Priority.h>
-#include <shibsp/remoting/ddf.h>
#include <exception>
#include <string>
@@ -162,31 +161,6 @@ 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 1f4d87e7..b740d4c7 100644
--- a/shibsp/handler/impl/AbstractHandler.cpp
+++ b/shibsp/handler/impl/AbstractHandler.cpp
@@ -128,19 +128,7 @@ 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) {
- 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("body").unsafe_string(request.getRequestBody());
}
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 f15f2b86..1d287bf3 100644
--- a/shibsp/handler/impl/TokenConsumer.cpp
+++ b/shibsp/handler/impl/TokenConsumer.cpp
@@ -27,7 +27,6 @@
#include "logging/Category.h"
#include "session/SessionCache.h"
#include "remoting/RemotingService.h"
-#include "util/CGIParser.h"
#include "util/Misc.h"
#include "util/URLEncoder.h"
@@ -74,30 +73,9 @@ TokenConsumer::TokenConsumer(const ptree& pt, const char* path)
pair<bool,long> TokenConsumer::run(SPRequest& request, bool isHandler) const
{
- 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()));
- }
- }
+ // TODO: check for session hook return to break loop.
- // Not a hook response, so process as a token-consumer operation.
+ string target;
try {
DDF input("token-consumer");
@@ -117,27 +95,14 @@ 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 (will be detached from output).
+ // Ownership of sessionData transfers on input to create call.
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);
@@ -146,7 +111,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('?')) + "?shibsp_hook=1";
+ returnURL = returnURL.substr(0, returnURL.find('?')) + "?hook=1";
string encodedTarget;
if (!target.empty()) {
@@ -169,46 +134,43 @@ 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);
- }
- 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);
- }
-
- // We do more probing in a GET recovery to prevent loops.
-
- const char* error_target = ex.getHTTPObject()["redirect"].string();
- if (!error_target) {
- error_target = target.empty() ? ex.getProperty(AgentException::TARGET_PROP_NAME) : target.c_str();
+ return unwrapResponse(request, output);
}
- 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));
- }
- }
+ // TODO: POST restoration...
- throw;
+ return unwrapResponse(request, output);
}
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 8a56eb97..f53170da 100644
--- a/shibsp/remoting/impl/AbstractRemotingService.cpp
+++ b/shibsp/remoting/impl/AbstractRemotingService.cpp
@@ -46,34 +46,18 @@ DDF AbstractRemotingService::send(const DDF& in) const
const char* event = output.getmember("event").string();
if (event && strcmp(event, "success")) {
- 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;
+ OperationException ex("Remote operation was unsuccessful.");
+ ex.addProperty(AgentException::EVENT_PROP_NAME, event);
+ if (in.name()) {
+ ex.addProperty("operation", in.name());
}
- 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;
+ const char* target = output.getmember("target").string();
+ if (target) {
+ ex.addProperty(AgentException::TARGET_PROP_NAME, target);
}
+
+ 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