Address review comments: Enforce stricter tag handling and image sanitation in notifications.

shell/plugins/notifications/NotificationLogic.js

@@ -5,30 +5,58 @@ 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 `<` and an optional `/`, the leading run of
+// letters and digits.
+function isImageTag(tag) {
+  var name = /^<\/?\s*([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.
 //
-// One replace() pass is not enough. String.replace scans left to right once,
-// so a payload spliced inside the literal "<img" prefix reassembles into a
-// live tag out of the surviving halves:
+// 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.
+//
+// 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">
-//     -> <img src="http://a/beacon.png">
 //
-// Repeat to a fixed point. Each pass can only shorten the string, so this
-// terminates.
+// 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 current = text
-  var previous
-  do {
-    previous = current
-    // The `$` alternative catches a tag left unterminated at the end of the
-    // string, which the renderer closes for itself.
-    current = current.replace(/<img[^>]*(?:>|$)/gi, "")
-  } while (current !== previous)
-  return current
+  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
 }
 
 function sanitizeBody(body, app, appIcon) {

test/shell.d/notifications-test.sh

@@ -18,20 +18,61 @@ assertEqual(
   'notifications strip inline image tags'
 )
 
-// The body renders as StyledText, which fetches <img src> over the network, so
-// the strip has to survive a payload built to outlive one replace() pass. A
-// single left-to-right pass consumes the inner tag and lets the outer halves
-// close up into a live tag: <im + g src="..."> .
-assertEqual(
-  notifications.sanitizeBody('<im<img src="http://host/decoy.png">g src="http://host/beacon.png">', 'Slack', ''),
-  '',
-  'notifications strip image tags that reassemble after one substitution'
+// The body renders as StyledText, which fetches <img src> 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 `>`.
+function survivingTagNames(text) {
+  const names = []
+  let i = 0
+  while (i < text.length) {
+    const open = text.indexOf('<', i)
+    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)
+    if (name) names.push(name[1].toLowerCase())
+    i = close === -1 ? text.length : close + 1
+  }
+  return names
+}
+
+function assertNoImageSurvives(body, description) {
+  const out = notifications.sanitizeBody(body, 'Slack', '')
+  const names = survivingTagNames(out)
+  assert(
+    !names.includes('img'),
+    description,
+    `input:  ${body}\noutput: ${out}\ntags:   ${JSON.stringify(names)}`
+  )
+}
+
+assertNoImageSurvives(
+  '<img src="http://host/plain.png">',
+  'notifications leave no image tag for a plain payload'
 )
 
-assertEqual(
-  notifications.sanitizeBody('<im<im<img src=a>g src=b>g src="http://host/deep.png">', 'Slack', ''),
-  '',
-  'notifications strip nested image tags to a fixed point'
+// A payload spliced inside the literal "<img" prefix. Qt reads ONE malformed
+// tag named `im` here and renders nothing; a stripper that deleted the inner
+// match would close the halves up into a live <img> the input never had.
+assertNoImageSurvives(
+  '<im<img src="http://host/decoy.png">g src="http://host/beacon.png">',
+  'notifications leave no image tag when a payload is spliced inside <img'
+)
+
+assertNoImageSurvives(
+  '<im<im<img src=a>g src=b>g src="http://host/deep.png">',
+  'notifications leave no image tag for a doubly nested payload'
+)
+
+assertNoImageSurvives(
+  '<img<img src="http://host/twin.png">',
+  'notifications leave no image tag when the outer tag is itself named img'
+)
+
+assertNoImageSurvives(
+  '< img src="http://host/spaced.png">',
+  'notifications leave no image tag when whitespace follows the angle bracket'
 )
 
 assertEqual(

test/shell.d/qml-text-format-test.sh

@@ -34,7 +34,10 @@ OPEN_ELEMENT = re.compile(r'(?:^|[:\s])([A-Z][A-Za-z0-9_.]*)\s*\{\s*$')
 PROP = re.compile(r'^\s*([A-Za-z_][A-Za-z0-9_.]*)\s*:')
 STRING_LITERAL = re.compile(r'"(?:[^"\\]|\\.)*"|\'(?:[^\'\\]|\\.)*\'')
 PROPERTY_DECL = re.compile(r'^\s*(?:readonly\s+)?property\b')
-ROOT_TEXT = re.compile(r'^Text\s*\{\s*$')
+# A binding that runs onto the next line: this line ends on an operator, or the
+# next line opens with one.
+TRAILING_OPERATOR = re.compile(r'(?:&&|\|\||[?:+\-*/,(\[=&|])$')
+LEADING_OPERATOR = re.compile(r'^\s*(?:&&|\|\||[?:+\-*/,)\]&|.])')
 
 
 def strip_noise(line, keep_strings=False):
@@ -76,6 +79,40 @@ def is_pure_literal(expr):
     return residue == '' and STRING_LITERAL.search(expr) is not None
 
 
+def binding_expression(lines, start):
+    """The whole right-hand side of the binding beginning on line `start`.
+
+    The literal exemption has to be judged on the complete expression. Reading
+    only the physical `text:` line would exempt `text: "prefix"` while
+    `+ externalValue` sits underneath, letting a dynamic AutoText binding
+    through. Reading a wrapped concatenation of literals as dynamic would be
+    the opposite error, so follow the expression to its end either way.
+    """
+    parts = []
+    parens = brackets = 0
+    i = start
+    while i < len(lines):
+        parts.append(strip_noise(lines[i], keep_strings=True))
+        counted = strip_noise(lines[i])
+        parens += counted.count('(') - counted.count(')')
+        brackets += counted.count('[') - counted.count(']')
+        following = strip_noise(lines[i + 1]) if i + 1 < len(lines) else ''
+        continues = (parens > 0 or brackets > 0
+                     or TRAILING_OPERATOR.search(counted.rstrip())
+                     or LEADING_OPERATOR.match(following))
+        if not continues:
+            break
+        i += 1
+
+    chunk = ' '.join(parts)
+    return chunk.split(':', 1)[1] if ':' in chunk else chunk
+
+
+def exempt_as_literal(lines, tline):
+    """True when the binding is only string literals, however many lines."""
+    return is_pure_literal(binding_expression(lines, tline))
+
+
 def blocks(lines):
     stack = []
     done = []
@@ -89,39 +126,70 @@ def blocks(lines):
             stack[-1]['props'].setdefault(prop.group(1), idx)
         n_open = code.count('{')
         n_close = code.count('}')
+        depth += n_open - n_close
         if opened and n_open > 0:
-            depth += 1
+            # OPEN_ELEMENT anchors at the end of the line, so the element it
+            # matched is the innermost one opened here and its depth is the
+            # depth after every brace on the line.
             stack.append({'name': opened.group(1), 'depth': depth,
                           'props': {}, 'start': idx})
-            depth += n_open - 1 - n_close
-        else:
-            depth += n_open - n_close
         while stack and depth < stack[-1]['depth']:
             done.append(stack.pop())
     done.extend(stack)
     return done
 
 
+INLINE_TEXT = re.compile(r'(?:^|[:\s])Text\s*\{([^{}]*)\}')
+INLINE_BINDING = re.compile(r'\btext\s*:\s*(.*?)\s*(?:;|$)')
+
+
+def inline_violations(lines, rel):
+    """Whole Text blocks written on one line.
+
+    OPEN_ELEMENT anchors at the end of the line, so the brace scanner never
+    sees these. A Repeater delegate is a plausible place for one.
+    """
+    out = []
+    for idx, raw in enumerate(lines):
+        code = strip_noise(raw, keep_strings=True)
+        for match in INLINE_TEXT.finditer(code):
+            body = match.group(1)
+            if 'textFormat' in body:
+                continue
+            binding = INLINE_BINDING.search(body)
+            if not binding or is_pure_literal(binding.group(1)):
+                continue
+            out.append(f'{rel}:{idx + 1}: inline Text block without textFormat')
+    return out
+
+
 root = Path(os.environ['ROOT'])
 found = []
 for path in sorted((root / 'shell').rglob('*.qml')):
     lines = path.read_text().splitlines()
     rel = path.relative_to(root)
-
-    # A component whose root element is a Text takes its binding from callers,
-    # so the default has to be declared in the component itself.
-    if lines and any(ROOT_TEXT.match(l) for l in lines[:40]):
-        if not any(re.match(r'\s*textFormat\s*:', l) for l in lines):
-            found.append(f'{rel}: root Text element declares no textFormat')
+    found.extend(inline_violations(lines, rel))
 
     for b in blocks(lines):
         if b['name'] != 'Text' or 'textFormat' in b['props']:
             continue
+
+        # Read the block's own properties. A nested child declaring textFormat
+        # says nothing about its parent, so `Text { Text { textFormat: ... } }`
+        # must still report the outer element.
+        # The root element of a component takes its binding from callers, so it
+        # needs the default whether or not this file binds `text`. Require both
+        # depth 1 and column 0: the scanner attributes one element per line, so
+        # a `Row { Text {` line would report depth 1 for a nested block, and
+        # falling through to the binding check below is the safe reading.
+        if b['depth'] == 1 and lines[b['start']].startswith('Text'):
+            found.append(f'{rel}:{b["start"] + 1}: root Text element declares no textFormat')
+            continue
+
         if 'text' not in b['props']:
             continue
         tline = b['props']['text']
-        expr = strip_noise(lines[tline], keep_strings=True).split(':', 1)[1]
-        if is_pure_literal(expr):
+        if exempt_as_literal(lines, tline):
             continue
         found.append(f'{rel}:{tline + 1}: text binding without textFormat')