Skip to content

Commit

Permalink
feat(editor): Implement HTML sanitization for Notification and Messag…
Browse files Browse the repository at this point in the history
…e components (n8n-io#4081)

* feat(editor): Implement HTML sanitization when using `dangerouslyUseHTMLString` option of Notification and Message components

* 🐛 Implement mechanism to allow for A href actions from locale strings

* 🐛 Prevent link action default

* ♻️ Use `xss` library instead of `sanitize-html` to handle sanitization

* 🔥 Remove `onLinkClick` functionality of `$showMessage`
  • Loading branch information
OlegIvaniv authored Sep 13, 2022
1 parent 3f2fce7 commit 9eac090
Show file tree
Hide file tree
Showing 8 changed files with 99 additions and 35 deletions.
2 changes: 2 additions & 0 deletions packages/editor-ui/src/App.vue
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@ import { mapGetters } from 'vuex';
import { userHelpers } from './components/mixins/userHelpers';
import { addHeaders, loadLanguage } from './plugins/i18n';
import { restApi } from '@/components/mixins/restApi';
import { globalLinkActions } from '@/components/mixins/globalLinkActions';
export default mixins(
showMessage,
userHelpers,
restApi,
globalLinkActions,
).extend({
name: 'App',
components: {
Expand Down
3 changes: 2 additions & 1 deletion packages/editor-ui/src/components/PageAlert.vue
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import mixins from 'vue-typed-mixins';
import { showMessage } from './mixins/showMessage';
import { ElMessageComponent } from 'element-ui/types/message';
import { sanitizeHtml } from '@/utils';
export default mixins(
showMessage,
Expand All @@ -28,7 +29,7 @@ export default mixins(
},
mounted() {
this.alert = this.$showAlert({
message: this.message,
message: sanitizeHtml(this.message),
type: 'warning',
duration: 0,
showClose: true,
Expand Down
48 changes: 48 additions & 0 deletions packages/editor-ui/src/components/mixins/globalLinkActions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/**
* Creates event listeners for `data-action` attribute to allow for actions to be called from locale without using
* unsafe onclick attribute
*/
import Vue from 'vue';

export const globalLinkActions = Vue.extend({
data(): {[key: string]: {[key: string]: Function}} {
return {
customActions: {},
};
},
mounted() {
window.addEventListener('click', this.delegateClick);
this.$root.$on('registerGlobalLinkAction', this.registerCustomAction);
},
destroyed() {
window.removeEventListener('click', this.delegateClick);
this.$root.$off('registerGlobalLinkAction', this.registerCustomAction);
},
computed: {
availableActions(): {[key: string]: Function} {
return {
reload: this.reload,
...this.customActions,
};
},
},
methods: {
registerCustomAction(key: string, action: Function) {
this.customActions[key] = action;
},
delegateClick(e: MouseEvent) {
const clickedElement = e.target;
if (!(clickedElement instanceof Element) || clickedElement.tagName !== 'A') return;

const actionAttribute = clickedElement.getAttribute('data-action');
if(actionAttribute && typeof this.availableActions[actionAttribute] === 'function') {
e.preventDefault();
this.availableActions[actionAttribute]();
}
},
reload() {
window.location.reload();
},
},
});

15 changes: 6 additions & 9 deletions packages/editor-ui/src/components/mixins/pushConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,12 @@ export const pushConnection = mixins(

let action;
if (!isSavingExecutions) {
action = '<a class="open-settings">Turn on saving manual executions</a> and run again to see what happened after this node.';
this.$root.$emit('registerGlobalLinkAction', 'open-settings', async () => {
if (this.$store.getters.isNewWorkflow) await this.saveAsNewWorkflow();
this.$store.dispatch('ui/openModal', WORKFLOW_SETTINGS_MODAL_KEY);
});

action = '<a data-action="open-settings">Turn on saving manual executions</a> and run again to see what happened after this node.';
}
else {
action = `<a href="/execution/${activeExecutionId}" target="_blank">View the execution</a> to see what happened after this node.`;
Expand All @@ -246,14 +251,6 @@ export const pushConnection = mixins(
message: `${action} <a href="https://docs.n8n.io/nodes/n8n-nodes-base.wait/" target="_blank">More info</a>`,
type: 'success',
duration: 0,
onLinkClick: async (e: HTMLLinkElement) => {
if (e.classList.contains('open-settings')) {
if (this.$store.getters.isNewWorkflow) {
await this.saveAsNewWorkflow();
}
this.$store.dispatch('ui/openModal', WORKFLOW_SETTINGS_MODAL_KEY);
}
},
});
} else if (runDataExecuted.finished !== true) {
this.$titleSet(workflow.name as string, 'ERROR');
Expand Down
30 changes: 7 additions & 23 deletions packages/editor-ui/src/components/mixins/showMessage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { ExecutionError } from 'n8n-workflow';
import { ElMessageBoxOptions } from 'element-ui/types/message-box';
import { ElMessage, ElMessageComponent, ElMessageOptions, MessageType } from 'element-ui/types/message';
import { isChildOf } from './helpers';
import { sanitizeHtml } from '@/utils';

let stickyNotificationQueue: ElNotificationComponent[] = [];

Expand All @@ -17,6 +18,8 @@ export const showMessage = mixins(externalHooks).extend({
track = true,
) {
messageData.dangerouslyUseHTMLString = true;
messageData.message = messageData.message ? sanitizeHtml(messageData.message) : messageData.message;

if (messageData.position === undefined) {
messageData.position = 'bottom-right';
}
Expand Down Expand Up @@ -47,7 +50,6 @@ export const showMessage = mixins(externalHooks).extend({
duration?: number,
customClass?: string,
closeOnClick?: boolean,
onLinkClick?: (e: HTMLLinkElement) => void,
type?: MessageType,
}) {
// eslint-disable-next-line prefer-const
Expand All @@ -64,26 +66,6 @@ export const showMessage = mixins(externalHooks).extend({
};
}

if (config.onLinkClick) {
const onLinkClick = (e: MouseEvent) => {
if (e && e.target && config.onLinkClick && isChildOf(notification.$el, e.target as Element)) {
const target = e.target as HTMLElement;
if (target && target.tagName === 'A') {
config.onLinkClick(e.target as HTMLLinkElement);
}
}
};
window.addEventListener('click', onLinkClick);

const cb = config.onClose;
config.onClose = () => {
window.removeEventListener('click', onLinkClick);
if (cb) {
cb();
}
};
}

notification = this.$showMessage({
title: config.title,
message: config.message,
Expand Down Expand Up @@ -159,7 +141,8 @@ export const showMessage = mixins(externalHooks).extend({
...(type && { type }),
};

await this.$confirm(message, headline, options);
const sanitizedMessage = sanitizeHtml(message);
await this.$confirm(sanitizedMessage, headline, options);
return true;
} catch (e) {
return false;
Expand All @@ -176,7 +159,8 @@ export const showMessage = mixins(externalHooks).extend({
...(type && { type }),
};

await this.$confirm(message, headline, options);
const sanitizedMessage = sanitizeHtml(message);
await this.$confirm(sanitizedMessage, headline, options);
return 'confirmed';
} catch (e) {
return e as string;
Expand Down
2 changes: 1 addition & 1 deletion packages/editor-ui/src/plugins/i18n/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,7 @@
"showMessage.ok": "OK",
"showMessage.showDetails": "Show Details",
"startupError": "Error connecting to n8n",
"startupError.message": "Could not connect to server. <a onclick='window.location.reload(false);'>Refresh</a> to try again",
"startupError.message": "Could not connect to server. <a data-action='reload'>Refresh</a> to try again",
"tagsDropdown.createTag": "Create tag \"{filter}\"",
"tagsDropdown.manageTags": "Manage tags",
"tagsDropdown.noMatchingTagsExist": "No matching tags exist",
Expand Down
32 changes: 32 additions & 0 deletions packages/editor-ui/src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import xss, { friendlyAttrValue } from 'xss';

export const omit = (keyToOmit: string, { [keyToOmit]: _, ...remainder }) => remainder;

export function isObjectLiteral(maybeObject: unknown): maybeObject is { [key: string]: string } {
Expand All @@ -12,3 +14,33 @@ export function isJsonKeyObject(item: unknown): item is {

return Object.keys(item).includes('json');
}

export function sanitizeHtml(dirtyHtml: string) {
const allowedAttributes = ['href','name', 'target', 'title', 'class', 'id'];
const allowedTags = ['p', 'strong', 'b', 'code', 'a', 'br', 'i', 'em', 'small' ];

const sanitizedHtml = xss(dirtyHtml, {
onTagAttr: (tag, name, value) => {
if (tag === 'img' && name === 'src') {
// Only allow http requests to supported image files from the `static` directory
const isImageFile = value.split('#')[0].match(/\.(jpeg|jpg|gif|png|webp)$/) !== null;
const isStaticImageFile = isImageFile && value.startsWith('/static/');
if (!value.startsWith('https://') && !isStaticImageFile) {
return '';
}
}

// Allow `allowedAttributes` and all `data-*` attributes
if(allowedAttributes.includes(name) || name.startsWith('data-')) return `${name}="${friendlyAttrValue(value)}"`;

return;
// Return nothing, means keep the default handling measure
},
onTag: (tag) => {
if(!allowedTags.includes(tag)) return '';
return;
},
});

return sanitizedHtml;
}
2 changes: 1 addition & 1 deletion packages/editor-ui/src/views/NodeView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,7 @@ export default mixins(
if ((data as IExecutionsSummary).waitTill) {
this.$showMessage({
title: this.$locale.baseText('nodeView.thisExecutionHasntFinishedYet'),
message: `<a onclick="window.location.reload(false);">${this.$locale.baseText('nodeView.refresh')}</a> ${this.$locale.baseText('nodeView.toSeeTheLatestStatus')}.<br/> <a href="https://docs.n8n.io/nodes/n8n-nodes-base.wait/" target="_blank">${this.$locale.baseText('nodeView.moreInfo')}</a>`,
message: `<a data-action="reload">${this.$locale.baseText('nodeView.refresh')}</a> ${this.$locale.baseText('nodeView.toSeeTheLatestStatus')}.<br/> <a href="https://docs.n8n.io/nodes/n8n-nodes-base.wait/" target="_blank">${this.$locale.baseText('nodeView.moreInfo')}</a>`,
type: 'warning',
duration: 0,
});
Expand Down

0 comments on commit 9eac090

Please sign in to comment.