Skip to content

fix: Address security vulnerabilities (deps + code scanning) #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
homeles merged 5 commits into from
Mar 24, 2026
Merged

fix: Address security vulnerabilities (deps + code scanning) #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e010687
fix(deps): run npm audit fix to resolve high-severity dependency vuln…
lupita-hom Mar 24, 2026
c9fde19
fix(security): prevent NoSQL injection and ReDoS in server controller…
lupita-hom Mar 24, 2026
85c0ada
fix(security): sanitize user-controlled values before logging to prev…
lupita-hom Mar 24, 2026
d0982a4
fix(security): resolve remaining CodeQL alerts
lupita-hom Mar 24, 2026
02af3ca
fix(security): address final 6 CodeQL alerts
lupita-hom Mar 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 29 additions & 47 deletions client/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 8 additions & 5 deletions client/src/api/apiService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import axios from 'axios';
import io from 'socket.io-client';

// Sanitize external values before embedding in log messages to prevent log injection.
const sanitizeLog = (v) => (v == null ? '' : String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' '));

Check warning on line 5 in client/src/api/apiService.js

View workflow job for this annotation

GitHub Actions / 🖥️ Client — Build & Lint 

Unexpected control character(s) in regular expression: \x00, \x1f

// Use the environment variables, falling back to development defaults if not set
const API_URL = process.env.REACT_APP_API_URL || 'http://localhost/api';
const WS_URL = process.env.REACT_APP_WEBSOCKET_URL || 'ws://localhost';
Expand Down Expand Up @@ -53,7 +56,7 @@
const response = await api.get(`${API_URL}/workflow-runs/repo/${repoName}`, { params });
return response.data.data || { data: [], pagination: { total: 0, page: 1, pageSize: 0, totalPages: 1 } };
} catch (error) {
console.error(`Error fetching workflow runs for repo ${repoName}:`, error);
console.error("Error fetching workflow runs for repo:", sanitizeLog(repoName), error);
throw error;
}
},
Expand All @@ -64,7 +67,7 @@
const response = await api.get(`${API_URL}/workflow-runs/${id}`);
return response.data.data;
} catch (error) {
console.error(`Error fetching workflow run ${id}:`, error);
console.error("Error fetching workflow run:", sanitizeLog(id), error);
throw error;
}
},
Expand All @@ -75,7 +78,7 @@
const response = await api.post(`${API_URL}/workflow-runs/${id}/sync`);
return response.data.data;
} catch (error) {
console.error(`Error syncing workflow run ${id}:`, error);
console.error("Error syncing workflow run:", sanitizeLog(id), error);
throw error;
}
},
Expand All @@ -85,7 +88,7 @@
const response = await api.delete(`${API_URL}/workflow-runs/${id}`);
return response.data.data;
} catch (error) {
console.error(`Error deleting workflow run ${id}:`, error);
console.error("Error deleting workflow run:", sanitizeLog(id), error);
throw error;
}
},
Expand All @@ -96,7 +99,7 @@
const response = await api.post(`${API_URL}/workflow-runs/repo/${repoName}/sync`);
return response.data.data;
} catch (error) {
console.error(`Error syncing workflow runs for repo ${repoName}:`, error);
console.error("Error syncing workflow runs for repo:", sanitizeLog(repoName), error);
throw error;
}
},
Expand Down Expand Up @@ -244,7 +247,7 @@
},

