Merge pull request #8397 from ErikMelton/unauthorized-http-get-requests-from-notifications

Require textFormat declaration for all Text elements

(cherry picked from commit 468b511249)

shell/plugins/notifications/NotificationLogic.js

@@ -5,8 +5,92 @@ function isChromiumDerived(app, appIcon) {
          source.indexOf("opera") >= 0
 }
 
+// True when a `<...>` run is an image tag, so the name is read the way Qt's
+// 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 = /^<[^A-Za-z0-9]*([A-Za-z0-9]+)/.exec(tag)
+  return !!name && name[1].toLowerCase() === "img"
+}
+
+// The body renders as StyledText so notifications can use the markup the
+// body-markup capability advertises (see Service.qml). StyledText honours
+// <img src>, and a remote src makes the shell issue an unauthenticated GET
+// 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, 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 `<img` to be dropped, never hide one — whereas honouring
+// quotes would let `<b title="a>b"><img src="http://host/x.png">` through.
+//
+// Deleting a substring is what makes a naive `/<img[^>]*>/g` unsafe. Given
+//
+//   <im<img src="http://a/decoy.png">g src="http://a/beacon.png">
+//
+// Qt reads ONE malformed tag named `im` and renders nothing, but removing the
+// inner match closes the surviving halves up into `<img src=".../beacon.png">`
+// — a live tag the input never contained. The stripper would be manufacturing
+// the very thing it exists to remove.
+//
+// Because every `<` opens a tag, the text between tags never contains one, so
+// dropping a tag cannot splice its neighbours into a new one. That makes a
+// single pass sufficient, with no re-scanning and no input bound to police.
+function stripImageTags(text) {
+  var out = ""
+  var i = 0
+
+  while (i < text.length) {
+    var open = text.indexOf("<", i)
+    if (open === -1) {
+      out += text.slice(i)
+      break
+    }
+
+    out += text.slice(i, open)
+
+    // An unterminated tag at the end of the string still reaches the renderer,
+    // which closes it itself, so treat the remainder as one tag.
+    var close = text.indexOf(">", open)
+    var tag = close === -1 ? text.slice(open) : text.slice(open, close + 1)
+
+    if (!isImageTag(tag)) out += tag
+    i = close === -1 ? text.length : close + 1
+  }
+
+  return out
+}
+
+// What the card renders, and the last thing to touch the string before Qt parses
+// it. The newline rewrite belongs here rather than in the card because it inserts
+// `<br/>` into text stripImageTags chose to KEEP, and a kept tag may hold a `<` of
+// its own: `<x`, newline, `<img src="http://…">` is one tag named `x` to both the
+// stripper and Qt, until the rewrite splits it into `<x<br/>` and a live image tag
+// the input never contained. Measured against Qt 6.11.2 — the rewritten form
+// fetches, the original does not. So strip again after, and what Qt parses is what
+// was checked last.
+function styledBody(body, app, appIcon) {
+  return stripImageTags(sanitizeBody(body, app, appIcon).replace(/\r\n|\r|\n/g, "<br/>"))
+}
+
 function sanitizeBody(body, app, appIcon) {
-  var text = String(body || "").replace(/<img[^>]*>/gi, "")
+  var text = stripImageTags(String(body || ""))
   if (!isChromiumDerived(app, appIcon)) return text
 
   return text
@@ -366,6 +450,7 @@ if (typeof module !== "undefined") {
   module.exports = {
     isChromiumDerived: isChromiumDerived,
     sanitizeBody: sanitizeBody,
+    styledBody: styledBody,
     summaryStartsWithGlyph: summaryStartsWithGlyph,
     shouldBypassDnd: shouldBypassDnd,
     isEphemeralApp: isEphemeralApp,

shell/plugins/notifications/components/NotificationCard.qml

@@ -44,7 +44,7 @@ BorderSurface {
   readonly property bool singleLineToast: sanitizedBody.length === 0
   readonly property bool collapseRedundantIcon: singleLineToast && !hasGlyph && summaryStartsWithGlyph
   readonly property string sanitizedBody: sanitizeBody(body)
-  readonly property string styledBody: sanitizedBody.replace(/\r\n|\r|\n/g, "<br/>")
+  readonly property string styledBody: NotificationLogic.styledBody(body, app, appIcon)
 
   readonly property color dimColor: Qt.darker(Color.notifications.text, 1.4)
   readonly property color bodyColor: Qt.darker(Color.notifications.text, 1.15)
@@ -133,6 +133,7 @@ BorderSurface {
         // Glyph fallback (Nerd Font character) when no image icon is
         // available. Used by omarchy-notification-send's `-g` flag.
         Text {
+          textFormat: Text.PlainText
           anchors.centerIn: parent
           visible: root.hasGlyph && smallIconImage.status !== Image.Ready
           text: root.glyph
@@ -143,6 +144,7 @@ BorderSurface {
       }
 
       Text {
+        textFormat: Text.PlainText
         Layout.alignment: Qt.AlignVCenter
         visible: root.compactGlyph
         text: root.glyph
@@ -157,6 +159,11 @@ BorderSurface {
         spacing: Style.space(2)
 
         Text {
+          // The spec defines the summary as a single line of plain text, so
+          // AutoText could only ever promote a hostile string to rich text.
+          // The body below is StyledText on purpose — see Service.qml's
+          // bodyMarkupSupported — and is stripped in NotificationLogic.
+          textFormat: Text.PlainText
           Layout.fillWidth: true
           visible: root.summary.length > 0
           text: root.summary