Disallow admin triggering of internal endpoints (#1030)

* Disallow admin triggering of internal endpoints

Stop simply relying on the presence of the X-AppEngine-QueueName as an
indicator that an endpoint has been triggered internally, as this allows
admins to trigger a remote execution vulnerability.

We now supplement this check by ensuring that there is no authenticated user.
Since only an admin user can set these headers, this means that the header
must have been set by an internal request.

Tested:
  In addition to the new unit test, verified on Crash that:
  - Internal requests are still getting authenticated via the internal auth
    mechanism.
  - Admin requests with the X-AppEngine-QueueName header are rejected as
    "unauthorized."

* Reformatted.

core/src/main/java/google/registry/request/auth/AppEngineInternalAuthenticationMechanism.java

@@ -17,6 +17,7 @@ package google.registry.request.auth;
 import static google.registry.request.auth.AuthLevel.APP;
 import static google.registry.request.auth.AuthLevel.NONE;
 
+import com.google.appengine.api.users.UserService;
 import javax.inject.Inject;
 import javax.servlet.http.HttpServletRequest;
 
@@ -24,29 +25,15 @@ import javax.servlet.http.HttpServletRequest;
  * Authentication mechanism which uses the X-AppEngine-QueueName header set by App Engine for
  * internal requests.
  *
- * <p>
+ * <p>Task queue push task requests set this header value to the actual queue name. Cron requests
- * Task queue push task requests set this header value to the actual queue name. Cron requests set
+ * set this header value to __cron, since that's actually the name of the hidden queue used for cron
- * this header value to __cron, since that's actually the name of the hidden queue used for cron
  * requests. Cron also sets the header X-AppEngine-Cron, which we could check, but it's simpler just
  * to check the one.
  *
- * <p>
+ * <p>App Engine allows app admins to set these headers for testing purposes. Because of this, we
- * App Engine allows app admins to set these headers for testing purposes. This means that this auth
+ * also need to verify that the current user is null, indicating that there is no user, to prevent
- * method is somewhat unreliable - any app admin can access any internal endpoint and pretend to be
+ * access by someone with "admin" privileges. If the user is an admin, UserService presumably must
- * the app itself by setting these headers, which would circumvent any finer-grained authorization
+ * return a User object.
- * if we added it in the future (assuming we did not apply it to the app itself). And App Engine's
- * concept of an "admin" includes all project owners, editors and viewers. So anyone with access to
- * the project will be able to access anything the app itself can access.
- *
- * <p>
- * For now, it's probably okay to allow this behavior, especially since it could indeed be
- * convenient for testing. If we wanted to revisit this decision in the future, we have a couple
- * options for locking this down:
- *
- * <ul>
- * <li>1. Always include the result of UserService.getCurrentUser() as the active user</li>
- * <li>2. Validate that the requests came from special AppEngine internal IPs</li>
- * </ul>
  *
  * <p>See <a href=
  * "https://cloud.google.com/appengine/docs/java/taskqueue/push/creating-handlers#reading_request_headers">task
@@ -57,12 +44,16 @@ public class AppEngineInternalAuthenticationMechanism implements AuthenticationM
   // As defined in the App Engine request header documentation.
   private static final String QUEUE_NAME_HEADER = "X-AppEngine-QueueName";
 
+  private UserService userService;
+
   @Inject
-  public AppEngineInternalAuthenticationMechanism() {}
+  public AppEngineInternalAuthenticationMechanism(UserService userService) {
+    this.userService = userService;
+  }
 
   @Override
   public AuthResult authenticate(HttpServletRequest request) {
-    if (request.getHeader(QUEUE_NAME_HEADER) == null) {
+    if (request.getHeader(QUEUE_NAME_HEADER) == null || userService.getCurrentUser() != null) {
       return AuthResult.create(NONE);
     } else {
       return AuthResult.create(APP);

core/src/test/java/google/registry/request/auth/RequestAuthenticatorTest.java

@@ -117,7 +117,7 @@ class RequestAuthenticatorTest {
 
   private RequestAuthenticator createRequestAuthenticator(UserService userService) {
     return new RequestAuthenticator(
-        new AppEngineInternalAuthenticationMechanism(),
+        new AppEngineInternalAuthenticationMechanism(fakeUserService),
         ImmutableList.of(
             new OAuthAuthenticationMechanism(
                 fakeOAuthService,
@@ -173,6 +173,16 @@ class RequestAuthenticatorTest {
     assertThat(authResult.get().userAuthInfo()).isEmpty();
   }
 
+  @Test
+  void testInternalAuth_failForAdminUser() {
+    when(req.getHeader("X-AppEngine-QueueName")).thenReturn("__cron");
+    fakeUserService.setUser(testUser, true /* isAdmin */);
+
+    Optional<AuthResult> authResult = runTest(fakeUserService, AUTH_INTERNAL_OR_ADMIN);
+
+    assertThat(authResult).isEmpty();
+  }
+
   @Test
   void testAnyUserAnyMethod_notLoggedIn() {
     Optional<AuthResult> authResult = runTest(fakeUserService, AUTH_ANY_USER_ANY_METHOD);