Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import type { DocViewFilterFn } from '@kbn/unified-doc-viewer/types';
import type { SharePluginStart } from '@kbn/share-plugin/public';
import type { CoreStart } from '@kbn/core-lifecycle-browser';
import type { DataViewField } from '@kbn/data-views-plugin/common';
import { escapeAndPreserveHighlightTags } from '@kbn/discover-utils';
import {
actionFilterForText,
actionFilterOutText,
Expand Down Expand Up @@ -108,11 +109,18 @@ export function CellActionsPopover({
`}
>
<strong>{name}</strong>{' '}
{typeof renderValue === 'function'
? renderValue(value)
: rawValue != null && typeof rawValue !== 'object'
? (rawValue as React.ReactNode)
: value}
{typeof renderValue === 'function' ? (
<>{renderValue(escapeAndPreserveHighlightTags(value))}</>
) : rawValue != null && typeof rawValue !== 'object' ? (
<>{rawValue as React.ReactNode}</>
) : (
<span
// eslint-disable-next-line react/no-danger
dangerouslySetInnerHTML={{
__html: escapeAndPreserveHighlightTags(value),
Comment thread Fixed
}}
/>
)}
</EuiText>
</EuiFlexItem>
<EuiFlexItem grow={false}>
Expand Down Expand Up @@ -225,7 +233,7 @@ export function FieldBadgeWithActions({
<span
// eslint-disable-next-line react/no-danger
dangerouslySetInnerHTML={{
__html: displayValue,
__html: escapeAndPreserveHighlightTags(displayValue),
}}
/>
</EuiBadge>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@ import {
LOG_LEVEL_REGEX,
OTEL_MESSAGE_FIELD,
} from '@kbn/discover-utils';
import { MESSAGE_FIELD } from '@kbn/discover-utils';
import { MESSAGE_FIELD, escapeAndPreserveHighlightTags } from '@kbn/discover-utils';
import type { EuiThemeComputed } from '@elastic/eui';
import { makeHighContrastColor, useEuiTheme } from '@elastic/eui';
import { useKibanaIsDarkMode } from '@kbn/react-kibana-context-theme';
import { escape } from 'lodash';
import { formatJsonDocumentForContent } from './utils';

interface ContentProps extends DataGridCellValueElementProps {
Expand Down Expand Up @@ -105,7 +104,10 @@ export const Content = ({
const isDarkTheme = useKibanaIsDarkMode();

const highlightedValue = useMemo(
() => (value ? getHighlightedMessage(escape(value), row, euiTheme, isDarkTheme) : value),
() =>
value
? getHighlightedMessage(escapeAndPreserveHighlightTags(value), row, euiTheme, isDarkTheme)
: value,
[value, row, euiTheme, isDarkTheme]
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ import {
TRACE_FIELDS,
getMessageFieldWithFallbacks,
} from '@kbn/discover-utils';
import { getAvailableTraceFields } from '@kbn/discover-utils/src';
import { escape } from 'lodash';
import { getAvailableTraceFields, escapeAndPreserveHighlightTags } from '@kbn/discover-utils';
import { Resource } from './resource';
import { Content } from './content';
import {
Expand Down Expand Up @@ -169,7 +168,10 @@ export const SummaryCellPopover = (props: AllSummaryColumnProps) => {
});
const messageCodeBlockProps = formattedValue
? { language: 'json', children: formattedValue }
: { language: 'txt', dangerouslySetInnerHTML: { __html: escape(value ?? '') } };
: {
language: 'txt',
dangerouslySetInnerHTML: { __html: escapeAndPreserveHighlightTags(value ?? '') },
};
Comment thread Fixed
const shouldRenderContent = Boolean(field && value);

const shouldRenderSource = !shouldRenderContent;
Expand Down
2 changes: 2 additions & 0 deletions src/platform/packages/shared/kbn-discover-utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export {
getIgnoredReason,
getMessageFieldWithFallbacks,
getAvailableResourceFields,
getAvailableTraceFields,
getLogLevelFieldWithFallback,
getLogEventTypeFieldWithFallback,
getLogExceptionTypeFieldWithFallback,
Expand All @@ -70,6 +71,7 @@ export {
getSortForSearchSource,
getEsQuerySort,
getTieBreakerFieldName,
escapeAndPreserveHighlightTags,
severityOrder,
} from './src';

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { escapeAndPreserveHighlightTags } from './escape_preserve_highlight_tags';

// Must match the html tags defined in @kbn/field-formats-plugin (html_tags.ts)
const PRE = '<mark class="ffSearch__highlight">';
const POST = '</mark>';

describe('escapeAndPreserveHighlightTags', () => {
it('escapes HTML when there are no highlight tags', () => {
expect(escapeAndPreserveHighlightTags('<hello>world</hello>')).toBe(
'&lt;hello&gt;world&lt;/hello&gt;'
);
});

it('preserves highlight wrappers while escaping the content', () => {
expect(escapeAndPreserveHighlightTags(`${PRE}<hello>${POST}`)).toBe(
`${PRE}&lt;hello&gt;${POST}`
);
});

it('returns only escaped text when there are multiple highlight regions', () => {
expect(escapeAndPreserveHighlightTags(`${PRE}hello${POST} + ${PRE}world${POST}`)).toBe(
'hello + world'
);
});

it('escapes plain <mark> tags that do not match the highlight format', () => {
expect(escapeAndPreserveHighlightTags('<mark><hello></mark>')).toBe(
'&lt;mark&gt;&lt;hello&gt;'
);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { escape } from 'lodash';

// TODO: These constants are duplicated from @kbn/field-formats-plugin (html_tags.ts).
// They are kept locally because packages cannot depend on plugins. This is a temporary
// workaround until we reach an agreement on how to handle formatted/highlighted content
// across packages and plugins.
const HIGHLIGHT_PRE_TAG = '<mark class="ffSearch__highlight">';
const HIGHLIGHT_POST_TAG = '</mark>';
const HIGHLIGHT_TAGS_REGEX = new RegExp(`${HIGHLIGHT_PRE_TAG}|${HIGHLIGHT_POST_TAG}`, 'g');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we consider ReDoS vulnerability here as a potential real threat?
I assume it's not very probable that someone internally changes <mark to <mark*, which could get us stuck in an infinite loop, just asking to be on a safe side.

A potentially safer way would be to use value.replace instead of regex, but I'm not very sure that we actually need it, more like thinking out loud

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really appreciate you taking the time to look into this!

The code scanning flagged this in a previous version of this PR (/<\/?mark[^>]*>/g), which was indeed a vulnerability. Right now, as you say, the only way this could be a problem is if someone modifies the RegExp. I think it’s fine to leave it as is for now, since it’s marked as a duplicate of an existing constant and shouldn’t be changed.

Also, this will be removed in the coming months once we implement a better approach for handling highlighting tags and rendering.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thanks for the explanation!


export function escapeAndPreserveHighlightTags(value: string): string {
const markTags: string[] = [];
const cleanText = value.replace(HIGHLIGHT_TAGS_REGEX, (match) => {
markTags.push(match);
return '';
});

const escapedText = escape(cleanText);

return markTags.length === 2 ? `${markTags[0]}${escapedText}${markTags[1]}` : escapedText;
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export * from './nested_fields';
export * from './get_field_value';
export * from './get_visible_columns';
export * from './convert_value_to_string';
export * from './escape_preserve_highlight_tags';
export * from './sorting';
export { DiscoverFlyouts, dismissAllFlyoutsExceptFor, dismissFlyouts } from './dismiss_flyouts';
export { prepareDataViewForEditing } from './prepare_data_view_for_editing';
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ import {
EuiSpacer,
EuiText,
} from '@elastic/eui';
import { escape } from 'lodash';

import {
getMessageFieldWithFallbacks,
type DataTableRecord,
type LogDocumentOverview,
} from '@kbn/discover-utils';
import { escapeAndPreserveHighlightTags } from '@kbn/discover-utils';
import type { ObservabilityStreamsFeature } from '@kbn/discover-shared-plugin/public';
import type { DataView } from '@kbn/data-views-plugin/common';
import { Badges } from '../badges/badges';
Expand All @@ -46,7 +46,10 @@ export const ContentBreakdown = ({

const messageCodeBlockProps = formattedValue
? { language: 'json', children: formattedValue }
: { language: 'txt', dangerouslySetInnerHTML: { __html: escape(value ?? '') } };
: {
language: 'txt',
dangerouslySetInnerHTML: { __html: escapeAndPreserveHighlightTags(value ?? '') },
Comment thread Fixed
};
const hasMessageField = field && value;

return (
Expand Down
Loading