ChangeEditModifier: Reject invalid file paths as '400 Bad Request'

Providing an invalid path is a user error and should not result in a '500 Internal Server Error' [1]. [1] AutoRetry: restapi.change.ChangeEdits.Post failed, retry with tracing enabled org.eclipse.jgit.dircache.InvalidPathException: Invalid path: foo/bar/ at org.eclipse.jgit.dircache.DirCacheEntry.checkPath(DirCacheEntry.java:837) at org.eclipse.jgit.dircache.DirCacheEntry.<init>(DirCacheEntry.java:284) at org.eclipse.jgit.dircache.DirCacheEntry.<init>(DirCacheEntry.java:266) at org.eclipse.jgit.dircache.DirCacheEditor.applyEdits(DirCacheEditor.java:163) at org.eclipse.jgit.dircache.DirCacheEditor.finish(DirCacheEditor.java:132) at com.google.gerrit.server.edit.tree.TreeCreator.applyPathEdits(TreeCreator.java:99) at com.google.gerrit.server.edit.tree.TreeCreator.createNewTree(TreeCreator.java:73) at com.google.gerrit.server.edit.tree.TreeCreator.createNewTreeAndGetId(TreeCreator.java:66) at com.google.gerrit.server.edit.ChangeEditModifier.createNewTree(ChangeEditModifier.java:492) at com.google.gerrit.server.edit.ChangeEditModifier.modifyTree(ChangeEditModifier.java:333) at com.google.gerrit.server.edit.ChangeEditModifier.renameFile(ChangeEditModifier.java:300) at com.google.gerrit.server.restapi.change.ChangeEdits$Post.apply(ChangeEdits.java:248) at com.google.gerrit.server.restapi.change.ChangeEdits$Post.apply(ChangeEdits.java:222) at com.google.gerrit.httpd.restapi.RestApiServlet.lambda$invokeRestCollectionModifyViewWithRetry$10(RestApiServlet.java:858) at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78) at com.github.rholder.retry.Retryer.call(Retryer.java:160) at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:560) at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:503) at com.google.gerrit.server.update.RetryableAction.call(RetryableAction.java:172) at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestEndpointWithRetry(RestApiServlet.java:883) at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestCollectionModifyViewWithRetry(RestApiServlet.java:853) at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:563) at javax.servlet.http.HttpServlet.service(HttpServlet.java:717) ... Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: I7693b4a6c39d7b773101ab5770b158433995f23d
2020-01-15 13:58:54 +01:00
parent e836b59cac
commit 36dcc21aad
4 changed files with 49 additions and 20 deletions
--- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java
+++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -51,6 +51,7 @@ import java.sql.Timestamp;
 import java.util.List;
 import java.util.Optional;
 import java.util.TimeZone;
+import org.eclipse.jgit.dircache.InvalidPathException;
 import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.NullProgressMonitor;
