Skip to content

Commit

Permalink
webview: Escape *everything* that isn't intended as HTML.
Browse files Browse the repository at this point in the history
If a message topic contained HTML metacharacters such as `<` and `&`,
we would nevertheless pass it right through into the rendered HTML.
Because topics are arbitrary text controlled by other users, that is
a bug and potentially a security vulnerability.

I tested with a topic of `hello <strong>topic</strong>`, as a proof
of concept -- and indeed I got the words "hello topic", with the
latter word in bold.

Fix that.  Also fix the many other places our code would recklessly
interpolate into HTML a value which is intended as text, not HTML.

Some of the places we fix happen to never contain HTML metacharacters;
others it'd take some work to be sure, and some look quite suspicious.
Rather than try to be clever to save a few microseconds where the
escaping happens not to matter, apply the only sane policy: *every*
value that isn't intended to function as live HTML, we escape.

The syntax used in this commit is kind of ugly, but it's a darn sight
better than suffering HTML injection, and it was quick to sweep
through systematically.  Nicer syntax to come.
  • Loading branch information
gnprice committed May 23, 2018
1 parent f2ee751 commit 9941453
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 33 deletions.
3 changes: 2 additions & 1 deletion src/webview/html/htmlBody.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* @flow */
import escape from 'lodash.escape';
import messageLoadingList from './messageLoadingListAsHtml';
import htmlScrollToBottom from './htmlScrollToBottom';

