From ac462fbb892569cee2b2823eb2316f1e8c05ca39 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 23 Jan 2012 11:18:34 +0100 Subject: [PATCH] Use SafeHtml for confirmation dialog Using SafeHtml for formatting the message in the confirmation dialog ensures that unsafe characters are properly escaped. E.g. for the deletion of branches the Gerrit WebUI shows a confirmation dialog to verify that the branches should be really deleted. This confirmation dialog includes the names of the branches to be deleted and is formatted by HTML. When embedding the branch names into the HTML unsafe characters were not escaped. It's not a big security issue because the characters that can be used within a branch name are limited, but it's cleaner to ensure the escaping. Change-Id: I5bf349baf76f27de4c5f24e1123f81310f6357c7 Signed-off-by: Edwin Kempin --- .../gerrit/client/ConfirmationDialog.java | 10 ++++++---- .../client/admin/ProjectBranchesScreen.java | 19 +++++++++++-------- .../gerrit/client/changes/ApprovalTable.java | 13 ++++++------- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ConfirmationDialog.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ConfirmationDialog.java index c4cb770bce..36169db03b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ConfirmationDialog.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ConfirmationDialog.java @@ -19,8 +19,9 @@ import com.google.gwt.event.dom.client.ClickHandler; import com.google.gwt.user.client.DOM; import com.google.gwt.user.client.ui.Button; import com.google.gwt.user.client.ui.FlowPanel; -import com.google.gwt.user.client.ui.HTML; +import com.google.gwt.user.client.ui.Widget; import com.google.gwtexpui.globalkey.client.GlobalKey; +import com.google.gwtexpui.safehtml.client.SafeHtml; import com.google.gwtexpui.user.client.AutoCenterDialogBox; public class ConfirmationDialog extends AutoCenterDialogBox { @@ -28,7 +29,7 @@ public class ConfirmationDialog extends AutoCenterDialogBox { private Button cancelButton; - public ConfirmationDialog(final String dialogTitle, final HTML message, + public ConfirmationDialog(final String dialogTitle, final SafeHtml message, final ConfirmationCallback callback) { super(/* auto hide */false, /* modal */true); setGlassEnabled(true); @@ -59,11 +60,12 @@ public class ConfirmationDialog extends AutoCenterDialogBox { buttons.add(cancelButton); final FlowPanel center = new FlowPanel(); - center.add(message); + final Widget msgWidget = message.toBlockWidget(); + center.add(msgWidget); center.add(buttons); add(center); - message.setWidth("400px"); + msgWidget.setWidth("400px"); setWidget(center); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectBranchesScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectBranchesScreen.java index 892aafcd6f..7c51700125 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectBranchesScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectBranchesScreen.java @@ -40,8 +40,8 @@ import com.google.gwt.user.client.ui.CheckBox; import com.google.gwt.user.client.ui.FlexTable.FlexCellFormatter; import com.google.gwt.user.client.ui.FlowPanel; import com.google.gwt.user.client.ui.Grid; -import com.google.gwt.user.client.ui.HTML; import com.google.gwt.user.client.ui.Label; +import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder; import com.google.gwtjsonrpc.client.RemoteJsonException; import java.util.HashSet; @@ -232,29 +232,32 @@ public class ProjectBranchesScreen extends ProjectScreen { } void deleteChecked() { - final StringBuilder message = new StringBuilder(); - message.append("").append(Gerrit.C.branchDeletionConfirmationMessage()).append(""); - message.append("

"); + final SafeHtmlBuilder b = new SafeHtmlBuilder(); + b.openElement("b"); + b.append(Gerrit.C.branchDeletionConfirmationMessage()); + b.closeElement("b"); + + b.openElement("p"); final HashSet ids = new HashSet(); for (int row = 1; row < table.getRowCount(); row++) { final Branch k = getRowItem(row); if (k != null && table.getWidget(row, 1) instanceof CheckBox && ((CheckBox) table.getWidget(row, 1)).getValue()) { if (!ids.isEmpty()) { - message.append(",
"); + b.append(",").br(); } - message.append(k.getName()); + b.append(k.getName()); ids.add(k.getNameKey()); } } - message.append("

"); + b.closeElement("p"); if (ids.isEmpty()) { return; } ConfirmationDialog confirmationDialog = new ConfirmationDialog(Gerrit.C.branchDeletionDialogTitle(), - new HTML(message.toString()), new ConfirmationCallback() { + b.toSafeHtml(), new ConfirmationCallback() { @Override public void onOk() { Util.PROJECT_SVC.deleteBranch(getProjectKey(), ids, diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java index 9dac20700e..d775bbc1e0 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java @@ -41,7 +41,6 @@ import com.google.gwt.user.client.Element; import com.google.gwt.user.client.ui.Composite; import com.google.gwt.user.client.ui.FlowPanel; import com.google.gwt.user.client.ui.Grid; -import com.google.gwt.user.client.ui.HTML; import com.google.gwt.user.client.ui.HTMLTable.CellFormatter; import com.google.gwt.user.client.ui.Image; import com.google.gwt.user.client.ui.Panel; @@ -330,15 +329,15 @@ public class ApprovalTable extends Composite { private void askForConfirmation(final String groupName, final int memberCount) { - final StringBuilder message = new StringBuilder(); - message.append(""); - message.append(Util.M.groupManyMembersConfirmation(groupName, - memberCount)); - message.append(""); + final SafeHtmlBuilder b = new SafeHtmlBuilder(); + b.openElement("b"); + b.append(Util.M + .groupManyMembersConfirmation(groupName, memberCount)); + b.closeElement("b"); final ConfirmationDialog confirmationDialog = new ConfirmationDialog(Util.C .approvalTableAddManyReviewersConfirmationDialogTitle(), - new HTML(message.toString()), new ConfirmationCallback() { + b.toSafeHtml(), new ConfirmationCallback() { @Override public void onOk() { addReviewers(reviewers, true);