From f006ef15133d60f543371d2addf2f1c03a5505ce Mon Sep 17 00:00:00 2001 From: benoit74 Date: Fri, 13 Nov 2026 13:14:50 +0000 Subject: [PATCH] Align this. rewriting with wabac.js's cleaner implementation We proposed webrecorder/wabac.js#395 for the newline/dot detection bug in replace_this_prop, but wabac.js landed webrecorder/wabac.js#398 instead, a cleaner fix for the same bug with slightly different edge case behavior: a leading `this.` at the very start of a script is now rewritten too, and a preceding `$` only blocks the rewrite when directly adjacent (a `.` still blocks it regardless of the amount of whitespace in between). --- CHANGELOG.md | 1 + src/zimscraperlib/rewriting/js.py | 52 +++++++++++++++------------- tests/rewriting/test_js_rewriting.py | 21 ++++++++--- 3 files changed, 44 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eeed6de..25210cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fix JS rewriting of `this.` so it reliably detects a leading newline (and prepends the needed `;`) regardless of the amount of whitespace, and no longer wrongly rewrites `this` when preceded by `.`/`$` and several spaces (#381) +- Align JS rewriting of `this.` with the cleaner implementation wabac.js landed instead of ours (webrecorder/wabac.js#398, superseding our own webrecorder/wabac.js#395): a leading `this.` at the very start of a script is now rewritten too, and a preceding `$` only blocks the rewrite when directly adjacent (a `.` still blocks it regardless of the amount of whitespace in between) - Patch vendored `wombat.js` so `location.href`/`.search`/`.pathname` (and the rest of the `Location` getters) prefer `wb_info.url` over re-deriving a value via `extractOriginalURL`, fixing their value for any archived page whose captured URL has a query string (#382, webrecorder/wombat#244) - Keep escaped Unicode characters in CSS strings instead of turning them into literal characters when rewriting (#377) - Patch vendored `wombat.js` so element properties (`img.src`, `script.src`, `link.href`, `a.href` and its derived `.pathname`/`.search`/..., `Attr.value`) return and forget a server-side-rewriter-recorded original the same way `getAttribute` already does (#386, webrecorder/wombat#240) diff --git a/src/zimscraperlib/rewriting/js.py b/src/zimscraperlib/rewriting/js.py index 3a7dca2..4a62735 100644 --- a/src/zimscraperlib/rewriting/js.py +++ b/src/zimscraperlib/rewriting/js.py @@ -134,36 +134,38 @@ def replace_this_prop() -> TransformationAction: """ Create a rewrite_function replacing "this" by `this_rw`. - Replacement happens only if "this" is not a property of an object, i.e. if there - is a previous token, and it is not "." or "$", once whitespace (including - newlines) in between is skipped over. The previous token is looked up in the - full original string rather than just the match, because another rule earlier - in `create_js_rules` may already have consumed (as part of an unrelated match) - whatever directly preceded that whitespace, e.g. the "=" in - `const location = this.location` is consumed by the `let/const location =` - passthrough rule. - - If that skipped whitespace contains a newline, a semi-colon is also prepended. - This guards against a JavaScript Automatic Semicolon Insertion (ASI) pitfall: - `this_rw` starts with an identifier immediately followed by `(`, so if it - starts a new line right after a statement with no trailing `;`, JS would - otherwise parse it as a call of whatever the previous statement evaluated to - (e.g. `a = 5\nthis.x = 1` would silently become `a = 5(this.x = 1)`). + Replacement happens unless "this" is a property of an object, i.e. unless it is + directly preceded by "$" (no whitespace in between), or preceded by "." (any + amount of whitespace, including newlines, in between). The start of the script + counts as neither, so a leading `this.` is rewritten. + + The previous token is looked up in the full original string rather than just the + match, because another rule earlier in `create_js_rules` may already have + consumed (as part of an unrelated match) whatever directly preceded that + whitespace, e.g. the "=" in `const location = this.location` is consumed by the + `let/const location =` passthrough rule. + + If there is a newline in the whitespace immediately preceding "this", a + semi-colon is also prepended. This guards against a JavaScript Automatic + Semicolon Insertion (ASI) pitfall: `this_rw` starts with an identifier + immediately followed by `(`, so if it starts a new line right after a statement + with no trailing `;`, JS would otherwise parse it as a call of whatever the + previous statement evaluated to (e.g. `a = 5\nthis.x = 1` would silently become + `a = 5(this.x = 1)`). """ def f(m_object: re.Match[str], _opts: dict[str, Any] | None) -> str: full_string = m_object.string pos = m_object.start() - 1 - has_newline = False - while pos >= 0 and full_string[pos].isspace(): - has_newline = has_newline or full_string[pos] == "\n" - pos -= 1 - if pos < 0 or full_string[pos] in ".$": - # No real previous token (start of script) is treated like "." or "$": - # kept as-is, consistent with this rule never firing on the very first - # token of a script. - return m_object[0] - if has_newline: + ws_start = pos + while ws_start >= 0 and full_string[ws_start].isspace(): + ws_start -= 1 + if ws_start >= 0: + prev_char = full_string[ws_start] + is_adjacent = ws_start == pos + if prev_char == "." or (prev_char == "$" and is_adjacent): + return m_object[0] + if "\n" in full_string[ws_start + 1 : pos + 1]: return m_object[0].replace("this", ";" + this_rw) return m_object[0].replace("this", this_rw) diff --git a/tests/rewriting/test_js_rewriting.py b/tests/rewriting/test_js_rewriting.py index bfe08c5..6a1d3af 100644 --- a/tests/rewriting/test_js_rewriting.py +++ b/tests/rewriting/test_js_rewriting.py @@ -31,6 +31,8 @@ def simple_js_rewriter( "'a||this||that", "(a,b,Q.contains(i[t], this))", "a = this.location.href; exports.Foo = Foo; /* export className */", + "this.window=red", + "aaa$ this.window=red", ] ) def rewrite_this_js_content(request: pytest.FixtureRequest): @@ -57,8 +59,9 @@ def test_this_js_rewrite( "aaa.this.window=red", "aaa. this.window=red", "aaa. this.window=red", + "aaa.\nthis.window=red", + "aaa.\n this.window=red", "aaa$this.window=red", - "aaa$ this.window=red", "a = this.color;", "return this.color", 'func(Function("return this.color"));', @@ -81,15 +84,24 @@ def test_this_no_js_rewrite( ) +@pytest.mark.parametrize( + "prefix", + ["aaa", "aaa$"], +) @pytest.mark.parametrize( "spaces", [0, 1, 2, 3, 4], ) -def test_this_js_rewrite_newline(simple_js_rewriter: JsRewriter, spaces: int): +def test_this_js_rewrite_newline( + simple_js_rewriter: JsRewriter, prefix: str, spaces: int +): + # a "$" right before "this" only blocks the rewrite when directly adjacent (no + # whitespace in between), so it does not block it here since a newline (and + # possibly spaces) is always inserted between "$" and "this" padding = " " * spaces assert ( - simple_js_rewriter.rewrite(f"aaa\n{padding}this.window=red") - == f"aaa\n{padding};_____WB$wombat$check$this$function_____(this)" + simple_js_rewriter.rewrite(f"{prefix}\n{padding}this.window=red") + == f"{prefix}\n{padding};_____WB$wombat$check$this$function_____(this)" ".window=red" ) @@ -562,7 +574,6 @@ def test_import_rewrite_force_not_module(simple_js_rewriter: JsRewriter): "this. $location = http://example.com/", "this. _location = http://example.com/", "this. alocation = http://example.com/", - "this.location = http://example.com/", ",eval(a)", "this.$eval(a)", "x = $eval; x(a);",