Skip to content

Commit

Permalink
Migrate codebase to use Object.hasOwn instead of Object.hasOwnProperty (
Browse files Browse the repository at this point in the history
elastic#186829)

## Summary

This PR has breadth, but not depth. This adds 3 new `eslint` rules. The
first two protect against the use of code generated from strings (`eval`
and friends), which will not work client-side due to our CSP, and is not
something we wish to support server-side. The last rule aims to prevent
a subtle class of bugs, and to defend against a subset of prototype
pollution exploits:

- `no-new-func` to be compliant with our CSP, and to prevent code
execution from strings server-side:
https://eslint.org/docs/latest/rules/no-new-func
- `no-implied-eval` to be compliant with our CSP, and to prevent code
execution from strings server-side:
https://eslint.org/docs/latest/rules/no-implied-eval. Note that this
function implies that it prevents no-new-func, but I don't see [test
cases](https://github.com/eslint/eslint/blob/main/tests/lib/rules/no-implied-eval.js)
covering this behavior, so I think we should play it safe and enable
both rules.
- `no-prototype-builtins` to prevent accessing shadowed properties:
https://eslint.org/docs/latest/rules/no-prototype-builtins


In order to be compliant with `no-prototype-builtins`, I've migrated all
usages and variants of `Object.hasOwnProperty` to use the newer
[`Object.hasOwn`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwn).
  • Loading branch information
legrego authored Aug 13, 2024
1 parent 386d290 commit 74d8858
Show file tree
Hide file tree
Showing 230 changed files with 458 additions and 437 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,10 @@ export const getSearchEmbeddableFactory = (services: Services) => {
)
.subscribe((next) => {
dataLoading$.next(false);
if (next && next.hasOwnProperty('count') && next.count !== undefined) {
if (next && Object.hasOwn(next, 'count') && next.count !== undefined) {
count$.next(next.count);
}
if (next && next.hasOwnProperty('error')) {
if (next && Object.hasOwn(next, 'error')) {
blockingError$.next(next.error);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export function TriggerContextExample({ uiActionsApi }: Props) {

const renderCellValue = useMemo(() => {
return ({ rowIndex, columnId }: EuiDataGridCellValueElementProps) => {
return rows.hasOwnProperty(rowIndex) ? rows[rowIndex][columnId] : null;
return Object.hasOwn(rows, rowIndex) ? rows[rowIndex][columnId] : null;
};
}, [rows]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe('parseClientOptions', () => {
it('`customHeaders` take precedence to default kibana headers', () => {
const customHeader: Record<string, string> = {};
for (const header in defaultHeaders) {
if (defaultHeaders.hasOwnProperty(header)) {
if (Object.hasOwn(defaultHeaders, header)) {
customHeader[header] = 'foo';
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ function validateAndMerge(
if (k.startsWith('_')) {
throw new Error(`Invalid mapping "${k}". Mappings cannot start with _.`);
}
if (dest.hasOwnProperty(k)) {
if (Object.hasOwn(dest, k)) {
throw new Error(`Cannot redefine core mapping "${k}".`);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export function createIndexMap({ kibanaIndexName, registry, indexMap }: CreateIn
const script = typeDef?.convertToAliasScript;
// Defaults to kibanaIndexName if indexPattern isn't defined
const indexPattern = typeDef?.indexPattern || kibanaIndexName;
if (!map.hasOwnProperty(indexPattern as string)) {
if (!Object.hasOwn(map, indexPattern as string)) {
map[indexPattern] = { typeMappings: {} };
}
map[indexPattern].typeMappings[type] = indexMap[type];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export const buildTypesMappings = (
types: SavedObjectsType[]
): SavedObjectsTypeMappingDefinitions => {
return types.reduce<SavedObjectsTypeMappingDefinitions>((acc, { name: type, mappings }) => {
const duplicate = acc.hasOwnProperty(type);
const duplicate = Object.hasOwn(acc, type);
if (duplicate) {
throw new Error(`Type ${type} is already defined.`);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export abstract class BaseUiSettingsClient implements IUiSettingsClient {
}

isOverridden(key: string) {
return this.overrides.hasOwnProperty(key);
return Object.hasOwn(this.overrides, key);
}

isSensitive(key: string): boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ export abstract class UiSettingsClientCommon extends BaseUiSettingsClient {
}

private assertUpdateAllowed(key: string) {
if (this.overrides.hasOwnProperty(key)) {
if (Object.hasOwn(this.overrides, key)) {
throw new CannotOverrideError(`Unable to update "${key}" because it is overridden`);
}
}
Expand All @@ -113,7 +113,7 @@ export abstract class UiSettingsClientCommon extends BaseUiSettingsClient {
// validate value read from saved objects as it can be changed via SO API
const filteredValues: UserProvided<T> = {};
for (const [key, userValue] of Object.entries(values)) {
if (userValue === null || this.overrides.hasOwnProperty(key)) continue;
if (userValue === null || Object.hasOwn(this.overrides, key)) continue;
try {
this.validateKey(key, userValue);
filteredValues[key] = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export class UiSettingsService
registerInternalRoutes(router);

// Register public routes by default unless the publicApiEnabled config setting is set to false
if (!config.hasOwnProperty('publicApiEnabled') || config.publicApiEnabled === true) {
if (!Object.hasOwn(config, 'publicApiEnabled') || config.publicApiEnabled === true) {
registerRoutes(router);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -472,9 +472,9 @@
}, this.createAnchor = function(row, column) {
return new Anchor(this, row, column)
}, this.$split = 0 === "aaa".split(/a/).length ? function(text) {
return text.replace(/\r\n|\r/g, "\n").split("\n")
return text.replace(/\r\n|\r/g, "\n").split("\n");
} : function(text) {
return text.split(/\r\n|\r|\n/)
return text.split(/\r\n|\r|\n/);
}, this.$detectNewLine = function(text) {
var match = text.match(/^.*?(\r\n|\r|\n)/m);
this.$autoNewLine = match ? match[1] : "\n", this._signal("changeNewLineMode")
Expand Down Expand Up @@ -711,9 +711,9 @@
}, exports.arrayRemove = function(array, value) {
for (var i = 0; array.length >= i; i++) value === array[i] && array.splice(i, 1)
}, exports.escapeRegExp = function(str) {
return str.replace(/([.*+?^${}()|[\]\/\\])/g, "\\$1")
return str.replace(/([.*+?^${}()|[\]\/\\])/g, "\\$1");
}, exports.escapeHTML = function(str) {
return str.replace(/&/g, "&#38;").replace(/"/g, "&#34;").replace(/'/g, "&#39;").replace(/</g, "&#60;")
return str.replace(/&/g, "&#38;").replace(/"/g, "&#34;").replace(/'/g, "&#39;").replace(/</g, "&#60;");
}, exports.getMatchOffsets = function(string, regExp) {
var matches = [];
return string.replace(regExp, function(str) {
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-cli-dev-mode/src/optimizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export class Optimizer {
);

const log = new ToolingLog();
const has = <T extends object>(obj: T, x: any): x is keyof T => obj.hasOwnProperty(x);
const has = <T extends object>(obj: T, x: any): x is keyof T => Object.hasOwn(obj, x);

log.setWriters([
{
Expand Down
4 changes: 2 additions & 2 deletions packages/kbn-data-service/src/search/tabify/tabify_docs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ function flattenAccum(
params?: TabifyDocsOptions
) {
for (const k in obj) {
if (!obj.hasOwnProperty(k)) {
if (!Object.hasOwn(obj, k)) {
continue;
}
const val = obj[k];
Expand Down Expand Up @@ -114,7 +114,7 @@ export function flattenHit(hit: Hit, indexPattern?: DataView, params?: TabifyDoc
// merged, since we would otherwise duplicate values, since ignore_field_values and _source
// contain the same values.
for (const fieldName in hit.ignored_field_values) {
if (!hit.ignored_field_values.hasOwnProperty(fieldName)) {
if (!Object.hasOwn(hit.ignored_field_values, fieldName)) {
continue;
}
const fieldValue = hit.ignored_field_values[fieldName];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ export const createStubClient = (
return { body: { ok: true } };
}),
create: sinon.spy(async ({ index }) => {
if (existingIndices.includes(index) || aliases.hasOwnProperty(index)) {
if (existingIndices.includes(index) || Object.hasOwn(aliases, index)) {
throw createEsClientError('resource_already_exists_exception');
} else {
existingIndices.push(index);
Expand Down
4 changes: 4 additions & 0 deletions packages/kbn-eslint-config/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -312,5 +312,9 @@ module.exports = {
'@kbn/imports/uniform_imports': 'error',
'@kbn/imports/no_unused_imports': 'error',
'@kbn/imports/no_boundary_crossing': 'error',

'no-new-func': 'error',
'no-implied-eval': 'error',
'no-prototype-builtins': 'error',
},
};
4 changes: 2 additions & 2 deletions packages/kbn-expect/expect.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function Assertion (obj, flag, parent) {
this.flags[flag] = true;

for (var i in parent.flags) {
if (parent.flags.hasOwnProperty(i)) {
if (Object.hasOwn(parent.flags, i)) {
this.flags[i] = true;
}
}
Expand All @@ -70,7 +70,7 @@ function Assertion (obj, flag, parent) {
};

for (var fn in Assertion.prototype) {
if (Assertion.prototype.hasOwnProperty(fn) && fn != name) {
if (Object.hasOwn(Assertion.prototype, fn) && fn != name) {
if (typeof this[name] === 'function' && fn === 'length') {
continue;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-flot-charts/lib/jquery_flot.js
Original file line number Diff line number Diff line change
Expand Up @@ -2750,7 +2750,7 @@ Licensed under the MIT license.
var ascending = options.legend.sorted != "descending";
entries.sort(function(a, b) {
return a.label == b.label ? 0 : (
(a.label < b.label) != ascending ? 1 : -1 // Logical XOR
((a.label < b.label) != ascending ? 1 : -1) // Logical XOR
);
});
}
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-handlebars/src/spec/index.regressions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ describe('Regressions', () => {
// It's valid to execute a block against an undefined context, but
// helpers can not do so, so we expect to have an empty object here;
for (const name in this) {
if (Object.prototype.hasOwnProperty.call(this, name)) {
if (Object.hasOwn(this, name)) {
return 'found';
}
}
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-handlebars/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export function allowUnsafeEval() {
try {
// Do not remove the `kbnUnsafeEvalTest` parameter.
// It is used for filtering out expected CSP failures, and must be the first piece of content in this function.
// eslint-disable-next-line no-new-func
new Function('kbnUnsafeEvalTest', 'return true;');
return true;
} catch (e) {
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-handlebars/src/visitor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ export class ElasticHandlebarsVisitor extends Handlebars.Visitor {
if (result == null) {
return result;
}
if (Object.prototype.hasOwnProperty.call(parent, propertyName)) {
if (Object.hasOwn(parent, propertyName)) {
return result;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ const mergeManagedProperties = (
if (
isBasicObjectProp(prop) &&
isManaged(prop) &&
!Object.prototype.hasOwnProperty.call(managedValue, prop.key.value)
!Object.hasOwn(managedValue, prop.key.value)
) {
remove(properties, prop);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export const SelectInput = ({
const options = useMemo(
() =>
optionsProp?.map((option) => ({
text: optionLabels.hasOwnProperty(option) ? optionLabels[option] : option,
text: Object.hasOwn(optionLabels, option) ? optionLabels[option] : option,
value: option,
})),
[optionsProp, optionLabels]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export const useSave = (params: UseSaveParameters) => {
await saveChanges(changes, params.scope);
params.clearChanges();
const requiresReload = params.fields.some(
(setting) => changes.hasOwnProperty(setting.id) && setting.requiresPageReload
(setting) => Object.hasOwn(changes, setting.id) && setting.requiresPageReload
);
if (requiresReload) {
showReloadPagePrompt();
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-mock-idp-plugin/public/role_switcher.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ const createForm = (url: string, fields: Record<string, string>) => {
form.setAttribute('action', url);

for (const key in fields) {
if (!fields.hasOwnProperty(key)) {
if (!Object.hasOwn(fields, key)) {
continue;
}
const input = document.createElement('input');
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-safer-lodash-set/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ set(child, 'foo', 3);
// object and the `parent` object has not been modified:
console.log(child.foo); // 3
console.log(parent.foo); // 1
console.log(Object.prototype.hasOwnProperty.call(child, 'foo')); // true
console.log(Object.hasOwn(child, 'foo')); // true
```

### The `path` must not access function prototypes
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-safer-lodash-set/test/fp_patch_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ setFunctions.forEach(([testPermutations, set, testName]) => {
t.notStrictEqual(arr, result);
t.ok(Array.isArray(result));
Object.keys(expected).forEach((key) => {
t.ok(Object.prototype.hasOwnProperty.call(result, key));
t.ok(Object.hasOwn(result, key));
t.deepEqual(result[key], expected[key]);
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-safer-lodash-set/test/patch_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ setAndSetWithFunctions.forEach(([set, testName]) => {
const arr = [];
set(arr, path, 'foo');
Object.keys(expected).forEach((key) => {
t.ok(Object.prototype.hasOwnProperty.call(arr, key));
t.ok(Object.hasOwn(arr, key));
t.deepEqual(arr[key], expected[key]);
});
t.end();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export function defaultValidationErrorHandler(
//
// The Hapi code we're 'overwriting' can be found here:
// https://github.com/hapijs/hapi/blob/master/lib/validation.js#L102
if (err && err.name === 'ValidationError' && err.hasOwnProperty('output')) {
if (err && err.name === 'ValidationError' && Object.hasOwn(err, 'output')) {
const validationError: HapiValidationError = err as HapiValidationError;
const validationKeys: string[] = [];

Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-std/src/ensure_deep_object.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ function walk(obj: any, keys: string[], value: any, path: string[]) {
return;
}

if (!obj.hasOwnProperty(key)) {
if (!Object.hasOwn(obj, key)) {
obj[key] = {};
}

Expand Down
10 changes: 2 additions & 8 deletions packages/kbn-std/src/ensure_no_unsafe_properties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,6 @@ interface StackItem {
previousKey: string | null;
}

// we have to do Object.prototype.hasOwnProperty because when you create an object using
// Object.create(null), and I assume other methods, you get an object without a prototype,
// so you can't use current.hasOwnProperty
const hasOwnProperty = (obj: any, property: string) =>
Object.prototype.hasOwnProperty.call(obj, property);

const isObject = (obj: any) => typeof obj === 'object' && obj !== null;

// we're using a stack instead of recursion so we aren't limited by the call stack
Expand All @@ -40,11 +34,11 @@ export function ensureNoUnsafeProperties(obj: any) {
continue;
}

if (hasOwnProperty(value, '__proto__')) {
if (Object.hasOwn(value, '__proto__')) {
throw new Error(`'__proto__' is an invalid key`);
}

if (hasOwnProperty(value, 'prototype') && previousKey === 'constructor') {
if (Object.hasOwn(value, 'prototype') && previousKey === 'constructor') {
throw new Error(`'constructor.prototype' is an invalid key`);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export const createAsyncInstance = <T>(
},

get(_, prop, receiver) {
if (loadingTarget.hasOwnProperty(prop)) {
if (Object.hasOwn(loadingTarget, prop)) {
return Reflect.get(loadingTarget as any, prop, receiver);
}

Expand All @@ -84,7 +84,7 @@ export const createAsyncInstance = <T>(
},

getOwnPropertyDescriptor(_, prop) {
if (loadingTarget.hasOwnProperty(prop)) {
if (Object.hasOwn(loadingTarget, prop)) {
return Reflect.getOwnPropertyDescriptor(loadingTarget, prop);
}

Expand All @@ -100,7 +100,7 @@ export const createAsyncInstance = <T>(
},

has(_, prop) {
if (!loadingTarget.hasOwnProperty(prop)) {
if (!Object.hasOwn(loadingTarget, prop)) {
return Reflect.has(loadingTarget, prop);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export function createVerboseInstance(
};
}

if (result.hasOwnProperty('thrown')) {
if (Object.hasOwn(result, 'thrown')) {
log.indent(-2);
throw result.thrown;
}
Expand Down
10 changes: 5 additions & 5 deletions packages/kbn-test/src/jest/setup/polyfills.jsdom.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ Object.defineProperty(window, 'MutationObserver', { value: MutationObserver });

require('whatwg-fetch');

if (!global.URL.hasOwnProperty('createObjectURL')) {
if (!Object.hasOwn(global.URL, 'createObjectURL')) {
Object.defineProperty(global.URL, 'createObjectURL', { value: () => '' });
}

// https://github.com/jsdom/jsdom/issues/2524
if (!global.hasOwnProperty('TextEncoder')) {
if (!Object.hasOwn(global, 'TextEncoder')) {
const customTextEncoding = require('@kayahr/text-encoding');
global.TextEncoder = customTextEncoding.TextEncoder;
global.TextDecoder = customTextEncoding.TextDecoder;
Expand All @@ -29,11 +29,11 @@ if (!global.hasOwnProperty('TextEncoder')) {
// https://github.com/jsdom/jsdom/issues/2555
global.Blob = require('blob-polyfill').Blob;

if (!global.hasOwnProperty('ResizeObserver')) {
if (!Object.hasOwn(global, 'ResizeObserver')) {
global.ResizeObserver = require('resize-observer-polyfill');
}

if (!global.hasOwnProperty('Worker')) {
if (!Object.hasOwn(global, 'Worker')) {
class Worker {
constructor(stringUrl) {
this.url = stringUrl;
Expand All @@ -49,7 +49,7 @@ if (!global.hasOwnProperty('Worker')) {

// Mocking matchMedia to resolve TypeError: window.matchMedia is not a function
// For more info, see https://jestjs.io/docs/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom
if (!global.hasOwnProperty('matchMedia')) {
if (!Object.hasOwn(global, 'matchMedia')) {
Object.defineProperty(global, 'matchMedia', {
writable: true,
// eslint-disable-next-line no-undef
Expand Down
Loading

0 comments on commit 74d8858

Please sign in to comment.