Merge pull request #8817 from openstreetmap/issue-8813

reduce direct html injection, add escaping where necessary

CHANGELOG.md

@@ -14,6 +14,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
 ##### YYYY-MMM-DD
 
 #### :newspaper: News
+#### :shield: Security
 #### :mega: Release Highlights
 #### :boom: Breaking Changes
 #### :tada: New Features
@@ -41,6 +42,8 @@ _Breaking developer changes, which may affect downstream projects or sites that
 
 #### :newspaper: News
 * We maintain a running changelog now! Upcoming changes will be added to the _[Unreleased](#Unreleased)_ section of this changelog as soon as they are ready in the [development version](https://ideditor.netlify.app/) of the iD editor. ([#8805])
+#### :shield: Security
+* Fix missing escaping of external texts and content such as OSM user names, OSM tags, etc. which had opened a way to inject arbitrary HTML into the iD editor, potentially making XSS attacks possible. ([#8813])
 #### :boom: Breaking Changes
 #### :tada: New Features
 #### :sparkles: Usability & Accessibility
@@ -55,6 +58,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
 * Fix glitching out turn restriction minimap on narrow sidebars ([#8792])
 * Fix a bug which made it impossible to switch to a custom TMS imagery layer after using a custom WMS source and vice versa ([#8057])
 #### :earth_asia: Localization
+* deprecate ~`t.html`~ for providing localized texts, which is replaced by the new method `t.append` which directly and safely appends the localized strings to the DOM. ([#8817])
 #### :hourglass: Performance
 #### :mortar_board: Walkthrough / Help
 #### :rocket: Presets
@@ -71,6 +75,8 @@ _Breaking developer changes, which may affect downstream projects or sites that
 [#8800]: https://github.com/openstreetmap/iD/pull/8800
 [#8805]: https://github.com/openstreetmap/iD/issues/8805
 [#8807]: https://github.com/openstreetmap/iD/issues/8807
+[#8813]: https://github.com/openstreetmap/iD/issues/8813
+[#8817]: https://github.com/openstreetmap/iD/pull/8817
 
 # 2.20.2
 ##### 2021-Oct-28

data/core.yaml

@@ -999,7 +999,7 @@ en:
       location: 'This feature was moved by both you and {user}.'
       nodelist: 'Nodes were changed by both you and {user}.'
       memberlist: 'Relation members were changed by both you and {user}.'
-      tags: 'You changed the <b>{tag}</b> tag to "{local}" and {user} changed it to "{remote}".'
+      tags: 'You changed the "{tag}" tag to "{local}" and {user} changed it to "{remote}".'
   success:
     just_edited: "You just edited OpenStreetMap!"
     thank_you: "Thank you for improving the map."

modules/actions/merge_remote_changes.js

@@ -1,5 +1,6 @@
 import deepEqual from 'fast-deep-equal';
 import { diff3Merge } from 'node-diff3';
+import { escape } from 'lodash';
 
 import { t } from '../core/localizer';
 import { actionDeleteMultiple } from './delete_multiple';
@@ -14,7 +15,7 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, discardTag
 
 
     function user(d) {
-        return (typeof formatUser === 'function') ? formatUser(d) : d;
+        return (typeof formatUser === 'function') ? formatUser(d) : escape(d);
     }
 
 
@@ -31,7 +32,7 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, discardTag
             return target.update({loc: remote.loc});
         }
 
-        _conflicts.push(t('merge_remote_changes.conflict.location', { user: user(remote.user) }));
+        _conflicts.push(t.html('merge_remote_changes.conflict.location', { user: { html: user(remote.user) } }));
         return target;
     }
 
@@ -64,7 +65,7 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, discardTag
                 } else if (deepEqual(c.o, c.b)) {  // only changed locally
                     nodes.push.apply(nodes, c.a);
                 } else {       // changed both locally and remotely
-                    _conflicts.push(t('merge_remote_changes.conflict.nodelist', { user: user(remote.user) }));
+                    _conflicts.push(t.html('merge_remote_changes.conflict.nodelist', { user: { html: user(remote.user) } }));
                     break;
                 }
             }
@@ -118,7 +119,7 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, discardTag
                 if (remote.visible) {
                     target = mergeLocation(remote, target);
                 } else {
-                    _conflicts.push(t('merge_remote_changes.conflict.deleted', { user: user(remote.user) }));
+                    _conflicts.push(t.html('merge_remote_changes.conflict.deleted', { user: { html: user(remote.user) } }));
                 }
 
                 if (_conflicts.length !== ccount) break;
@@ -149,7 +150,7 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, discardTag
             return target.update({members: remote.members});
         }
 
-        _conflicts.push(t('merge_remote_changes.conflict.memberlist', { user: user(remote.user) }));
+        _conflicts.push(t.html('merge_remote_changes.conflict.memberlist', { user: { html: user(remote.user) } }));
         return target;
     }
 
@@ -176,9 +177,8 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, discardTag
 
             if (o[k] !== b[k] && a[k] !== b[k]) {    // changed remotely..
                 if (o[k] !== a[k]) {      // changed locally..
-                    _conflicts.push(t('merge_remote_changes.conflict.tags',
-                        { tag: k, local: a[k], remote: b[k], user: user(remote.user) }));
-
+                    _conflicts.push(t.html('merge_remote_changes.conflict.tags',
+                        { tag: k, local: a[k], remote: b[k], user: { html: user(remote.user) } }));
                 } else {                  // unchanged locally, accept remote change..
                     if (b.hasOwnProperty(k)) {
                         tags[k] = b[k];
@@ -224,7 +224,7 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, discardTag
                 return graph.replace(target);
 
             } else {
-                _conflicts.push(t('merge_remote_changes.conflict.deleted', { user: user(remote.user) }));
+                _conflicts.push(t.html('merge_remote_changes.conflict.deleted', { user: { html: user(remote.user) } }));
                 return graph;  // do nothing
             }
         }

modules/behavior/draw_way.js

@@ -447,7 +447,7 @@ export function behaviorDrawWay(context, wayID, mode, startGraph) {
                 context.ui().flash
                     .duration(4000)
                     .iconName('#iD-icon-no')
-                    .label(t('operations.follow.error.needs_more_initial_nodes'))();
+                    .label(t.html('operations.follow.error.needs_more_initial_nodes'))();
                 return;
             }
 
@@ -461,7 +461,7 @@ export function behaviorDrawWay(context, wayID, mode, startGraph) {
                 context.ui().flash
                     .duration(4000)
                     .iconName('#iD-icon-no')
-                    .label(t(`operations.follow.error.intersection_of_multiple_ways.${featureType}`))();
+                    .label(t.html(`operations.follow.error.intersection_of_multiple_ways.${featureType}`))();
                 return;
             }
 
@@ -472,7 +472,7 @@ export function behaviorDrawWay(context, wayID, mode, startGraph) {
                 context.ui().flash
                     .duration(4000)
                     .iconName('#iD-icon-no')
-                    .label(t(`operations.follow.error.intersection_of_different_ways.${featureType}`))();
+                    .label(t.html(`operations.follow.error.intersection_of_different_ways.${featureType}`))();
                 return;
             }
 
@@ -501,7 +501,7 @@ export function behaviorDrawWay(context, wayID, mode, startGraph) {
             context.ui().flash
                 .duration(4000)
                 .iconName('#iD-icon-no')
-                .label(t('operations.follow.error.unknown'))();
+                .label(t.html('operations.follow.error.unknown'))();
         }
     }
 

modules/core/localizer.js

@@ -1,3 +1,5 @@
+import { escape } from 'lodash-es';
+
 import { fileFetcher } from './file_fetcher';
 import { utilDetect } from '../util/detect';
 import { utilStringQs } from '../util';
@@ -347,14 +349,42 @@ export function coreLocalizer() {
     };
 
     // Returns the localized text wrapped in an HTML element encoding the locale info
+    /**
+     * @deprecated This method is considered deprecated. Instead, use the direct DOM manipulating
+     *             method `t.append`.
+     */
     localizer.t.html = function(stringId, replacements, locale) {
-        const info = localizer.tInfo(stringId, replacements, locale);
-        // text may be empty or undefined if `replacements.default` is
-        return info.text ? localizer.htmlForLocalizedText(info.text, info.locale) : '';
+      // replacement string might be html unsafe, so we need to escape it except if it is explicitly marked as html code
+      replacements = Object.assign({}, replacements);
+      for (var k in replacements) {
+        if (typeof replacements[k] === 'string') {
+          replacements[k] = escape(replacements[k]);
+        }
+        if (typeof replacements[k] === 'object' && typeof replacements[k].html === 'string') {
+          replacements[k] = replacements[k].html;
+        }
+      }
+
+      const info = localizer.tInfo(stringId, replacements, locale);
+      // text may be empty or undefined if `replacements.default` is
+      if (info.text) {
+        return `<span class="localized-text" lang="${info.locale || 'und'}">${info.text}</span>`;
+      } else {
+        return '';
+      }
     };
 
-    localizer.htmlForLocalizedText = function(text, localeCode) {
-        return `<span class="localized-text" lang="${localeCode || 'unknown'}">${text}</span>`;
+    // Adds localized text wrapped as an HTML span element with locale info to the DOM
+    localizer.t.append = function(stringId, replacements, locale) {
+      return function(selection) {
+        const info = localizer.tInfo(stringId, replacements, locale);
+        return selection.append('span')
+            .attr('class', 'localized-text')
+            .attr('lang', info.locale || 'und')
+            .text((replacements && replacements.prefix || '')
+                + info.text
+                + (replacements &&replacements.suffix || ''));
+      };
     };
 
     localizer.languageName = (code, options) => {

modules/core/uploader.js

@@ -1,4 +1,5 @@
 import { dispatch as d3_dispatch } from 'd3-dispatch';
+import { escape } from 'lodash-es';
 
 import { fileFetcher } from './file_fetcher';
 import { actionDiscardTags } from '../actions/discard_tags';
@@ -218,7 +219,7 @@ export function coreUploader(context) {
                 };
             }
             function formatUser(d) {
-                return '<a href="' + osm.userURL(d) + '" target="_blank">' + d + '</a>';
+                return '<a href="' + osm.userURL(d) + '" target="_blank">' + escape(d) + '</a>';
             }
             function entityName(entity) {
                 return utilDisplayName(entity) || (utilDisplayType(entity.id) + ' ' + entity.id);

modules/modes/drag_node.js

@@ -239,7 +239,7 @@ export function modeDragNode(context) {
                 context.ui().flash
                     .duration(4000)
                     .iconName('#iD-icon-no')
-                    .label(t('operations.connect.' + isInvalid,
+                    .label(t.html('operations.connect.' + isInvalid,
                         { relation: presetManager.item('type/restriction').name() }
                     ))();
             }
@@ -248,7 +248,7 @@ export function modeDragNode(context) {
             context.ui().flash
                 .duration(3000)
                 .iconName('#iD-icon-no')
-                .label(t('self_intersection.error.' + errorID))();
+                .label(t.html('self_intersection.error.' + errorID))();
         } else {
             if (nope) {   // about to un-nope, remove hint
                 context.ui().flash

modules/modes/draw_area.js

@@ -12,7 +12,7 @@ export function modeDrawArea(context, wayID, startGraph, button) {
         .on('rejectedSelfIntersection.modeDrawArea', function() {
             context.ui().flash
                 .iconName('#iD-icon-no')
-                .label(t('self_intersection.error.areas'))();
+                .label(t.html('self_intersection.error.areas'))();
         });
 
     mode.wayID = wayID;

modules/modes/draw_line.js

@@ -12,7 +12,7 @@ export function modeDrawLine(context, wayID, startGraph, button, affix, continui
         .on('rejectedSelfIntersection.modeDrawLine', function() {
             context.ui().flash
                 .iconName('#iD-icon-no')
-                .label(t('self_intersection.error.lines'))();
+                .label(t.html('self_intersection.error.lines'))();
         });
 
     mode.wayID = wayID;

modules/modes/select.js

@@ -400,7 +400,7 @@ export function modeSelect(context, selectedIDs) {
                         .duration(4000)
                         .iconName('#iD-icon-no')
                         .iconClass('operation disabled')
-                        .label(t('operations.scale.' + disabled + '.' + multi))();
+                        .label(t.html('operations.scale.' + disabled + '.' + multi))();
                 } else {
                     const pivot = context.projection(extent.center());
                     const annotation = t('operations.scale.annotation.' + (isUp ? 'up' : 'down') + '.feature', { n: selectedIDs.length });

modules/renderer/background_source.js

@@ -1,5 +1,6 @@
 import { geoArea as d3_geoArea, geoMercatorRaw as d3_geoMercatorRaw } from 'd3-geo';
 import { json as d3_json } from 'd3-fetch';
+import { escape } from 'lodash';
 
 import { t, localizer } from '../core/localizer';
 import { geoExtent, geoSphericalDistance } from '../geo';
@@ -68,19 +69,19 @@ export function rendererBackgroundSource(data) {
 
     source.name = function() {
         var id_safe = source.id.replace(/\./g, '<TX_DOT>');
-        return t('imagery.' + id_safe + '.name', { default: _name });
+        return t('imagery.' + id_safe + '.name', { default: escape(_name) });
     };
 
 
     source.label = function() {
         var id_safe = source.id.replace(/\./g, '<TX_DOT>');
-        return t.html('imagery.' + id_safe + '.name', { default: _name });
+        return t.html('imagery.' + id_safe + '.name', { default: escape(_name) });
     };
 
 
     source.description = function() {
         var id_safe = source.id.replace(/\./g, '<TX_DOT>');
-        return t.html('imagery.' + id_safe + '.description', { default: _description });
+        return t.html('imagery.' + id_safe + '.description', { default: escape(_description) });
     };
 
 

modules/renderer/tile_layer.js

@@ -238,17 +238,19 @@ export function rendererTileLayer(context) {
 
             debug
                 .selectAll('.tile-label-debug-coord')
-                .html(function(d) { return d[2] + ' / ' + d[0] + ' / ' + d[1]; });
+                .text(function(d) { return d[2] + ' / ' + d[0] + ' / ' + d[1]; });
 
             debug
                 .selectAll('.tile-label-debug-vintage')
                 .each(function(d) {
                     var span = d3_select(this);
                     var center = context.projection.invert(tileCenter(d));
                     _source.getMetadata(center, d, function(err, result) {
-                        span.html((result && result.vintage && result.vintage.range) ||
-                            t('info_panels.background.vintage') + ': ' + t('info_panels.background.unknown')
-                        );
+                        if (result && result.vintage && result.vintage.range) {
+                          span.text(result.vintage.range);
+                        } else {
+                          span.html(t.html('info_panels.background.vintage') + ': ' + t.html('info_panels.background.unknown'));
+                        }
                     });
                 });
         }

modules/services/kartaview.js

@@ -276,22 +276,22 @@ export default {
         controlsEnter
             .append('button')
             .on('click.back', step(-1))
-            .html('◄');
+            .text('◄');
 
         controlsEnter
             .append('button')
             .on('click.rotate-ccw', rotate(-90))
-            .html('⤿');
+            .text('⤿');
 
         controlsEnter
             .append('button')
             .on('click.rotate-cw', rotate(90))
-            .html('⤾');
+            .text('⤾');
 
         controlsEnter
             .append('button')
             .on('click.forward', step(1))
-            .html('►');
+            .text('►');
 
         wrapEnter
             .append('div')
@@ -428,7 +428,7 @@ export default {
 
         var wrap = context.container().select('.photoviewer .kartaview-wrapper');
         var imageWrap = wrap.selectAll('.kartaview-image-wrap');
-        var attribution = wrap.selectAll('.photo-attribution').html('');
+        var attribution = wrap.selectAll('.photo-attribution').text('');
 
         wrap
             .transition()
@@ -455,30 +455,30 @@ export default {
                     .attr('class', 'captured_by')
                     .attr('target', '_blank')
                     .attr('href', 'https://kartaview.org/user/' + encodeURIComponent(d.captured_by))
-                    .html('@' + d.captured_by);
+                    .text('@' + d.captured_by);
 
                 attribution
                     .append('span')
-                    .html('|');
+                    .text('|');
             }
 
             if (d.captured_at) {
                 attribution
                     .append('span')
                     .attr('class', 'captured_at')
-                    .html(localeDateString(d.captured_at));
+                    .text(localeDateString(d.captured_at));
 
                 attribution
                     .append('span')
-                    .html('|');
+                    .text('|');
             }
 
             attribution
                 .append('a')
                 .attr('class', 'image-link')
                 .attr('target', '_blank')
                 .attr('href', 'https://kartaview.org/details/' + d.sequence_id + '/' + d.sequence_index)
-                .html('kartaview.org');
+                .text('kartaview.org');
         }
 
         return this;