Skip to content

[DRAFT PR] Critical code review for Inji Web Release 0.14.0#840

Merged
Prafulrakhade merged 0 commit intomasterfrom
release-0.19.x
Sep 17, 2025
Merged

[DRAFT PR] Critical code review for Inji Web Release 0.14.0#840
Prafulrakhade merged 0 commit intomasterfrom
release-0.19.x

Conversation

@swatigoel
Copy link
Contributor

@swatigoel swatigoel commented Aug 20, 2025

No description provided.

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 20, 2025
View reviewed changes
src/main/java/io/mosip/mimoto/service/CredentialPDFGeneratorService.java
VelocityContext velocityContext = new VelocityContext(data);

StringWriter writer = new StringWriter();
Velocity.evaluate(velocityContext, writer, "Credential Template", credentialTemplate);

Check failure

Code scanning / CodeQL

Server-side template injection

Template, which may contain code, depends on a [user-provided value](1).

Copilot Autofix

AI 7 months ago

To fix this vulnerability, ensure that template code provided to Velocity.evaluate is not attacker-controlled. The best-practice is to never accept templates from dynamic, user-facing APIs; use only trusted, static templates bundled with your application.

  • In Utilities.getCredentialSupportedTemplateString, restrict the loading of templates to only those available within a trusted directory in the classpath/package, or maintain a hardcoded whitelist of valid template filenames.
  • Never fetch template content over REST from a user-specified URL or based on user input. If dynamic templates are truly necessary, strictly enforce a whitelist.
  • In CredentialPDFGeneratorService.renderVCInCredentialTemplate, throw an exception or use a safe default if the template string comes from an unsafe source.

Steps to implement the fix:

  • Edit Utilities.getCredentialSupportedTemplateString to remove fetching templates from REST API, and restrict template selection to secure local resources only (e.g., via ClassPathResource or checked file path).
  • In local mode, retain the check against path traversal and only allow templates from the bounded, trusted directory.
  • In all other cases, always return a safe default template.