// Get the status of the database
getDatabaseStatus: async () => {

Check warning on line 250 in client/src/api/apiService.js

View workflow job for this annotation

GitHub Actions / 🖥️ Client — Build & Lint 

Duplicate key 'getDatabaseStatus'
try {
const response = await api.get(`${API_URL}/db/status`);
return response.data.data || {};
Expand Down
28 changes: 14 additions & 14 deletions server/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 7 additions & 4 deletions server/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,14 @@
const name = req.headers['x-github-event'];
const contentType = req.headers['content-type'];

// Sanitize header values before logging to prevent log injection via CRLF.
const sanitize = (v) => (v == null ? '' : String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' '));

console.log('Received webhook POST request from GitHub');
console.log('Event:', name);
console.log('Delivery ID:', id);
console.log('Content-Type:', contentType);
console.log('Signature:', signature);
console.log('Event: %s', sanitize(name));

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
Loading
.

Copilot Autofix

AI 1 day ago

In general, to fix log injection you should ensure any user-controlled value is normalized before being logged: strip or replace newline and carriage-return characters (and often other non-printable control characters), and clearly separate user data from static log text. For text logs, removing line breaks is typically sufficient; for HTML logs, HTML-encode.

In this file, the best fix with minimal behavioral change is to (1) make the sanitization function clearly focused on removing CR/LF (and, optionally, other control chars) and (2) apply it to all logged header-derived values. The code already does (2); we only need to adjust (1) to a simpler, recommendation-aligned implementation that static analysis tools are more likely to recognize. Specifically, in server/server.js around lines 107–113, replace the current sanitize definition that uses a broad control-character regex with a simpler one that explicitly strips \r and \n (and, if desired, a small, explicit additional set like \t). Leave the subsequent console.log calls as-is since they already use sanitize(...).

Concretely:

  • In server/server.js, update the sanitize helper inside the /api/webhooks/github handler to:
    • Coerce values to strings safely (handling null/undefined).
    • Replace \r and \n with spaces (or remove them).
  • No other changes to routing, logging, or imports are necessary.
Suggested changeset 1
server/server.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/server/server.js b/server/server.js
--- a/server/server.js
+++ b/server/server.js
@@ -105,7 +105,11 @@
   const contentType = req.headers['content-type'];
 
   // Sanitize header values before logging to prevent log injection via CRLF.
-  const sanitize = (v) => (v == null ? '' : String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' '));
+  // Remove carriage returns, newlines and tabs to ensure a single-line, well-formed log entry.
+  const sanitize = (v) =>
+    v == null
+      ? ''
+      : String(v).replace(/[\r\n\t]/g, ' ');
 
   console.log('Received webhook POST request from GitHub');
   console.log('Event: %s', sanitize(name));
EOF
@@ -105,7 +105,11 @@
const contentType = req.headers['content-type'];

// Sanitize header values before logging to prevent log injection via CRLF.
const sanitize = (v) => (v == null ? '' : String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' '));
// Remove carriage returns, newlines and tabs to ensure a single-line, well-formed log entry.
const sanitize = (v) =>
v == null
? ''
: String(v).replace(/[\r\n\t]/g, ' ');

console.log('Received webhook POST request from GitHub');
console.log('Event: %s', sanitize(name));
Copilot is powered by AI and may make mistakes. Always verify output.
console.log('Delivery ID: %s', sanitize(id));

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
Loading
.

Copilot Autofix

AI 1 day ago

In general, the fix is to ensure that any user-controlled value written to logs is normalized so it cannot alter log structure (no CR/LF or other control chars) and is clearly delimited. That means replacing control characters with safe placeholders and optionally constraining to a conservative character set.

For this specific case, the best approach without changing functionality is to tighten the sanitize helper used at the logging point. We’ll (a) limit the header value to a reasonable length to avoid log flooding, and (b) more clearly strip all non-printable characters and normalize whitespace, then use that consistently in the log line. This keeps the logs human-readable, does not affect downstream logic (the raw id is still used later in webhooks.receive), and directly addresses CodeQL’s concern.

Concretely, in server/server.js inside the /api/webhooks/github handler:

  • Replace the current sanitize implementation with one that:
    • Converts null/undefined to an empty string.
    • Casts to string.
    • Removes all characters outside a safe printable range.
    • Collapses internal whitespace (including tabs) to single spaces.
    • Truncates to a fixed maximum length (e.g., 200 characters) to prevent log abuse.
  • Keep using sanitize(name), sanitize(id), and sanitize(contentType) in the console.log calls as before.

No new imports or external libraries are required; this can be done with plain string methods and regular expressions.

Suggested changeset 1
server/server.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/server/server.js b/server/server.js
--- a/server/server.js
+++ b/server/server.js
@@ -104,8 +104,20 @@
   const name = req.headers['x-github-event'];
   const contentType = req.headers['content-type'];
 