@@ -243,13 +244,14 @@ public class ChangeEditModifier {
   * @param filePath the path of the file whose contents should be modified
   * @param newContent the new file content
   * @throws AuthException if the user isn't authenticated or not allowed to use change edits
+   * @throws BadRequestException if the user provided bad input (e.g. invalid file paths)
   * @throws InvalidChangeOperationException if the file already had the specified content
   * @throws PermissionBackendException
   * @throws ResourceConflictException if the project state does not permit the operation
   */
  public void modifyFile(
      Repository repository, ChangeNotes notes, String filePath, RawInput newContent)
-      throws AuthException, InvalidChangeOperationException, IOException,
+      throws AuthException, BadRequestException, InvalidChangeOperationException, IOException,
          PermissionBackendException, ResourceConflictException {
    modifyTree(repository, notes, new ChangeFileContentModification(filePath, newContent));
  }
@@ -262,12 +264,13 @@ public class ChangeEditModifier {
   * @param notes the {@link ChangeNotes} of the change whose change edit should be modified
   * @param file path of the file which should be deleted
   * @throws AuthException if the user isn't authenticated or not allowed to use change edits
+   * @throws BadRequestException if the user provided bad input (e.g. invalid file paths)
   * @throws InvalidChangeOperationException if the file does not exist
   * @throws PermissionBackendException
   * @throws ResourceConflictException if the project state does not permit the operation
   */
  public void deleteFile(Repository repository, ChangeNotes notes, String file)
-      throws AuthException, InvalidChangeOperationException, IOException,
+      throws AuthException, BadRequestException, InvalidChangeOperationException, IOException,
          PermissionBackendException, ResourceConflictException {
    modifyTree(repository, notes, new DeleteFileModification(file));
  }
@@ -281,6 +284,7 @@ public class ChangeEditModifier {
   * @param currentFilePath the current path/name of the file
   * @param newFilePath the desired path/name of the file
   * @throws AuthException if the user isn't authenticated or not allowed to use change edits
+   * @throws BadRequestException if the user provided bad input (e.g. invalid file paths)
   * @throws InvalidChangeOperationException if the file was already renamed to the specified new
   *     name
   * @throws PermissionBackendException
@@ -288,7 +292,7 @@ public class ChangeEditModifier {
   */
  public void renameFile(
      Repository repository, ChangeNotes notes, String currentFilePath, String newFilePath)
-      throws AuthException, InvalidChangeOperationException, IOException,
+      throws AuthException, BadRequestException, InvalidChangeOperationException, IOException,
          PermissionBackendException, ResourceConflictException {
    modifyTree(repository, notes, new RenameFileModification(currentFilePath, newFilePath));
  }
@@ -306,14 +310,14 @@ public class ChangeEditModifier {
   * @throws PermissionBackendException
   */
  public void restoreFile(Repository repository, ChangeNotes notes, String file)
-      throws AuthException, InvalidChangeOperationException, IOException,
+      throws AuthException, BadRequestException, InvalidChangeOperationException, IOException,
          PermissionBackendException, ResourceConflictException {
    modifyTree(repository, notes, new RestoreFileModification(file));
  }

  private void modifyTree(
      Repository repository, ChangeNotes notes, TreeModification treeModification)
-      throws AuthException, IOException, InvalidChangeOperationException,
+      throws AuthException, BadRequestException, IOException, InvalidChangeOperationException,
          PermissionBackendException, ResourceConflictException {
    assertCanEdit(notes);

@@ -358,8 +362,8 @@ public class ChangeEditModifier {
      ChangeNotes notes,
      PatchSet patchSet,
      List<TreeModification> treeModifications)
-      throws AuthException, IOException, InvalidChangeOperationException, MergeConflictException,
-          PermissionBackendException, ResourceConflictException {
+      throws AuthException, BadRequestException, IOException, InvalidChangeOperationException,
+          MergeConflictException, PermissionBackendException, ResourceConflictException {
    assertCanEdit(notes);

    Optional<ChangeEdit> optionalChangeEdit = lookupChangeEdit(notes);
@@ -469,10 +473,15 @@ public class ChangeEditModifier {

  private static ObjectId createNewTree(
      Repository repository, RevCommit baseCommit, List<TreeModification> treeModifications)
-      throws IOException, InvalidChangeOperationException {
-    TreeCreator treeCreator = new TreeCreator(baseCommit);
-    treeCreator.addTreeModifications(treeModifications);
-    ObjectId newTreeId = treeCreator.createNewTreeAndGetId(repository);
+      throws BadRequestException, IOException, InvalidChangeOperationException {
+    ObjectId newTreeId;
+    try {
+      TreeCreator treeCreator = new TreeCreator(baseCommit);
+      treeCreator.addTreeModifications(treeModifications);
+      newTreeId = treeCreator.createNewTreeAndGetId(repository);
+    } catch (InvalidPathException e) {
+      throw new BadRequestException(e.getMessage());
+    }

    if (ObjectId.equals(newTreeId, baseCommit.getTree())) {
      throw new InvalidChangeOperationException("no changes were made");
--- a/java/com/google/gerrit/server/restapi/change/ApplyFix.java
+++ b/java/com/google/gerrit/server/restapi/change/ApplyFix.java
@@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.change;

 import com.google.gerrit.extensions.common.EditInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.Response;
@@ -66,8 +67,8 @@ public class ApplyFix implements RestModifyView<FixResource, Void> {

  @Override
  public Response<EditInfo> apply(FixResource fixResource, Void nothing)
-      throws AuthException, ResourceConflictException, IOException, ResourceNotFoundException,
-          PermissionBackendException {
+      throws AuthException, BadRequestException, ResourceConflictException, IOException,
+          ResourceNotFoundException, PermissionBackendException {
    RevisionResource revisionResource = fixResource.getRevisionResource();
    Project.NameKey project = revisionResource.getProject();
    ProjectState projectState = projectCache.checkedGet(project);
--- a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java
+++ b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java
@@ -118,7 +118,8 @@ public class ChangeEdits implements ChildCollection<ChangeResource, ChangeEditRe

    @Override
    public Response<?> apply(ChangeResource resource, IdString id, Put.Input input)
-        throws AuthException, ResourceConflictException, IOException, PermissionBackendException {
+        throws AuthException, ResourceConflictException, BadRequestException, IOException,
+            PermissionBackendException {
      putEdit.apply(resource, id.get(), input.content);
      return Response.none();
    }
@@ -135,7 +136,8 @@ public class ChangeEdits implements ChildCollection<ChangeResource, ChangeEditRe

    @Override
    public Response<?> apply(ChangeResource rsrc, IdString id, Input in)
-        throws IOException, AuthException, ResourceConflictException, PermissionBackendException {
+        throws IOException, AuthException, BadRequestException, ResourceConflictException,
+            PermissionBackendException {
      return deleteContent.apply(rsrc, id.get());
    }
  }
@@ -236,7 +238,8 @@ public class ChangeEdits implements ChildCollection<ChangeResource, ChangeEditRe

    @Override
    public Response<?> apply(ChangeResource resource, Post.Input input)
-        throws AuthException, IOException, ResourceConflictException, PermissionBackendException {
+        throws AuthException, BadRequestException, IOException, ResourceConflictException,
+            PermissionBackendException {
      Project.NameKey project = resource.getProject();
      try (Repository repository = repositoryManager.openRepository(project)) {
        if (isRestoreFile(input)) {
@@ -281,12 +284,14 @@ public class ChangeEdits implements ChildCollection<ChangeResource, ChangeEditRe

    @Override
    public Response<?> apply(ChangeEditResource rsrc, Input input)
-        throws AuthException, ResourceConflictException, IOException, PermissionBackendException {
+        throws AuthException, ResourceConflictException, BadRequestException, IOException,
+            PermissionBackendException {
      return apply(rsrc.getChangeResource(), rsrc.getPath(), input.content);
    }

    public Response<?> apply(ChangeResource rsrc, String path, RawInput newContent)
-        throws ResourceConflictException, AuthException, IOException, PermissionBackendException {
+        throws ResourceConflictException, AuthException, BadRequestException, IOException,
+            PermissionBackendException {
      if (Strings.isNullOrEmpty(path) || path.charAt(0) == '/') {
        throw new ResourceConflictException("Invalid path: " + path);
      }
@@ -320,12 +325,14 @@ public class ChangeEdits implements ChildCollection<ChangeResource, ChangeEditRe

    @Override
    public Response<?> apply(ChangeEditResource rsrc, Input input)
-        throws AuthException, ResourceConflictException, IOException, PermissionBackendException {
+        throws AuthException, BadRequestException, ResourceConflictException, IOException,
+            PermissionBackendException {
      return apply(rsrc.getChangeResource(), rsrc.getPath());
    }

    public Response<?> apply(ChangeResource rsrc, String filePath)
-        throws AuthException, IOException, ResourceConflictException, PermissionBackendException {
+        throws AuthException, BadRequestException, IOException, ResourceConflictException,
+            PermissionBackendException {
      try (Repository repository = repositoryManager.openRepository(rsrc.getProject())) {
        editModifier.deleteFile(repository, rsrc.getNotes(), filePath);
      } catch (InvalidChangeOperationException e) {
--- a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
+++ b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
@@ -24,6 +24,7 @@ import static com.google.gerrit.extensions.common.testing.EditInfoSubject.assert
 import static com.google.gerrit.extensions.restapi.testing.BinaryResultSubject.assertThat;
 import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG;
 import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.concurrent.TimeUnit.SECONDS;
 import static java.util.stream.Collectors.toList;
@@ -52,6 +53,7 @@ import com.google.gerrit.extensions.common.DiffInfo;
 import com.google.gerrit.extensions.common.EditInfo;
 import com.google.gerrit.extensions.common.FileInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.BinaryResult;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -435,6 +437,16 @@ public class ChangeEditIT extends AbstractDaemonTest {
    assertThat(getFileContentOfEdit(changeId, FILE_NAME)).isAbsent();
  }

+  @Test
+  public void renameExistingFileToInvalidPath() throws Exception {
+    createEmptyEditFor(changeId);
+    BadRequestException badRequest =
+        assertThrows(
+            BadRequestException.class,
+            () -> gApi.changes().id(changeId).edit().renameFile(FILE_NAME, "invalid/path/"));
+    assertThat(badRequest.getMessage()).isEqualTo("Invalid path: invalid/path/");
+  }
+
  @Test
  public void createEditByDeletingExistingFileRest() throws Exception {
    adminRestSession.delete(urlEditFile(changeId, FILE_NAME)).assertNoContent();