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
diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java index c05a47d..128388d 100644 --- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -53,6 +53,7 @@ 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; @@ -250,13 +251,14 @@ * @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)); } @@ -269,12 +271,13 @@ * @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)); } @@ -288,6 +291,7 @@ * @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 @@ -295,7 +299,7 @@ */ 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)); } @@ -313,14 +317,14 @@ * @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); @@ -370,8 +374,8 @@ 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); @@ -486,10 +490,15 @@ 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.isEqual(newTreeId, baseCommit.getTree())) { throw new InvalidChangeOperationException("no changes were made");
diff --git a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java index aa36b73..aa82247 100644 --- a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java +++ b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java
@@ -138,7 +138,8 @@ @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()); } } @@ -239,7 +240,8 @@ @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)) { @@ -325,12 +327,14 @@ @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) {
diff --git a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java index b0f183e..2883d8c 100644 --- a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java +++ b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
@@ -59,6 +59,7 @@ 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.server.ChangeMessagesUtil; @@ -436,6 +437,16 @@ } @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(); assertThat(getFileContentOfEdit(changeId, FILE_NAME)).isAbsent();