From 083f59282b01d41a0f2e9a2bf081f23d7ac18d7a Mon Sep 17 00:00:00 2001 From: David Avendasora Date: Fri, 25 Apr 2014 18:17:54 -0400 Subject: [PATCH] Improve documentation explaining why decoding is being done when and where it is. Refactor individual logic blocks (fetching the ERAttachment, evaluating if a request is legitimate) into their own methods to make code more digestible and better encapsulated. --- .../ERAttachmentRequestHandler.java | 143 +++++++++++++----- .../er/attachment/model/ERAttachment.java | 9 +- 2 files changed, 116 insertions(+), 36 deletions(-) diff --git a/Frameworks/BusinessLogic/ERAttachment/Sources/er/attachment/ERAttachmentRequestHandler.java b/Frameworks/BusinessLogic/ERAttachment/Sources/er/attachment/ERAttachmentRequestHandler.java index 08751fc8173..07d3e7b095e 100644 --- a/Frameworks/BusinessLogic/ERAttachment/Sources/er/attachment/ERAttachmentRequestHandler.java +++ b/Frameworks/BusinessLogic/ERAttachment/Sources/er/attachment/ERAttachmentRequestHandler.java @@ -4,6 +4,8 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; +import java.net.URI; +import java.net.URISyntaxException; import java.util.NoSuchElementException; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -76,33 +78,35 @@ public WOResponse handleRequest(WORequest request) { WOContext context = application.createContextForRequest(request); WOResponse response = application.createResponseInContext(context); - String sessionIdKey = WOApplication.application().sessionIdKey(); + String sessionIdKey = application.sessionIdKey(); String sessionId = (String) request.formValueForKey(sessionIdKey); if (sessionId == null) { sessionId = request.cookieValueForKey(sessionIdKey); } context._setRequestSessionID(sessionId); if (context._requestSessionID() != null) { - WOApplication.application().restoreSessionWithID(sessionId, context); + application.restoreSessionWithID(sessionId, context); } + try { - WODynamicURL url = request._uriDecomposed(); - String requestHandlerPath = url.requestHandlerPath(); - Matcher idMatcher = Pattern.compile("^id/(\\d+)/").matcher(requestHandlerPath); - String idStr; - String webPath; - if (idMatcher.find()) { - idStr = idMatcher.group(1); - webPath = idMatcher.replaceFirst("/"); - } - else { + final WODynamicURL url = request._uriDecomposed(); + final String requestPath = url.requestHandlerPath(); + final Matcher idMatcher = Pattern.compile("^id/(\\d+)/").matcher(requestPath); + + final Integer requestedAttachmentID; + String requestedWebPath; + + final boolean requestedPathContainsAnAttachmentID = idMatcher.find(); + if (requestedPathContainsAnAttachmentID) { + requestedAttachmentID = Integer.valueOf(idMatcher.group(1)); + requestedWebPath = idMatcher.replaceFirst("/"); + } else { // MS: This is kind of goofy because we lookup by path, your web path needs to // have a leading slash on it. - webPath = "/" + requestHandlerPath; - idStr = null; + requestedWebPath = "/" + requestPath; + requestedAttachmentID = null; } - webPath = ERXStringUtilities.urlDecode(webPath); try { InputStream attachmentInputStream; @@ -116,23 +120,8 @@ public WOResponse handleRequest(WORequest request) { editingContext.lock(); try { - ERAttachment attachment; - if (idStr != null) { - EOGlobalID gid = EOKeyGlobalID.globalIDWithEntityName(ERAttachment.ENTITY_NAME, new Object[] { Integer.parseInt(idStr) }); - attachment = (ERAttachment) ERXEOGlobalIDUtilities.fetchObjectWithGlobalID(editingContext, gid); - String actualWebPath = attachment.webPath(); - if (!actualWebPath.equals(webPath)) { - // Aaron Rosenzweig - April 24, 2014 - If the file name has %20 in it on the server already (because it was uploaded that way)... - // then we need to compare "decoded" with "decoded" for fairness - String urlDecodedActualPath = ERXStringUtilities.urlDecode(actualWebPath); - if ( ! urlDecodedActualPath.equals(webPath)) { - throw new SecurityException("You are not allowed to view the requested attachment."); - } - } - } - else { - attachment = ERAttachment.fetchRequiredAttachmentWithWebPath(editingContext, webPath); - } + ERAttachment attachment = fetchAttachmentFor(editingContext, requestedAttachmentID, requestedWebPath); + if (_delegate != null && !_delegate.attachmentVisible(attachment, request, context)) { throw new SecurityException("You are not allowed to view the requested attachment."); } @@ -145,15 +134,15 @@ public WOResponse handleRequest(WORequest request) { } InputStream rawAttachmentInputStream = attachmentProcessor.attachmentInputStream(attachment); attachmentInputStream = new BufferedInputStream(rawAttachmentInputStream, bufferSize); - } - finally { + } finally { editingContext.unlock(); } + response.setHeader(mimeType, "Content-Type"); response.setHeader(String.valueOf(length), "Content-Length"); if (proxyAsAttachment) { - response.setHeader("attachment; filename=\"" + fileName+"\"", "Content-Disposition"); + response.setHeader("attachment; filename=\"" + fileName + "\"", "Content-Disposition"); } response.setStatus(200); @@ -192,4 +181,88 @@ public WOResponse handleRequest(WORequest request) { application.sleep(); } } + + + /** + * + * + * @param editingContext + * the {@link EOEditingContext} that the result will be inserted into + * @param attachmentPrimaryKey + * the primaryKey value of an existing ERAttachment in the database + * @param requestedWebPath + * a URL-encoded portion of the requested ERAttachment path including the file name of the attachment + * @return an attachment that matches either both the {@code attachmentPrimaryKey} and the {@code requestedWebPath}, + * or just the {@code reqestedWebPath}. + * + * @author davendasora + * @since Apr 25, 2014 + */ + public static ERAttachment fetchAttachmentFor(final EOEditingContext editingContext, final Integer attachmentPrimaryKey, final String requestedWebPath) { + ERAttachment attachment; + if (attachmentPrimaryKey != null) { + final EOGlobalID gid = EOKeyGlobalID.globalIDWithEntityName(ERAttachment.ENTITY_NAME, new Object[] {(attachmentPrimaryKey)}); + attachment = (ERAttachment) ERXEOGlobalIDUtilities.fetchObjectWithGlobalID(editingContext, gid); + + /* + * Ensure the attachment request is a legitimate one by comparing the attachment's webPath to the + * requestedWebPath. + */ + final boolean requestedWebPathIsInvalid = !ERAttachmentRequestHandler.requestedWebPathIsForAttachment(requestedWebPath, attachment); + if (requestedWebPathIsInvalid) { + throw new SecurityException("You are not allowed to view the requested attachment."); + } + } else { + /* + * If the webPath value that is stored in the database (actualWebPath) contains "%20" (or any other %nn + * code) the following fetch will not find it because the requestedWebPath needs to be decoded, which will + * remove any occurrences of "%20" from it, causing it to no longer match the actualWebPath value. + */ + String decodedRequestedWebPath; + try { + decodedRequestedWebPath = new URI(requestedWebPath).getPath(); + attachment = ERAttachment.fetchRequiredAttachmentWithWebPath(editingContext, decodedRequestedWebPath); + } catch (URISyntaxException exception) { + attachment = null; + exception.printStackTrace(); + } + } + return attachment; + } + + + /** + * Takes into account potential URL encoding differences between the {@code requestedWebPath} and the + * {@code attachment}'s {@link ERAttachment#webPath() webPath()} attribute. e.g., "/the/web/path/My Attachment.jpg" + * will match "/the/web/path/My%20Attachment.jpg" + * + * @param requestedWebPath + * a String to compare to the {@code attachment} parameter's + * @param attachment + * an ERAttachment + * @return {@code true} if the {@code requestedWebPath} matches {@code attachment.webPath()}. {@code false} if not. + * + * @author davendasora + * @since Apr 25, 2014 + */ + public static boolean requestedWebPathIsForAttachment(final String requestedWebPath, final ERAttachment attachment) { + final String actualWebPath = attachment.webPath(); + /* + * We are using the form-data decoder (URLDecoder.decode(String)) instead of the more appropriate URI#getPath() + * because we only need to ensure that both webPath values decode identically. We can't use URI.getPath() here + * because the webPath value stored in the database (actualWebPath) may contain illegal values (spaces, etc.) + */ + final String decodedWebPath = ERXStringUtilities.urlDecode(actualWebPath); + final String decodedRequestedWebPath = ERXStringUtilities.urlDecode(requestedWebPath); + + /* + * Aaron Rosenzweig - April 24, 2014 - Because the attachment may have been originally uploaded with "%20" (or + * another %nn value) already in the file name, we need to compare both the stored value (actualWebPath) and a + * decoded version (decodedWebPath) against the requested value (requestedWebPath) otherwise we could + * incorrectly throw a SecurityException. + */ + final boolean requestedWebPathIsDifferentFromTheAttachmentWebPath = decodedRequestedWebPath.equals(actualWebPath) + || decodedRequestedWebPath.equals(decodedWebPath); + return requestedWebPathIsDifferentFromTheAttachmentWebPath; + } } \ No newline at end of file diff --git a/Frameworks/BusinessLogic/ERAttachment/Sources/er/attachment/model/ERAttachment.java b/Frameworks/BusinessLogic/ERAttachment/Sources/er/attachment/model/ERAttachment.java index 49ab514c2d8..7082fc4df1f 100644 --- a/Frameworks/BusinessLogic/ERAttachment/Sources/er/attachment/model/ERAttachment.java +++ b/Frameworks/BusinessLogic/ERAttachment/Sources/er/attachment/model/ERAttachment.java @@ -6,11 +6,13 @@ import org.apache.log4j.Logger; import com.webobjects.eocontrol.EOEditingContext; +import com.webobjects.eocontrol.EOQualifier; import er.attachment.processors.ERAttachmentProcessor; import er.attachment.utils.ERMimeType; import er.attachment.utils.ERMimeTypeManager; import er.extensions.foundation.ERXFileUtilities; +import er.extensions.qualifiers.ERXKeyValueQualifier; /** * @@ -138,9 +140,14 @@ public String extension() { * */ public static ERAttachment fetchRequiredAttachmentWithWebPath(EOEditingContext editingContext, String webPath) { - ERAttachment attachment = ERAttachment.fetchRequiredERAttachment(editingContext, ERAttachment.WEB_PATH_KEY, webPath); + ERAttachment attachment = ERAttachment.fetchRequiredERAttachment(editingContext, thatAreForWebPath(webPath)); return attachment; } + + public static EOQualifier thatAreForWebPath(String webPath) { + final ERXKeyValueQualifier qualifier = ERAttachment.WEB_PATH.is(webPath); + return qualifier; + } @Override public void didDelete(EOEditingContext ec) {