f83fd4fd350727eb3a42a709849bab0c4658ab84
braney
  Fri Aug 21 10:17:16 2026 -0700
htmlSanitize: three fixes from a review of it, refs #38126

Read the scheme of an href or src the way a browser arrives at it.  A browser
turns a numeric character reference into its character before it decides what
the scheme is, and it accepts one written with any number of leading zeros and
with no closing semicolon at all.  Decode those the same way, then insist that
what stands in front of the first slash is either a plain scheme we allow or a
plain path.  A named entity, a backslash or a control character in that part of
the value means we do not print the link, because those are the ways the check
gets walked around.  This keeps the 33 encoded mailto links two hubs write, and
they were the only links in the public hubs the plainer rule would have lost.

Treat a trailing slash on a kept element as the nothing that HTML says it is.
Otherwise <div/> came out as an open div and the rest of our own page sat
inside it.

Remember when the search for a closing tag has run off the end of the input.
Every later search for that same tag runs off the end too, so a page made of
two hundred thousand unclosed tags no longer costs one pass over the page each.

The unit test grows a case for each of the three.

diff --git src/lib/htmlSanitize.c src/lib/htmlSanitize.c
index cd0f30273d8..07b85af685a 100644
--- src/lib/htmlSanitize.c
+++ src/lib/htmlSanitize.c
@@ -153,30 +153,32 @@
         char key[256];
         safef(key, sizeof key, "%s.%s", attrRules[i].element, attr);
         hashAdd(attrHash, key, NULL);
         }
     freeMem(dupe);
     }
 keepHash = hashOfWords(keepElements, 7);        /* last, it is the flag that we are built */
 }
 
 struct sanitizer
 /* State of one pass over a piece of HTML. */
     {
     struct dyString *out;       /* Sanitized HTML accumulates here. */
     struct slName *openStack;   /* Elements opened and not yet closed, innermost first. */
     int depth;                  /* Length of openStack. */
+    struct hash *exhausted;     /* Elements we have already looked for a closing tag of
+                                 * and not found, so there is no point looking again. */
     boolean report;             /* Collecting messages about what we removed? */
     struct hash *seen;          /* Messages reported already. */
     struct slName *removed;     /* Messages, in the order we first hit them. */
     };
 
 static void noteRemoved(struct sanitizer *san, char *format, ...)
 /* Record a message naming something we took out, once per distinct message. */
 {
 if (!san->report)
     return;
 char message[512];
 va_list args;
 va_start(args, format);
 vsnprintf(message, sizeof message, format, args);
 va_end(args);
@@ -340,114 +342,162 @@
             dyStringAppend(dy, "&quot;");
             break;
         case '<':
             dyStringAppend(dy, "&lt;");
             break;
         case '>':
             dyStringAppend(dy, "&gt;");
             break;
         default:
             dyStringAppendC(dy, *s);
             break;
         }
     }
 }
 
-static char *urlScheme(char *val)
-/* Return the scheme of val, lower cased and freshly allocated, or NULL if it has none.
- * Entities and padding are decoded first, so that &#106;avascript: and "java\tscript:"
- * both come out as javascript. */
+static char *decodeNumericRefs(char *s)
+/* Return a copy of s with numeric character references turned into the characters they
+ * name, which is what a browser does before it looks for a scheme.  Only &#NN and &#xNN
+ * are decoded, with or without the closing semicolon, because that is what a browser
+ * accepts.  A named entity is left alone and the caller refuses the URL over it.  A
+ * character above ASCII cannot be part of a scheme, so one stand-in character does for
+ * all of them. */
 {
-struct dyString *dy = dyStringNew(64);
-char *p = val;
+struct dyString *dy = dyStringNew(strlen(s)+1);
+char *p = s;
 while (*p != 0)
     {
-    if (*p == '&')
+    if (p[0] == '&' && p[1] == '#')
         {
-        char *semi = strchr(p, ';');
-        int c = -1;
-        if (semi != NULL && semi - p <= 10)
+        char *digits = p+2;
+        int base = 10;
+        if (*digits == 'x' || *digits == 'X')
             {
-            if (p[1] == '#')
-                {
-                if (p[2] == 'x' || p[2] == 'X')
-                    c = strtol(p+3, NULL, 16);
-                else
-                    c = atoi(p+2);
+            base = 16;
+            digits += 1;
             }
-            else if (startsWithNoCase("&colon;", p))
-                c = ':';
-            else if (startsWithNoCase("&tab;", p))
-                c = '\t';
-            else if (startsWithNoCase("&newline;", p))
-                c = '\n';
-            }
-        if (c > 0 && c < 128)
+        char *end = NULL;
+        errno = 0;
+        long value = strtol(digits, &end, base);
+        if (end != digits)
             {
-            if (c > ' ')
-                dyStringAppendC(dy, tolower(c));
-            p = semi+1;
+            if (*end == ';')
+                end += 1;
+            if (value > 0 && value < 128 && errno == 0)
+                dyStringAppendC(dy, (char)value);
+            else
+                dyStringAppendC(dy, '~');
+            p = end;
             continue;
             }
         }
-    if ((unsigned char)*p > ' ')
-        dyStringAppendC(dy, tolower(*p));
-    ++p;
+    dyStringAppendC(dy, *p);
+    p += 1;
+    }
+return dyStringCannibalize(&dy);
 }
