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,36 +17,23 @@
 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;
 
 /**
  * Authentication mechanism which uses the X-AppEngine-QueueName header set by App Engine for
  * internal requests.
  *
- * <p>
- * Task queue push task requests set this header value to the actual queue name. Cron requests set
- * this header value to __cron, since that's actually the name of the hidden queue used for cron
+ * <p>Task queue push task requests set this header value to the actual queue name. Cron requests
+ * set 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>
- * App Engine allows app admins to set these headers for testing purposes. This means that this auth
- * method is somewhat unreliable - any app admin can access any internal endpoint and pretend to be
- * the app itself by setting these headers, which would circumvent any finer-grained authorization
- * 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>App Engine allows app admins to set these headers for testing purposes. Because of this, we
+ * also need to verify that the current user is null, indicating that there is no user, to prevent
+ * access by someone with "admin" privileges. If the user is an admin, UserService presumably must
+ * return a User object.
  *
  * <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 @@ void beforeEach() {
 
   private RequestAuthenticator createRequestAuthenticator(UserService userService) {
     return new RequestAuthenticator(
-        new AppEngineInternalAuthenticationMechanism(),
+        new AppEngineInternalAuthenticationMechanism(fakeUserService),
         ImmutableList.of(
             new OAuthAuthenticationMechanism(
                 fakeOAuthService,
@@ -173,6 +173,16 @@ void testInternalAuth_success() {
     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);