Check for inbound emails that the change is visible
Via inbound emails users could post comments on changes that were not
visible to them (changes of projects to which they have no read access,
non-visible private changes).
The user posting the comment didn't get added as a reviewer though. This
means that this hole couldn't be used to gain access to private changes.
A visibility check wasn't done so far since we used an InternalQuery
which has enforceVisibility = false by default.
With this change the error message if a change/project doesn't exist or
is not visible is always the same so that users cannot probe the
existence of a project or change.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I98e1af7d0386fd7b29162fd76ef99270d7e0defc
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index 2b4482a..3a35d80 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -232,7 +232,10 @@
throws UpdateException, RestApiException {
try (ManualRequestContext ctx = oneOffRequestContext.openAs(sender)) {
List<ChangeData> changeDataList =
- queryProvider.get().byLegacyChangeId(Change.id(metadata.changeNumber));
+ queryProvider
+ .get()
+ .enforceVisibility(true)
+ .byLegacyChangeId(Change.id(metadata.changeNumber));
if (changeDataList.isEmpty()) {
sendRejectionEmail(message, InboundEmailRejectionSender.Error.CHANGE_NOT_FOUND);
return;
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
index b86aa43..85c0212 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
@@ -16,6 +16,8 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.mock;
@@ -28,7 +30,9 @@
import com.google.common.collect.Streams;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.entities.EmailHeader;
+import com.google.gerrit.entities.Permission;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
@@ -44,6 +48,7 @@
import com.google.gerrit.mail.MailMessage;
import com.google.gerrit.mail.MailProcessingUtil;
import com.google.gerrit.server.mail.receive.MailProcessor;
+import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gerrit.testing.FakeEmailSender.Message;
import com.google.gerrit.testing.TestCommentHelper;
import com.google.inject.Inject;
@@ -61,6 +66,7 @@
@Inject private MailProcessor mailProcessor;
@Inject private AccountOperations accountOperations;
@Inject private TestCommentHelper testCommentHelper;
+ @Inject private ProjectOperations projectOperations;
private static final CommentValidator mockCommentValidator = mock(CommentValidator.class);
@@ -276,6 +282,69 @@
}
@Test
+ public void sendNotificationOnProjectNotFound() throws Exception {
+ String ts =
+ MailProcessingUtil.rfcDateformatter.format(
+ ZonedDateTime.ofInstant(TimeUtil.now(), ZoneId.of("UTC")));
+
+ String changeUrl = canonicalWebUrl.get() + "c/non-existing-project/+/123";
+
+ // Build Message
+ String txt = newPlaintextBody(changeUrl + "/1", "Test Message", null, null);
+ MailMessage.Builder b =
+ messageBuilderWithDefaultFields()
+ .from(user.getNameEmail())
+ .textContent(txt + textFooterForChange(123, ts));
+
+ sender.clear();
+ mailProcessor.process(b.build());
+
+ assertNotifyTo(user);
+ Message message = sender.nextMessage();
+ assertThat(message.body())
+ .contains(
+ "Gerrit Code Review was unable to process your email because the change was not"
+ + " found.");
+ assertThat(message.headers()).containsKey("Subject");
+ }
+
+ @Test
+ public void sendNotificationOnProjectNotVisible() throws Exception {
+ String changeId = createChangeWithReview();
+ ChangeInfo changeInfo = gApi.changes().id(changeId).get();
+
+ String ts =
+ MailProcessingUtil.rfcDateformatter.format(
+ ZonedDateTime.ofInstant(
+ gApi.changes().id(changeId).get().updated.toInstant(), ZoneId.of("UTC")));
+
+ // Block read permissions on the project.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+
+ // Build Message
+ String txt = newPlaintextBody(getChangeUrl(changeInfo) + "/1", "Test Message", null, null);
+ MailMessage.Builder b =
+ messageBuilderWithDefaultFields()
+ .from(user.getNameEmail())
+ .textContent(txt + textFooterForChange(changeInfo._number, ts));
+
+ sender.clear();
+ mailProcessor.process(b.build());
+
+ assertNotifyTo(user);
+ Message message = sender.nextMessage();
+ assertThat(message.body())
+ .contains(
+ "Gerrit Code Review was unable to process your email because the change was not"
+ + " found.");
+ assertThat(message.headers()).containsKey("Subject");
+ }
+
+ @Test
public void sendNotificationOnChangeNotFound() throws Exception {
String changeId = createChangeWithReview();
ChangeInfo changeInfo = gApi.changes().id(changeId).get();
@@ -303,7 +372,39 @@
assertThat(message.body())
.contains(
"Gerrit Code Review was unable to process your email because the change was not"
- + " found.\nMaybe the change got deleted?");
+ + " found.");
+ assertThat(message.headers()).containsKey("Subject");
+ }
+
+ @Test
+ public void sendNotificationOnChangeNotVisible() throws Exception {
+ String changeId = createChangeWithReview();
+ ChangeInfo changeInfo = gApi.changes().id(changeId).get();
+
+ String ts =
+ MailProcessingUtil.rfcDateformatter.format(
+ ZonedDateTime.ofInstant(
+ gApi.changes().id(changeId).get().updated.toInstant(), ZoneId.of("UTC")));
+
+ // Make change private so that it's no visible to user.
+ gApi.changes().id(changeId).setPrivate(true);
+
+ // Build Message
+ String txt = newPlaintextBody(getChangeUrl(changeInfo) + "/1", "Test Message", null, null);
+ MailMessage.Builder b =
+ messageBuilderWithDefaultFields()
+ .from(user.getNameEmail())
+ .textContent(txt + textFooterForChange(changeInfo._number, ts));
+
+ sender.clear();
+ mailProcessor.process(b.build());
+
+ assertNotifyTo(user);
+ Message message = sender.nextMessage();
+ assertThat(message.body())
+ .contains(
+ "Gerrit Code Review was unable to process your email because the change was not"
+ + " found.");
assertThat(message.headers()).containsKey("Subject");
}
diff --git a/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy b/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
index 1b50efb..a9e0a44 100644
--- a/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
+++ b/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
@@ -68,7 +68,8 @@
{template InboundEmailRejection_CHANGE_NOT_FOUND kind="text"}
Gerrit Code Review was unable to process your email because the change was not found.
{\n}
- Maybe the change got deleted?
+ Maybe the project doesn't exist or is not visible? Maybe the change is not visible or got
+ deleted?
{call InboundEmailRejectionFooter /}
{/template}
diff --git a/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy b/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
index 9662497..3444b7f 100644
--- a/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
+++ b/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
@@ -87,7 +87,8 @@
Gerrit Code Review was unable to process your email because the change was not found.
</p>
<p>
- Maybe the change got deleted?
+ Maybe the project doesn't exist or is not visible? Maybe the change is not visible or got
+ deleted?
<p>
{call InboundEmailRejectionFooterHtml /}
{/template}
