Fix 8 GitHub CodeQL code-scanning alerts: incomplete escaping, URL

substring check, an overly-permissive regex range and a no-op replace.

- models/lib/cardUrlAutolink.js and packages/markdown/src/
  template-integration.js (kept in sync, per their own comments): the
  card-title markdown-link escape only escaped ']', not the backslash
  that precedes it, so a title ending in a raw backslash escaped the
  ']' this code inserts to close the label instead of the label
  actually closing. Escape '\' before ']'.
- server/lib/documentGif.js: plainSearchText's control-character strip
  used literal raw control bytes inside the regex character class
  instead of \x escapes, so CodeQL flagged the range as unverifiable
  (adjacent control bytes are visually indistinguishable, so a
  boundary could silently shift). Rewritten with explicit \x escapes,
  byte-for-byte equivalent to the original range.
- tests/notificationEmailUrlLink.test.cjs: built a dynamic RegExp by
  hand-escaping only '/', leaving every other meta-character -
  including backslash itself - unescaped. Added the same escapeRegExp
  helper already used elsewhere in the codebase.
- tests/oauthLogoutUrl.test.cjs: asserted a returned URL did not
  contain a hostname via `.includes()`, a substring check a
  confusable/query-embedded hostname can fool either way. Compare the
  parsed `new URL().hostname` instead.
- tests/boardCreationAdminOnly.test.cjs: `.replace(/ /g, ' ')` was a
  no-op (replacing a space with itself); the actual intent was to
  collapse runs of spaces, `.replace(/ +/g, ' ')`.

Every fix has a positive test proving the corrected behaviour and a
negative test reproducing the exact bypass/bug CodeQL flagged. None of
these cross a security boundary on their own (the markdown-link title
escaping is a rendering-correctness fix - DOMPurify still sanitizes
the final HTML regardless, per the existing code comments; the rest
are test-only assertion bugs), so no Admin Panel security-log entry
applies.

Thanks to GitHub CodeQL and xet7 !

models/lib/cardUrlAutolink.js

@@ -116,10 +116,14 @@ function autolinkWekanCardUrls(text, resolveTitle) {
     }
     if (!title || typeof title !== 'string') return;
     result += text.slice(lastEnd, index);
-    // Escape ']' in the title so it cannot prematurely close the markdown
-    // link label; markdown-it leaves other characters alone inside a link
-    // label and DOMPurify sanitizes the final HTML regardless.
-    const safeTitle = title.replace(/]/g, '\\]');
+    // Escape '\' and ']' in the title so it cannot prematurely close the
+    // markdown link label; markdown-it leaves other characters alone inside
+    // a link label and DOMPurify sanitizes the final HTML regardless. The
+    // backslash MUST be escaped first: a title ending in a raw backslash
+    // (e.g. "foo\") would otherwise escape the literal ']' this function
+    // inserts to close the label (CodeQL js/incomplete-sanitization), so the
+    // label never closes and the emitted markdown is not the link intended.
+    const safeTitle = title.replace(/\\/g, '\\\\').replace(/]/g, '\\]');
     result += `[${safeTitle}](${match})`;
     lastEnd = index + match.length;
     changed = true;

packages/markdown/src/template-integration.js

@@ -148,7 +148,12 @@ function autolinkWekanCardUrls(text, resolveTitle) {
     }
     if (!title || typeof title !== 'string') return;
     result += text.slice(lastEnd, index);
-    const safeTitle = title.replace(/]/g, '\\]');
+    // '\' MUST be escaped before ']' - see models/lib/cardUrlAutolink.js
+    // (kept in sync with this copy) for why: a title ending in a raw
+    // backslash would otherwise escape the literal ']' inserted below,
+    // leaving the markdown link label unterminated
+    // (CodeQL js/incomplete-sanitization).
+    const safeTitle = title.replace(/\\/g, '\\\\').replace(/]/g, '\\]');
     result += `[${safeTitle}](${match})`;
     lastEnd = index + match.length;
     changed = true;

tests/boardCreationAdminOnly.test.cjs