Suggested changeset 2
src/main/java/io/mosip/mimoto/service/CredentialPDFGeneratorService.java

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/main/java/io/mosip/mimoto/service/CredentialPDFGeneratorService.java b/src/main/java/io/mosip/mimoto/service/CredentialPDFGeneratorService.java
--- a/src/main/java/io/mosip/mimoto/service/CredentialPDFGeneratorService.java
+++ b/src/main/java/io/mosip/mimoto/service/CredentialPDFGeneratorService.java
@@ -202,6 +202,10 @@
 
     private ByteArrayInputStream renderVCInCredentialTemplate(Map<String, Object> data, String issuerId, String credentialConfigurationId) {
         String credentialTemplate = utilities.getCredentialSupportedTemplateString(issuerId, credentialConfigurationId);
+        // Fail-fast if the template is null or unexpectedly empty
+        if (credentialTemplate == null || credentialTemplate.trim().isEmpty()) {
+            throw new IllegalArgumentException("Credential template is missing or invalid.");
+        }
         Properties props = new Properties();
         props.setProperty("resource.loader", "class");
         props.setProperty("class.resource.loader.class", "org.apache.velocity.runtime.resource.loader.ClasspathResourceLoader");
EOF
@@ -202,6 +202,10 @@

private ByteArrayInputStream renderVCInCredentialTemplate(Map<String, Object> data, String issuerId, String credentialConfigurationId) {
String credentialTemplate = utilities.getCredentialSupportedTemplateString(issuerId, credentialConfigurationId);
// Fail-fast if the template is null or unexpectedly empty
if (credentialTemplate == null || credentialTemplate.trim().isEmpty()) {
throw new IllegalArgumentException("Credential template is missing or invalid.");
}
Properties props = new Properties();
props.setProperty("resource.loader", "class");
props.setProperty("class.resource.loader.class", "org.apache.velocity.runtime.resource.loader.ClasspathResourceLoader");
src/main/java/io/mosip/mimoto/util/Utilities.java
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/main/java/io/mosip/mimoto/util/Utilities.java b/src/main/java/io/mosip/mimoto/util/Utilities.java
--- a/src/main/java/io/mosip/mimoto/util/Utilities.java
+++ b/src/main/java/io/mosip/mimoto/util/Utilities.java
@@ -187,24 +187,21 @@
     }
     public String getCredentialSupportedTemplateString(String issuerId, String credentialConfigurationId) {
         String templateFileName = String.format("%s-%s-template.html", issuerId, credentialConfigurationId);
-        if(activeProfile.contains("local")) {
-            Path basePath = Paths.get("templates").toAbsolutePath().normalize();
-            Path resolvedPath = basePath.resolve(templateFileName).normalize();
+        Path basePath = Paths.get("templates").toAbsolutePath().normalize();
+        Path resolvedPath = basePath.resolve(templateFileName).normalize();
 
-            if (!resolvedPath.startsWith(basePath)) {
-                throw new SecurityException("Attempted path traversal attack: " + resolvedPath);
-            }
+        if (!resolvedPath.startsWith(basePath)) {
+            throw new SecurityException("Attempted path traversal attack: " + resolvedPath);
+        }
 
-            Resource credentialTemplateResource = new ClassPathResource(resolvedPath.toString());
-            try {
-                return Files.readString(credentialTemplateResource.getFile().toPath());
-            } catch (IOException e) {
-                log.error(ExceptionUtils.getStackTrace(e));
-            }
-            return credentialTemplateHtmlString;
+        Resource credentialTemplateResource = new ClassPathResource(resolvedPath.toString());
+        try {
+            return Files.readString(credentialTemplateResource.getFile().toPath());
+        } catch (IOException e) {
+            log.error(ExceptionUtils.getStackTrace(e));
         }
-        String specificCredentialPDFTemplate = getJson("", templateFileName);
-        return !StringUtils.isEmpty(specificCredentialPDFTemplate)? specificCredentialPDFTemplate : getJson("", credentialTemplatePath);
+        // Return safe default template if not found
+        return credentialTemplateHtmlString;
     }
 
     public static String[] handleExceptionWithErrorCode(Exception exception, String flowErrorCode) {
EOF
@@ -187,24 +187,21 @@
}
public String getCredentialSupportedTemplateString(String issuerId, String credentialConfigurationId) {
String templateFileName = String.format("%s-%s-template.html", issuerId, credentialConfigurationId);
if(activeProfile.contains("local")) {
Path basePath = Paths.get("templates").toAbsolutePath().normalize();
Path resolvedPath = basePath.resolve(templateFileName).normalize();
Path basePath = Paths.get("templates").toAbsolutePath().normalize();
Path resolvedPath = basePath.resolve(templateFileName).normalize();

if (!resolvedPath.startsWith(basePath)) {
throw new SecurityException("Attempted path traversal attack: " + resolvedPath);
}
if (!resolvedPath.startsWith(basePath)) {
throw new SecurityException("Attempted path traversal attack: " + resolvedPath);
}

Resource credentialTemplateResource = new ClassPathResource(resolvedPath.toString());
try {
return Files.readString(credentialTemplateResource.getFile().toPath());
} catch (IOException e) {
log.error(ExceptionUtils.getStackTrace(e));
}
return credentialTemplateHtmlString;
Resource credentialTemplateResource = new ClassPathResource(resolvedPath.toString());
try {
return Files.readString(credentialTemplateResource.getFile().toPath());
} catch (IOException e) {
log.error(ExceptionUtils.getStackTrace(e));
}
String specificCredentialPDFTemplate = getJson("", templateFileName);
return !StringUtils.isEmpty(specificCredentialPDFTemplate)? specificCredentialPDFTemplate : getJson("", credentialTemplatePath);
// Return safe default template if not found
return credentialTemplateHtmlString;
}

public static String[] handleExceptionWithErrorCode(Exception exception, String flowErrorCode) {
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
@swatigoel swatigoel marked this pull request as draft August 20, 2025 07:30
swatigoel
swatigoel commented Aug 20, 2025
View reviewed changes
swatigoel
swatigoel commented Aug 20, 2025
View reviewed changes
swatigoel
swatigoel commented Aug 20, 2025
View reviewed changes
api-test/pom.xml
<organizationUrl>https://github.com/mosip/mimoto</organizationUrl>
</developer>
</developers>

Copy link
Contributor Author

@swatigoel swatigoel Aug 20, 2025

Choose a reason for hiding this comment

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

version should be 0.19.0-SNAPSHOT

Copy link
Contributor Author

@swatigoel swatigoel Aug 21, 2025

Choose a reason for hiding this comment

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

@mohanachandran-s api test version should be 0.19.0-SNAPSHOT
cc: @Gurpreet41082

swatigoel
swatigoel commented Aug 20, 2025
View reviewed changes
swatigoel
swatigoel commented Aug 20, 2025
View reviewed changes
swatigoel
swatigoel commented Aug 20, 2025
View reviewed changes
swatigoel
swatigoel commented Aug 20, 2025
View reviewed changes
swatigoel
swatigoel commented Aug 20, 2025
View reviewed changes
swatigoel
swatigoel commented Aug 20, 2025
View reviewed changes
swatigoel
swatigoel commented Aug 20, 2025
View reviewed changes
@Prafulrakhade Prafulrakhade merged commit 090badb into master Sep 17, 2025
19 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Gurpreet41082 Gurpreet41082 Gurpreet41082 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@swatigoel @Gurpreet41082 @Prafulrakhade