[GitHub] [druid] gianm commented on a diff in pull request #13813: Revert PreResponseAuthorizationCheckFilter

["gianm (via GitHub)" <gi...@apache.org>]

gianm commented on code in PR #13813:
URL: https://github.com/apache/druid/pull/13813#discussion_r1109359321


##########
server/src/main/java/org/apache/druid/server/security/PreResponseAuthorizationCheckFilter.java:
##########
@@ -179,21 +179,10 @@ private void handleAuthorizationCheckError(
 
   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.

Review Comment:
   I would say the reason is different. It's not that the filter values giving the user good feedback. It's more that the filter doesn't want to run at all. It's like an assert -- it's intended to never fire in production. But, we have some APIs for which the filter _would_ currently execute on non-200 responses. Those APIs need to be fixed before we can tighten up the filter, else the filter would run in production, which we don't want.



-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org