Skip to content
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

Jump to bottom

Fix code scanning alert no. 8: Uncontrolled data used in path expression #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix code scanning alert no. 8: Uncontrolled data used in path expression #15

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
861a7ff
Fix code scanning alert no. 8: Uncontrolled data used in path expression
gitworkflows Oct 23, 2024
7d38eff
Merge branch 'master' into alert-autofix-8
gitworkflows Oct 23, 2024
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
8 changes: 7 additions & 1 deletion server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,16 @@
}
})
// Middleware to serve any .yml files in USER_DATA_DIR with optional protection
.get('/*.yml', protectConfig, (req, res) => {
const ymlFile = req.path.split('/').pop();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enhance input validation for yml file paths

The current implementation extracts the filename without proper sanitization, which could potentially allow malicious filenames. Consider adding these security measures:

  1. Validate file extension
  2. Sanitize filename
  3. Check for null/undefined values

Apply this improvement:

-const ymlFile = req.path.split('/').pop();
+const ymlFile = req.path.split('/').pop();
+if (!ymlFile || !ymlFile.endsWith('.yml')) {
+  return res.status(400).send('Invalid file type');
+}
+// Sanitize: Remove any characters that aren't alphanumeric, dash, underscore or dot
+const sanitizedFile = ymlFile.replace(/[^a-zA-Z0-9\-_.]/g, '');
+if (sanitizedFile !== ymlFile) {
+  return res.status(400).send('Invalid filename');
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const ymlFile = req.path.split('/').pop();
const ymlFile = req.path.split('/').pop();
if (!ymlFile || !ymlFile.endsWith('.yml')) {
return res.status(400).send('Invalid file type');
}
// Sanitize: Remove any characters that aren't alphanumeric, dash, underscore or dot
const sanitizedFile = ymlFile.replace(/[^a-zA-Z0-9\-_.]/g, '');
if (sanitizedFile !== ymlFile) {
return res.status(400).send('Invalid filename');
}

res.sendFile(path.join(__dirname, process.env.USER_DATA_DIR || 'user-data', ymlFile));
const userDataDir = path.resolve(__dirname, process.env.USER_DATA_DIR || 'user-data');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Extract repeated USER_DATA_DIR logic into a constant

The process.env.USER_DATA_DIR || 'user-data' logic is used in multiple places. Consider extracting this into a constant at the top of the file to improve maintainability and ensure consistency.

const USER_DATA_DIR = path.resolve(__dirname, process.env.USER_DATA_DIR || 'user-data');

// Later in the code:
const userDataDir = USER_DATA_DIR;

const resolvedPath = path.resolve(userDataDir, ymlFile);
if (!resolvedPath.startsWith(userDataDir)) {
res.status(403).send('Forbidden');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 suggestion (security): Enhance error handling with logging for security monitoring

While sending a 403 status is appropriate, consider adding logging for these access attempts. This can help with security monitoring and detecting potential attacks.

const logger = require('./logger'); // Assume a logger is set up

logger.warn(`Forbidden access attempt: ${resolvedPath}`);
res.status(403).send('Forbidden');

return;
}
res.sendFile(resolvedPath);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling and security controls for file sending

The current implementation lacks error handling and additional security controls for file sending.

Apply these security improvements:

-res.sendFile(resolvedPath);
+res.sendFile(resolvedPath, {
+  headers: {
+    'Content-Type': 'application/x-yaml',
+    'X-Content-Type-Options': 'nosniff'
+  },
+  maxAge: '1h',
+  lastModified: true
+}, (err) => {
+  if (err) {
+    console.error(`[Error] Failed to send file ${ymlFile}:`, err);
+    if (err.code === 'ENOENT') {
+      res.status(404).send('File not found');
+    } else {
+      res.status(500).send('Internal server error');
+    }
+  }
+});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
res.sendFile(resolvedPath);
res.sendFile(resolvedPath, {
headers: {
'Content-Type': 'application/x-yaml',
'X-Content-Type-Options': 'nosniff'
},
maxAge: '1h',
lastModified: true
}, (err) => {
if (err) {
console.error(`[Error] Failed to send file ${ymlFile}:`, err);
if (err.code === 'ENOENT') {
res.status(404).send('File not found');
} else {
res.status(500).send('Internal server error');
}
}
});

Comment on lines +204 to +210
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Based on the analysis of the results, I can now provide a final response:

Path traversal protection needed for static file serving middleware

The codebase has multiple file access patterns that require attention:

  • The reviewed endpoint correctly implements path traversal protection using startsWith check
  • However, there are three express.static middleware configurations in server.js (lines 213-215) serving files from:
    • user-data directory
    • dist directory
    • public directory
  • These static middleware configurations don't have explicit path traversal protection, relying solely on Express's built-in security

Other file operations in the codebase are either:

  • Reading configuration files using absolute paths (services/ssl-server.js, services/config-validator.js)
  • Writing to specific configuration files (services/save-config.js)
  • Serving the index.html fallback (server.js line 219)
🔗 Analysis chain

Verify similar file handling patterns across the codebase

Let's verify if there are other endpoints that might need similar path traversal protection.

Let me analyze the findings and generate additional verification scripts to better understand the file access patterns and security measures.

This will help us understand:

  1. The context around file operations to see if they have proper security checks
  2. The configuration of static file serving middleware
  3. Other route handlers that might be serving files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find other potential file access patterns that might need similar protection

# Look for file system operations
echo "Searching for file system operations..."
rg -n "(?:readFile|writeFile|readdir|sendFile|createReadStream)" --type js

# Look for path manipulation
echo "\nSearching for path manipulation..."
rg -n "(?:path\.join|path\.resolve|req\.path)" --type js

# Look for static file serving
echo "\nSearching for static file serving..."
rg -n "(?:express\.static|res\.sendFile)" --type js

Length of output: 2413

Script:

#!/bin/bash
# Let's examine the context around file operations to understand security measures

# Check file access patterns in server.js
echo "Examining server.js file operations context..."
rg -B 5 -A 5 "(?:readFile|writeFile|sendFile)" server.js

# Check static file serving middleware setup
echo "\nExamining static file serving setup..."
rg -B 3 -A 3 "express\.static" server.js

# Check if there are any other routes handling files
echo "\nChecking for route handlers with file operations..."
ast-grep --pattern 'app.$_($_, function ($_, $_) {
  $$$
  res.sendFile($_);
  $$$
}'

Length of output: 2105

})

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
Loading
, but is not rate-limited.
// Serves up static files
.use(express.static(path.join(__dirname, process.env.USER_DATA_DIR || 'user-data')))
.use(express.static(path.join(__dirname, 'dist')))
Expand Down
Loading