-char *clean = dyStringCannibalize(&dy);
-char *colon = strchr(clean, ':');
+
+static char *urlScheme(char *val, boolean *retSuspect)
+/* Return the scheme of val, lower cased and freshly allocated, or NULL if it has none.
+ * Set retSuspect when the text in front of the path holds something that could hide a
+ * scheme from us and still be one to a browser: a named entity, a backslash, or a control
+ * character.  Reading the text this way, rather than copying every rule a browser has for
+ * repairing a broken URL, is the point.  Matching those rules exactly is how a check like
+ * this gets beaten. */
+{
+*retSuspect = FALSE;
+char *decoded = decodeNumericRefs(val);
+char *s = decoded;
+while (*s != 0 && (unsigned char)*s <= ' ')
+    ++s;
 char *scheme = NULL;
-if (colon != NULL)
+char *p;
+for (p = s;  *p != 0;  ++p)
     {
-    char *pathStart = strpbrk(clean, "/?#");
-    if (pathStart == NULL || colon < pathStart)
+    if (*p == '/' || *p == '?' || *p == '#')
+        break;                          /* a path, query or anchor starts, so no scheme */
+    if (*p == '&' || *p == '\\' || (unsigned char)*p < ' ' || *p == 0x7f)
         {
-        *colon = 0;
-        scheme = cloneString(clean);
+        *retSuspect = TRUE;
+        break;
+        }
+    if (*p == ':')
+        {
+        int len = p - s;
+        char buf[33];
+        if (len < 1 || len >= sizeof buf)
+            {
+            *retSuspect = TRUE;
+            break;
+            }
+        memcpy(buf, s, len);
+        buf[len] = 0;
+        tolowers(buf);
+        boolean plain = isalpha((unsigned char)buf[0]);
+        char *c;
+        for (c = buf;  plain && *c != 0;  ++c)
+            {
+            if (!isalnum((unsigned char)*c) && *c != '+' && *c != '.' && *c != '-')
+                plain = FALSE;
             }
+        if (plain)
+            scheme = cloneString(buf);
+        else
+            *retSuspect = TRUE;
+        break;
         }
-freeMem(clean);
+    }
+freeMem(decoded);
 return scheme;
 }
 
 static boolean urlOk(char *val, struct sanitizer *san)
 /* Is this a URL we are willing to print? */
 {
-char *scheme = urlScheme(val);
+boolean suspect = FALSE;
+char *scheme = urlScheme(val, &suspect);
+if (suspect)
+    {
+    noteRemoved(san, "removed a link that does not read as a plain web address");
+    return FALSE;
+    }
 if (scheme == NULL)
-    return TRUE;                /* relative, or a same page anchor */
+    return TRUE;
 boolean ok = (hashLookup(schemeHash, scheme) != NULL);
 if (!ok)
     noteRemoved(san, "removed a link that used the %s: scheme", scheme);
 freeMem(scheme);
 return ok;
 }
 
 static boolean iframeSrcOk(char *src)
 /* Does src point at one of the video hosts we allow in a frame? */
 {
 if (isEmpty(src))
     return FALSE;
-char *scheme = urlScheme(src);
+boolean suspect = FALSE;
+char *scheme = urlScheme(src, &suspect);
+if (suspect)
+    return FALSE;
 if (scheme != NULL)
     {
     boolean https = sameString(scheme, "https");
     freeMem(scheme);
     if (!https)
         return FALSE;
     }
-else if (!startsWith("//", src))
+else if (!startsWith("//", skipLeadingSpaces(src)))
     return FALSE;
 char *host = stringIn("//", src);
 if (host == NULL)
     return FALSE;
 host += 2;
 int len = strcspn(host, "/?#:");
 char hostName[256];
 if (len >= sizeof hostName)
     return FALSE;
 memcpy(hostName, host, len);
 hostName[len] = 0;
 tolowers(hostName);
 return (hashLookup(videoHostHash, hostName) != NULL);
 }
 
