Skip to content

Security hardening and code quality improvements#10

Merged
jongio merged 3 commits into
mainfrom
mq
Mar 3, 2026
Merged

Security hardening and code quality improvements#10
jongio merged 3 commits into
mainfrom
mq

Conversation

@jongio
Copy link
Copy Markdown
Owner

@jongio jongio commented Mar 2, 2026

MQ + Hack Analysis (Dual-Model: Opus 4.6 + Codex 5.3)

HIGH

  • HTTPS downgrade via redirect (CWE-319) - Reject non-HTTPS redirects in update-registry.js
  • No artifact domain validation (CWE-20) - Added ALLOWED_ARTIFACT_HOST allowlist
  • Response size DoS (CWE-400) - Added 5MB size limit on artifact downloads

MEDIUM

  • Weak checksum acceptance (CWE-328) - Reject MD5/SHA1 in validate-registry.js
  • Missing CSP headers (CWE-16) - Added CSP meta tag to website
  • Regex injection (CWE-1333) - Added escapeRegExp() in update-readme-versions.js
  • Unused code removed from validate-registry.js

Refactoring

  • Extracted compareSemver to shared scripts/lib/semver.js (DRY)
  • Added azd-rest to install/uninstall/watch scripts
  • Updated dependencies, pinned astro 5.17.3
  • Updated SECURITY.md with GitHub Security Advisories link

11 files changed, +219/-95 lines

MQ + Hack dual-model analysis (Opus 4.6 + Codex 5.3) findings and fixes:

HIGH:
- HTTPS downgrade via redirect following in update-registry.js (CWE-319)
- No artifact domain validation - added ALLOWED_ARTIFACT_HOST allowlist (CWE-20)
- Response size DoS - added 5MB size limit on artifact downloads (CWE-400)

MEDIUM:
- Weak checksum acceptance (MD5/SHA1) rejected in validate-registry.js (CWE-328)
- Missing CSP headers on website (CWE-16)
- Regex injection in update-readme-versions.js - added escapeRegExp (CWE-1333)
- Removed unused code in validate-registry.js

