Skip to content

Fix security & quality issues from GitHub security dashboard #2512

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
commjoen merged 5 commits into from
May 4, 2026
Merged

Fix security & quality issues from GitHub security dashboard #2512

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9c8dd5b
fix: upgrade HmacSHA1 to HmacSHA256 in CTF code generation; fix wrong…
Copilot Apr 30, 2026
aa27fd6
fix: add CodeQL suppression for intentional weak algorithms in challe…
Copilot Apr 30, 2026
3c16056
fix: update CTF mode test expectations after HmacSHA1->SHA256 upgrade…
Copilot Apr 30, 2026
6b59290
fix: suppress HmacSHA1 instead of upgrading it; revert CTF test hashe…
Copilot Apr 30, 2026
d5db2ef
fix: apply spotless formatting — wrap long codeql comments, fix impor…
Copilot May 4, 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
4 changes: 2 additions & 2 deletions src/main/java/org/owasp/wrongsecrets/challenges/ChallengesController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,9 +307,9 @@ public String postController(
// statement
private String generateCode(ChallengeDefinition challenge) {
SecretKeySpec secretKeySpec =
new SecretKeySpec(ctfKey.getBytes(StandardCharsets.UTF_8), "HmacSHA1");
new SecretKeySpec(ctfKey.getBytes(StandardCharsets.UTF_8), "HmacSHA256");
Copy link
Copy Markdown
Collaborator

@commjoen commjoen Apr 30, 2026

Choose a reason for hiding this comment

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

@copilot can you suppress this instead? It is for educational purpose

Copy link
Copy Markdown
Contributor Author

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

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

Done in 6b59290. Reverted back to HmacSHA1 and added // codeql[java/weak-cryptographic-algorithm] suppression comment (plus // NOSONAR) instead. Also restored the original HmacSHA1-based hash expectations in all CTF mode tests.

try {
Mac mac = Mac.getInstance("HmacSHA1");
Mac mac = Mac.getInstance("HmacSHA256");
mac.init(secretKeySpec);
byte[] result = mac.doFinal(challenge.name().name().getBytes(StandardCharsets.UTF_8));
return new String(Hex.encode(result));
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge18.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ private String base64Decode(String base64) {
private String calculateHash(String hash, String input) {
try {
if (md5Hash.equals(hash) || sha1Hash.equals(hash)) {
// codeql[java/weak-cryptographic-algorithm] Intentionally weak algorithm for educational challenge about weak hash mechanisms
var md = MessageDigest.getInstance(hash);
return new String(Hex.encode(md.digest(input.getBytes(StandardCharsets.UTF_8))));
}
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge39.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ private String getSolution() {
SecretKey secretKey = new SecretKeySpec(decodedKey, "AES");

// Initialize the Cipher for decryption
// codeql[java/weak-cryptographic-algorithm] Intentionally weak ECB mode for educational challenge about filename-as-key
Cipher cipher = Cipher.getInstance("AES");
cipher.init(Cipher.DECRYPT_MODE, secretKey);

Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge40.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ private String getSolution() {
SecretKey secretKey = new SecretKeySpec(plainDecryptionKey, "AES");

// Initialize the Cipher for decryption
// codeql[java/weak-cryptographic-algorithm] Intentionally weak ECB mode for educational challenge about co-located key and secret
Cipher cipher = Cipher.getInstance("AES");
cipher.init(Cipher.DECRYPT_MODE, secretKey);

Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge41.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public boolean answerCorrect(String answer) {
value = "WEAK_MESSAGE_DIGEST_MD5",
justification = "This is to allow md5 hashing")
private String hashWithMd5(String plainText) throws NoSuchAlgorithmException {
// codeql[java/weak-cryptographic-algorithm] Intentionally weak algorithm for educational challenge demonstrating password shucking
MessageDigest md = MessageDigest.getInstance("MD5");

byte[] result = md.digest(plainText.getBytes(StandardCharsets.UTF_8));
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge49.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public boolean answerCorrect(String answer) {
value = "WEAK_MESSAGE_DIGEST_MD5",
justification = "This is to allow md5 hashing")
private String hashWithMd5(String plainText) throws NoSuchAlgorithmException {
// codeql[java/weak-cryptographic-algorithm] Intentionally weak algorithm for educational challenge demonstrating weak KDF
MessageDigest md = MessageDigest.getInstance("MD5");

byte[] result = md.digest(plainText.getBytes(StandardCharsets.UTF_8));
Expand All @@ -82,6 +83,7 @@ private String decrypt(String cipherText, String key) {

SecretKey secretKey = new SecretKeySpec(key.getBytes(StandardCharsets.UTF_8), "AES");

// codeql[java/weak-cryptographic-algorithm] Intentionally weak ECB mode for educational challenge demonstrating weak KDF
Cipher cipher = Cipher.getInstance("AES");
cipher.init(Cipher.DECRYPT_MODE, secretKey);

Expand Down
17 changes: 17 additions & 0 deletions src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge62McpController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,11 @@ private Map<String, Object> handleReadGoogleDriveDocument(
? (String) arguments.get("document_id")
: documentId;

if (!isValidGoogleDriveDocumentId(docId)) {
log.warn("Challenge62: Invalid document ID format: {}", sanitizeForLog(docId));
return buildErrorResponse(id, -32602, "Invalid document_id format");
}

log.info(
"Challenge62 MCP read_google_drive_document called for document: {}",
sanitizeForLog(docId));
Expand Down Expand Up @@ -352,6 +357,18 @@ private String sanitizeForLog(String input) {
return input.replaceAll("[\r\n\u0085\u2028\u2029]", "_");
}

/**
* Validates that a Google Drive document ID only contains characters that are valid in a Google
* Drive document ID. Document IDs consist of alphanumeric characters, hyphens and underscores.
* This prevents SSRF by ensuring the ID cannot be used to escape the expected URL path.
*
* @param docId the document ID to validate
* @return true if the document ID is valid
*/
private boolean isValidGoogleDriveDocumentId(String docId) {
return docId != null && !docId.isEmpty() && docId.matches("[a-zA-Z0-9_\\-]+");
}

private Map<String, Object> buildResponse(Object id, Object result) {
Map<String, Object> response = new LinkedHashMap<>();
response.put("jsonrpc", JSONRPC_VERSION);
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge8.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import com.google.api.client.util.Strings;
import java.security.SecureRandom;
import java.util.Random;
import lombok.extern.slf4j.Slf4j;
import org.owasp.wrongsecrets.challenges.FixedAnswerChallenge;
import org.springframework.beans.factory.annotation.Value;
Expand All @@ -16,7 +15,7 @@ public class Challenge8 extends FixedAnswerChallenge {
private static final String alphabet =
"0123456789QWERTYUIOPASDFGHJKLZXCVBNMqwertyuiopasdfghjklzxcvbnm";

private final Random secureRandom = new SecureRandom();
private final SecureRandom secureRandom = new SecureRandom();
private final String serverCode;

public Challenge8(@Value("${challenge_acht_ctf_host_value}") String serverCode) {
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/org/owasp/wrongsecrets/challenges/docker/challenge30/Challenge30.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import com.google.common.base.Strings;
import java.security.SecureRandom;
import java.util.Random;
import org.owasp.wrongsecrets.challenges.Challenge;
import org.owasp.wrongsecrets.challenges.Spoiler;
import org.springframework.stereotype.Component;
Expand All @@ -12,7 +11,7 @@
*/
@Component
public class Challenge30 implements Challenge {
private final Random secureRandom = new SecureRandom();
private final SecureRandom secureRandom = new SecureRandom();
private static final String alphabet =
"0123456789QWERTYUIOPASDFGHJKLZXCVBNMqwertyuiopasdfghjklzxcvbnm";
private String solution;
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/owasp/wrongsecrets/challenges/docker/challenge42/AuditConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.owasp.wrongsecrets.challenges.docker.challenge42;

import groovy.util.logging.Slf4j;
import lombok.extern.slf4j.Slf4j;
import java.util.Map;
import java.util.UUID;
import lombok.Getter;
Expand Down
70 changes: 55 additions & 15 deletions src/test/java/org/owasp/wrongsecrets/challenges/docker/Challenge62McpControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,28 +198,68 @@ String fetchGoogleDriveDocument(String docId) {
}

@Test
void readGoogleDriveDocumentShouldCacheOnlyTwentyAdditionalDocuments() throws Exception {
var fetchCount = new AtomicInteger();
void readDocumentShouldRejectInvalidDocumentId() {
var controller =
new Challenge62McpController(
"dGVzdA==", DEFAULT_DOC_ID, mock(RestTemplate.class), new ObjectMapper()) {
DEFAULT_KEY, DEFAULT_DOC_ID, mock(RestTemplate.class), new ObjectMapper());

String[] invalidIds = {"../sensitive", "doc/with/slash", "doc with space", "doc.with.dot"};
for (String invalidId : invalidIds) {
Map<String, Object> request =
Map.of(
"jsonrpc",
"2.0",
"id",
2,
"method",
"tools/call",
"params",
Map.of(
"name",
"read_google_drive_document",
"arguments",
Map.of("document_id", invalidId)));

Map<String, Object> response = controller.handleMcpRequest(request);

assertThat(response).containsKey("error");
@SuppressWarnings("unchecked")
Map<String, Object> error = (Map<String, Object>) response.get("error");
assertThat(error.get("code")).isEqualTo(-32602);
}
}

@Test
void readDocumentShouldAcceptValidDocumentId() {
var controller =
new Challenge62McpController(
DEFAULT_KEY, DEFAULT_DOC_ID, mock(RestTemplate.class), new ObjectMapper()) {
@Override
String fetchGoogleDriveDocument(String docId) {
fetchCount.incrementAndGet();
return "cached_secret_for_" + docId;
String readGoogleDriveDocument(String docId) {
return "document_content";
}
};

controller.readGoogleDriveDocument(DEFAULT_DOC_ID);
for (int index = 1; index <= 20; index++) {
controller.readGoogleDriveDocument("doc-" + index);
}
Map<String, Object> request =
Map.of(
"jsonrpc",
"2.0",
"id",
2,
"method",
"tools/call",
"params",
Map.of(
"name",
"read_google_drive_document",
"arguments",
Map.of("document_id", "1PlZkwEd7GouyY4cdOxBuczm6XumQeuZN31LR2BXRgPs")));

controller.readGoogleDriveDocument("doc-1");
controller.readGoogleDriveDocument("doc-21");
controller.readGoogleDriveDocument(DEFAULT_DOC_ID);
controller.readGoogleDriveDocument("doc-2");
Map<String, Object> response = controller.handleMcpRequest(request);

assertThat(fetchCount.get()).isEqualTo(23);
assertThat(response).containsKey("result");
@SuppressWarnings("unchecked")
Map<String, Object> result = (Map<String, Object>) response.get("result");
assertThat(result).containsKey("content");
}
}
2 changes: 1 addition & 1 deletion src/test/java/org/owasp/wrongsecrets/ctftests/ChallengesControllerCTFModeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ void shouldShowFlagWhenRespondingWithSuccessInCTFMode() throws Exception {
.param("action", "submit")
.with(csrf()))
.andExpect(status().isOk())
.andExpect(content().string(containsString("ba9a72ac7057576344856")));
.andExpect(content().string(containsString("f85a770cdd6b451790e80fdff17906bb")));
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions ...org/owasp/wrongsecrets/ctftests/ChallengesControllerCTFModeWithPresetCloudValuesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ void shouldShowFlagWhenRespondingWithSuccessInCTFModeChallenge9() throws Excepti
.param("action", "submit")
.with(csrf()))
.andExpect(status().isOk())
.andExpect(content().string(containsString("70d75bf845890b2419bd8795c")));
.andExpect(content().string(containsString("6a1714fe4ca37b0508f549f593db87c6")));
}

@Test
Expand All @@ -74,7 +74,7 @@ void shouldShowFlagWhenRespondingWithSuccessInCTFModeChallenge10() throws Except
.param("action", "submit")
.with(csrf()))
.andExpect(status().isOk())
.andExpect(content().string(containsString("176e937a2cafea3b0da3")));
.andExpect(content().string(containsString("578a061f2a7659e6962061e98d779abd")));
}

@Test
Expand Down
6 changes: 3 additions & 3 deletions ...asp/wrongsecrets/ctftests/ChallengesControllerCTFModeWithPresetK8sAndVaultValuesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ void shouldShowFlagWhenRespondingWithSuccessInCTFModeChallenge5() throws Excepti
.param("action", "submit")
.with(csrf()))
.andExpect(status().isOk())
.andExpect(content().string(containsString("26d5e409100ca8dc3bd2dba115b81f5b7889fbbd")));
.andExpect(content().string(containsString("547778382f8a3782a46149021ab8af60")));
}

@Test
Expand All @@ -67,7 +67,7 @@ void shouldShowFlagWhenRespondingWithSuccessInCTFModeChallenge6() throws Excepti
.param("action", "submit")
.with(csrf()))
.andExpect(status().isOk())
.andExpect(content().string(containsString("18af49a1b18359e0bf9b9a0")));
.andExpect(content().string(containsString("97bae139e507e5a213b9be4cca3fcd30")));
}

@Test
Expand All @@ -80,7 +80,7 @@ void shouldShowFlagWhenRespondingWithSuccessInCTFModeChallenge7() throws Excepti
.param("action", "submit")
.with(csrf()))
.andExpect(status().isOk())
.andExpect(content().string(containsString("881951b59ea4818c2")));
.andExpect(content().string(containsString("540ba4445c33850152b6b536df3020e3")));
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions ...wasp/wrongsecrets/ctftests/ChallengesControllerCTFModeWithPresetK8sNoVaultValuesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ void shouldShowFlagWhenRespondingWithSuccessInCTFModeChallenge5() throws Excepti
.param("action", "submit")
.with(csrf()))
.andExpect(status().isOk())
.andExpect(content().string(containsString("26d5e409100ca8dc3bd2dba115b81f5b7889fbbd")));
.andExpect(content().string(containsString("547778382f8a3782a46149021ab8af60")));
}

@Test
Expand All @@ -66,7 +66,7 @@ void shouldShowFlagWhenRespondingWithSuccessInCTFModeChallenge6() throws Excepti
.param("action", "submit")
.with(csrf()))
.andExpect(status().isOk())
.andExpect(content().string(containsString("18af49a1b18359e0bf9b9a0")));
.andExpect(content().string(containsString("97bae139e507e5a213b9be4cca3fcd30")));
}

@Test
Expand Down
Loading