@@ -690,77 +740,91 @@
         char *src = attributeValue(attrText, tagEnd, "src");
         kill = !iframeSrcOk(src);
         if (kill)
             {
             noteRemoved(san, "removed an iframe, we only allow one that plays a video "
                              "from a site we know");
             noted = TRUE;
             }
         freeMem(src);
         }
     if (kill)
         {
         if (!isVoid && tagEnd[-1] != '/')
             {
             boolean rawText = (hashLookup(rawTextHash, name) != NULL);
-            char *afterClose = skipToClose(s, name, rawText);
+            /* Once the search for a closing tag has run off the end of the input, every
+             * later search for that same tag will too, and repeating it on a page built
+             * of thousands of unclosed tags would cost us a pass each time. */
+            char *afterClose = NULL;
+            if (hashLookup(san->exhausted, name) == NULL)
+                {
+                afterClose = skipToClose(s, name, rawText);
+                if (afterClose == NULL)
+                    hashAdd(san->exhausted, name, NULL);
+                }
             if (afterClose != NULL)
                 s = afterClose;
             else if (rawText)
                 s += strlen(s);         /* never closed, and its content is not for reading */
             }
         if (!noted && hashLookup(silentKillHash, name) == NULL)
             noteRemoved(san, "removed the %s element and everything inside it", name);
         continue;
         }
     if (hashLookup(keepHash, name) == NULL)
         continue;                       /* tag goes, text inside it stays */
     if (!isVoid && san->depth >= maxNestDepth)
         continue;
     dyStringPrintf(san->out, "<%s", name);
     writeAttributes(san, name, attrText, tagEnd);
     dyStringAppendC(san->out, '>');
-    if (!isVoid && tagEnd[-1] != '/')
+    if (!isVoid)
         {
+        /* A trailing slash does not close an element like this one, whatever the author
+         * meant by it, so remember it as open.  Anything still open at the end is closed
+         * for us, which stops a page ending up inside a hub's div. */
         slNameAddHead(&san->openStack, name);
         san->depth += 1;
         }
     }
 while (san->openStack != NULL)
     {
     struct slName *top = slPopHead(&san->openStack);
     dyStringPrintf(san->out, "</%s>", top->name);
     freeMem(top);
     }
 }
 
 char *htmlSanitizeReport(char *html, struct slName **retRemoved)
 /* Like htmlSanitize, and if retRemoved is not NULL also return a list of one-line messages
  * naming each kind of thing that was removed.  The list is NULL when nothing was removed. */
 {
 if (retRemoved != NULL)
     *retRemoved = NULL;
 if (html == NULL)
     return NULL;
 initTables();
 struct sanitizer san;
 ZeroVar(&san);
 san.out = dyStringNew(strlen(html) + 128);
 san.report = (retRemoved != NULL);
-if (san.report)
+san.exhausted = hashNew(6);
+if (retRemoved != NULL)
     san.seen = hashNew(0);
 sanitizeOnePass(html, &san);
-if (san.report)
+hashFree(&san.exhausted);
+if (retRemoved != NULL)
     {
     slReverse(&san.removed);
     *retRemoved = san.removed;
     hashFree(&san.seen);
     }
 return dyStringCannibalize(&san.out);
 }
 
 char *htmlSanitize(char *html)
 /* Return a cloned copy of html holding only allowlisted elements, attributes and style
  * properties. */
 {
 return htmlSanitizeReport(html, NULL);
 }