Address PR review comments: security, robustness, and correctness fixes

- Fix frontmatter parsing in ensure-skill-version-check.js to use line-based
  regex instead of indexOf, handling CRLF and avoiding false matches
- Reject newline characters in yamlStr() for both serverlogic and cloudflow
  metadata scripts to prevent invalid YAML generation
- Accept optional catch binding syntax (catch { }) in validator
- Strip comments and string literals before disallowed-token checks in
  validator to prevent false positives
- Add HTTPS URL validation in list-custom-actions.js before passing to
  getAuthToken() to prevent shell injection
- Fix header comment in list-cloud-flows.js to match actual output shape
- Remove duplicate test in list-cloud-flows.test.js

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: 2 people

plugins/power-pages/scripts/list-custom-actions.js

@@ -150,6 +150,19 @@ async function main() {
   }
 
   const cleanUrl = envUrl.replace(/\/+$/, '');
+
+  // Validate the URL is a strict HTTPS URL to prevent shell injection via getAuthToken
+  try {
+    const parsed = new URL(cleanUrl);
+    if (parsed.protocol !== 'https:') {
+      process.stderr.write('Error: environmentUrl must use HTTPS.\n');
+      process.exit(1);
+    }
+  } catch {
+    process.stderr.write(`Error: Invalid URL: "${cleanUrl}"\n`);
+    process.exit(1);
+  }
+
   const token = getAuthToken(cleanUrl);
   if (!token) {
     process.stderr.write(

plugins/power-pages/scripts/tests/list-cloud-flows.test.js

@@ -27,10 +27,3 @@ test('fails when pac CLI is not available (empty PATH)', () => {
   assert.match(result.stderr, /pac auth who|environment|unable/i);
 });
 
-test('fails when az CLI is not available for token (empty PATH)', () => {
-  // With empty PATH, pac auth who also fails, so the first error fires
-  const result = runListCloudFlows({ PATH: '' });
-  assert.notEqual(result.status, 0);
-  // Should fail at authentication stage
-  assert.ok(result.stderr.length > 0, 'should produce an error message');
-});

plugins/power-pages/skills/add-cloud-flow/scripts/create-cloud-flow-metadata.js

@@ -110,7 +110,12 @@ if (fs.existsSync(filePath)) {
 const uuid = generateUuid();
 
 // Serialize a string value safely for YAML: always single-quote, escaping internal single quotes.
+// Rejects newlines since single-quoted YAML scalars cannot span lines without breaking structure.
 function yamlStr(val) {
+  if (/[\r\n]/.test(val)) {
+    process.stderr.write(`Error: Value contains newline characters which are not supported in single-line YAML fields: "${val.slice(0, 50)}..."\n`);
+    process.exit(1);
+  }
   return "'" + val.replace(/'/g, "''") + "'";
 }
 

plugins/power-pages/skills/add-cloud-flow/scripts/list-cloud-flows.js

@@ -15,7 +15,7 @@
 //   node list-cloud-flows.js
 //
 // Output (JSON to stdout):
-//   { "flows": [ { "id": "<guid>", "name": "<string>", "description": "<string>", "state": "Active|Draft" } ] }
+//   { "flows": [ { "id": "<guid>", "flowRpName": "<string>", "displayName": "<string>", "description": "<string>", "state": "Active|Draft" } ] }
 //
 // Exits with code 1 on errors (messages to stderr).
 

plugins/power-pages/skills/add-server-logic/scripts/create-serverlogic-metadata.js

@@ -77,7 +77,12 @@ if (fs.existsSync(filePath)) {
 const uuid = generateUuid();
 
 // Serialize a string value safely for YAML: always single-quote, escaping internal single quotes.
+// Rejects newlines since single-quoted YAML scalars cannot span lines without breaking structure.
 function yamlStr(val) {
+  if (/[\r\n]/.test(val)) {
+    console.error(`Error: Value contains newline characters which are not supported in single-line YAML fields: "${val.slice(0, 50)}..."`);
+    process.exit(1);
+  }
   return "'" + val.replace(/'/g, "''") + "'";
 }
 

plugins/power-pages/skills/add-server-logic/scripts/validate-serverlogic.js

@@ -134,7 +134,7 @@ runValidation((cwd) => {
         const fnBody = content.slice(bodyStart, bodyEnd);
         if (!/\btry\s*\{/.test(fnBody)) {
           errors.push(`${dirName}.js: function '${fn}' is missing try/catch error handling`);
-        } else if (!/\bcatch\s*\(/.test(fnBody)) {
+        } else if (!/\bcatch\s*[({]/.test(fnBody)) {
           errors.push(`${dirName}.js: function '${fn}' has try but is missing a catch block`);
         }
         if (isAsync && !/\bawait\b/.test(fnBody)) {
@@ -176,26 +176,30 @@ runValidation((cwd) => {
       }
     }
 
+    // Strip comments and string literals so disallowed-token checks don't false-positive
+    // on occurrences inside documentation comments or string values.
+    const strippedContent = stripCommentsAndStrings(content);
+
     // Check: no require/import statements
-    if (/\brequire\s*\(/.test(content) || /\bimport\s+/.test(content)) {
+    if (/\brequire\s*\(/.test(strippedContent) || /\bimport\s+/.test(strippedContent)) {
       errors.push(`${dirName}.js: contains require() or import — no external dependencies allowed`);
     }
 
     // Check: no browser APIs
     for (const api of BROWSER_APIS) {
       const regex = new RegExp(`\\b${api}`, 'g');
-      if (regex.test(content)) {
+      if (regex.test(strippedContent)) {
         errors.push(`${dirName}.js: contains browser API '${api.replace('\\.', '')}' — not available in server logic runtime`);
       }
     }
 
     // Check: no console usage
-    if (/\bconsole\s*\./.test(content)) {
+    if (/\bconsole\s*\./.test(strippedContent)) {
       errors.push(`${dirName}.js: contains console.* — use Server.Logger instead`);
     }
 
     // Check: no 'function delete()'
-    if (/(?:async\s+)?function\s+delete\s*\(/m.test(content)) {
+    if (/(?:async\s+)?function\s+delete\s*\(/m.test(strippedContent)) {
       errors.push(`${dirName}.js: uses 'function delete()' — 'delete' is a reserved word, use 'del' instead`);
     }
   }
@@ -221,6 +225,45 @@ function findServerLogicDirs(dir) {
   return dirs;
 }
 
+/**
+ * Replace all comments and string literals with whitespace so that regex
+ * checks for disallowed tokens don't match inside non-code contexts.
+ */
+function stripCommentsAndStrings(src) {
+  let result = '';
+  let i = 0;
+  while (i < src.length) {
+    const ch = src[i];
+    // Line comment
+    if (ch === '/' && src[i + 1] === '/') {
+      while (i < src.length && src[i] !== '\n') { result += ' '; i++; }
+      continue;
+    }
+    // Block comment
+    if (ch === '/' && src[i + 1] === '*') {
+      result += ' '; i++;
+      result += ' '; i++;
+      while (i < src.length - 1 && !(src[i] === '*' && src[i + 1] === '/')) { result += ' '; i++; }
+      if (i < src.length) { result += ' '; i++; }
+      if (i < src.length) { result += ' '; i++; }
+      continue;
+    }
+    // String literal
+    if (ch === '\'' || ch === '"' || ch === '`') {
+      result += ' '; i++;
+      while (i < src.length && src[i] !== ch) {
+        if (src[i] === '\\') { result += ' '; i++; }
+        result += ' '; i++;
+      }
+      if (i < src.length) { result += ' '; i++; }
+      continue;
+    }
+    result += ch;
+    i++;
+  }
+  return result;
+}
+
 /**
  * Find all top-level function names using brace-depth tracking.
  * Skips string literals and comments so nested functions are not reported.

scripts/ensure-skill-version-check.js

@@ -33,16 +33,17 @@ function hasVersionCheck(content) {
 }
 
 function addVersionCheck(content) {
-  // Find the closing --- of the YAML frontmatter (second occurrence of ---)
-  const firstIdx = content.indexOf('---');
-  if (firstIdx === -1) return content;
-  const secondIdx = content.indexOf('---', firstIdx + 3);
-  if (secondIdx === -1) return content;
-  const insertPos = secondIdx + 3;
+  // Match YAML frontmatter: starts with --- on its own line, ends with --- on its own line.
+  // Use line-based matching to avoid false positives from --- inside body or values,
+  // and handle both LF and CRLF line endings.
+  const match = content.match(/^---[ \t]*\r?\n[\s\S]*?\r?\n---[ \t]*\r?\n/);
+  if (!match) return content;
+  const insertPos = match[0].length;
   return (
     content.slice(0, insertPos) +
-    '\n\n' +
+    '\n' +
     VERSION_CHECK_LINE +
+    '\n' +
     content.slice(insertPos)
   );
 }