fix(plugins): desktop lint masks a new RegExp("<script…") only where it is used as a matcher
Review finding (MAJOR-1): masking the first argument of every new RegExp(...)
also hid `el.innerHTML = new RegExp("<script src=x></script>").source`, a real
injection through .source — desktop_surface_findings returned 0 findings for
that line. The literal is now masked only when the constructor is the argument
of .replace/.replaceAll/.split/.match/.matchAll/.search or the receiver of
.test/.exec; every other use (.source, .toString(), template interpolation) is
a string-builder and keeps firing. The existing test gains the .source line
(must flag) and a .test() line (must not).
This commit is contained in:
@@ -43,15 +43,21 @@ _COMMENT = re.compile(r"/\*.*?\*/|(?<![:\w])//[^\n]*", re.S)
|
||||
# still the payload of an ``innerHTML`` write and keeps firing. The lookbehind keeps division
|
||||
# (``a / b / c``) from reading as a literal. The pattern string handed straight to ``new RegExp(``
|
||||
# is the same sanitiser spelled for a dynamic flag (rss-reader split it into ``"<scr"+"ipt"`` to
|
||||
# dodge this rule) — only that first-argument literal is masked, not the flags or anything after.
|
||||
# dodge this rule) — but only where the constructor is USED as a matcher: the argument of a string
|
||||
# method (``html.replace(new RegExp("<script…", flags), '')``) or the receiver of ``.test``/``.exec``.
|
||||
# Anywhere else (``el.innerHTML = new RegExp("<script src=x></script>").source``) the constructor is
|
||||
# a string-builder and its literal keeps firing.
|
||||
_REGEX_LITERAL = re.compile(r"(?<![\w)\]])/(?:[^/\\\n\[]|\\.|\[(?:[^\]\\\n]|\\.)*\])+/[a-z]*")
|
||||
_REGEXP_CTOR_PATTERN = re.compile(r"\bnew\s+RegExp\(\s*(?:\"(?:[^\"\\\n]|\\.)*\"|'(?:[^'\\\n]|\\.)*')")
|
||||
_JS_STRING = r"(?:\"(?:[^\"\\\n]|\\.)*\"|'(?:[^'\\\n]|\\.)*')"
|
||||
_REGEXP_CTOR_MATCHER = re.compile(
|
||||
r"\.(?:replace|replaceAll|split|match|matchAll|search)\(\s*new\s+RegExp\(\s*" + _JS_STRING
|
||||
+ r"|\bnew\s+RegExp\(\s*" + _JS_STRING + r"(?=[^()\n]*\)\s*\.\s*(?:test|exec)\()")
|
||||
_MARKUP_RULES = frozenset({"script injection"})
|
||||
|
||||
|
||||
def _mask_regex_literals(source: str) -> str:
|
||||
masked = _REGEX_LITERAL.sub(lambda m: " " * len(m.group(0)), source)
|
||||
return _REGEXP_CTOR_PATTERN.sub(lambda m: " " * len(m.group(0)), masked)
|
||||
return _REGEXP_CTOR_MATCHER.sub(lambda m: " " * len(m.group(0)), masked)
|
||||
|
||||
|
||||
def desktop_surface_findings(source: str) -> List[Tuple[str, int]]:
|
||||
|
||||
@@ -294,15 +294,20 @@ class TestDesktopSurface:
|
||||
"const tag = document.createElement('script'); tag.src = 'https://evil.example/x.js'; document.head.append(tag)\n"
|
||||
"const dyn = html.replace(new RegExp(\"<script[\\\\s\\\\S]*?<\\\\/script>\", flags), '')\n"
|
||||
"el.innerHTML = new RegExp('x') && '<script>alert(1)</script>'\n"
|
||||
"el.innerHTML = new RegExp(\"<script src=x></script>\").source\n"
|
||||
"if (new RegExp(\"<script\\\\b\", 'i').test(html)) reject()\n"
|
||||
))
|
||||
report = validate_plugin_dir(d)
|
||||
failed = {name: detail for name, ok, detail in report.checks if not ok}
|
||||
assert "desktop surface" in failed
|
||||
assert ":1)" not in failed["desktop surface"]
|
||||
assert ":5)" not in failed["desktop surface"] # the same sanitiser via the RegExp constructor
|
||||
assert ":8)" not in failed["desktop surface"] # a RegExp that only TESTS markup
|
||||
assert "script injection (desktop/plugin.js:3)" in failed["desktop surface"]
|
||||
assert "script injection (desktop/plugin.js:4)" in failed["desktop surface"]
|
||||
assert "script injection (desktop/plugin.js:6)" in failed["desktop surface"]
|
||||
# ``.source`` hands the pattern text back as a string: the constructor is the payload, not a sanitiser.
|
||||
assert "script injection (desktop/plugin.js:7)" in failed["desktop surface"]
|
||||
|
||||
def test_prototype_patch_and_chunk_import_fail(self, tmp_path):
|
||||
d = self._desktop_plugin(tmp_path, (
|
||||
|
||||
@@ -88,9 +88,11 @@ The catalog is designed so you know exactly what you're installing:
|
||||
the obvious moves outside the plugin SDK (patching built-in prototypes,
|
||||
`eval`, importing anything other than `@hermes/plugin-sdk`/`react`,
|
||||
including remote scripts), and the app's loader refuses every non-SDK
|
||||
import again at load time. The lint reads a `<script` regex — a literal or
|
||||
the pattern string of `new RegExp(...)` — as the sanitiser it is, not as
|
||||
injection; a `<script` string written into the DOM still fails. Treat the
|
||||
import again at load time. The lint reads a `<script` regex — a literal, or
|
||||
the pattern string of a `new RegExp(...)` passed straight to
|
||||
`.replace()`/`.split()`/`.match()` or used as `.test()`/`.exec()` — as the
|
||||
sanitiser it is, not as injection; a `<script` string written into the DOM,
|
||||
including one built from `new RegExp(...).source`, still fails. Treat the
|
||||
lint as a review aid, not a
|
||||
guarantee; give Desktop halves the same scrutiny you'd give a Python half.
|
||||
- **Capability declarations.** Entries state up front which tools, hooks, and
|
||||
|
||||
Reference in New Issue
Block a user