Revert PreResponseAuthorizationCheckFilter (#13813)

Make it permissive like it used to be again so that we
ensure that validation errors make it out.
This commit is contained in:
imply-cheddar 2023-05-19 10:16:43 +09:00 committed by GitHub
parent 51f722b7f1
commit e9fed1445f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 32 additions and 15 deletions

View File

@ -179,21 +179,10 @@ public class PreResponseAuthorizationCheckFilter implements Filter
private static boolean statusShouldBeHidden(int status)
{
// We allow 404s (not found) to not be rewritten to forbidden because consistently returning 404s is a way to leak
// less information when something wasn't able to be done anyway. I.e. if we pretend that the thing didn't exist
// when the authorization fails, then there is no information about whether the thing existed. If we return
// a 403 when authorization fails and a 404 when authorization succeeds, but it doesn't exist, then we have
// leaked that it could maybe exist, if the authentication credentials were good.
//
// We also allow 307s (temporary redirect) to not be hidden as they are used to redirect to the leader.
switch (status) {
case HttpServletResponse.SC_FORBIDDEN:
case HttpServletResponse.SC_NOT_FOUND:
case HttpServletResponse.SC_TEMPORARY_REDIRECT:
return false;
default:
return true;
}
// Hide any 200s because a 200 response could contain stuff that we don't want seen, so we hide that. It's also
// possible that errors can leak information, but that's something we cannot truly fix here. We choose to let
// those error messages through because this filter values giving the user good feedback instead.
return status / 100 == 2;
}
public static void sendJsonError(HttpServletResponse resp, int error, String errorJson, OutputStream outputStream)

View File

@ -106,6 +106,34 @@ public class PreResponseAuthorizationCheckFilterTest
Assert.assertEquals(403, resp.getStatus());
}
@Test
public void testMissingAuthorizationCheck401ResponseAndNotCommitted() throws ServletException, IOException
{
EmittingLogger.registerEmitter(new NoopServiceEmitter());
AuthenticationResult authenticationResult = new AuthenticationResult("so-very-valid", "so-very-valid", null, null);
MockHttpServletRequest req = new MockHttpServletRequest();
req.requestUri = "uri";
req.method = "GET";
req.remoteAddr = "1.2.3.4";
req.remoteHost = "aHost";
MockHttpServletResponse resp = new MockHttpServletResponse();
resp.setStatus(401);
req.attributes.put(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult);
PreResponseAuthorizationCheckFilter filter = new PreResponseAuthorizationCheckFilter(
authenticators,
new DefaultObjectMapper()
);
filter.doFilter(req, resp, (request, response) -> {
});
Assert.assertEquals(401, resp.getStatus());
}
@Test
public void testMissingAuthorizationCheckWithForbidden() throws Exception
{