Skip to content

fix: Resolve CodeQL security vulnerabilities (type confusion and Helmet misconfiguration) - #80

Merged
MarkProminic merged 3 commits into
mainfrom
devin/1759282625-fix-codeql-security-issues
Oct 1, 2025
Merged

MarkProminic merged 3 commits into
mainfrom
devin/1759282625-fix-codeql-security-issues

Conversation

@MarkProminic

@MarkProminic MarkProminic commented Oct 1, 2025 •

Copy link
Copy Markdown
Contributor

Fix CodeQL Security Vulnerabilities

Summary

This PR resolves 6 CodeQL security vulnerabilities identified in the codebase:

  • Type confusion through parameter tampering (Critical): Added type validation in config/i18n.js to prevent array/string confusion in locale parameter handling
  • Insecure Helmet configuration (High): Removed explicit false settings for CSP and HSTS to use secure defaults
  • Missing CSRF middleware (High): Added lusca.csrf() middleware after session setup to protect against CSRF attacks
  • Uncontrolled path injection (High): Added path validation in 3 folder creation endpoints to prevent directory traversal attacks
  • Reflected XSS (High): Sanitized requestPath with escapeHtml before injecting into HTML <base> tag
  • Clear-text logging of sensitive data (Medium): Removed API key names from access logs to prevent sensitive data exposure

Review & Testing Checklist for Human

⚠️ Security changes require thorough testing - 5 critical items to verify:

  • CSRF Protection: Test that legitimate form submissions and API requests still work (including file uploads, folder creation, API key management) while malicious cross-site requests are blocked
  • Folder Creation: Verify folder creation works in all paths (/folders, /*splat/folders, file upload with folder creation) and that path traversal attempts (using ../, absolute paths, etc.) are properly blocked
  • Static Content Serving: Test that static HTML files with <base> href injection still display correctly after XSS sanitization, especially with special characters in paths
  • Helmet Security Headers: Verify that CSP and HSTS headers are present and properly configured in browser dev tools (should use secure defaults, not be disabled)
  • i18n Functionality: Test language switching with various parameter types (string, array, malformed) to ensure type validation doesn't break legitimate locale detection

Test Plan

  1. Test CSRF: Try making POST requests without CSRF tokens (should fail) and with tokens (should succeed)
  2. Test path injection: Attempt folder creation with paths like ../../../etc, absolute paths, and verify they're rejected
  3. Test XSS: Navigate to paths with special characters like <script> and verify they're escaped in HTML
  4. Test i18n: Use ?lang=en, ?lang[]=en, and malformed parameters to verify proper handling

Notes

…et misconfiguration)

- Add type checking for req.query.lang to prevent type confusion attacks in i18n middleware
- Remove explicit disabling of Helmet's contentSecurityPolicy and hsts features
- Fixes CodeQL alerts #19, #20 (type confusion) and #23 (insecure Helmet config)
@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

- Add lusca CSRF middleware after session setup
- Validate constructed folder paths stay within SERVED_DIR in all 3 folder creation endpoints
- Fixes CodeQL alerts for missing CSRF and uncontrolled path injection
- Sanitize requestPath with escapeHtml before injecting into HTML base href
- Remove API key names from access logs to prevent clear-text logging of sensitive info
- Fixes CodeQL alerts for reflected XSS and clear-text logging
@MarkProminic
MarkProminic merged commit 00628a8 into main Oct 1, 2025
4 checks passed
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.

1 participant