Vojtech Szocs has posted comments on this change. Change subject: webadmin: unescape cell tooltips string values ......................................................................
Patch Set 2: For SafeHtmlCellWithTooltip, I think the answer to "is SafeHtml value in getTooltip method already escaped?" always depends on code that calls ColumnResizeCellTable.createHeader(Column,SafeHtml) method. In case table header text contains HTML, the value is always assumed to be safe (passed directly via ColumnResizeCellTable.addColumn(Column,SafeHtml) *or* indirectly via ColumnResizeCellTable.createHeader(Column,String,boolean) where allowHtml=true) and therefore it's NOT escaped. So it seems that strings like this: foo'bar are coming right from GWT i18n (Constants) implementation, i.e. the input value is already escaped without any code on our side. In general, cells render potentially unsafe values and therefore HTML rendered by these cells should be always escaped: * TextCellWithTooltip.render already escapes the (String) value --> used ONLY for content (not for header) * SafeHtmlCellWithTooltip.render however does NOT escape the (SafeHtml) value --> this is OK for header but NOT OK for content, we should distingish these two cases by creating dedicated cell class Now, for the tooltip, since the value can potentially be escaped due to GWT i18n, we still *have* to do "new HTML(valueAsString).getText()" anyway, i.e. un-escape the potentially-already-escaped value. > 1. Why is it relevant for TextCellWithTooltip? The value should already be an > unescaped string; shouldn't we simply return the value? I.e. remove > 'getEscapedValue' invocation (and maybe unescape the value again just to be > safe...) You're right, TextCellWithTooltip is currently used only for table content cells and getTooltip method shouldn't call getEscapedValue, but just return raw value (tooltip will be interpreted as raw string). To be perfectly safe, TextCellWithTooltip.getTooltip should do "new HTML(valueAsString).getText()" since the value can potentially contain HTML content. Guys, is it OK if I make above-mentioned changes to this patch? * add separate cell class --> SafeHtmlCellWithTooltip dedicated for use with table headers * modify existing SafeHtmlCellWithTooltip.render to escape content (used with table content, as we don't expect table content cell values to contain further HTML) * modify existing TextCellWithTooltip.getTooltip not to use getEscapedValue -- To view, visit http://gerrit.ovirt.org/19018 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2da6812694737c212352afda48fa9a50e97f8d60 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