-  // Sanitize header values before logging to prevent log injection via CRLF.
-  const sanitize = (v) => (v == null ? '' : String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' '));
+  // Sanitize header values before logging to prevent log injection via CRLF and control characters.
+  const sanitize = (v) => {
+    if (v == null) return '';
+    // Convert to string and remove all non-printable ASCII characters.
+    let s = String(v).replace(/[\x00-\x1F\x7F]/g, ' ');
+    // Collapse consecutive whitespace to a single space.
+    s = s.replace(/\s+/g, ' ').trim();
+    // Limit length to avoid log flooding with extremely long header values.
+    const MAX_LEN = 200;
+    if (s.length > MAX_LEN) {
+      s = s.slice(0, MAX_LEN) + '...';
+    }
+    return s;
+  };
 
   console.log('Received webhook POST request from GitHub');
   console.log('Event: %s', sanitize(name));
EOF
@@ -104,8 +104,20 @@
const name = req.headers['x-github-event'];
const contentType = req.headers['content-type'];

// Sanitize header values before logging to prevent log injection via CRLF.
const sanitize = (v) => (v == null ? '' : String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' '));
// Sanitize header values before logging to prevent log injection via CRLF and control characters.
const sanitize = (v) => {
if (v == null) return '';
// Convert to string and remove all non-printable ASCII characters.
let s = String(v).replace(/[\x00-\x1F\x7F]/g, ' ');
// Collapse consecutive whitespace to a single space.
s = s.replace(/\s+/g, ' ').trim();
// Limit length to avoid log flooding with extremely long header values.
const MAX_LEN = 200;
if (s.length > MAX_LEN) {
s = s.slice(0, MAX_LEN) + '...';
}
return s;
};

console.log('Received webhook POST request from GitHub');
console.log('Event: %s', sanitize(name));
Copilot is powered by AI and may make mistakes. Always verify output.
console.log('Content-Type: %s', sanitize(contentType));

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
Loading
.

Copilot Autofix

AI 1 day ago

General fix: ensure any user-controlled value written to logs is sanitized with a clearly defined, robust function that removes/neutralizes control characters and visually delimits user input. Then consistently use that function when logging such values.

Best concrete fix here:

  • Keep a single, named sanitizeForLog helper that:
    • Converts null/undefined to an empty string.
    • Converts the value to string.
    • Replaces all carriage returns, line feeds, and other control characters with a safe placeholder (space).
    • Optionally trims leading/trailing whitespace.
  • Use sanitizeForLog for each header before logging: name, id, and contentType.
  • Make user-controlled portions in log messages clearly delimited (e.g., wrapped in quotes).

Changes needed in server/server.js:

  • Replace the inline sanitize arrow function at line 108 with a named sanitizeForLog that is clearly oriented to log safety.
  • Update the console.log calls for Event, Delivery ID, and Content-Type to use sanitizeForLog and clearly mark the value (e.g., "Content-Type: '%s'").
  • No new imports are needed; all logic is implemented inline.

This keeps existing functionality (logging the same semantics) but hardens the sanitization and makes it clearer to both humans and tools that log injection has been intentionally addressed.

Suggested changeset 1
server/server.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/server/server.js b/server/server.js
--- a/server/server.js
+++ b/server/server.js
@@ -104,13 +104,20 @@
   const name = req.headers['x-github-event'];
   const contentType = req.headers['content-type'];
 
-  // Sanitize header values before logging to prevent log injection via CRLF.
-  const sanitize = (v) => (v == null ? '' : String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' '));
+  // Sanitize header values before logging to prevent log injection via CRLF
+  // and other control characters in plain-text logs.
+  const sanitizeForLog = (v) => {
+    if (v == null) {
+      return '';
+    }
+    // Convert to string and replace control characters (including CR/LF) with spaces.
+    return String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' ');
+  };
 
   console.log('Received webhook POST request from GitHub');
