diff --git a/shell/plugins/notifications/NotificationLogic.js b/shell/plugins/notifications/NotificationLogic.js index 67b7bcca..b3f7be09 100644 --- a/shell/plugins/notifications/NotificationLogic.js +++ b/shell/plugins/notifications/NotificationLogic.js @@ -6,10 +6,21 @@ function isChromiumDerived(app, appIcon) { } // True when a `<...>` run is an image tag, so the name is read the way Qt's -// parser reads it: after the `<` and an optional `/`, the leading run of -// letters and digits. +// parser reads it: after the `<`, the leading run of letters and digits. +// +// Skip everything up to that run rather than matching the separator, because +// there is no JavaScript expression for what Qt skips. QQuickStyledText calls +// skipSpace(), which is QChar::isSpace(), and that set is not `\s`: Qt counts +// U+0085 NEL and `\s` does not, while `\s` counts U+FEFF and Qt does not. A +// name read with `\s` therefore misses a tag written as `<`, U+0085, `img`: +// Qt skips the NEL, reads `img` and issues the GET, while the regex finds no +// name at all and the tag is kept. Measured against Qt 6.11.2. +// +// Over-skipping is the safe direction. It can only classify more runs as +// images, and dropping a run never manufactures a tag: a dropped run joins two +// stretches of text that each contain no `<`. function isImageTag(tag) { - var name = /^<\/?\s*([A-Za-z0-9]+)/.exec(tag) + var name = /^<[^A-Za-z0-9]*([A-Za-z0-9]+)/.exec(tag) return !!name && name[1].toLowerCase() === "img" } @@ -19,8 +30,15 @@ function isImageTag(tag) { // with no user action, so image tags go before the renderer sees them. // // Work in whole tags, never in substrings of one. A `<` opens a tag that runs -// to the next `>`, nested `<` and all — that is how Qt's parser bounds it — -// and only a tag whose own name is `img` is dropped. +// to the next `>`, nested `<` and all, and only a tag whose own name is `img` +// is dropped. +// +// That is the conservative bound, not Qt's exact one: Qt lets a `>` inside a +// quoted attribute value pass without closing the tag, so a Qt tag can be +// longer than the run taken here. Do not "correct" this to match Qt. Taking +// the shorter run only ever splits one Qt tag into several, and a split can +// only expose an `` through. // // Deleting a substring is what makes a naive `/]*>/g` unsafe. Given // diff --git a/test/shell.d/notifications-test.sh b/test/shell.d/notifications-test.sh index 370598f6..6dfc54a8 100644 --- a/test/shell.d/notifications-test.sh +++ b/test/shell.d/notifications-test.sh @@ -21,7 +21,10 @@ assertEqual( // The body renders as StyledText, which fetches over the network. The // invariant that matters is not a particular output string but that no tag Qt // would honour as an image survives, so assert that directly. Tags are bounded -// the way Qt bounds them: a `<` opens a tag that runs to the next `>`. +// the conservative way the stripper bounds them: a `<` opens a tag that runs to +// the next `>`. Qt's own bound can be longer, since a `>` inside a quoted +// attribute value does not close a tag there — which only ever splits one Qt +// tag into several here, so a name this helper reads is a name Qt reads too. function survivingTagNames(text) { const names = [] let i = 0 @@ -30,7 +33,11 @@ function survivingTagNames(text) { if (open === -1) break const close = text.indexOf('>', open) const tag = close === -1 ? text.slice(open) : text.slice(open, close + 1) - const name = /^<\/?\s*([A-Za-z0-9]+)/.exec(tag) + // Read the name the way Qt does, skipping anything that is not part of it. + // Matching the separator with \s instead would give this helper the same + // blind spot as the code it is checking — Qt skips U+0085 and \s does not — + // and an assertion that shares the implementation's bug proves nothing. + const name = /^<[^A-Za-z0-9]*([A-Za-z0-9]+)/.exec(tag) if (name) names.push(name[1].toLowerCase()) i = close === -1 ? text.length : close + 1 } @@ -75,6 +82,23 @@ assertNoImageSurvives( 'notifications leave no image tag when whitespace follows the angle bracket' ) +// Qt skips the separator between `<` and the tag name with QChar::isSpace(), +// which counts U+0085 NEL. JavaScript's \s does not. Reading the name with \s +// finds none here, keeps the tag, and Qt then reads `img` and fetches it — +// measured against Qt 6.11.2, where this exact body makes a StyledText Text +// issue an outbound GET. Asserted on the whole output rather than through +// assertNoImageSurvives so it holds even if that helper is ever loosened. +assertEqual( + notifications.sanitizeBody('<\u0085img src="http://host/nel.png">after', 'Slack', ''), + 'after', + 'notifications strip an image tag whose separator is U+0085, which Qt skips but \\s does not' +) + +assertNoImageSurvives( + '<\u0085img src="http://host/nel2.png">', + 'notifications leave no image tag when U+0085 follows the angle bracket' +) + assertEqual( notifications.sanitizeBody('trailing