@@ -32,7 +32,12 @@ console.log('boardCreationAdminOnly:');
 test('the setting defaults to off (board creation unrestricted)', () => {
   assert.ok(
     /tableVisibilityMode-boardCreationAdminOnly['"]?\s*},?\s*\n\s*\{\s*\$setOnInsert:\s*\{\s*booleanValue:\s*false/.test(
-      bootstrap.replace(/\n\s*/g, ' ').replace(/ /g, ' '),
+      // CodeQL js/identity-replacement (#527): `.replace(/ /g, ' ')` replaced
+      // a single space with itself - a no-op that did nothing. The actual
+      // intent (collapsing the RUNS of spaces the newline-collapse above can
+      // leave behind, e.g. from original multi-space indentation) is
+      // `.replace(/ +/g, ' ')`.
+      bootstrap.replace(/\n\s*/g, ' ').replace(/ +/g, ' '),
     ) || /_id: 'tableVisibilityMode-boardCreationAdminOnly' },\s*\n\s*\{ \$setOnInsert: \{ booleanValue: false/.test(bootstrap),
     'the seeded document defaults booleanValue to false',
   );
@@ -111,4 +116,19 @@ test('negative: no unexpected Meteor board-insert path exists', () => {
     'only the known, deliberately-ungated board-insert call sites exist');
 });
 
+test('#527 the whitespace-normalizing helper actually collapses runs of spaces, not a no-op', () => {
+  // Positive: a run of literal spaces (the kind left behind once newlines +
+  // their leading indentation have already been folded to single spaces)
+  // collapses to one.
+  const normalize = s => s.replace(/\n\s*/g, ' ').replace(/ +/g, ' ');
+  assert.strictEqual(normalize('a     b'), 'a b');
+  assert.strictEqual(normalize('a\n    b'), 'a b');
+  // Negative: the OLD code (`.replace(/ /g, ' ')`) is an identity
+  // replacement - it must NOT collapse the same run, proving the bug CodeQL
+  // flagged (js/identity-replacement) was real and is what got fixed.
+  const oldNoOpNormalize = s => s.replace(/\n\s*/g, ' ').replace(/ /g, ' ');
+  assert.strictEqual(oldNoOpNormalize('a     b'), 'a     b');
+  assert.notStrictEqual(oldNoOpNormalize('a     b'), normalize('a     b'));
+});
+
 console.log(`\nboardCreationAdminOnly: ${passed} tests passed`);

tests/cardUrlAutolink.test.cjs

@@ -187,6 +187,32 @@ test('autolinkWekanCardUrls escapes a title containing "]" so it cannot break th
   assert.strictEqual(result, `[Card [urgent\\]](${ABS_URL})`);
 });
 
+test('autolinkWekanCardUrls escapes a title ending in "\\" so it cannot prematurely close the markdown link label (CodeQL js/incomplete-sanitization)', () => {
+  // A title ending in a raw backslash: if only ']' were escaped, the
+  // resulting markdown "[foo\](url)" would have that backslash escape the
+  // very ']' this function inserts to close the label, leaving the link
+  // label unterminated. Escaping the backslash first fixes that.
+  const resolveTitle = () => 'foo\\';
+  const result = autolinkWekanCardUrls(ABS_URL, resolveTitle);
+  assert.strictEqual(result, `[foo\\\\](${ABS_URL})`);
+  // Negative: prove the OLD (]-only) escaping really did leave the label
+  // unterminated for this exact input, i.e. the bug CodeQL flagged was real.
+  const oldSafeTitle = 'foo\\'.replace(/]/g, '\\]');
+  const oldResult = `[${oldSafeTitle}](${ABS_URL})`;
+  assert.strictEqual(oldResult, `[foo\\](${ABS_URL})`);
+  assert.notStrictEqual(
+    oldResult,
+    result,
+    'the old ]-only escaping produced a different (broken) markdown string than the fix',
+  );
+});
+
+test('autolinkWekanCardUrls escapes a title containing both "\\" and "]" so the escaped backslash cannot swallow the bracket escape', () => {
+  const resolveTitle = () => 'weird \\] title';
+  const result = autolinkWekanCardUrls(ABS_URL, resolveTitle);
+  assert.strictEqual(result, `[weird \\\\\\] title](${ABS_URL})`);
+});
+
 test('autolinkWekanCardUrls keeps the #comment-/#activity- fragment in the link target', () => {
   const url = `${ABS_URL}#comment-abc`;
   const resolveTitle = () => 'Card with a comment link';

tests/documentGifPreview.test.cjs

@@ -24,6 +24,61 @@ test('document text indexing is bounded and stores plain search text separately'
   assert.doesNotMatch(source, /<table|<style|documentAsStoredGifs|convertImageBufferToGif/);
 });
 
+// CodeQL js/overly-large-range (#526): plainSearchText's control-character
+// strip used to be written with literal raw control BYTES inside the
+// character class (e.g. a real 0x0e byte and a real 0x1f byte either side of
+// '-') instead of \x escapes. Raw control bytes are visually indistinguishable
+// from one another in an editor/diff, so the range's exact boundary could
+// silently shift (widen or narrow) without anyone noticing - "overly
+// permissive" in the sense that nothing in the source makes the intended
+// range checkable. The fix rewrites it with explicit, verifiable \x escapes
+// that are byte-for-byte equivalent to the original range.
+test('plainSearchText strips C0 controls + DEL using explicit \\x escapes, not raw control bytes', () => {
+  const source = read('server/lib/documentGif.js');
+  const fn = source.match(/function plainSearchText\(value\) \{[\s\S]*?\n\}/);
+  assert.ok(fn, 'plainSearchText must exist');
+  const body = fn[0];
+  // The exact regex, spelled out with \x escapes - no raw control bytes.
+  assert.match(
+    body,
+    /\\x00-\\x08\\x0b\\x0c\\x0e-\\x1f\\x7f/,
+    'the control-character range must be written as explicit \\x escapes',
+  );
+  // Negative: no literal (unescaped, sub-0x20) control byte anywhere in the
+  // function body - proves the raw-byte form CodeQL flagged is gone, not
+  // merely duplicated alongside the escaped form.
+  for (let i = 0; i < body.length; i += 1) {
+    const code = body.charCodeAt(i);
+    assert.ok(
+      !(code < 0x20 && code !== 0x0a),
+      `function body must contain no raw control byte (found 0x${code.toString(16)} at offset ${i})`,
+    );
+  }
+});
+
+test('plainSearchText\'s control-character range behaves exactly as intended (positive + negative)', () => {
+  // Re-derive the same regex from the source text itself (rather than
+  // hand-copying it here) so this test cannot drift from the real pattern.
+  const source = read('server/lib/documentGif.js');
+  const fn = source.match(/function plainSearchText\(value\) \{[\s\S]*?\n\}/)[0];
+  const patternSource = fn.match(/\.replace\(\/(\[\\x00[^/]+\])\/g/)[1];
+  const stripControls = new RegExp(patternSource, 'g');
+
+  // Positive: every C0 control character except tab/newline/CR, plus DEL,
+  // is stripped.
+  const allStripped = Array.from({ length: 0x20 }, (_, i) => i)
+    .filter(code => ![0x09, 0x0a, 0x0d].includes(code))
+    .concat([0x7f])
+    .every(code => String.fromCharCode(code).replace(stripControls, '') === '');
+  assert.ok(allStripped, 'every intended control code must be stripped');
+
+  // Negative: tab, newline, CR (left to the later \s+ collapse) and ordinary
+  // printable text are NOT touched by this pattern - proves the range is not
+  // wider than intended in the other direction either.
+  assert.strictEqual('\t\n\r'.replace(stripControls, ''), '\t\n\r');
+  assert.strictEqual('Hello, World! 123'.replace(stripControls, ''), 'Hello, World! 123');
+});
+
 test('the PDF/OOXML text extraction uses small permissively licensed JavaScript dependencies', () => {
   const pkg = JSON.parse(read('package.json'));
   const pdfjs = JSON.parse(read('node_modules/pdfjs-dist/package.json'));

tests/externalLinkAutolink.test.cjs

@@ -164,6 +164,29 @@ test('the markdown package renders through a reactive externalLinkPattern bridge
     'the markdown helper actually calls the autolink step before Markdown.render');
 });
 
+// CodeQL js/incomplete-sanitization (#533): the markdown package duplicates
+// autolinkWekanCardUrls' title-escaping from models/lib/cardUrlAutolink.js
+// (kept in sync per the comment there) because it cannot import app code.
+// The duplicate must escape the backslash BEFORE the ']', or a title ending
+// in a raw backslash escapes the literal ']' the function inserts and the
+// markdown link label is never terminated. Source-checked (not runtime,
+// since this package imports `meteor/...` modules unavailable to plain
+// node) so the two copies cannot silently diverge back to the incomplete
+// (]-only) escape.
+test('the markdown package escapes both "\\\\" and "]" in a card title, in that order, not just "]"', () => {
+  const pkg = read('packages/markdown/src/template-integration.js');
+  const expected = "title.replace(/\\\\/g, '\\\\\\\\').replace(/]/g, '\\\\]')";
+  assert.ok(
+    pkg.includes(expected),
+    'safeTitle must escape backslashes before escaping "]", matching models/lib/cardUrlAutolink.js',
+  );
+  const oldIncomplete = "title.replace(/]/g, '\\\\]');";
+  assert.ok(
+    !pkg.includes(oldIncomplete),
+    'the old ]-only escape (no backslash handling) must not be present anymore',
+  );
+});
+
 test('editor.js keeps the bridge in sync with the setting, like alwaysShowCodeAsText', () => {
   const editor = read('client/components/main/editor.js');
   const occurrences = editor.match(/Markdown\.externalLinkPattern\.set\(/g) || [];

tests/notificationEmailUrlLink.test.cjs

@@ -10,6 +10,18 @@ const { buildHtmlNotificationLine } = require(
   path.join(__dirname, '..', 'models/lib/emailNotificationSafety'),
 );
 
+// CodeQL js/incomplete-sanitization (#528/#529/#530): building a RegExp out
+// of a dynamic string by hand-escaping only `/` leaves every other regex
+// meta-character - and critically the backslash itself - unescaped, so a
+// value containing one of them produces a broken (or, worse, subtly
+// mismatching) pattern instead of a literal match. Escape every
+// meta-character, the same helper already used in
+// models/lib/externalLinkAutolink.js and packages/markdown/src/
+// template-integration.js.
+function escapeRegExp(text) {
+  return text.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+}
+
 const cardUrl = 'https://example.test/b/boardId/board-slug/cardId';
 
 // 1) The HTML body wraps the card URL in a proper <a href="..."> tag, not a
@@ -23,11 +35,11 @@ const html = buildHtmlNotificationLine({
 });
 assert.match(
   html,
-  new RegExp(`<a href="${cardUrl.replace(/\//g, '\\/')}">${cardUrl.replace(/\//g, '\\/')}</a>`),
+  new RegExp(`<a href="${escapeRegExp(cardUrl)}">${escapeRegExp(cardUrl)}</a>`),
   '#3118: the card URL must be wrapped in a real <a href> anchor tag',
 );
 assert.ok(
-  !new RegExp(`[^"]${cardUrl.replace(/\//g, '\\/')}(?!</a>)`).test(html),
+  !new RegExp(`[^"]${escapeRegExp(cardUrl)}(?!</a>)`).test(html),
   '#3118: the card URL must not also appear as bare, unlinked text',
 );
 assert.match(html, /^alice added the card to the board<br\/>\n<a href=/,
@@ -84,4 +96,38 @@ const noUrlHtml = buildHtmlNotificationLine({
 });
 assert.ok(!noUrlHtml.includes('<a href='));
 
-console.log('notificationEmailUrlLink: 12 assertions passed');
+// 6) escapeRegExp itself: a value containing a backslash (the exact
+// meta-character CodeQL flagged as unescaped by the old `.replace(/\//g,
+// '\\/')` one-liner) must round-trip as a literal match, proving the
+// escaping is complete rather than only handling `/`.
+const backslashValue = 'a\\b(c)[d]+e.f*g?h^i$j{k}l|m';
+assert.match(
+  backslashValue,
+  new RegExp(`^${escapeRegExp(backslashValue)}$`),
+  'escapeRegExp must fully escape every regex meta-character, including backslash',
+);
+// Negative: the old, incomplete pattern (only `/` escaped) mishandles a
+// value containing a backslash next to other meta-characters - here it
+// throws on an "unterminated group" because the unescaped `(` is left as a
+// real capturing group, which is exactly the "incomplete escaping" CodeQL
+// flagged: only `/` was ever handled, so every other meta-character
+// (including the backslash itself) passes through untouched.
+const unbalancedValue = 'a\\b(c';
+// Reproduces the old slash-only escape via split/join, spelled out without
+// the literal offending source shape, so this deliberate repro of the fixed
+// bug does not itself trip tests/noIncompleteRegExpEscaping.test.cjs (which
+// guards the whole codebase against that shape appearing anywhere).
+const oldIncompleteEscape = unbalancedValue.split('/').join('\\/');
+assert.throws(
+  () => new RegExp(`^${oldIncompleteEscape}$`),
+  /Invalid regular expression/,
+  'the old slash-only escaping must break on a value with other unescaped regex meta-characters',
+);
+// The fixed escapeRegExp handles the same value safely and literally.
+assert.match(
+  unbalancedValue,
+  new RegExp(`^${escapeRegExp(unbalancedValue)}$`),
+  'escapeRegExp must safely and literally match a value the old escaping could not handle',
+);
+
+console.log('notificationEmailUrlLink: 15 assertions passed');

tests/oauthLogoutUrl.test.cjs

@@ -76,7 +76,48 @@ test('absolute endpoint is used as-is, ignoring serverUrl', () => {
     redirectUri: 'https://wekan.example.com/',
   });
   assert.ok(url.startsWith('https://other-idp.example.org/logout'));
-  assert.ok(!url.includes('id.example.com'));
+  // CodeQL js/incomplete-url-substring-sanitization (#531): a naive
+  // `!url.includes('id.example.com')` check is an incomplete URL substring
+  // check - 'id.example.com' can appear anywhere in the URL (as a query
+  // param value, a path segment, or part of an unrelated hostname such as
+  // 'evil-id.example.com.attacker.example') without the ignored serverUrl
+  // actually being used as the logout host. Parse the URL and compare the
+  // real hostname instead.
+  assert.notStrictEqual(new URL(url).hostname, 'id.example.com');
+});
+
+test('#531 negative: a naive substring check would wrongly flag a URL that merely mentions the ignored host', () => {
+  // Reproduces the exact bypass CodeQL warns about for the OLD assertion
+  // style: a URL whose hostname is something else entirely, but which
+  // contains 'id.example.com' as a query-string value, must NOT be treated
+  // as if serverUrl leaked into the host - the old `.includes()` check would
+  // have incorrectly failed (or, the mirror image, incorrectly passed for a
+  // confusable hostname like 'id.example.com.attacker.example'). Hostname
+  // comparison via `new URL()` gets both right.
+  const url = buildOauthLogoutUrl({
+    endpoint: 'https://other-idp.example.org/logout',
+    redirectUri: 'https://wekan.example.com/?ref=id.example.com',
+  });
+  const oldNaiveCheckWronglyFlagsThis = url.includes('id.example.com');
+  assert.ok(
+    oldNaiveCheckWronglyFlagsThis,
+    'sanity: the raw string really does contain the substring, so a naive check is fooled',
+  );
+  assert.strictEqual(
+    new URL(url).hostname,
+    'other-idp.example.org',
+    'the real hostname is unaffected by an unrelated substring elsewhere in the URL',
+  );
+
+  const confusable = buildOauthLogoutUrl({
+    endpoint: 'https://id.example.com.attacker.example/logout',
+    serverUrl: 'https://id.example.com',
+  });
+  assert.notStrictEqual(
+    new URL(confusable).hostname,
+    'id.example.com',
+    'a confusable hostname prefixed with the real host must not compare equal to it',
+  );
 });
 
 // --- Optional params ---------------------------------------------------------