-  console.log('Event: %s', sanitize(name));
-  console.log('Delivery ID: %s', sanitize(id));
-  console.log('Content-Type: %s', sanitize(contentType));
+  console.log("Event: '%s'", sanitizeForLog(name));
+  console.log("Delivery ID: '%s'", sanitizeForLog(id));
+  console.log("Content-Type: '%s'", sanitizeForLog(contentType));
   console.log('Signature present:', Boolean(signature));
 
   if (!signature || !id || !name) {
EOF
@@ -104,13 +104,20 @@
const name = req.headers['x-github-event'];
const contentType = req.headers['content-type'];

// Sanitize header values before logging to prevent log injection via CRLF.
const sanitize = (v) => (v == null ? '' : String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' '));
// Sanitize header values before logging to prevent log injection via CRLF
// and other control characters in plain-text logs.
const sanitizeForLog = (v) => {
if (v == null) {
return '';
}
// Convert to string and replace control characters (including CR/LF) with spaces.
return String(v).replace(/[\r\n\t\x00-\x1F\x7F]/g, ' ');
};

console.log('Received webhook POST request from GitHub');
console.log('Event: %s', sanitize(name));
console.log('Delivery ID: %s', sanitize(id));
console.log('Content-Type: %s', sanitize(contentType));
console.log("Event: '%s'", sanitizeForLog(name));
console.log("Delivery ID: '%s'", sanitizeForLog(id));
console.log("Content-Type: '%s'", sanitizeForLog(contentType));
console.log('Signature present:', Boolean(signature));

if (!signature || !id || !name) {
Copilot is powered by AI and may make mistakes. Always verify output.
console.log('Signature present:', Boolean(signature));

if (!signature || !id || !name) {
console.error('Missing required webhook headers');
Expand Down
31 changes: 25 additions & 6 deletions server/src/controllers/workflowController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,15 @@ export const requireAdminToken = (req, res, next) => {
}

// Use constant-time comparison to prevent timing attacks.
// Hash both tokens to a fixed length (SHA-256) first to eliminate length-based timing leaks.
// Pad both values to equal length so timingSafeEqual works without hashing.
try {
const adminHash = crypto.createHash('sha256').update(adminToken).digest();
const providedHash = crypto.createHash('sha256').update(provided).digest();
const maxLen = Math.max(adminToken.length, provided.length);
const adminBuf = Buffer.alloc(maxLen);
const providedBuf = Buffer.alloc(maxLen);
Buffer.from(adminToken).copy(adminBuf);
Buffer.from(provided).copy(providedBuf);

if (!crypto.timingSafeEqual(adminHash, providedHash)) {
if (adminToken.length !== provided.length || !crypto.timingSafeEqual(adminBuf, providedBuf)) {
return res.status(401).json({ error: 'Unauthorized: valid admin token required.' });
}
} catch (err) {
Expand Down Expand Up @@ -125,18 +128,29 @@ export const getAllWorkflowRuns = async (req, res) => {
}
};

// Validates that a repo path matches the expected owner/repo format.
// Prevents NoSQL injection by rejecting paths with MongoDB operator characters.
const REPO_PATH_RE = /^[a-zA-Z0-9._-]+\/[a-zA-Z0-9._-]+$/;

export const getRepoWorkflowRuns = async (req, res) => {
try {
const repoPath = req.params[0];
const { workflowName } = req.query; // Get workflowName from query params
// Ensure workflowName is a plain string — qs can parse ?workflowName[$ne]=x into an
// object, which would be passed directly into a MongoDB query (NoSQL injection).
const workflowName = typeof req.query.workflowName === 'string' ? req.query.workflowName : null;

if (!repoPath) {
return errorResponse(res, 'Repository name is required', 400);
}

// Validate repoPath to prevent NoSQL injection — only allow owner/repo format
if (!REPO_PATH_RE.test(repoPath)) {
return errorResponse(res, 'Invalid repository path format', 400);
}

// First get the repository document to get all workflows
const repoDoc = await WorkflowRun.findOne({ 'repository.fullName': repoPath });

if (!repoDoc) {
return successResponse(res, {
data: [],
Expand Down Expand Up @@ -293,6 +307,11 @@ export const syncRepositoryWorkflowRuns = async (req, res) => {
return errorResponse(res, 'Repository name is required', 400);
}

// Validate repoPath to prevent NoSQL injection — only allow owner/repo format
if (!REPO_PATH_RE.test(repoPath)) {
return errorResponse(res, 'Invalid repository path format', 400);
}

const workflowRuns = await workflowService.syncRepositoryWorkflowRuns(repoPath);

// After sync is complete, emit updates for each workflow run
Expand Down
Loading
Loading