Fail 'Get Diff' requests for file sizes that exceed 50Mb
We add a new DiffValidator extension point with one implementation that
checks the file size and fails the diff if the file size or size delta
exceeded 50Mb.
The validation is performed in PatchScriptFactory after the file diff is
computed through DiffOperations. This codepath is executed on the 'Get
Diff' request path, but not on 'List Files'.
The validation is done after the diff computation and not before it for
two reasons:
1) 'List Files' and 'Get Diff' both use the exact same logic in
DiffOperations. This was designed that way because some of the fields
that we return with the 'List Files' response require performing the
file diff (e.g. lines_inserted/deleted). If we fail the request
before the diff computation, this will mean that both 'List Files'
and 'Get Diff' will be affected. We don't want to do that so that
existing changes with large file sizes can continue to load and show
the files in the web UI, but only expanding diffs to fail. Though
this can be solved with some refactoring of the code to separate both
code paths and only fail 'Get Diff' requests "before" the diff
computation, doing that will duplicate the code that's already
encapsulated and executed inside the diff cache (reading the tree,
getting file size).
2) Doing the validation after the diff computation is still beneficial,
since we'll fail and skip the remaining code that formats the entire
two sides of the diff (requires loading the file). Also we fail
instead of returning a response of very large size. Note that the
diff computation in DiffOperations only returns the list of edits
(start/end lines and positions at both sides as integers).
Google-Bug-Id: b/288895561
Release-Notes: Fail 'Get Diff' requests for file sizes that exceed 50Mb
Change-Id: I174b58239b67abc8891f5b1c914dca86e10cdb93
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index e0a4269..a6bbeab 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -175,7 +175,9 @@
import com.google.gerrit.server.notedb.DeleteZombieCommentsRefs;
import com.google.gerrit.server.notedb.NoteDbModule;
import com.google.gerrit.server.notedb.StoreSubmitRequirementsOp;
+import com.google.gerrit.server.patch.DiffFileSizeValidator;
import com.google.gerrit.server.patch.DiffOperationsImpl;
+import com.google.gerrit.server.patch.DiffValidator;
import com.google.gerrit.server.patch.PatchListCacheImpl;
import com.google.gerrit.server.patch.PatchScriptFactory;
import com.google.gerrit.server.patch.PatchScriptFactoryForAutoFix;
@@ -403,6 +405,8 @@
.to(SubmitRequirementConfigValidator.class);
DynamicSet.bind(binder(), CommitValidationListener.class).to(PrologRulesWarningValidator.class);
DynamicSet.setOf(binder(), CommentValidator.class);
+ DynamicSet.setOf(binder(), DiffValidator.class);
+ DynamicSet.bind(binder(), DiffValidator.class).to(DiffFileSizeValidator.class);
DynamicSet.setOf(binder(), ChangeMessageModifier.class);
DynamicSet.setOf(binder(), RefOperationValidationListener.class);
DynamicSet.setOf(binder(), OnSubmitValidationListener.class);
diff --git a/java/com/google/gerrit/server/git/LargeObjectException.java b/java/com/google/gerrit/server/git/LargeObjectException.java
index 04db42c..145b631 100644
--- a/java/com/google/gerrit/server/git/LargeObjectException.java
+++ b/java/com/google/gerrit/server/git/LargeObjectException.java
@@ -25,6 +25,10 @@
private static final long serialVersionUID = 1L;
+ public LargeObjectException(String message) {
+ super(message);
+ }
+
public LargeObjectException(String message, org.eclipse.jgit.errors.LargeObjectException cause) {
super(message, cause);
}
diff --git a/java/com/google/gerrit/server/patch/DiffFileSizeValidator.java b/java/com/google/gerrit/server/patch/DiffFileSizeValidator.java
new file mode 100644
index 0000000..14a0f7b
--- /dev/null
+++ b/java/com/google/gerrit/server/patch/DiffFileSizeValidator.java
@@ -0,0 +1,52 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.patch;
+
+import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_CORE_SECTION;
+import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_BIGFILE_THRESHOLD;
+import static org.eclipse.jgit.storage.pack.PackConfig.DEFAULT_BIG_FILE_THRESHOLD;
+
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.git.LargeObjectException;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.inject.Inject;
+import org.eclipse.jgit.lib.Config;
+
+public class DiffFileSizeValidator implements DiffValidator {
+ static final int DEFAULT_MAX_FILE_SIZE = DEFAULT_BIG_FILE_THRESHOLD;
+ private static final String ERROR_MESSAGE =
+ "File size for file %s exceeded the max file size threshold. Threshold = %d bytes, Actual size = %d bytes";
+
+ final int maxFileSize;
+
+ @Inject
+ public DiffFileSizeValidator(@GerritServerConfig Config cfg) {
+ this.maxFileSize =
+ cfg.getInt(CONFIG_CORE_SECTION, CONFIG_KEY_BIGFILE_THRESHOLD, DEFAULT_MAX_FILE_SIZE);
+ }
+
+ @Override
+ public void validate(FileDiffOutput fileDiff) throws LargeObjectException {
+ if (fileDiff.size() > maxFileSize) {
+ throw new LargeObjectException(
+ String.format(ERROR_MESSAGE, fileDiff.getDefaultPath(), maxFileSize, fileDiff.size()));
+ }
+ if (fileDiff.sizeDelta() > maxFileSize) {
+ throw new LargeObjectException(
+ String.format(
+ ERROR_MESSAGE, fileDiff.getDefaultPath(), maxFileSize, fileDiff.sizeDelta()));
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/patch/DiffValidator.java b/java/com/google/gerrit/server/patch/DiffValidator.java
new file mode 100644
index 0000000..aee3c8b
--- /dev/null
+++ b/java/com/google/gerrit/server/patch/DiffValidator.java
@@ -0,0 +1,26 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.patch;
+
+import com.google.gerrit.extensions.annotations.ExtensionPoint;
+import com.google.gerrit.server.git.LargeObjectException;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+
+/** Interface to validate diff outputs. */
+@ExtensionPoint
+public interface DiffValidator {
+ void validate(FileDiffOutput fileDiffOutput)
+ throws LargeObjectException, DiffNotAvailableException;
+}
diff --git a/java/com/google/gerrit/server/patch/DiffValidators.java b/java/com/google/gerrit/server/patch/DiffValidators.java
new file mode 100644
index 0000000..964353d
--- /dev/null
+++ b/java/com/google/gerrit/server/patch/DiffValidators.java
@@ -0,0 +1,37 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.patch;
+
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.git.LargeObjectException;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.inject.Inject;
+
+/** Validates {@link FileDiffOutput}(s) after they are computed by the {@link DiffOperations}. */
+public class DiffValidators {
+ DynamicSet<DiffValidator> diffValidators;
+
+ @Inject
+ public DiffValidators(DynamicSet<DiffValidator> diffValidators) {
+ this.diffValidators = diffValidators;
+ }
+
+ public void validate(FileDiffOutput fileDiffOutput)
+ throws LargeObjectException, DiffNotAvailableException {
+ for (DiffValidator validator : diffValidators) {
+ validator.validate(fileDiffOutput);
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
index 3baa3b1..5015c768 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptFactory.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
@@ -94,6 +94,7 @@
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
private final DiffOperations diffOperations;
+ private final DiffValidators diffValidators;
private final Change.Id changeId;
@@ -109,6 +110,7 @@
PermissionBackend permissionBackend,
ProjectCache projectCache,
DiffOperations diffOperations,
+ DiffValidators diffValidators,
@Assisted ChangeNotes notes,
@Assisted String fileName,
@Assisted("patchSetA") @Nullable PatchSet.Id patchSetA,
@@ -124,6 +126,7 @@
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
this.diffOperations = diffOperations;
+ this.diffValidators = diffValidators;
this.fileName = fileName;
this.psa = patchSetA;
@@ -144,6 +147,7 @@
PermissionBackend permissionBackend,
ProjectCache projectCache,
DiffOperations diffOperations,
+ DiffValidators diffValidators,
@Assisted ChangeNotes notes,
@Assisted String fileName,
@Assisted int parentNum,
@@ -159,6 +163,7 @@
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
this.diffOperations = diffOperations;
+ this.diffValidators = diffValidators;
this.fileName = fileName;
this.psa = null;
@@ -220,13 +225,14 @@
}
private PatchScript getPatchScript(Repository git, ObjectId aId, ObjectId bId)
- throws IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException, LargeObjectException {
FileDiffOutput fileDiffOutput =
aId == null
? diffOperations.getModifiedFileAgainstParent(
notes.getProjectName(), bId, parentNum, fileName, diffPrefs.ignoreWhitespace)
: diffOperations.getModifiedFile(
notes.getProjectName(), aId, bId, fileName, diffPrefs.ignoreWhitespace);
+ diffValidators.validate(fileDiffOutput);
return newBuilder().toPatchScript(git, fileDiffOutput);
}
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
index 9286f47..9107dde 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
@@ -64,6 +64,10 @@
*/
public abstract Optional<String> newPath();
+ public String getDefaultPath() {
+ return oldPath().isPresent() ? oldPath().get() : newPath().get();
+ }
+
/**
* The file mode of the old file at the old git tree diff identified by {@link #oldCommitId()}
* ()}.
diff --git a/javatests/com/google/gerrit/server/patch/DiffValidatorsTest.java b/javatests/com/google/gerrit/server/patch/DiffValidatorsTest.java
new file mode 100644
index 0000000..fa1d09e
--- /dev/null
+++ b/javatests/com/google/gerrit/server/patch/DiffValidatorsTest.java
@@ -0,0 +1,74 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.patch;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.entities.Patch.ChangeType;
+import com.google.gerrit.entities.Patch.FileMode;
+import com.google.gerrit.entities.Patch.PatchType;
+import com.google.gerrit.server.git.LargeObjectException;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.testing.InMemoryModule;
+import com.google.inject.Guice;
+import com.google.inject.Inject;
+import com.google.inject.Injector;
+import java.util.Optional;
+import org.eclipse.jgit.lib.ObjectId;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Test class for {@link DiffValidators}. */
+public class DiffValidatorsTest {
+ @Inject private DiffValidators diffValidators;
+
+ @Before
+ public void setUpInjector() throws Exception {
+ Injector injector = Guice.createInjector(new InMemoryModule());
+ injector.injectMembers(this);
+ }
+
+ @Test
+ public void fileSizeExceeded() {
+ int largeSize = 100000000;
+ FileDiffOutput fileDiff =
+ FileDiffOutput.builder()
+ .oldCommitId(ObjectId.zeroId())
+ .newCommitId(ObjectId.zeroId())
+ .comparisonType(ComparisonType.againstRoot())
+ .changeType(ChangeType.ADDED)
+ .patchType(Optional.of(PatchType.UNIFIED))
+ .oldPath(Optional.empty())
+ .newPath(Optional.of("f.txt"))
+ .oldMode(Optional.empty())
+ .newMode(Optional.of(FileMode.REGULAR_FILE))
+ .headerLines(ImmutableList.of())
+ .edits(ImmutableList.of())
+ .size(largeSize)
+ .sizeDelta(largeSize)
+ .build();
+ Exception thrown =
+ assertThrows(LargeObjectException.class, () -> diffValidators.validate(fileDiff));
+ assertThat(thrown)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "File size for file f.txt exceeded the max file size threshold."
+ + " Threshold = %d bytes, Actual size = %d bytes",
+ DiffFileSizeValidator.DEFAULT_MAX_FILE_SIZE, largeSize));
+ }
+}
