Files: Validate parent option to prevent internal server error
If an invalid parent number is given for a merge commit, the
request fails with an internal server error due to array index
out of bounds. For regular commits, the given parent number is
ignored by the patch list loader, which defaults to parent 1, so
no error occurs.
Validate that the given parent number is sane, and throw a bad
request if it is not.
Add test coverage.
Change-Id: I4d5386be0b95f47aa74dabf0eb04db6a75642c7b
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 23a2f5c..5ecb028 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -926,20 +926,66 @@
public void filesOnMergeCommitChange() throws Exception {
PushOneCommit.Result r = createMergeCommitChange("refs/for/master");
- // list files against auto-merge
+ // List files against auto-merge
assertThat(gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).files().keySet())
.containsExactly(COMMIT_MSG, MERGE_LIST, "foo", "bar");
- // list files against parent 1
+ // List files against parent 1
assertThat(gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).files(1).keySet())
.containsExactly(COMMIT_MSG, MERGE_LIST, "bar");
- // list files against parent 2
+ // List files against parent 2
assertThat(gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).files(2).keySet())
.containsExactly(COMMIT_MSG, MERGE_LIST, "foo");
}
@Test
+ public void filesOnMergeCommitChangeWithInvalidParent() throws Exception {
+ PushOneCommit.Result r = createMergeCommitChange("refs/for/master");
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .files(3)
+ .keySet());
+ assertThat(thrown).hasMessageThat().isEqualTo("invalid parent number: 3");
+ thrown =
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .files(-1)
+ .keySet());
+ assertThat(thrown).hasMessageThat().isEqualTo("invalid parent number: -1");
+ }
+
+ @Test
+ public void listFilesWithInvalidParent() throws Exception {
+ PushOneCommit.Result result1 = createChange();
+ String changeId = result1.getChangeId();
+ PushOneCommit.Result result2 = amendChange(changeId, SUBJECT, "b.txt", "b");
+ String revId2 = result2.getCommit().name();
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(changeId).revision(revId2).files(2).keySet());
+ assertThat(thrown).hasMessageThat().isEqualTo("invalid parent number: 2");
+
+ thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(changeId).revision(revId2).files(-1).keySet());
+ assertThat(thrown).hasMessageThat().isEqualTo("invalid parent number: -1");
+ }
+
+ @Test
public void listFilesOnDifferentBases() throws Exception {
RevCommit initialCommit = getHead(repo());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
index c167e31..03fd07b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
@@ -118,6 +118,7 @@
private final PatchListCache patchListCache;
private final PatchSetUtil psUtil;
private final DynamicItem<AccountPatchReviewStore> accountPatchReviewStore;
+ private final Provider<GetCommit> getCommit;
@Inject
ListFiles(
@@ -128,7 +129,8 @@
GitRepositoryManager gitManager,
PatchListCache patchListCache,
PatchSetUtil psUtil,
- DynamicItem<AccountPatchReviewStore> accountPatchReviewStore) {
+ DynamicItem<AccountPatchReviewStore> accountPatchReviewStore,
+ Provider<GetCommit> getCommit) {
this.db = db;
this.self = self;
this.fileInfoJson = fileInfoJson;
@@ -137,6 +139,7 @@
this.patchListCache = patchListCache;
this.psUtil = psUtil;
this.accountPatchReviewStore = accountPatchReviewStore;
+ this.getCommit = getCommit;
}
public ListFiles setReviewed(boolean r) {
@@ -166,7 +169,11 @@
resource.getChange(),
resource.getPatchSet().getRevision(),
baseResource.getPatchSet()));
- } else if (parentNum > 0) {
+ } else if (parentNum != 0) {
+ int parents = getCommit.get().apply(resource).value().parents.size();
+ if (parentNum < 0 || parentNum > parents) {
+ throw new BadRequestException(String.format("invalid parent number: %d", parentNum));
+ }
r =
Response.ok(
fileInfoJson.toFileInfoMap(
