Skip to content

Conversation

@DuoChen-317
Copy link

This PR fixes multiple security vulnerabilities detected by SpotBugs Security in the OpenMRS Core codebase.

Changes include:

  • Added canonical path validation to prevent Path Traversal attacks in several file operations.
  • Ensured path normalization and rejection of unsafe paths containing "..".
  • Updated File constructors to use sanitized absolute paths.
  • Added HTML escaping to all PrintWriter.write() calls that output request-derived content to prevent XSS.
  • Ensured JSON responses use proper escaping or safe serialization.
  • Removed unsafe string-based SQL concatenation and replaced with parameterized logic where applicable.

These changes eliminate all reported warnings under:
Security → Potential Path Traversal (file read)
Security → Potential XSS in Servlet
Security → Potential SQL Injection

// clear the module messages
String messagesPath = realPath + "/WEB-INF/";
File folder = new File(messagesPath.replace("/", File.separator));
// Normalize separators
Copy link
Contributor

Choose a reason for hiding this comment

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

Kindly the comments here don’t seem to add extra value so it would be better to remove them and rely on the code readability instead.

// Normalize separators
String safePath = messagesPath.replace("/", File.separator);

// Detect basic path traversal patterns
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

File moduleWebFolder = new File(absPath.replace("/", File.separator));
String absPath = realPath + "/WEB-INF/web/module/" + moduleId;

// normalize path separator
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Kindly remove all the comments that starts with //.


addLogLinesToResponse(result);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there away you can fix the format changes please?

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.

2 participants