rombert commented on code in PR #88:
URL:
https://github.com/apache/sling-org-apache-sling-engine/pull/88#discussion_r4072342421
##########
src/main/java/org/apache/sling/engine/impl/SlingJakartaHttpServletResponseImpl.java:
##########
@@ -375,9 +375,81 @@ public void setContentType(final String type) {
}
}
+ @Override
+ public void setCharacterEncoding(final String charset) {
+ boolean isCommitedDueToSendErrorOrRedirect = this.isCommitted()
+ && (CommitReason.SEND_ERROR == this.committedReason
+ || CommitReason.SEND_REDIRECT == this.committedReason);
+ if (isCommitedDueToSendErrorOrRedirect || !isInclude()) {
+ super.setCharacterEncoding(charset);
+ return;
+ }
+ final Optional<String> message =
checkCharacterEncodingOverride(charset);
+ if (message.isPresent()) {
+ if (isCheckContentTypeOnInclude()) {
+ requestData.getRequestProgressTracker().log("ERROR: " +
message.get());
+ LOG.error(CALL_STACK_MESSAGE + getCurrentStackTrace());
+ throw new ContentTypeChangeException(message.get());
+ }
+ if (isProtectHeadersOnInclude()) {
+ LOG.error(message.get());
+ LOG.error(CALL_STACK_MESSAGE + getCurrentStackTrace());
+ requestData.getRequestProgressTracker().log("ERROR: " +
message.get());
+ return;
+ }
+ LOG.warn(message.get());
+ LOG.warn(CALL_STACK_MESSAGE + getCurrentStackTrace());
+ requestData.getRequestProgressTracker().log("WARN: " +
message.get());
+ super.setCharacterEncoding(charset);
+ } else {
+ super.setCharacterEncoding(charset);
+ }
+ }
+
+ @Override
+ public void setCharacterEncoding(final Charset charset) {
+ // funnel the Charset variant through the checked String variant so
+ // that the include protections cannot be bypassed via this method
+ this.setCharacterEncoding(charset == null ? null : charset.name());
+ }
+
+ /**
+ * Checks if the response character encoding is being changed by an include
+ * and provides a message to log if it is. Changing the character encoding
+ * changes the charset parameter of the 'Content-Type' header and is
+ * therefore subject to the same include protections as
+ * {@link #setContentType(String)}.
+ *
+ * @param charset the character encoding that is being set
+ * @return an optional message to log
+ */
+ protected Optional<String> checkCharacterEncodingOverride(@Nullable String
charset) {
+ // A previously detected violation must not disable the check itself -
+ // otherwise the second and any later override attempt within the same
+ // request would pass unchecked even though the first one was blocked.
+ final boolean isFirstViolation =
+
requestData.getSlingRequestProcessor().getContentTypeHeaderState() !=
ContentTypeHeaderState.VIOLATED;
+ final String currentCharset = getCharacterEncoding();
+ if (charset != null && charset.equalsIgnoreCase(currentCharset)) {
Review Comment:
Charset aliases that differ by more than case are treated as an override
here. For example, `UTF8` and `UTF-8` identify the same effective encoding, but
with `sling.includes.checkcontenttype` enabled this can throw
`ContentTypeChangeException`. The charset comparison in
`checkContentTypeOverride` has the same issue. Could we compare canonical
`Charset` values when both names are supported, while retaining a textual
fallback for invalid or unrecognised names permitted by the Servlet API?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
