Skip to content

Commit

Permalink
Improve documentation explaining why decoding is being done when and …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@avendasora @recurve
avendasora authored and recurve committed Apr 26, 2014
1 parent d8c49c3 commit 083f592
Show file tree
Hide file tree
Showing 2 changed files with 116 additions and 36 deletions.
143 changes: 108 additions & 35 deletions Frameworks/BusinessLogic/ERAttachment/Sources/er/attachment/ERAttachmentRequestHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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.");
}
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
}
9 changes: 8 additions & 1 deletion Frameworks/BusinessLogic/ERAttachment/Sources/er/attachment/model/ERAttachment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
* <span class="en">
Expand Down Expand Up @@ -138,9 +140,14 @@ public String extension() {
* </span>
*/
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) {
Expand Down

0 comments on commit 083f592

Please sign in to comment.