Skip to content

Commit

Permalink
fix: allow target="_blank" attribute for user-generated string (fix #…
Browse files Browse the repository at this point in the history
  • Loading branch information
adhrinae authored Oct 21, 2021
1 parent bd180bf commit 248131a
Show file tree
Hide file tree
Showing 4 changed files with 112 additions and 4 deletions.
66 changes: 66 additions & 0 deletions src/js/common/sanitizer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/**
* @fileoverview Sanitizer module in order to prevent XSS attacks.
* @author NHN FE Development Lab <[email protected]>
*/
'use strict';

var DOMPurify = require('dompurify');

// For temporarily saving original target value
var TEMP_TARGET_ATTRIBUTE = 'data-target-temp';

/**
* Add DOMPurify hook to handling exceptional rules for certain HTML attributes.
* Should be set when the calendar instance is created.
*/
function addAttributeHooks() {
DOMPurify.addHook('beforeSanitizeAttributes', function(node) {
var targetValue;
// Preserve default target attribute value
if (node.tagName === 'A') {
targetValue = node.getAttribute('target');

if (targetValue) {
node.setAttribute(TEMP_TARGET_ATTRIBUTE, targetValue);
} else {
// set default value
node.setAttribute('target', '_self');
}
}
});

DOMPurify.addHook('afterSanitizeAttributes', function(node) {
if (node.tagName === 'A' && node.hasAttribute(TEMP_TARGET_ATTRIBUTE)) {
node.setAttribute('target', node.getAttribute(TEMP_TARGET_ATTRIBUTE));
node.removeAttribute(TEMP_TARGET_ATTRIBUTE);

// Additionally set `rel="noopener"` to prevent another security issue.
if (node.getAttribute('target') === '_blank') {
node.setAttribute('rel', 'noopener');
}
}
});
}

/**
* Remove all attribute sanitizing hooks.
* Use it in `Calendar#destroy`.
*/
function removeAttributeHooks() {
DOMPurify.removeAllHooks();
}

/**
* Prevent XSS attack by sanitizing input string values via DOMPurify
* @param {string} str target string value
* @returns {string} sanitized string
*/
function sanitize(str) {
return DOMPurify.sanitize(str);
}

module.exports = {
sanitize: sanitize,
addAttributeHooks: addAttributeHooks,
removeAttributeHooks: removeAttributeHooks
};
4 changes: 2 additions & 2 deletions src/js/controller/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@
*/
'use strict';

var DOMPurify = require('dompurify');
var util = require('tui-code-snippet');
var Schedule = require('../model/schedule');
var ScheduleViewModel = require('../model/viewModel/scheduleViewModel');
var datetime = require('../common/datetime');
var common = require('../common/common');
var Theme = require('../theme/theme');
var tz = require('../common/timezone');
var sanitizer = require('../common/sanitizer');
var TZDate = tz.Date;

var SCHEDULE_VULNERABLE_OPTIONS = ['title', 'body', 'location', 'state', 'category', 'dueDateClass'];
Expand All @@ -24,7 +24,7 @@ var SCHEDULE_VULNERABLE_OPTIONS = ['title', 'body', 'location', 'state', 'catego
function sanitizeOptions(options) {
util.forEachArray(SCHEDULE_VULNERABLE_OPTIONS, function(prop) {
if (options[prop]) {
options[prop] = DOMPurify.sanitize(options[prop]);
options[prop] = sanitizer.sanitize(options[prop]);
}
});

Expand Down
7 changes: 5 additions & 2 deletions src/js/factory/calendar.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

var GA_TRACKING_ID = 'UA-129951699-1';

var DOMPurify = require('dompurify');
var util = require('tui-code-snippet'),
Handlebars = require('handlebars-template-loader/runtime');
var dw = require('../common/dw');
Expand All @@ -20,6 +19,7 @@ var tz = require('../common/timezone');
var TZDate = tz.Date;
var config = require('../config');
var reqAnimFrame = require('../common/reqAnimFrame');
var sanitizer = require('../common/sanitizer');

var mmin = Math.min;

Expand Down Expand Up @@ -679,6 +679,7 @@ function Calendar(container, options) {
* destroy calendar instance.
*/
Calendar.prototype.destroy = function() {
sanitizer.removeAttributeHooks();
this._dragHandler.destroy();
this._controller.off();
this._layout.clear();
Expand Down Expand Up @@ -763,6 +764,8 @@ Calendar.prototype._initialize = function(options) {
this._setAdditionalInternalOptions(this._options);

this.changeView(viewName, true);

sanitizer.addAttributeHooks();
};

/**
Expand All @@ -779,7 +782,7 @@ Calendar.prototype._setAdditionalInternalOptions = function(options) {
return function() {
var template = templateFn.apply(null, arguments);

return DOMPurify.sanitize(template);
return sanitizer.sanitize(template);
};
};
var zones, offsetCalculator;
Expand Down
39 changes: 39 additions & 0 deletions test/app/common/sanitizer.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';

var sanitizer = require('../../../src/js/common/sanitizer');

describe('common/sanitizer', function() {
beforeEach(function() {
sanitizer.addAttributeHooks();
});

afterEach(function() {
sanitizer.removeAttributeHooks();
});

it("should leave target='_blank' attribute and add 'rel=noopener'", function() {
var targetStr = '<a href="https://example.com" target="_blank">A link to open in a new tab</a>';
var expected = '<a href="https://example.com" target="_blank" rel="noopener">A link to open in a new tab</a>';

var result = sanitizer.sanitize(targetStr);

expect(result).toBe(expected);
});

it('should preserve other target attribute values', function() {
var targetStr = '<a href="https://example.com" target="_self"></a>';

var result = sanitizer.sanitize(targetStr);

expect(result).toBe(targetStr);
});

it('should allow a target attribute for anchor tags only', function() {
var targetStr = '<form href="https://example.com" target="_blank"></form>';
var expected = '<form href="https://example.com"></form>';

var result = sanitizer.sanitize(targetStr);

expect(result).toBe(expected);
});
});

0 comments on commit 248131a

Please sign in to comment.