DRY refactor: extracted compareSemver to shared scripts/lib/semver.js module.
Updated dependencies, pinned astro@5.17.3 (5.18.0 Windows build bug).
Added azd-rest to install/uninstall/watch scripts.
Updated SECURITY.md with GitHub Security Advisories link.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
function isAllowedArtifactUrl(url) {
try {
const parsed = new URL(url);
return parsed.protocol === 'https:' && parsed.hostname.endsWith(ALLOWED_ARTIFACT_HOST);

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization High

'
github.com
' may be preceded by an arbitrary host name.

Copilot Autofix

AI 3 months ago

In general, to fix this kind of problem you must avoid substring/suffix checks for host validation and instead compare the parsed hostname against a whitelist of exact allowed hostnames or well-defined patterns that cannot be spoofed by prefixing or embedding the allowed host. For GitHub artifacts, that usually means allowing only github.com and possibly objects.githubusercontent.com or similar, as explicit, complete hostnames.

For this code, the safest change that preserves functionality and intent is to replace the endsWith(ALLOWED_ARTIFACT_HOST) check with an equality check against a whitelist of allowed hostnames. Since the surrounding comment says “Allowed hostname for artifact download URLs (GitHub releases only)” and the constant is singular, we can update it to an array of explicit hostnames and check includes(parsed.hostname). This avoids accidental acceptance of evilgithub.com while still allowing additional GitHub-related artifact hosts if desired in the future.

Concretely, in scripts/update-registry.js:

  • Change ALLOWED_ARTIFACT_HOST from a string to an array like ['github.com'] (or include any additional strictly-known hosts).
  • Update isAllowedArtifactUrl so that it returns true only if:
    • parsed.protocol === 'https:', and
    • ALLOWED_ARTIFACT_HOSTS.includes(parsed.hostname) (exact match, case-sensitive, which is fine given Node’s URL normalization).
  • Optionally, rename the constant to ALLOWED_ARTIFACT_HOSTS to reflect that it is now a list, but keep usage limited to the provided snippet.

No new imports are needed; we continue using the built-in URL class and existing imports only.

Suggested changeset 1
scripts/update-registry.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/scripts/update-registry.js b/scripts/update-registry.js
--- a/scripts/update-registry.js
+++ b/scripts/update-registry.js
@@ -54,8 +54,8 @@
   'https://raw.githubusercontent.com/jongio/azd-rest/refs/heads/main/registry.json',
 ];
 
-// Allowed hostname for artifact download URLs (GitHub releases only)
-const ALLOWED_ARTIFACT_HOST = 'github.com';
+// Allowed hostnames for artifact download URLs (GitHub releases only)
+const ALLOWED_ARTIFACT_HOSTS = ['github.com'];
 
 /**
  * Validate that an artifact URL points to an allowed domain.
@@ -65,7 +65,7 @@
 function isAllowedArtifactUrl(url) {
   try {
     const parsed = new URL(url);
-    return parsed.protocol === 'https:' && parsed.hostname.endsWith(ALLOWED_ARTIFACT_HOST);
+    return parsed.protocol === 'https:' && ALLOWED_ARTIFACT_HOSTS.includes(parsed.hostname);
   } catch {
     return false;
   }
EOF
@@ -54,8 +54,8 @@
'https://raw.githubusercontent.com/jongio/azd-rest/refs/heads/main/registry.json',
];

// Allowed hostname for artifact download URLs (GitHub releases only)
const ALLOWED_ARTIFACT_HOST = 'github.com';
// Allowed hostnames for artifact download URLs (GitHub releases only)
const ALLOWED_ARTIFACT_HOSTS = ['github.com'];

/**
* Validate that an artifact URL points to an allowed domain.
@@ -65,7 +65,7 @@
function isAllowedArtifactUrl(url) {
try {
const parsed = new URL(url);
return parsed.protocol === 'https:' && parsed.hostname.endsWith(ALLOWED_ARTIFACT_HOST);
return parsed.protocol === 'https:' && ALLOWED_ARTIFACT_HOSTS.includes(parsed.hostname);
} catch {
return false;
}
Copilot is powered by AI and may make mistakes. Always verify output.
} else {
try {
const parsed = new URL(artifact.url);
if (!parsed.hostname.endsWith(ALLOWED_ARTIFACT_HOST)) {

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization High

'
github.com
' may be preceded by an arbitrary host name.

Copilot Autofix

AI 3 months ago

In general, instead of checking hostname.endsWith(ALLOWED_ARTIFACT_HOST), compare the parsed hostname against a whitelist of exactly allowed hosts (and, if needed, explicitly listed subdomains). This avoids accepting arbitrary domains that merely contain the allowed domain as a suffix.

For this script, the best fix with minimal functional change is:

  • Replace the endsWith check with an exact‑match check against a small array of allowed hostnames.
  • Keep using new URL() to parse the URL; do not rely on string operations on the full URL.
  • Define an ALLOWED_ARTIFACT_HOSTS array near the existing ALLOWED_ARTIFACT_HOST (or in place of it, if that constant is defined in this file), and use includes(parsed.hostname) instead of endsWith.

Concretely, in validateAllVersions (around lines 169–176), change:

if (!parsed.hostname.endsWith(ALLOWED_ARTIFACT_HOST)) {
  fail(...);
}

to:

if (!ALLOWED_ARTIFACT_HOSTS.includes(parsed.hostname)) {
  fail(...);
}

and add a definition of ALLOWED_ARTIFACT_HOSTS in this file (for example, near the top, after the other constants). Since we are told to only modify code shown here and cannot see where ALLOWED_ARTIFACT_HOST is defined, we introduce ALLOWED_ARTIFACT_HOSTS locally and keep using the existing constant in the error message to avoid changing behavior elsewhere.

Suggested changeset 1
scripts/validate-registry.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/scripts/validate-registry.js b/scripts/validate-registry.js
--- a/scripts/validate-registry.js
+++ b/scripts/validate-registry.js
@@ -31,6 +31,11 @@
   'linux/arm64',
 ];
 
+// Only these exact hosts are allowed for artifact URLs.
+const ALLOWED_ARTIFACT_HOSTS = [
+  ALLOWED_ARTIFACT_HOST,
+];
+
 // Allowed hostname for artifact download URLs
 const ALLOWED_ARTIFACT_HOST = 'github.com';
 
@@ -168,7 +173,7 @@
       } else {
         try {
           const parsed = new URL(artifact.url);
-          if (!parsed.hostname.endsWith(ALLOWED_ARTIFACT_HOST)) {
+          if (!ALLOWED_ARTIFACT_HOSTS.includes(parsed.hostname)) {
             fail(
               `[${extId}@${ver.version}] ${platform}: URL from disallowed domain — ${artifact.url}`
             );
EOF
@@ -31,6 +31,11 @@
'linux/arm64',
];

// Only these exact hosts are allowed for artifact URLs.
const ALLOWED_ARTIFACT_HOSTS = [
ALLOWED_ARTIFACT_HOST,
];

// Allowed hostname for artifact download URLs
const ALLOWED_ARTIFACT_HOST = 'github.com';

@@ -168,7 +173,7 @@
} else {
try {
const parsed = new URL(artifact.url);
if (!parsed.hostname.endsWith(ALLOWED_ARTIFACT_HOST)) {
if (!ALLOWED_ARTIFACT_HOSTS.includes(parsed.hostname)) {
fail(
`[${extId}@${ver.version}] ${platform}: URL from disallowed domain — ${artifact.url}`
);
Copilot is powered by AI and may make mistakes. Always verify output.
jongio and others added 2 commits March 3, 2026 06:05
Add permissions: contents: read to resolve CodeQL
actions/missing-workflow-permissions alert.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jongio jongio merged commit 32f4f80 into main Mar 3, 2026
3 of 5 checks passed
@jongio jongio deleted the mq branch March 3, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants