Skip to content
Open
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
46 changes: 46 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,19 @@ module.exports = {
name: 'lodash/fp/assocPath',
message: 'Please use @elastic/safer-lodash-set instead',
},
{
name: 'lodash',
importNames: ['template'],
message: 'lodash.template is not allowed due to security concerns',
},
{
name: 'lodash.template',
message: 'lodash.template is not allowed due to security concerns',
},
{
name: 'lodash/template',
message: 'lodash.template is not allowed due to security concerns',
},
],
},
],
Expand All @@ -627,6 +640,14 @@ module.exports = {
name: 'lodash/setWith',
message: 'Please use @elastic/safer-lodash-set instead',
},
{
name: 'lodash.template',
message: 'lodash.template is not allowed due to security concerns',
},
{
name: 'lodash/template',
message: 'lodash.template is not allowed due to security concerns',
},
],
},
],
Expand Down Expand Up @@ -672,6 +693,16 @@ module.exports = {
property: 'assocPath',
message: 'Please use @elastic/safer-lodash-set instead',
},
{
object: 'lodash',
property: 'template',
message: 'lodash.template is not allowed due to security concerns',
},
{
object: '_',
property: 'template',
message: 'lodash.template is not allowed due to security concerns',
},
],
},
},
Expand Down Expand Up @@ -741,6 +772,21 @@ module.exports = {
'error',
{
patterns: ['lodash/*', '!lodash/fp'],
paths: [
{
name: 'lodash',
importNames: ['template'],
message: 'lodash.template is not allowed due to security concerns',
},
{
name: 'lodash.template',
message: 'lodash.template is not allowed due to security concerns',
},
{
name: 'lodash/template',
message: 'lodash.template is not allowed due to security concerns',
},
],
},
],
},
Expand Down
5 changes: 5 additions & 0 deletions changelogs/fragments/10920.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
breaking:
- Remove usage of lodash template ([#10920](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10920))

infra:
- Add in eslint rule to prevent the use of lodash template ([#10920](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10920))
6 changes: 2 additions & 4 deletions src/plugins/data/common/field_formats/converters/color.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,13 @@
*/

import { i18n } from '@osd/i18n';
import { findLast, cloneDeep, template, escape } from 'lodash';
import { findLast, cloneDeep, escape } from 'lodash';
import { OSD_FIELD_TYPES } from '../../osd_field_types/types';
import { FieldFormat } from '../field_format';
import { HtmlContextTypeConvert, FIELD_FORMAT_IDS } from '../types';
import { asPrettyString } from '../utils';
import { DEFAULT_CONVERTER_COLOR } from '../constants/color_default';

const convertTemplate = template('<span style="<%- style %>"><%- val %></span>');

export class ColorFormat extends FieldFormat {
static id = FIELD_FORMAT_IDS.COLOR;
static title = i18n.translate('data.fieldFormats.color.title', {
Expand Down Expand Up @@ -78,6 +76,6 @@ export class ColorFormat extends FieldFormat {
let style = '';
if (color.text) style += `color: ${color.text};`;
if (color.background) style += `background-color: ${color.background};`;
return convertTemplate({ val, style });
return `<span style="${escape(style)}">${escape(asPrettyString(val))}</span>`;
};
}
152 changes: 152 additions & 0 deletions src/plugins/data/common/field_formats/converters/source.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,156 @@ describe('Source Format', () => {
'<span>{&quot;foo&quot;:&quot;bar&quot;,&quot;number&quot;:42,&quot;hello&quot;:&quot;&lt;h1&gt;World&lt;/h1&gt;&quot;,&quot;also&quot;:&quot;with \\&quot;quotes\\&quot; or &#39;single quotes&#39;&quot;}</span>'
);
});

describe('templateHtml rendering', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adds in tests for testing the output of htmlConvert within source.ts, since this wasn't in there beforehand.

test('should render a definition list with field name and value', () => {
const mockIndexPattern = {
formatHit: jest.fn().mockReturnValue({
fieldName: 'field value',
}),
};

const hit = {
_source: { fieldName: 'field value' },
};

const field = { name: 'fieldName' };

const result = convertHtml(hit, { field, hit, indexPattern: mockIndexPattern });

expect(result).toContain('<dl class="source truncate-by-height">');
expect(result).toContain('<dt>fieldName:</dt>');
expect(result).toContain('<dd>field value</dd>');
expect(mockIndexPattern.formatHit).toHaveBeenCalledWith(hit);
});

test('should render multiple fields in the definition list', () => {
const mockIndexPattern = {
formatHit: jest.fn().mockReturnValue({
firstName: 'John',
lastName: 'Doe',
age: '30',
}),
};

const hit = {
_source: { firstName: 'John', lastName: 'Doe', age: 30 },
};

const field = { name: 'firstName' };

const result = convertHtml(hit, { field, hit, indexPattern: mockIndexPattern });

expect(result).toContain('<dt>firstName:</dt>');
expect(result).toContain('<dd>John</dd>');
expect(result).toContain('<dt>lastName:</dt>');
expect(result).toContain('<dd>Doe</dd>');
expect(result).toContain('<dt>age:</dt>');
expect(result).toContain('<dd>30</dd>');
});

test('should escape HTML in field names to prevent XSS', () => {
const mockIndexPattern = {
formatHit: jest.fn().mockReturnValue({
'<script>alert("xss")</script>': 'malicious field name',
}),
};

const hit = {
_source: { '<script>alert("xss")</script>': 'malicious field name' },
};

const field = { name: 'test' };

const result = convertHtml(hit, { field, hit, indexPattern: mockIndexPattern });

// Field name should be escaped
expect(result).toContain('&lt;script&gt;alert(&quot;xss&quot;)&lt;/script&gt;');
// Should NOT contain executable script tag
expect(result).not.toContain('<script>alert("xss")</script>');
});

test('should preserve HTML markup in pre-formatted field values', () => {
const mockIndexPattern = {
formatHit: jest.fn().mockReturnValue({
colorField: '<span style="color: red">red text</span>',
urlField: '<a href="http://example.com">link</a>',
highlightField: 'text with <mark>highlighted</mark> term',
}),
};

const hit = {
_source: {
colorField: 'red text',
urlField: 'http://example.com',
highlightField: 'text with highlighted term',
},
};

const field = { name: 'test' };

const result = convertHtml(hit, { field, hit, indexPattern: mockIndexPattern });

// HTML markup from formatters should be preserved (not escaped)
expect(result).toContain('<span style="color: red">red text</span>');
expect(result).toContain('<a href="http://example.com">link</a>');
expect(result).toContain('text with <mark>highlighted</mark> term');
});

test('should use indexPattern.formatHit to format field values', () => {
const mockFormatHit = jest.fn().mockReturnValue({
formattedField: 'formatted value',
});

const mockIndexPattern = {
formatHit: mockFormatHit,
};

const hit = {
_source: { formattedField: 'raw value' },
};

const field = { name: 'formattedField' };

convertHtml(hit, { field, hit, indexPattern: mockIndexPattern });

// Verify that indexPattern.formatHit was called with the hit
expect(mockFormatHit).toHaveBeenCalledWith(hit);
expect(mockFormatHit).toHaveBeenCalledTimes(1);
});

test('should include highlighted fields from hit.highlight', () => {
const mockIndexPattern = {
formatHit: jest.fn().mockReturnValue({
highlightedField: 'text with <mark>match</mark>',
regularField: 'regular value',
}),
};

const hit = {
_source: {
highlightedField: 'text with match',
regularField: 'regular value',
},
highlight: {
highlightedField: ['text with <mark>match</mark>'],
},
};

const field = { name: 'test' };

const result = convertHtml(hit, { field, hit, indexPattern: mockIndexPattern });

// Both highlighted and regular fields should be present
expect(result).toContain('<dt>highlightedField:</dt>');
expect(result).toContain('<dd>text with <mark>match</mark></dd>');
expect(result).toContain('<dt>regularField:</dt>');
expect(result).toContain('<dd>regular value</dd>');

// Highlighted field should appear before regular field
const highlightedIndex = result.indexOf('highlightedField');
const regularIndex = result.indexOf('regularField');
expect(highlightedIndex).toBeLessThan(regularIndex);
});
});
});
47 changes: 17 additions & 30 deletions src/plugins/data/common/field_formats/converters/source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,41 +28,13 @@
* under the License.
*/

import { template, escape, keys } from 'lodash';
import { escape, keys } from 'lodash';
import { shortenDottedString } from '../../utils';
import { OSD_FIELD_TYPES } from '../../osd_field_types/types';
import { FieldFormat } from '../field_format';
import { TextContextTypeConvert, HtmlContextTypeConvert, FIELD_FORMAT_IDS } from '../types';
import { UI_SETTINGS } from '../../constants';

/**
* Remove all of the whitespace between html tags
* so that inline elements don't have extra spaces.
*
* If you have inline elements (span, a, em, etc.) and any
* amount of whitespace around them in your markup, then the
* browser will push them apart. This is ugly in certain
* scenarios and is only fixed by removing the whitespace
* from the html in the first place (or ugly css hacks).
*
* @param {string} html - the html to modify
* @return {string} - modified html
*/
function noWhiteSpace(html: string) {
const TAGS_WITH_WS = />\s+</g;
return html.replace(TAGS_WITH_WS, '><');
}

const templateHtml = `
<dl class="source truncate-by-height">
<% defPairs.forEach(function (def) { %>
<dt><%- def[0] %>:</dt>
<dd><%= def[1] %></dd>
<%= ' ' %>
<% }); %>
</dl>`;
const doTemplate = template(noWhiteSpace(templateHtml));

export class SourceFormat extends FieldFormat {
static id = FIELD_FORMAT_IDS._SOURCE;
static title = '_source';
Expand Down Expand Up @@ -92,6 +64,21 @@ export class SourceFormat extends FieldFormat {
pairs.push([newField, val]);
}, []);

return doTemplate({ defPairs: highlightPairs.concat(sourcePairs) });
const defPairs = highlightPairs.concat(sourcePairs);
/**
* Build HTML without whitespace between tags to prevent extra spacing.
*
* If you have inline elements (span, a, em, etc.) and any amount of
* whitespace around them in your markup, then the browser will push
* them apart. This is ugly in certain scenarios and is only fixed by
* removing the whitespace from the html in the first place (or ugly css hacks).
*
* Note: The space after </dd> is intentional to provide proper spacing between
* definition pairs.
*/
const pairHtml = defPairs
.map((def) => `<dt>${escape(def[0])}:</dt><dd>${def[1]}</dd> `)
.join('');
return `<dl class="source truncate-by-height">${pairHtml}</dl>`;
};
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,8 @@ import { Tooltip } from '../components/tooltip';
import { Chart } from './_chart';
import { TimeMarker } from './time_marker';
import { seriesTypes } from './point_series/series_types';
import touchdownTmplHtml from '../partials/touchdown.tmpl.html';

const seriTypes = seriesTypes;
const touchdownTmpl = _.template(touchdownTmplHtml);
/**
* Line Chart Visualization
*
Expand Down Expand Up @@ -191,7 +189,14 @@ export class PointSeries extends Chart {
}

function textFormatter() {
return touchdownTmpl(callPlay(d3.event));
const { wholeBucket } = callPlay(d3.event);
return `<p class="visTooltip__header">
<i class="fa fa-info-circle visTooltip__headerIcon"></i>
<span class="visTooltip__headerText">
${wholeBucket ? 'Part of this bucket' : 'This area'}
may contain partial data. The selected time range does not fully cover it.
</span>
</p>`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

were u able to test this visually by any chance?

}

const endzoneTT = new Tooltip('endzones', this.handler.el, textFormatter, null);
Expand Down
1 change: 0 additions & 1 deletion src/setup_node_env/harden/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,3 @@
*/

require('./child_process');
require('./lodash_template');
Loading
Loading