Skip to content

Avoid reflecting invalid URL parameter - prevent false positive #6596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 23, 2025
Merged

Avoid reflecting invalid URL parameter - prevent false positive #6596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e556ea4
Avoid reflecting invalid URL parameter in error to prevent false posi…
labkey-jeckels Apr 22, 2025
bfd0395
Avoid logging for client disconnect exceptions
labkey-jeckels Apr 22, 2025
cb0169a
Simpler handling for conditional logging
labkey-jeckels Apr 22, 2025
b231d09
Even simpler handling for webdav exceptions
labkey-jeckels Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions api/src/org/labkey/api/view/FileServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
public class FileServlet extends HttpServlet
{
private static final Logger _log = LogManager.getLogger(FileServlet.class);
private static final String PAGE_FLOW_ATTR = FileServlet.class.getName() + ".pageFlow";

@Override
protected void service(HttpServletRequest request, HttpServletResponse response)
Expand Down Expand Up @@ -65,11 +64,6 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
request.setAttribute(ViewServlet.ORIGINAL_URL_URLHELPER, helper);
}

//The servlet path looks like a pageflow. Stash this away so that
//handlers can create links to other static files.
//We assume that this servlet is mapped to /something/*
String servletPath = request.getServletPath();
request.setAttribute(PAGE_FLOW_ATTR, servletPath.substring(1));
String dispatchUrl = extraPath + "/filecontent-sendFile.view?" + (null == fileNameParam ? "fileName=" + PageFlowUtil.encodeURIComponent(fileName) : "");
// NOTE other parameters seem to get magically propagated...
RequestDispatcher r = request.getRequestDispatcher(dispatchUrl);
Expand Down
22 changes: 8 additions & 14 deletions core/src/org/labkey/core/webdav/DavController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,15 +344,8 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
{
ExceptionUtil.doErrorRedirect(response, ex.getURL());
}
catch (ConfigurationException ex)
{
_log.error("Unexpected exception, might be related to server configuration problems", ex);
_webdavresponse.sendError(WebdavStatus.SC_INTERNAL_SERVER_ERROR, ex);
}
catch (Exception ex)
{
_log.error("unexpected exception", ex);
ExceptionUtil.logExceptionToMothership(request, ex);
_webdavresponse.sendError(WebdavStatus.SC_INTERNAL_SERVER_ERROR, ex);
}
}
Expand Down Expand Up @@ -432,7 +425,12 @@ WebdavStatus sendError(WebdavStatus status)

WebdavStatus sendError(WebdavStatus status, Exception x)
{
_log.error(x instanceof ConfigurationException ? "Configuration Exception" : "Unexpected exception", x);
ExceptionUtil.logExceptionToMothership(getRequest(), x);
if (x instanceof ConfigurationException)
{
// These exceptions are skipped in the logExceptionToMothership() call. They're useful to log here
_log.error("Unexpected exception, likely caused by server configuration problems", x);
}
String message = x.getMessage() != null ? x.getMessage() : status.message;
if (x instanceof ConfigurationException)
message += "\nThis may be a server configuration problem. Contact the site administrator.";
Expand Down Expand Up @@ -502,10 +500,7 @@ else if (CONTENT_TYPE_HTML.equals(getRequest().getHeader("Content-Type)")) ||
}
catch (Exception x)
{
if (!ExceptionUtil.isClientAbortException(x))
{
_log.error("unexpected error", x);
}
ExceptionUtil.logExceptionToMothership(getRequest(), x);
}
return _status;
}
Expand All @@ -519,7 +514,7 @@ WebdavStatus setStatus(WebdavStatus status)
}
catch (Exception x)
{
_log.error("unexpected error", x);
ExceptionUtil.logExceptionToMothership(getRequest(), x);
}
_status = status;
return status;
Expand Down Expand Up @@ -780,7 +775,6 @@ else if ("GET".equals(method) && HttpUtil.isBrowser(getRequest()))
}
catch (ConfigurationException ex)
{
_log.error("Unexpected exception, might be related to server configuration problems", ex);
getResponse().sendError(WebdavStatus.SC_INTERNAL_SERVER_ERROR, ex);
}
catch (DavException dex)
Expand Down
10 changes: 8 additions & 2 deletions filecontent/src/org/labkey/filecontent/FileContentController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1466,8 +1466,14 @@ public RenderStyle getRenderStyle()
if (null == renderAs)
return FileContentController.RenderStyle.PAGE;

//Will throw illegal argument exception for other values...
return FileContentController.RenderStyle.valueOf(renderAs.toUpperCase());
try
{
return FileContentController.RenderStyle.valueOf(renderAs.toUpperCase());
}
catch (IllegalArgumentException e)
{
throw new NotFoundException("Invalid renderAs value");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since PAGE appears to be the default, you could default to that for invalid value as well:

EnumUtils.getEnum(FileContentController.RenderStyle.class, renderAs.toUpperCase(), FileContentController.RenderStyle.PAGE);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that but opted to be a little stricter. I don't think we've seen this exception aside from the crawler/scanner so I don't think we need to be overly tolerant.

Good to know about EnumUtils though - that wasn't on my radar.

}

public String getFileSet()
Expand Down