Notes about eslint-plugin-security

[fregante]

(This is a non-issue, just for discussion purposes)

security/detect-object-injection has a lot of false positives. Could a solution be in this long SO answer? https://stackoverflow.com/q/57960770

Related (but no solution here)

• A more relevant "detect-object-injection" eslint-community/eslint-plugin-security#21

[twschiller]

I don't mind having to write warning suppressions with justification. The main thing is getting the number of down to a level where we're not in a "boy who cried wolf situation" during development and PR review

In "normal" application code, we should by and large be able to use Map or another safe alternative

The main areas where Map wouldn't be feasible right now:

• Redux code: because the state has objects keyed by dynamic fields. Mitigated by 1) validation of ids, 2) use of Immer by Redux tool kit? State shape could be re-written, bu persisted state would need to be migrated
• Brick templating: this is actually the area where prototype pollution and similar concerns are relevant. We can/should eventually be executing templating in the sandbox
• "Page script" getters and setters: another area we can't avoid, and where prototype pollution is relevant. This can't be sandboxed. For getters, we should be freezing/sealing. For setters we should be sanitizing property name and checking for hasOwnProperty, etc.

Overall, this becomes more relevant when we introduce the public marketplace. In that world, we can also "flag" certain kinds of bricks as higher risk due to their potential behavior

[fregante]

I've seen this justification a few times in the code:

pixiebrix-extension/src/background/auth.ts

Lines 71 to 73 in 86fd023

	// OK because we're guarding with hasOwnProperty
	// eslint-disable-next-line security/detect-object-injection,@typescript-eslint/no-dynamic-delete
	delete current[key];

If that's correct, what do you think about creating a safeGet/safeSet function pair? Like

export function safeGet<
  TKey extends string,
  TObject extends Record<TKey, unknown>
>(object: TObject, key: TKey): TObject[TKey] {
  if (Object.prototype.hasOwnProperty.call(object, key)) {
    // eslint-disable-next-line security/detect-object-injection -- Safe!
    return object[key];
  }

  throw new Error(`Key ${key} is not a direct property of object`);
}

(types are ok but are likely improvable)

This would let us change this:

// Make sure we don't install the content script multiple times
// eslint-disable-next-line security/detect-object-injection -- using PIXIEBRIX_SYMBOL
const existing: string = window[PIXIEBRIX_SYMBOL];

to

import {safeGet} from "@/utils";
const existing: string = safeGet(window, PIXIEBRIX_SYMBOL);

[fregante]

Closing as my last attempt at this was not merged:

• #670: Use safe getters/setters to fix most security/detect-object-injection issues permanently #3394