Expand All @@ -8,7 +9,7 @@ export default (content: string, showMessagePlaceholders: boolean): string => `
${content}
<div id="message-loading" class="${showMessagePlaceholders ? '' : 'hidden'}">
<div id="message-loading" class="${escape(showMessagePlaceholders ? '' : 'hidden')}">
${messageLoadingList}
</div>
Expand Down
15 changes: 8 additions & 7 deletions src/webview/html/messageAsHtml.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
/* @flow */
import escape from 'lodash.escape';
import type { FlagsState, ReactionType, RealmEmojiType } from '../../types';
import { shortTime } from '../../utils/date';
import messageTagsAsHtml from './messageTagsAsHtml';
import messageReactionListAsHtml from './messageReactionListAsHtml';

const messageDiv = (id: number, msgClass: string, flags: Object): string =>
`<div
class="message ${msgClass}"
id="msg-${id}"
data-msg-id="${id}"
${flags.map(flag => `data-${flag}="true" `).join('')}
class="message ${escape(msgClass)}"
id="msg-${escape(id)}"
data-msg-id="${escape(id)}"
${flags.map(flag => `data-${escape(flag)}="true" `).join('')}
>`;

const messageSubheader = ({
Expand All @@ -23,10 +24,10 @@ const messageSubheader = ({
}) => `
<div class="subheader">
<div class="username">
${fromName}
${escape(fromName)}
</div>
<div class="timestamp">
${shortTime(new Date(timestamp * 1000), twentyFourHourTime)}
${escape(shortTime(new Date(timestamp * 1000), twentyFourHourTime))}
</div>
</div>
`;
Expand Down Expand Up @@ -113,7 +114,7 @@ const fullMessageAsHtml = ({
}: FullMessageProps) => `
${messageDiv(id, 'message-full', flags)}
<div class="avatar">
<img src="${avatarUrl}" class="avatar-img" data-email="${fromEmail}">
<img src="${escape(avatarUrl)}" class="avatar-img" data-email="${escape(fromEmail)}">
</div>
<div class="content">
${messageSubheader({ fromName, timestamp, twentyFourHourTime })}
Expand Down
32 changes: 19 additions & 13 deletions src/webview/html/messageHeaderAsHtml.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* @flow */
import escape from 'lodash.escape';
import type { Auth, Message, Narrow, Subscription } from '../../types';
import {
isStreamNarrow,
Expand Down Expand Up @@ -35,9 +36,9 @@ export default ({
<div
class="header-wrapper header topic-text"
data-narrow="${topicNarrowStr}"
data-msg-id="${item.id}"
data-msg-id="${escape(item.id)}"
>
${item.match_subject || item.subject}
${escape(item.match_subject || item.subject)}
</div>
`;
}
Expand All @@ -51,12 +52,15 @@ export default ({
const topicNarrowStr = stringifyNarrow(topicNarrow(item.display_recipient, item.subject));

return `
<div class="header-wrapper stream-header" data-msg-id="${item.id}">
<div class="header stream-text" style="color: ${textColor}; background: ${backgroundColor}" data-narrow="${streamNarrowStr}">
# ${item.display_recipient}
<div class="header-wrapper stream-header" data-msg-id="${escape(item.id)}">
<div class="header stream-text"
style="color: ${escape(textColor)};
background: ${escape(backgroundColor)}"
data-narrow="${streamNarrowStr}">
# ${escape(item.display_recipient)}
</div>
<div class="header topic-text" data-narrow="${topicNarrowStr}">
${item.match_subject || item.subject}
${escape(item.match_subject || item.subject)}
</div>
</div>
`;
Expand All @@ -80,13 +84,15 @@ export default ({
const privateNarrowStr = stringifyNarrow(narrowObj);

return `
<div class="header-wrapper private-header header" data-narrow="${privateNarrowStr}" data-msg-id="${
item.id
}">
${recipients
.map(r => r.full_name)
.sort()
.join(', ')}
<div class="header-wrapper private-header header"
data-narrow="${privateNarrowStr}"
data-msg-id="${escape(item.id)}">
${escape(
recipients
.map(r => r.full_name)
.sort()
.join(', '),
)}
</div>
`;
}
Expand Down
12 changes: 7 additions & 5 deletions src/webview/html/messageReactionAsHtml.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
/* @flow */
import escape from 'lodash.escape';
import type { ReactionType } from '../../types';
import emojiMap from '../../emoji/emojiMap';

const getRealmEmojiHtml = (realmEmoji: ReactionType): string =>
`<img class="realm-reaction" src="${realmEmoji.source_url}"/>
`<img class="realm-reaction" src="${escape(realmEmoji.source_url)}"/>
`;

export default (messageId: number, reaction: ReactionType, realmEmoji: Object): string =>
`<span onClick="" class="reaction${reaction.selfReacted ? ' self-voted' : ''}" data-name="${
reaction.name
}" data-code="${reaction.code}" data-type="${reaction.type}">${
`<span onClick="" class="reaction${escape(reaction.selfReacted ? ' self-voted' : '')}"
data-name="${escape(reaction.name)}"
data-code="${escape(reaction.code)}"
data-type="${escape(reaction.type)}">${
realmEmoji[reaction.name]
? getRealmEmojiHtml(realmEmoji[reaction.name])
: emojiMap[reaction.name]
} ${reaction.count}
} ${escape(reaction.count)}
</span>`;
3 changes: 2 additions & 1 deletion src/webview/html/messageTagsAsHtml.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* @flow */
import escape from 'lodash.escape';
import distanceInWordsToNow from 'date-fns/distance_in_words_to_now';

export default (flags: Object, timeEdited?: number): string => {
Expand All @@ -8,7 +9,7 @@ export default (flags: Object, timeEdited?: number): string => {
const editedTime = timeEdited ? distanceInWordsToNow(timeEdited * 1000) : '';

return `<div class="message-tags">
${timeEdited ? `<span class="message-tag">edited ${editedTime} ago</span>` : ''}
${timeEdited ? `<span class="message-tag">edited ${escape(editedTime)} ago</span>` : ''}
${flags.indexOf('starred') > -1 ? '<span class="message-tag">starred</span>' : ''}
</div>
`;
Expand Down
9 changes: 5 additions & 4 deletions src/webview/html/messageTypingAsHtml.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* @flow */
import escape from 'lodash.escape';
import type { User } from '../../types';
import { getFullUrl } from '../../utils/url';
import { getGravatarFromEmail } from '../../utils/avatar';
Expand All @@ -7,10 +8,10 @@ const typingAvatar = (realm: string, user: User): string => `
<div class="avatar">
<img
class="avatar-img"
data-email="${user.email}"
src="${
user.avatar_url ? getFullUrl(user.avatar_url, realm) : getGravatarFromEmail(user.email)
}">
data-email="${escape(user.email)}"
src="${escape(
user.avatar_url ? getFullUrl(user.avatar_url, realm) : getGravatarFromEmail(user.email),
)}">
</div>
`;

Expand Down
5 changes: 3 additions & 2 deletions src/webview/html/timeRowAsHtml.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
/* @flow */
import escape from 'lodash.escape';
import type { Message } from '../../types';
import { humanDate } from '../../utils/date';

export default (timestamp: number, nextMessage: Message): string => `
<div class="timerow" data-msg-id="${nextMessage.id}">
<div class="timerow" data-msg-id="${escape(nextMessage.id)}">
<div class="timerow-left"></div>
${humanDate(new Date(timestamp * 1000))}
${escape(humanDate(new Date(timestamp * 1000)))}
<div class="timerow-right"></div>
</div>
`;

0 comments on commit 9941453

Please sign in to comment.