Change Details

Follows-up from T214451#4961656, and: >>! Change 486145 (//merged//): > [mediawiki/extensions/MobileFrontend@master] Explicitly pass in parseHTML > [[https://gerrit.wikimedia.org/r/486145]] I'm glad to hear `util.parseHTML()` is only used for literal inline HTML strings, with no variables or user-input passed in. Background: The only reason jQuery offers `$.parseHTML(String html)` in addition to `$(String html)` is for security reasons. `$.parseHTML` exists to allow a caller to safely parse a string without the current document's execution context being involved. E.g. inline scripts and legacy event attributes. The above patch has made it insecure by passing in the one Document object the methods exists for to secure: The global one. Thus effectively turning it into a home-made version of `$(String html)`. This may be fine for the currently audited calls to this functions but is risky for two reasons: * Given the purpose of `util` in MobileFrontend as a jQuery-proxy, it is reasonable for developers to expect it will behave as `$.parseHTML`. Specifically, its security semantics. When reviewing its 3-line source code in `mobile.startup/util`, the innocent-looking passing of the global `document` object likely does not suffice to signal most readers that unsafe JavaScript execution may be ahead. This is **dangerous**. * It is now effectively a duplicate of `$()`, which is also in use. I propose to deprecate `util.parseHTML` in favour of `$()`. Or perhaps rename like `util.evalHTML`, and update remaining uses of `$()`. = AC [] Rename `util.parseHTML` to `util.evalHTML` or another name that signals it deviates from the default jQuery method [] Adapt all old client calls to use the new name [] Remove the following comment from https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/ed393746f6ae602666438517db37ed878108c22c/src/mobile.mediaViewer/ImageCarousel.js#L237-L242 as it is outdated. The comment was valid when util.parseHTML did not pass the global document. Additionally, the explicit global document arg can be removed since it is the default now.

Follows-up from T214451#4961656, and: >>! Change 486145 (//merged//): > [mediawiki/extensions/MobileFrontend@master] Explicitly pass in parseHTML > [[https://gerrit.wikimedia.org/r/486145]] I'm glad to hear `util.parseHTML()` is only used for literal inline HTML strings, with no variables or user-input passed in. Background: The only reason jQuery offers `$.parseHTML(String html)` in addition to `$(String html)` is for security reasons. `$.parseHTML` exists to allow a caller to safely parse a string without the current document's execution context being involved. E.g. inline scripts and legacy event attributes. The above patch has made it insecure by passing in the one Document object the methods exists for to secure: The global one. Thus effectively turning it into a home-made version of `$(String html)`. This may be fine for the currently audited calls to this functions but is risky for two reasons: * Given the purpose of `util` in MobileFrontend as a jQuery-proxy, it is reasonable for developers to expect it will behave as `$.parseHTML`. Specifically, its security semantics. When reviewing its 3-line source code in `mobile.startup/util`, the innocent-looking passing of the global `document` object likely does not suffice to signal most readers that unsafe JavaScript execution may be ahead. This is **dangerous**. * It is now effectively a duplicate of `$()`, which is also in use. I propose to deprecate `util.parseHTML` in favour of `$()`. Or perhaps rename like `util.evalHTML`, and update remaining uses of `$()`. = AC [] Rename `util.parseHTML` to `util.evalHTML` or another name that signals it deviates from the default jQuery method [] Adapt all old client calls to use the new name [] Remove the following comment from https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/ed393746f6ae602666438517db37ed878108c22c/src/mobile.mediaViewer/ImageCarousel.js#L237-L242 as it is outdated. The comment was valid when util.parseHTML did not pass the global document. Additionally, the explicit global document arg can be removed from that call since it is the default now.

Follows-up from T214451#4961656, and: >>! Change 486145 (//merged//): > [mediawiki/extensions/MobileFrontend@master] Explicitly pass in parseHTML > [[https://gerrit.wikimedia.org/r/486145]] I'm glad to hear `util.parseHTML()` is only used for literal inline HTML strings, with no variables or user-input passed in. Background: The only reason jQuery offers `$.parseHTML(String html)` in addition to `$(String html)` is for security reasons. `$.parseHTML` exists to allow a caller to safely parse a string without the current document's execution context being involved. E.g. inline scripts and legacy event attributes. The above patch has made it insecure by passing in the one Document object the methods exists for to secure: The global one. Thus effectively turning it into a home-made version of `$(String html)`. This may be fine for the currently audited calls to this functions but is risky for two reasons: * Given the purpose of `util` in MobileFrontend as a jQuery-proxy, it is reasonable for developers to expect it will behave as `$.parseHTML`. Specifically, its security semantics. When reviewing its 3-line source code in `mobile.startup/util`, the innocent-looking passing of the global `document` object likely does not suffice to signal most readers that unsafe JavaScript execution may be ahead. This is **dangerous**. * It is now effectively a duplicate of `$()`, which is also in use. I propose to deprecate `util.parseHTML` in favour of `$()`. Or perhaps rename like `util.evalHTML`, and update remaining uses of `$()`. = AC [] Rename `util.parseHTML` to `util.evalHTML` or another name that signals it deviates from the default jQuery method [] Adapt all old client calls to use the new name [] Remove the following comment from https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/ed393746f6ae602666438517db37ed878108c22c/src/mobile.mediaViewer/ImageCarousel.js#L237-L242 as it is outdated. The comment was valid when util.parseHTML did not pass the global document. Additionally, the explicit global document arg can be removed from that call since it is the default now.