diff --git a/ChangeLog b/ChangeLog index b9422491..e0fe56de 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,7 @@ - bump to 2.4.16.2dev - add some resilience when both Forwarded and X-Forwarded-* are configured - fix disabled OIDCStateCookiePrefix command; closes #1254; thanks @damisanet +- remove support for OIDCHTMLErrorTemplate, deprecated since 2.4.14 08/26/2024 - fix parsing OIDCXForwardedHeaders; closes #1250; thanks @maltesmann diff --git a/src/cfg/cfg.c b/src/cfg/cfg.c index 497f4683..1e504acb 100644 --- a/src/cfg/cfg.c +++ b/src/cfg/cfg.c @@ -495,25 +495,6 @@ const char *oidc_cmd_post_preserve_templates_set(cmd_parms *cmd, void *m, const OIDC_CFG_MEMBER_FUNC_GET(post_preserve_template, const char *) OIDC_CFG_MEMBER_FUNC_GET(post_restore_template, const char *) -const char *oidc_cmd_html_error_template_set(cmd_parms *cmd, void *ptr, const char *arg) { - oidc_cfg_t *cfg = (oidc_cfg_t *)ap_get_module_config(cmd->server->module_config, &auth_openidc_module); - oidc_swarn( - cmd->server, OIDCHTMLErrorTemplate - " is deprecated; please use the standard Apache features to deal with the " OIDC_ERROR_ENVVAR - " and " OIDC_ERROR_DESC_ENVVAR - " environment variables set by this module, see: https://httpd.apache.org/docs/2.4/custom-error.html"); - const char *rv = NULL; - if (_oidc_strcmp(arg, OIDC_HTML_ERROR_TEMPLATE_DEPRECATED) == 0) - cfg->error_template = OIDC_HTML_ERROR_TEMPLATE_DEPRECATED; - else - rv = oidc_cfg_parse_filename(cmd->pool, arg, &cfg->error_template); - return OIDC_CONFIG_DIR_RV(cmd, rv); -} - -const char *oidc_cfg_html_error_template_get(oidc_cfg_t *cfg) { - return cfg->error_template; -} - const char *oidc_cmd_ca_bundle_path_set(cmd_parms *cmd, void *ptr, const char *arg) { oidc_cfg_t *cfg = (oidc_cfg_t *)ap_get_module_config(cmd->server->module_config, &auth_openidc_module); const char *rv = oidc_cfg_parse_filename(cmd->pool, arg, &cfg->ca_bundle_path); @@ -665,7 +646,6 @@ void *oidc_cfg_server_create(apr_pool_t *pool, server_rec *svr) { c->crypto_passphrase.secret1 = NULL; c->crypto_passphrase.secret2 = NULL; - c->error_template = NULL; c->post_preserve_template = NULL; c->post_restore_template = NULL; @@ -802,7 +782,6 @@ void *oidc_cfg_server_merge(apr_pool_t *pool, void *BASE, void *ADD) { c->crypto_passphrase.secret2 = base->crypto_passphrase.secret2; } - c->error_template = add->error_template != NULL ? add->error_template : base->error_template; c->post_preserve_template = add->post_preserve_template != NULL ? add->post_preserve_template : base->post_preserve_template; c->post_restore_template = diff --git a/src/cfg/cfg.h b/src/cfg/cfg.h index 245cfd69..3ed0eec9 100644 --- a/src/cfg/cfg.h +++ b/src/cfg/cfg.h @@ -67,7 +67,6 @@ #define OIDCMetadataDir "OIDCMetadataDir" #define OIDCSessionCacheFallbackToCookie "OIDCSessionCacheFallbackToCookie" #define OIDCSessionCookieChunkSize "OIDCSessionCookieChunkSize" -#define OIDCHTMLErrorTemplate "OIDCHTMLErrorTemplate" #define OIDCPreservePostTemplates "OIDCPreservePostTemplates" #define OIDCProviderMetadataRefreshInterval "OIDCProviderMetadataRefreshInterval" #define OIDCBlackListedClaims "OIDCBlackListedClaims" diff --git a/src/cfg/cfg_int.h b/src/cfg/cfg_int.h index fa5f0810..2bf62d15 100644 --- a/src/cfg/cfg_int.h +++ b/src/cfg/cfg_int.h @@ -125,8 +125,6 @@ struct oidc_cfg_t { /* (optional) default URL to go to after logout */ char *default_slo_url; - /* HTML to display error messages+description */ - char *error_template; /* Javascript template to preserve POST data */ char *post_preserve_template; /* Javascript template to restore POST data */ diff --git a/src/cfg/cmds.c b/src/cfg/cmds.c index 2dedae97..e6c32783 100644 --- a/src/cfg/cmds.c +++ b/src/cfg/cmds.c @@ -177,11 +177,6 @@ const command_rec oidc_cfg_cmds[] = { OIDCSessionCookieChunkSize, session_cookie_chunk_size, "Chunk size for client-cookie session storage type in bytes. Defaults to 4k. Set 0 to suppress chunking."), - OIDC_CFG_CMD( - AP_INIT_TAKE1, - OIDCHTMLErrorTemplate, - html_error_template, - "Name of a HTML error template: needs to contain two \"%s\" characters, one for the error message, one for the description."), OIDC_CFG_CMD( AP_INIT_TAKE2, OIDCPreservePostTemplates, diff --git a/src/handle/authz.c b/src/handle/authz.c index 966ec1f1..0e36ee4c 100644 --- a/src/handle/authz.c +++ b/src/handle/authz.c @@ -482,10 +482,7 @@ static authz_status oidc_authz_24_unauthorized_user(request_rec *r) { case OIDC_UNAUTZ_RETURN403: case OIDC_UNAUTZ_RETURN401: OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHZ_ACTION_401); - oidc_util_html_send_error(r, oidc_cfg_html_error_template_get(c), "Authorization Error", - oidc_cfg_dir_unauthz_arg_get(r), HTTP_UNAUTHORIZED); - if (oidc_cfg_html_error_template_get(c)) - r->header_only = 1; + oidc_util_html_send_error(r, "Authorization Error", oidc_cfg_dir_unauthz_arg_get(r), HTTP_UNAUTHORIZED); return AUTHZ_DENIED; case OIDC_UNAUTZ_RETURN302: OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHZ_ACTION_302); diff --git a/src/handle/discovery.c b/src/handle/discovery.c index 6ac171ce..83b26519 100644 --- a/src/handle/discovery.c +++ b/src/handle/discovery.c @@ -137,7 +137,7 @@ int oidc_discovery_request(request_rec *r, oidc_cfg_t *cfg) { /* get a list of all providers configured in the metadata directory */ apr_array_header_t *arr = NULL; if (oidc_metadata_list(r, cfg, &arr) == FALSE) - return oidc_util_html_send_error(r, oidc_cfg_html_error_template_get(cfg), "Configuration Error", + return oidc_util_html_send_error(r, "Configuration Error", "No configured providers found, contact your administrator", HTTP_UNAUTHORIZED); @@ -324,7 +324,7 @@ int oidc_discovery_response(request_rec *r, oidc_cfg_t *c) { if (target_link_uri == NULL) { if (oidc_cfg_default_sso_url_get(c) == NULL) { - return oidc_util_html_send_error(r, oidc_cfg_html_error_template_get(c), "Invalid Request", + return oidc_util_html_send_error(r, "Invalid Request", "SSO to this module without specifying a \"target_link_uri\" " "parameter is not possible because " OIDCDefaultURL " is not set.", @@ -335,7 +335,7 @@ int oidc_discovery_response(request_rec *r, oidc_cfg_t *c) { /* do open redirect prevention, step 1 */ if (oidc_discovery_target_link_uri_match(r, c, target_link_uri) == FALSE) { - return oidc_util_html_send_error(r, oidc_cfg_html_error_template_get(c), "Invalid Request", + return oidc_util_html_send_error(r, "Invalid Request", "\"target_link_uri\" parameter does not match configuration settings, " "aborting to prevent an open redirect.", HTTP_UNAUTHORIZED); @@ -343,8 +343,7 @@ int oidc_discovery_response(request_rec *r, oidc_cfg_t *c) { /* do input validation on the target_link_uri parameter value, step 2 */ if (oidc_validate_redirect_url(r, c, target_link_uri, TRUE, &error_str, &error_description) == FALSE) { - return oidc_util_html_send_error(r, oidc_cfg_html_error_template_get(c), error_str, error_description, - HTTP_UNAUTHORIZED); + return oidc_util_html_send_error(r, error_str, error_description, HTTP_UNAUTHORIZED); } /* see if this is a static setup */ @@ -352,7 +351,7 @@ int oidc_discovery_response(request_rec *r, oidc_cfg_t *c) { if ((oidc_provider_static_config(r, c, &provider) == TRUE) && (issuer != NULL)) { if (_oidc_strcmp(oidc_cfg_provider_issuer_get(provider), issuer) != 0) { return oidc_util_html_send_error( - r, oidc_cfg_html_error_template_get(c), "Invalid Request", + r, "Invalid Request", apr_psprintf( r->pool, "The \"iss\" value must match the configured providers' one (%s != %s).", @@ -378,7 +377,7 @@ int oidc_discovery_response(request_rec *r, oidc_cfg_t *c) { if (oidc_proto_discovery_url_based(r, c, user, &issuer) == FALSE) { /* something did not work out, show a user facing error */ - return oidc_util_html_send_error(r, oidc_cfg_html_error_template_get(c), "Invalid Request", + return oidc_util_html_send_error(r, "Invalid Request", "Could not resolve the provided user identifier to an OpenID " "Connect provider; check your syntax.", HTTP_NOT_FOUND); @@ -398,7 +397,7 @@ int oidc_discovery_response(request_rec *r, oidc_cfg_t *c) { if (oidc_proto_discovery_account_based(r, c, issuer, &issuer) == FALSE) { /* something did not work out, show a user facing error */ - return oidc_util_html_send_error(r, oidc_cfg_html_error_template_get(c), "Invalid Request", + return oidc_util_html_send_error(r, "Invalid Request", "Could not resolve the provided account name to an OpenID " "Connect provider; check your syntax.", HTTP_NOT_FOUND); @@ -439,7 +438,7 @@ int oidc_discovery_response(request_rec *r, oidc_cfg_t *c) { } /* something went wrong */ - return oidc_util_html_send_error(r, oidc_cfg_html_error_template_get(c), "Invalid Request", + return oidc_util_html_send_error(r, "Invalid Request", "Could not find valid provider metadata for the selected OpenID Connect " "provider; contact the administrator", HTTP_NOT_FOUND); diff --git a/src/handle/logout.c b/src/handle/logout.c index 6bda68d3..9e2d023a 100644 --- a/src/handle/logout.c +++ b/src/handle/logout.c @@ -471,8 +471,7 @@ int oidc_logout(request_rec *r, oidc_cfg_t *c, oidc_session_t *session) { /* do input validation on the logout parameter value */ if (oidc_validate_redirect_url(r, c, url, TRUE, &error_str, &error_description) == FALSE) { - return oidc_util_html_send_error(r, oidc_cfg_html_error_template_get(c), error_str, - error_description, HTTP_BAD_REQUEST); + return oidc_util_html_send_error(r, error_str, error_description, HTTP_BAD_REQUEST); } } diff --git a/src/handle/response.c b/src/handle/response.c index dbc3d00c..7715f83a 100644 --- a/src/handle/response.c +++ b/src/handle/response.c @@ -75,9 +75,8 @@ static int oidc_response_authorization_error(request_rec *r, oidc_cfg_t *c, oidc if ((prompt != NULL) && (_oidc_strcmp(prompt, OIDC_PROTO_PROMPT_NONE) == 0)) { return oidc_response_redirect_parent_window_to_logout(r, c); } - return oidc_util_html_send_error( - r, oidc_cfg_html_error_template_get(c), apr_psprintf(r->pool, "OpenID Connect Provider error: %s", error), - error_description, oidc_cfg_html_error_template_get(c) ? OK : HTTP_BAD_REQUEST); + return oidc_util_html_send_error(r, apr_psprintf(r->pool, "OpenID Connect Provider error: %s", error), + error_description, HTTP_BAD_REQUEST); } /* handle the browser back on an authorization response */ @@ -380,7 +379,7 @@ static apr_byte_t oidc_response_proto_state_restore(request_rec *r, oidc_cfg_t * if ((oidc_cfg_default_sso_url_get(c) == NULL) || (apr_table_get(r->subprocess_env, "OIDC_NO_DEFAULT_URL_ON_STATE_TIMEOUT") != NULL)) { oidc_util_html_send_error( - r, oidc_cfg_html_error_template_get(c), "Invalid Authentication Response", + r, "Invalid Authentication Response", apr_psprintf(r->pool, "This is due to a timeout; please restart your authentication session by " "re-entering the URL/bookmark you originally wanted to access: %s", @@ -564,18 +563,6 @@ static int oidc_response_process(request_rec *r, oidc_cfg_t *c, oidc_session_t * oidc_error(r, "invalid authorization response state and no default SSO URL is set, sending an error..."); - if (oidc_cfg_html_error_template_get(c)) { - // retain backwards compatibility - int rc = HTTP_BAD_REQUEST; - if ((r->user) && (_oidc_strncmp(r->user, "", 1) == 0)) { - r->header_only = 1; - r->user = NULL; - rc = OK; - } - OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_STATE_MISMATCH); - return rc; - } - // if error text was already produced (e.g. state timeout) then just return with a 400 if (apr_table_get(r->subprocess_env, OIDC_ERROR_ENVVAR) != NULL) { OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_STATE_EXPIRED); @@ -584,8 +571,7 @@ static int oidc_response_process(request_rec *r, oidc_cfg_t *c, oidc_session_t * OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_STATE_MISMATCH); - return oidc_util_html_send_error(r, oidc_cfg_html_error_template_get(c), - "Invalid Authorization Response", + return oidc_util_html_send_error(r, "Invalid Authorization Response", "Could not match the authorization response to an earlier request via " "the state parameter and corresponding state cookie", HTTP_BAD_REQUEST); @@ -721,7 +707,7 @@ int oidc_response_authorization_post(request_rec *r, oidc_cfg_t *c, oidc_session ((apr_table_elts(params)->nelts == 1) && apr_table_get(params, OIDC_PROTO_RESPONSE_MODE) && (_oidc_strcmp(apr_table_get(params, OIDC_PROTO_RESPONSE_MODE), OIDC_PROTO_RESPONSE_MODE_FRAGMENT) == 0))) { return oidc_util_html_send_error( - r, oidc_cfg_html_error_template_get(c), "Invalid Request", + r, "Invalid Request", "You've hit an OpenID Connect Redirect URI with no parameters, this is an invalid request; you " "should not open this URL in your browser directly, or have the server administrator use a " "different " OIDCRedirectURI " setting.", diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c index 27336348..6f4ac95f 100644 --- a/src/mod_auth_openidc.c +++ b/src/mod_auth_openidc.c @@ -1504,8 +1504,7 @@ int oidc_handle_redirect_uri_request(request_rec *r, oidc_cfg_t *c, oidc_session /* something went wrong */ return oidc_util_html_send_error( - r, oidc_cfg_html_error_template_get(c), "Invalid Request", - apr_psprintf(r->pool, "The OpenID Connect callback URL received an invalid request"), + r, "Invalid Request", apr_psprintf(r->pool, "The OpenID Connect callback URL received an invalid request"), HTTP_INTERNAL_SERVER_ERROR); } diff --git a/src/util.c b/src/util.c index 8b21bc9d..fee3c648 100644 --- a/src/util.c +++ b/src/util.c @@ -1102,8 +1102,6 @@ int oidc_util_html_send(request_rec *r, const char *title, const char *html_head return oidc_util_http_send(r, html, _oidc_strlen(html), OIDC_HTTP_CONTENT_TYPE_TEXT_HTML, status_code); } -static char *html_error_template_contents = NULL; - /* * escape characters in an HTML/Javascript template */ @@ -1145,34 +1143,7 @@ apr_byte_t oidc_util_html_send_in_template(request_rec *r, const char *filename, /* * send a user-facing error to the browser */ -int oidc_util_html_send_error(request_rec *r, const char *html_template, const char *error, const char *description, - int status_code) { - - char *html = ""; - int rc = status_code; - - if (html_template != NULL) { - - if (_oidc_strcmp(html_template, OIDC_HTML_ERROR_TEMPLATE_DEPRECATED) != 0) { - - rc = oidc_util_html_send_in_template(r, html_template, &html_error_template_contents, error, - OIDC_POST_PRESERVE_ESCAPE_HTML, description, - OIDC_POST_PRESERVE_ESCAPE_HTML, status_code); - - } else { - - if (error != NULL) { - html = apr_psprintf(r->pool, "%s
Error:
%s", html, - oidc_util_html_escape(r->pool, error)); - } - if (description != NULL) { - html = apr_psprintf(r->pool, "%s
Description:
%s", html, - oidc_util_html_escape(r->pool, description)); - } - - rc = oidc_util_html_send(r, "Error", NULL, NULL, html, status_code); - } - } +int oidc_util_html_send_error(request_rec *r, const char *error, const char *description, int status_code) { oidc_debug(r, "setting " OIDC_ERROR_ENVVAR " environment variable to: %s", error); apr_table_set(r->subprocess_env, OIDC_ERROR_ENVVAR, error ? error : ""); @@ -1180,7 +1151,7 @@ int oidc_util_html_send_error(request_rec *r, const char *html_template, const c oidc_debug(r, "setting " OIDC_ERROR_DESC_ENVVAR " environment variable to: %s", description); apr_table_set(r->subprocess_env, OIDC_ERROR_DESC_ENVVAR, description ? description : ""); - return rc; + return status_code; } /* the maximum size of data that we accept in a single POST value: 1MB */ diff --git a/src/util.h b/src/util.h index a3565904..c4a62c79 100644 --- a/src/util.h +++ b/src/util.h @@ -81,8 +81,7 @@ int oidc_util_html_send(request_rec *r, const char *title, const char *html_head apr_byte_t oidc_util_file_read(request_rec *r, const char *path, apr_pool_t *pool, char **result); apr_byte_t oidc_util_file_write(request_rec *r, const char *path, const char *data); apr_byte_t oidc_util_issuer_match(const char *a, const char *b); -int oidc_util_html_send_error(request_rec *r, const char *html_template, const char *error, const char *description, - int status_code); +int oidc_util_html_send_error(request_rec *r, const char *error, const char *description, int status_code); apr_byte_t oidc_util_json_array_has_value(request_rec *r, json_t *haystack, const char *needle); void oidc_util_set_app_info(request_rec *r, const char *s_key, const char *s_value, const char *claim_prefix, oidc_appinfo_pass_in_t pass_in, oidc_appinfo_encoding_t encoding);