Skip to content

Sandboxed URL creation to prevent SSRF attacks #6

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

Closed
wants to merge 1 commit into from
Closed

Sandboxed URL creation to prevent SSRF attacks #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
<sonar.projectKey>iluwatar_java-design-patterns</sonar.projectKey>
<sonar.moduleKey>${project.artifactId}</sonar.moduleKey>
<sonar.projectName>Java Design Patterns</sonar.projectName>
<versions.java-security-toolkit>1.1.3</versions.java-security-toolkit>
</properties>
<modules>
<module>abstract-factory</module>
Expand Down Expand Up @@ -248,6 +249,11 @@
<version>${system-lambda.version}</version>
<scope>test</scope>
</dependency>
Copy link
Author

Choose a reason for hiding this comment

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

This library holds security tools for protecting Java API calls.

License: MIT ✅ | Open source ✅ | More facts

<dependency>
<groupId>io.github.pixee</groupId>
<artifactId>java-security-toolkit</artifactId>
<version>${versions.java-security-toolkit}</version>
</dependency>
</dependencies>
</dependencyManagement>
<dependencies>
Copy link

Choose a reason for hiding this comment

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

Code Review:

  1. Bug Risks:

    • The addition of <versions.java-security-toolkit>1.1.3</versions.java-security-toolkit> in properties but referencing ${versions.java-security-toolkit} without defining it may lead to build failures or unexpected behavior.

  2. Improvement Suggestions:

    • Ensure that all placeholders, like ${versions.java-security-toolkit}, are defined and resolved correctly.
    • Consider adding comments where necessary to explain the purpose of these changes for better code maintainability.
    • Run a full build and test cycle to ensure this patch does not introduce any unintended side effects.
    • Check if the versions being used are compatible with other dependencies in the project.
    • Consider documenting why the java-security-toolkit version was updated to 1.1.3.

  3. Additional Remarks:

    • Review how this change fits into the overall architecture and design of the project.
    • Make sure your IDE or build system resolves and uses the correct values for variables like ${versions.java-security-toolkit}.
    • Verify if this updated version has any breaking changes compared to the previous version used in the project.

Summary:

The code patch introduces a new dependency on java-security-toolkit, updating its version to 1.1.3. Ensure that placeholder values are correctly resolved and that all dependencies are compatible with this newer version. Add comments for clarity and test thoroughly to prevent build issues.

Expand Down
4 changes: 4 additions & 0 deletions promise/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Author

Choose a reason for hiding this comment

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

This library holds security tools for protecting Java API calls.

License: MIT ✅ | Open source ✅ | More facts

<groupId>io.github.pixee</groupId>
<artifactId>java-security-toolkit</artifactId>
</dependency>
</dependencies>
<build>
<plugins>
Copy link

Choose a reason for hiding this comment

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

From the provided code patch, here is a brief code review:

  • Bug Risks:

    • Missing Version: The <dependency> block for io.github.pixee:java-security-toolkit lacks a version. This may lead to dependency resolution issues or potential mismatches.
    • Scope: It's essential to determine if io.github.pixee:java-security-toolkit should be included as a test dependency or a regular dependency based on its usage.

  • Improvement Suggestions:

    • Version Specification: Add a specific version to the io.github.pixee:java-security-toolkit dependency to ensure consistent and predictable builds.
    • Consistency: Ensure that dependencies are managed consistently throughout the project. If most dependencies include versions, follow the same convention here.

Consider the context of your project and determine the appropriate version and scope for the io.github.pixee:java-security-toolkit dependency to avoid runtime issues and maintain a consistent and manageable build configuration.

Expand Down
4 changes: 3 additions & 1 deletion promise/src/main/java/com/iluwatar/promise/Utility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
*/
package com.iluwatar.promise;

import io.github.pixee.security.HostValidator;
import io.github.pixee.security.Urls;
import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
Expand Down Expand Up @@ -98,7 +100,7 @@ public static Integer countLines(String fileLocation) {
*/
public static String downloadFile(String urlString) throws IOException {
LOGGER.info("Downloading contents from url: {}", urlString);
var url = new URL(urlString);
var url = Urls.create(urlString, Urls.HTTP_PROTOCOLS, HostValidator.DENY_COMMON_INFRASTRUCTURE_TARGETS);
var file = File.createTempFile("promise_pattern", null);
try (var bufferedReader = new BufferedReader(new InputStreamReader(url.openStream()));
var writer = new FileWriter(file)) {
Comment on lines 101 to 106
Copy link

Choose a reason for hiding this comment

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

The downloadFile method does not handle potential exceptions that could be thrown by Urls.create or File.createTempFile. If either of these methods throws an exception, it will not be caught, and the method will fail without providing a meaningful error message or performing any cleanup.

Recommended Solution:
Add appropriate exception handling for Urls.create and File.createTempFile to ensure that any exceptions are caught and handled gracefully, possibly logging the error and providing a meaningful message to the caller.

Copy link

Choose a reason for hiding this comment

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

Code Review Summary:

Bug Risks:

  1. Missing Exception Handling: Consider adding proper exception handling for potential errors that could arise during URL creation and file operations.

Improvements:

  1. Variable Naming and Clarity: Improve naming conventions for better code readability and maintenance.
  2. Security Concerns: Ensure the security implementation is thorough and covers all necessary aspects.
  3. Resource Management: Consider closing resources appropriately, especially when dealing with IO operations.
  4. Testing: Implement robust testing for this code patch to cover multiple scenarios.

Detailed Analysis:

  1. Exception Handling in downloadFile method:

    • Add proper error handling mechanisms for potential exceptions that might occur during URL creation and file operations.

  2. Variable Naming and Clarity:

    • Consider using more descriptive names for variables like urlString, url, file, bufferedReader, and writer to enhance code readability.

  3. Security Concerns:

    • Verify the adequacy of the security implementation, specifically pertaining to the HostValidator and URL creation process.

  4. Resource Management:

    • Ensure proper resource management by closing resources appropriately, possibly in a finally block or by utilizing try-with-resources where applicable.

  5. Testing:

    • Develop robust test cases to validate behavior under various conditions, ensuring the reliability and correctness of the methods.

By addressing these points, you can enhance the overall quality and maintainability of the codebase.

Expand Down