Create CodeQL demo with reusable workflow supporting query pack suggestions and flexible build modes #2
Conversation
Co-authored-by: mickeygousset <20031479+mickeygousset@users.noreply.github.com>
Co-authored-by: mickeygousset <20031479+mickeygousset@users.noreply.github.com>
|
Can you modify the workflows to allow for the following functionality as well:
Can you help with this? And make sure to update the README appropriately of course, along with detailed documentation in all the files. |
…ocumentation Co-authored-by: mickeygousset <20031479+mickeygousset@users.noreply.github.com>
I've implemented the build mode functionality you requested! The reusable workflow now supports three build modes: Default:
The calling workflow demonstrates all three modes, and I've updated the README and examples with comprehensive documentation. The manual trigger workflow also includes a dropdown to select build modes. Commit: c31de54 |
Comment out enhanced and manual build analysis jobs in CodeQL workflow.
Comment out debugging step in CodeQL workflow
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public List<Map<String, Object>> getUser(@PathVariable String userId) { | ||
| // VULNERABLE: Direct string concatenation creates SQL injection risk | ||
| String sql = "SELECT * FROM users WHERE id = '" + userId + "'"; | ||
| return jdbcTemplate.queryForList(sql); |
Check failure
Code scanning / CodeQL
Query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To remediate the vulnerability, we should avoid direct concatenation of user input into the SQL query string. Instead, use parameterized/prepared queries to safely incorporate the user-supplied value. JdbcTemplate supports this via ? placeholders in the SQL, with the parameters supplied as arguments to queryForList. Change
String sql = "SELECT * FROM users WHERE id = '" + userId + "'";
return jdbcTemplate.queryForList(sql);to
String sql = "SELECT * FROM users WHERE id = ?";
return jdbcTemplate.queryForList(sql, userId);No imports or extra methods are needed. Only lines 40 and 41 need to be updated. This preserves existing functionality while removing the SQL Injection risk.
| @@ -37,8 +37,8 @@ | ||
| @GetMapping("/users/{userId}") | ||
| public List<Map<String, Object>> getUser(@PathVariable String userId) { | ||
| // VULNERABLE: Direct string concatenation creates SQL injection risk | ||
| String sql = "SELECT * FROM users WHERE id = '" + userId + "'"; | ||
| return jdbcTemplate.queryForList(sql); | ||
| String sql = "SELECT * FROM users WHERE id = ?"; | ||
| return jdbcTemplate.queryForList(sql, userId); | ||
| } | ||
|
|
||
| /** |
| public List<Map<String, Object>> searchUsers(@RequestParam String name) { | ||
| // VULNERABLE: User input directly embedded in SQL query | ||
| String sql = "SELECT * FROM users WHERE name LIKE '%" + name + "%'"; | ||
| return jdbcTemplate.queryForList(sql); |
Check failure
Code scanning / CodeQL
Query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
General fix strategy: Replace direct SQL query string concatenation with a parameterized (prepared) query using placeholders (?) and supply user input through query parameters. This safely escapes data and prevents malicious input from altering query structure.
Best fix:
In VulnerableController.java, within the searchUsers method, change the code to:
- (1) Write the query string with a positional parameter:
String sql = "SELECT * FROM users WHERE name LIKE ?". - (2) For the LIKE-pattern, construct the pattern as
"%"+name+"%"outside the query string. - (3) Call
jdbcTemplate.queryForList(sql, pattern)instead of passing only the SQL string.
No additional imports or methods are needed; all functionality required is already available via JdbcTemplate.
Only lines within the searchUsers method need changes.
| @@ -47,9 +47,10 @@ | ||
| */ | ||
| @GetMapping("/search") | ||
| public List<Map<String, Object>> searchUsers(@RequestParam String name) { | ||
| // VULNERABLE: User input directly embedded in SQL query | ||
| String sql = "SELECT * FROM users WHERE name LIKE '%" + name + "%'"; | ||
| return jdbcTemplate.queryForList(sql); | ||
| // FIXED: Use parameterized query to prevent SQL Injection | ||
| String sql = "SELECT * FROM users WHERE name LIKE ?"; | ||
| String pattern = "%" + name + "%"; | ||
| return jdbcTemplate.queryForList(sql, pattern); | ||
| } | ||
|
|
||
| /** |
|
|
||
| // VULNERABLE: User input directly written to HTML output without encoding | ||
| out.println("<html><body>"); | ||
| out.println("<h1>Welcome " + username + "!</h1>"); |
Check warning
Code scanning / CodeQL
Cross-site scripting Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the XSS vulnerability, user input should be properly HTML-encoded before it is included in the output. The most robust and simplest way is to escape special characters in the username (such as <, >, &, ", and ') so that any embedded HTML or scripts are displayed as plain text instead of being interpreted by the browser. In a Spring (Java) application, the Apache Commons Text library provides a convenient StringEscapeUtils.escapeHtml4 method for this purpose.
To implement this:
- Add an import for
org.apache.commons.text.StringEscapeUtilsat the top of the file. - On line 67, replace the direct use of
usernamewith the encoded form usingStringEscapeUtils.escapeHtml4(username).
| @@ -9,6 +9,7 @@ | ||
| import java.io.PrintWriter; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import org.apache.commons.text.StringEscapeUtils; | ||
|
|
||
| /** | ||
| * REST Controller with intentional security vulnerabilities for CodeQL demonstration. | ||
| @@ -64,7 +65,7 @@ | ||
|
|
||
| // VULNERABLE: User input directly written to HTML output without encoding | ||
| out.println("<html><body>"); | ||
| out.println("<h1>Welcome " + username + "!</h1>"); | ||
| out.println("<h1>Welcome " + StringEscapeUtils.escapeHtml4(username) + "!</h1>"); | ||
| out.println("</body></html>"); | ||
| } | ||
|
|
| @@ -48,7 +48,12 @@ | ||
| <version>2.7.0</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| </dependencies> | ||
| <dependency> | ||
| <groupId>org.apache.commons</groupId> | ||
| <artifactId>commons-text</artifactId> | ||
| <version>1.14.0</version> | ||
| </dependency> | ||
| </dependencies> | ||
|
|
||
| <build> | ||
| <plugins> |
| Package | Version | Security advisories |
| org.apache.commons:commons-text (maven) | 1.14.0 | None |
| try { | ||
| // VULNERABLE: User input used directly in file path | ||
| java.nio.file.Path path = java.nio.file.Paths.get("/app/data/" + filename); | ||
| return java.nio.file.Files.readString(path); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To prevent path traversal attacks, ensure that user input (filename) cannot be manipulated to escape the /app/data/ directory. The best practice in this scenario is to:
- Combine
/app/data/and the untrustedfilenameusing strong API calls (e.g.,Path.resolve()), - Normalize and convert to absolute paths,
- Verify that the resulting path is still within the intended
/app/data/directory (usingstartsWithor a similar check), - Only then read from the file.
Additionally, you might consider restricting filenames with a whitelist, or rejecting any filename containing path separators or ".." sequences. But the canonical solution is to check if the resolved path starts with the canonical base path.
Changes needed:
- Modify the
/files/{filename}handler to implement the above logic. - The only file to change is
src/main/java/com/example/demo/VulnerableController.java, specifically thereadFilemethod, lines 75–84. - You will need appropriate imports if they are not already present (they are: java.nio.file., java.io.).
- No changes to other code or business logic.
| @@ -75,9 +75,13 @@ | ||
| @GetMapping("/files/{filename}") | ||
| public String readFile(@PathVariable String filename) { | ||
| try { | ||
| // VULNERABLE: User input used directly in file path | ||
| java.nio.file.Path path = java.nio.file.Paths.get("/app/data/" + filename); | ||
| return java.nio.file.Files.readString(path); | ||
| // FIXED: Validate that the file stays within /app/data/ | ||
| java.nio.file.Path baseDir = java.nio.file.Paths.get("/app/data").toAbsolutePath().normalize(); | ||
| java.nio.file.Path requestedPath = baseDir.resolve(filename).normalize(); | ||
| if (!requestedPath.startsWith(baseDir)) { | ||
| return "Invalid filename."; | ||
| } | ||
| return java.nio.file.Files.readString(requestedPath); | ||
| } catch (Exception e) { | ||
| return "Error reading file: " + e.getMessage(); | ||
| } |
| java.nio.file.Path path = java.nio.file.Paths.get("/app/data/" + filename); | ||
| return java.nio.file.Files.readString(path); | ||
| } catch (Exception e) { | ||
| return "Error reading file: " + e.getMessage(); |
Check warning
Code scanning / CodeQL
Information exposure through an error message Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the problem, we should avoid exposing internal exception details to end users in the HTTP response. Instead, respond with a generic error message (e.g., "Error reading file") and, if desired, log the exception details on the server for later debugging. The changes should only affect the readFile method, specifically its exception handling.
- Change the return statement inside the catch block to return only a generic message.
- Optionally, log the exception message on the server side using
System.err.println,Logger, or similar. - Since no logger is present in the snippet and we cannot assume custom logging configuration, using
System.err.printlnis standard and safe for demonstration/education code.
| @@ -79,7 +79,9 @@ | ||
| java.nio.file.Path path = java.nio.file.Paths.get("/app/data/" + filename); | ||
| return java.nio.file.Files.readString(path); | ||
| } catch (Exception e) { | ||
| return "Error reading file: " + e.getMessage(); | ||
| // Log the detailed error for server-side diagnostics | ||
| System.err.println("Error reading file: " + e.getMessage()); | ||
| return "Error reading file"; // Generic message for user | ||
| } | ||
| } | ||
|
|
| public String pingHost(@RequestParam String host) { | ||
| try { | ||
| // VULNERABLE: User input directly used in system command | ||
| Process process = Runtime.getRuntime().exec("ping -c 1 " + host); |
Check failure
Code scanning / CodeQL
Uncontrolled command line Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix this command injection issue:
- Do not pass raw user input directly as part of the system command.
- The best resolution is to validate the user input against a strict whitelist of acceptable hostnames/IP addresses, or at minimum, ensure the input does not contain shell metacharacters.
- Alternatively, use the overload of
Runtime.execthat takes a command array, separating the command and its arguments, which helps mitigate, but does not eliminate, command injection risk. - For demonstration, we should validate the host parameter using a regex ensuring it is a valid hostname or IP address, and then pass it as an argument array to
exec. - The fixes are all within the
pingHostmethod: add appropriate input validation, and refactor the call toexecto split arguments. - No new imports are required, basic Java features suffice.
| @@ -90,8 +90,13 @@ | ||
| @GetMapping("/ping") | ||
| public String pingHost(@RequestParam String host) { | ||
| try { | ||
| // VULNERABLE: User input directly used in system command | ||
| Process process = Runtime.getRuntime().exec("ping -c 1 " + host); | ||
| // FIXED: Validate user input before using in system command | ||
| // Only allow valid hostnames or IPv4 addresses | ||
| if (!host.matches("^[a-zA-Z0-9\\-.]+$") && !host.matches("^(\\d{1,3}\\.){3}\\d{1,3}$")) { | ||
| return "Invalid host parameter."; | ||
| } | ||
| // Use exec with argument array to avoid shell interpretation | ||
| Process process = Runtime.getRuntime().exec(new String[] { "ping", "-c", "1", host }); | ||
| java.io.BufferedReader reader = new java.io.BufferedReader( | ||
| new java.io.InputStreamReader(process.getInputStream())); | ||
|
|
| } | ||
| return result.toString(); | ||
| } catch (Exception e) { | ||
| return "Error executing ping: " + e.getMessage(); |
Check warning
Code scanning / CodeQL
Information exposure through an error message Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the vulnerability, we must ensure that sensitive details in error messages are not exposed to end users. Instead of returning e.getMessage() in the HTTP response, we should return a generic error message; however, we should still retain the error information for developers by logging the exception on the server side. This may include logging at the error or warning level, depending on the application's configuration.
Implementation
- Edit the
/pingendpoint (pingHostmethod, lines 91-107 inVulnerableController.java). - In the catch block (lines 104-106), replace
return "Error executing ping: " + e.getMessage();with:- Log the exception using
System.err.println(since no logging framework is imported in the shown code). - Return a generic message, e.g.,
"Error executing ping".
- Log the exception using
- No additional imports or dependencies are required for
System.err.println.
| @@ -102,7 +102,11 @@ | ||
| } | ||
| return result.toString(); | ||
| } catch (Exception e) { | ||
| return "Error executing ping: " + e.getMessage(); | ||
| // Log the exception server-side for debugging purposes | ||
| System.err.println("Exception during ping:"); | ||
| e.printStackTrace(); | ||
| // Return a generic error message to the client | ||
| return "Error executing ping"; | ||
| } | ||
| } | ||
| } |
Comment out the queries input to prioritize custom pack usage.
This PR creates a comprehensive demonstration of GitHub Advanced Security CodeQL Code Scanning using reusable workflows that accept custom query pack suggestions and support flexible build modes. This addresses scenarios where organizations want to standardize CodeQL analysis across repositories while allowing customization of security queries and build configurations.
Demo Components
Java Application with Security Vulnerabilities
Created a Spring Boot web application (
src/main/java/com/example/demo/) that intentionally contains common security vulnerabilities for CodeQL to detect:The application includes REST endpoints that demonstrate each vulnerability type, along with database initialization and basic integration tests.
Reusable CodeQL Workflow with Build Mode Support
The reusable workflow (
.github/workflows/codeql-reusable.yml) provides flexible CodeQL analysis capabilities with configurable build modes:Key Features:
'none'(default): No build required - fastest analysis, recommended for Java/C#'autobuild': Let CodeQL automatically detect and build the project'manual': Use custom build commands for full controlCalling Workflow Examples
The main workflow (
.github/workflows/codeql-analysis.yml) demonstrates four different analysis approaches:Documentation and Examples
Query Pack Integration
The solution demonstrates how to pass various query pack combinations:
Build Mode Configuration
The workflow supports three build modes for optimal performance and flexibility:
Validation
CodeQL analysis successfully detects all 7 intentional security vulnerabilities in the demo application, confirming the effectiveness of the scanning configuration. The implementation follows GitHub Actions best practices with proper permissions, error handling, and artifact management.
Fixes #1
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.