From abcc2b08c1e36d348be73ceb4c13b649650ec13a Mon Sep 17 00:00:00 2001
From: Martin Seemann <martin.seemann@intenta.de>
Date: Sun, 2 May 2021 19:01:42 +0200
Subject: [PATCH 1/2] Make cache editing more robust against corner cases

---
 src/api.ts        |  2 +
 src/cache-view.ts | 99 ++++++++++++++++++++++++++++++++++-------------
 src/cache.ts      | 88 +++++++++++++++++++++++++++++++----------
 3 files changed, 142 insertions(+), 47 deletions(-)

diff --git a/src/api.ts b/src/api.ts
index e55c05836..52bad91ac 100644
--- a/src/api.ts
+++ b/src/api.ts
@@ -90,6 +90,8 @@ export interface CacheEntryProperties {
   value: any;
   /** Whether this entry is ADVANCED, meaning it hidden from the user. */
   advanced: boolean;
+  /** List of allowed values, as specified by STRINGS property */
+  choices: string[];
 }
 
 /**
diff --git a/src/cache-view.ts b/src/cache-view.ts
index d488bb2fd..837bf31fd 100644
--- a/src/cache-view.ts
+++ b/src/cache-view.ts
@@ -15,6 +15,8 @@ const localize: nls.LocalizeFunc = nls.loadMessageBundle();
 export interface IOption {
     key: string;    // same as CMake cache variable key names
     type: string;   // "Bool" for boolean and "String" for anything else for now
+    helpString: string;
+    choices: string[];
     value: string;  // value from the cache file or changed in the UI
     dirty: boolean; // if the variable was edited in the UI
 }
@@ -224,7 +226,7 @@ export class ConfigurationWebview {
         // Static cache entries are set automatically by CMake, overriding any value set by the user in this view.
         // Not useful to show these entries in the list.
         if (entry.type !== api.CacheEntryType.Static) {
-          options.push({ key: entry.key, type: (entry.type === api.CacheEntryType.Bool) ? "Bool" : "String", value: entry.value, dirty: false });
+          options.push({ key: entry.key, helpString: entry.helpString, choices: entry.choices, type: (entry.type === api.CacheEntryType.Bool) ? "Bool" : "String", value: entry.value, dirty: false });
         }
       }
 
@@ -288,6 +290,10 @@ export class ConfigurationWebview {
             border: 1px solid var(--vscode-settings-textInputBorder);
           }
 
+          .invalid-selection {
+            background-color: #4e2621;
+          }
+
           .vscode-light .input-disabled {
               background-color:rgba(255, 255, 255, 0.4);
               color: rgb(138, 138, 138);
@@ -420,16 +426,30 @@ export class ConfigurationWebview {
         </style>
         <script>
           const vscode = acquireVsCodeApi();
-          function toggleKey(id) {
-            const label = document.getElementById('LABEL_' + id);
-            label.textContent = label.textContent == 'ON' ? 'OFF' : 'ON';
-            const checkbox = document.getElementById(id);
-            vscode.postMessage({key: id, type: "Bool", value: checkbox.checked});
+          function updateCheckboxState(checkbox) {
+            checkbox.labels.forEach(label => label.textContent = checkbox.checked ? 'ON' : 'OFF');
+          }
+          function toggleKey(checkbox) {
+            updateCheckboxState(checkbox);
+            vscode.postMessage({key: checkbox.id, type: "Bool", value: checkbox.checked});
             document.getElementById('not-saved').classList.remove('invisible');
           }
-          function edit(id) {
-            const editbox = document.getElementById(id);
-            vscode.postMessage({key: id, type: "String", value: editbox.value});
+          function validateInput(editbox) {
+            const list = editbox.list;
+            if (list) {
+              let found = false;
+              for (const opt of list.options) {
+                if (opt.value === editbox.value) {
+                  found = true;
+                  break;
+                }
+              }
+              editbox.classList.toggle('invalid-selection', !found);
+            }
+          }
+          function edit(editbox) {
+            validateInput(editbox);
+            vscode.postMessage({key: editbox.id, type: "String", value: editbox.value});
             document.getElementById('not-saved').classList.remove('invisible');
           }
           function save() {
@@ -446,6 +466,17 @@ export class ConfigurationWebview {
               }
             }
           }
+
+          window.onload = function() {
+            document.querySelectorAll('.cmake-input-bool').forEach(checkbox => {
+              updateCheckboxState(checkbox);
+              checkbox.onclick = () => toggleKey(checkbox);
+            });
+            document.querySelectorAll('.cmake-input-text').forEach(editbox => {
+              validateInput(editbox)
+              editbox.oninput = () => edit(editbox);
+            });
+          }
         </script>
     </head>
     <body>
@@ -467,26 +498,42 @@ export class ConfigurationWebview {
 
     // compile a list of table rows that contain the key and value pairs
     const tableRows = this._options.map(option => {
+
+      // HTML attributes may not contain literal double quotes or ambiguous ampersands
+      const escapeAttribute = (text: string) => {
+        return text.replace(/&/g, "&amp;").replace(/"/g, "&quot;");
+      };
+      // Escape HTML special characters that may not occur literally in any text
+      const escapeHtml = (text: string) => {
+        return escapeAttribute(text)
+          .replace(/</g, "&lt;")
+          .replace(/>/g, "&gt;")
+          .replace(/'/g, "&#039;")
+          .replace(/ /g, "&nbsp;"); // we are usually dealing with single line entities - avoid unintential line breaks
+      };
+
+      const id = escapeAttribute(option.key);
+      let editControls = '';
+
       if (option.type === "Bool") {
-        return `<tr class="content-tr">
-        <td></td>
-        <td>${option.key}</td>
-        <td>
-          <input class="cmake-input-bool" id="${option.key}" onclick="toggleKey('${option.key}')"
-                 type="checkbox" ${util.isTruthy(option.value) ? 'checked' : ''}>
-          <label id="LABEL_${option.key}" for="${option.key}">${util.isTruthy(option.value) ? `ON` : `OFF`}</label>
-        </td>
-      </tr>`;
+        editControls = `<input class="cmake-input-bool" id="${id}" type="checkbox" ${util.isTruthy(option.value) ? 'checked' : ''}>
+          <label id="LABEL_${id}" for="${id}"/>`;
       } else {
-        return `<tr class="content-tr">
-        <td></td>
-        <td>${option.key}</td>
-        <td>
-          <input id="${option.key}" value="${option.value}" style="width: 90%;"
-                 type="text" oninput="edit('${option.key}')">
-        </td>
-      </tr>`;
+        const hasChoices = option.choices.length > 0;
+        if (hasChoices) {
+          editControls = `<datalist id="CHOICES_${id}">
+            ${option.choices.map(ch => `<option value="${escapeAttribute(ch)}">`).join()}
+          </datalist>`;
+        }
+        editControls += `<input class="cmake-input-text" id="${id}" value="${escapeAttribute(option.value)}" style="width: 90%;"
+          type="text" ${hasChoices ? `list="CHOICES_${id}"` : ''}>`;
       }
+
+      return `<tr class="content-tr">
+      <td></td>
+      <td title="${escapeAttribute(option.helpString)}">${escapeHtml(option.key)}</td>
+      <td>${editControls}</td>
+    </tr>`;
     });
 
     html = html.replace(key, tableRows.join(""));
diff --git a/src/cache.ts b/src/cache.ts
index 224195178..3bed79192 100644
--- a/src/cache.ts
+++ b/src/cache.ts
@@ -8,7 +8,7 @@ import {fs} from './pr';
 import rollbar from './rollbar';
 import * as util from './util';
 import * as nls from 'vscode-nls';
-import { isBoolean } from 'util';
+import {isBoolean, isString} from 'util';
 
 nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })();
 const localize: nls.LocalizeFunc = nls.loadMessageBundle();
@@ -18,14 +18,17 @@ const log = logging.createLogger('cache');
 
 /**
  * Implements access to CMake cache entries. See `api.CacheEntry` for more
- * information. This type is immutable.
+ * information.
  */
 export class Entry implements api.CacheEntry {
   private readonly _type: api.CacheEntryType = api.CacheEntryType.Uninitialized;
   private readonly _docs: string = '';
   private readonly _key: string = '';
   private readonly _value: any = null;
-  private readonly _advanced: boolean = false;
+  advanced: boolean = false;
+  choices: string[] = [];
+
+  serializedKey: string = '';
 
   get type() { return this._type; }
 
@@ -37,8 +40,6 @@ export class Entry implements api.CacheEntry {
 
   as<T>(): T { return this.value as T; }
 
-  get advanced() { return this._advanced; }
-
   /**
    * Create a new Cache Entry instance. Doesn't modify any files. You probably
    * want to get these from the `CMakeCache` object instead.
@@ -50,6 +51,7 @@ export class Entry implements api.CacheEntry {
    */
   constructor(key: string, value: string, type: api.CacheEntryType, docs: string, advanced: boolean) {
     this._key = key;
+    this.serializedKey = key; // may be overwritten later with quoted version of `key`
     this._type = type;
     if (type === api.CacheEntryType.Bool) {
       this._value = util.isTruthy(value);
@@ -57,7 +59,7 @@ export class Entry implements api.CacheEntry {
       this._value = value;
     }
     this._docs = docs;
-    this._advanced = advanced;
+    this.advanced = advanced;
   }
 }
 
@@ -136,6 +138,8 @@ export class CMakeCache {
 
     const entries = new Map<string, Entry>();
     let docs_acc = '';
+    const advancedNames: string[] = [];
+    const choices: Map<string, string[]> = new Map();
     for (const line of lines) {
       if (line.startsWith('//')) {
         docs_acc += /^\/\/(.*)/.exec(line)![1] + ' ';
@@ -145,14 +149,20 @@ export class CMakeCache {
           rollbar.error(localize('failed.to.read.line.from.cmake.cache.file', 'Failed to read a line from a CMake cache file {0}', line));
           continue;
         }
-        const [, , quoted_name, unquoted_name, typename, valuestr] = match;
+        const [, serializedName, quoted_name, unquoted_name, typename, valuestr] = match;
         const name = quoted_name || unquoted_name;
         if (!name || !typename)
           continue;
         log.trace(localize('read.line.in.cache', 'Read line in cache with {0}={1}, {2}={3}, {4}={5}', 'name', name, 'typename', typename, 'valuestr', valuestr));
-        if (name.endsWith('-ADVANCED') && valuestr === '1') {
-          log.trace(localize('skipping.variable', 'Skipping {0} variable', '*-ADVANCED'));
-          // We skip the ADVANCED property variables. They're a little odd.
+        if (name.endsWith('-ADVANCED')) {
+          if (valuestr === '1') {
+            const entryName = name.substr(0, name.lastIndexOf('-'));
+            advancedNames.push(entryName);
+          }
+        } else if (name.endsWith('-MODIFIED')) {
+          // ignore irrelevant entry property
+        } else if (name.endsWith('-STRINGS')) {
+          choices.set(name.substr(0, name.lastIndexOf('-')), valuestr.split(';'));
         } else {
           const key = name;
           const typemap = {
@@ -171,12 +181,33 @@ export class CMakeCache {
             rollbar.error(localize('cache.entry.unknown', 'Cache entry \'{0}\' has unknown type: \'{1}\'', name, typename));
           } else {
             log.trace(localize('constructing.new.cache.entry', 'Constructing a new cache entry from the given line'));
-            entries.set(name, new Entry(key, valuestr, type, docs, false));
+            const entry = new Entry(key, valuestr, type, docs, false);
+            entry.serializedKey = serializedName;
+            entries.set(name, entry);
           }
         }
       }
     }
 
+    // Update `advanced` attribute
+    advancedNames.forEach(name => {
+      const entry = entries.get(name);
+      if (entry) {
+        entry.advanced = true;
+      } else {
+        log.warning(localize('nonexisting.advanced.entry', 'Nonexisting cache entry \'{0}\' marked as advanced', name));
+      }
+    });
+    // update `choices`
+    choices.forEach((list, name) => {
+      const entry = entries.get(name);
+      if (entry) {
+        entry.choices = list;
+      } else {
+        log.warning(localize('ignore.strings.for.nonexisting.entry', 'Ignoring `STRINGS` property for nonexisting cache entry \'{0}\'', name));
+      }
+    });
+
     log.trace(localize('parsed.cache.entries', 'Parsed {0} cache entries', entries.size));
     return entries;
   }
@@ -188,17 +219,32 @@ export class CMakeCache {
    * @param value Boolean value
    */
   private replace(content: string, key: string, value: string): string {
-    const re = key + ':[^=]+=(.+)';
-    const found = content.match(re);
-
-    if (found && found.length >= 2) {
-      const line = found[0];
-      const currentVal = found[1];
-      const newValueLine = line.replace(currentVal, isBoolean(value) ? (value ? "TRUE" : "FALSE") : value);
-      return content.replace(line, newValueLine);
-    } else {
-      return content;
+
+    const entry = this._entries.get(key);
+    if (entry !== undefined) {
+      // cmake variable name may contain characters with special meanings in regex
+      const escapedKey = entry.serializedKey.replace(/[^A-Za-z0-9_]/g, '\\$&');
+      const re = RegExp(`^${escapedKey}(:[^=]+=)(.+)`, 'm');
+      const found = content.match(re);
+
+      if (found && found.length >= 3) {
+        const line = found[0];
+        const type = found[1];
+
+        // FIXME: How can `value` be boolean desipte being marked as string in the signature?
+
+        if (isString(value)) {
+          const newlineIndex = value.search(/[\r\n]/);
+          if (newlineIndex >= 0) {
+            value = value.substring(0, newlineIndex);
+            log.warning(localize('cache.value.truncation.warning', 'Newline(s) found in cache entry \'{0}\'. Value has been truncated to \'{1}\'', key, value));
+          }
+        }
+        const newValueLine = entry.serializedKey + type + (isBoolean(value) ? (value ? "TRUE" : "FALSE") : value);
+        return content.replace(line, newValueLine);
+      }
     }
+    return content;
   }
 
   /**

From 108f087ca60849a0a868341a30a612eb80faf646 Mon Sep 17 00:00:00 2001
From: Andreea Isac <andris@microsoft.com>
Date: Tue, 13 Jul 2021 15:29:22 -0700
Subject: [PATCH 2/2] Fix linter errors.

---
 src/cache-view.ts | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/cache-view.ts b/src/cache-view.ts
index 24aa23e39..f13acdeaa 100644
--- a/src/cache-view.ts
+++ b/src/cache-view.ts
@@ -501,17 +501,14 @@ export class ConfigurationWebview {
     const tableRows = this._options.map(option => {
 
       // HTML attributes may not contain literal double quotes or ambiguous ampersands
-      const escapeAttribute = (text: string) => {
-        return text.replace(/&/g, "&amp;").replace(/"/g, "&quot;");
-      };
+      const escapeAttribute = (text: string) => text.replace(/&/g, "&amp;").replace(/"/g, "&quot;");
       // Escape HTML special characters that may not occur literally in any text
-      const escapeHtml = (text: string) => {
-        return escapeAttribute(text)
+      const escapeHtml = (text: string) =>
+        escapeAttribute(text)
           .replace(/</g, "&lt;")
           .replace(/>/g, "&gt;")
           .replace(/'/g, "&#039;")
           .replace(/ /g, "&nbsp;"); // we are usually dealing with single line entities - avoid unintential line breaks
-      };
 
       const id = escapeAttribute(option.key);
       let editControls = '';