Skip to content

add URLRedirect #4

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

Open
cccfeng wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add URLRedirect #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b59c342
add URLRedirect
cccfeng Mar 16, 2025
3de1ba5
Create CSRF.java
cccfeng May 7, 2025
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
29 changes: 29 additions & 0 deletions CSRF.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package org.joychou.controller;

import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.ResponseBody;

/**
* check csrf using spring-security
* Access http://localhost:8080/csrf/ -> click submit
*
* @author JoyChou (joychou@joychou.org) @2019-05-31
*/
@Controller
@RequestMapping("/csrf")
public class CSRF {

@GetMapping("/")
public String index() {
return "form";
}

@PostMapping("/post")
@ResponseBody
public String post() {
return "CSRF passed.";
}
}
93 changes: 93 additions & 0 deletions src/main/java/org/joychou/controller/URLRedirect2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package org.joychou.controller;

import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;

import javax.servlet.RequestDispatcher;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;

import org.joychou.security.SecurityUtil;

/**
* The vulnerability code and security code of Java url redirect.
* The security code is checking whitelist of url redirect.
*
* @author JoyChou (joychou@joychou.org)
* @version 2017.12.28
*/

@Controller
@RequestMapping("/urlRedirect")
public class URLRedirect {

/**
* http://localhost:8080/urlRedirect/redirect?url=http://www.baidu.com
*/
@GetMapping("/redirect")
public String redirect(@RequestParam("url") String url) {
return "redirect:" + url;

Check failure

Code scanning / SonarCloud

HTTP request redirections should not be open to forging attacks High

Change this code to not perform redirects based on user-controlled data. See more on SonarQube Cloud
}
Comment on lines +28 to +34
Copy link

@coderabbitai coderabbitai bot Mar 16, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

存在开放式重定向漏洞

该方法未对URL参数进行验证，直接拼接到重定向字符串中，容易被攻击者利用进行钓鱼攻击。

建议实现URL验证逻辑，类似于sendRedirect_seccode方法中的安全检查：

@GetMapping("/redirect")
public String redirect(@RequestParam("url") String url) {
+    if (SecurityUtil.checkURL(url) == null) {
+        return "redirect:/error";
+    }
    return "redirect:" + url;
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* http://localhost:8080/urlRedirect/redirect?url=http://www.baidu.com
*/
@GetMapping("/redirect")
public String redirect(@RequestParam("url") String url) {
return "redirect:" + url;
}
/**
* http://localhost:8080/urlRedirect/redirect?url=http://www.baidu.com
*/
@GetMapping("/redirect")
public String redirect(@RequestParam("url") String url) {
if (SecurityUtil.checkURL(url) == null) {
return "redirect:/error";
}
return "redirect:" + url;
}



/**
* http://localhost:8080/urlRedirect/setHeader?url=http://www.baidu.com
*/
@RequestMapping("/setHeader")
@ResponseBody
public static void setHeader(HttpServletRequest request, HttpServletResponse response) {
String url = request.getParameter("url");
response.setStatus(HttpServletResponse.SC_MOVED_PERMANENTLY); // 301 redirect
response.setHeader("Location", url);

Check failure

Code scanning / SonarCloud

HTTP request redirections should not be open to forging attacks High

Change this code to not perform redirects based on user-controlled data. See more on SonarQube Cloud
}
Comment on lines +40 to +46
Copy link

@coderabbitai coderabbitai bot Mar 16, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

静态方法和开放式重定向漏洞

  1. 控制器中不应使用静态方法，这可能会导致Spring依赖注入失效
  2. 该方法未对URL参数进行验证，存在开放式重定向漏洞

建议进行以下修改：

@RequestMapping("/setHeader")
@ResponseBody
- public static void setHeader(HttpServletRequest request, HttpServletResponse response) {
+ public void setHeader(HttpServletRequest request, HttpServletResponse response) {
    String url = request.getParameter("url");
+    if (SecurityUtil.checkURL(url) == null) {
+        response.setStatus(HttpServletResponse.SC_FORBIDDEN);
+        try {
+            response.getWriter().write("url forbidden");
+        } catch (IOException e) {
+            // 处理异常
+        }
+        return;
+    }
    response.setStatus(HttpServletResponse.SC_MOVED_PERMANENTLY); // 301 redirect
    response.setHeader("Location", url);
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@RequestMapping("/setHeader")
@ResponseBody
public static void setHeader(HttpServletRequest request, HttpServletResponse response) {
String url = request.getParameter("url");
response.setStatus(HttpServletResponse.SC_MOVED_PERMANENTLY); // 301 redirect
response.setHeader("Location", url);
}
@RequestMapping("/setHeader")
@ResponseBody
public void setHeader(HttpServletRequest request, HttpServletResponse response) {
String url = request.getParameter("url");
if (SecurityUtil.checkURL(url) == null) {
response.setStatus(HttpServletResponse.SC_FORBIDDEN);
try {
response.getWriter().write("url forbidden");
} catch (IOException e) {
// 处理异常
}
return;
}
response.setStatus(HttpServletResponse.SC_MOVED_PERMANENTLY); // 301 redirect
response.setHeader("Location", url);
}



/**
* http://localhost:8080/urlRedirect/sendRedirect?url=http://www.baidu.com
*/
@RequestMapping("/sendRedirect")
@ResponseBody
public static void sendRedirect(HttpServletRequest request, HttpServletResponse response) throws IOException {
String url = request.getParameter("url");
response.sendRedirect(url); // 302 redirect

Check failure

Code scanning / SonarCloud

HTTP request redirections should not be open to forging attacks High

Change this code to not perform redirects based on user-controlled data. See more on SonarQube Cloud
}
Comment on lines +52 to +57
Copy link

@coderabbitai coderabbitai bot Mar 16, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

静态方法和开放式重定向漏洞

  1. 控制器中不应使用静态方法，这可能会导致Spring依赖注入失效
  2. 该方法未对URL参数进行验证，存在开放式重定向漏洞

建议进行以下修改：

@RequestMapping("/sendRedirect")
@ResponseBody
- public static void sendRedirect(HttpServletRequest request, HttpServletResponse response) throws IOException {
+ public void sendRedirect(HttpServletRequest request, HttpServletResponse response) throws IOException {
    String url = request.getParameter("url");
+    if (SecurityUtil.checkURL(url) == null) {
+        response.setStatus(HttpServletResponse.SC_FORBIDDEN);
+        response.getWriter().write("url forbidden");
+        return;
+    }
    response.sendRedirect(url); // 302 redirect
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@RequestMapping("/sendRedirect")
@ResponseBody
public static void sendRedirect(HttpServletRequest request, HttpServletResponse response) throws IOException {
String url = request.getParameter("url");
response.sendRedirect(url); // 302 redirect
}
@RequestMapping("/sendRedirect")
@ResponseBody
public void sendRedirect(HttpServletRequest request, HttpServletResponse response) throws IOException {
String url = request.getParameter("url");
if (SecurityUtil.checkURL(url) == null) {
response.setStatus(HttpServletResponse.SC_FORBIDDEN);
response.getWriter().write("url forbidden");
return;
}
response.sendRedirect(url); // 302 redirect
}



/**
* Safe code. Because it can only jump according to the path, it cannot jump according to other urls.
* http://localhost:8080/urlRedirect/forward?url=/urlRedirect/test
*/
@RequestMapping("/forward")
@ResponseBody
public static void forward(HttpServletRequest request, HttpServletResponse response) {
String url = request.getParameter("url");
RequestDispatcher rd = request.getRequestDispatcher(url);
try {
rd.forward(request, response);
} catch (Exception e) {
e.printStackTrace();
}
}
Comment on lines +64 to +74
Copy link

@coderabbitai coderabbitai bot Mar 16, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

静态方法和异常处理问题

  1. 控制器中不应使用静态方法，这可能会导致Spring依赖注入失效
  2. 该方法的异常处理不当，仅打印堆栈跟踪而不返回适当的错误响应
  3. 虽然注释中提到这是安全的代码，但如果没有对url参数进行适当的验证，可能会受到路径遍历攻击

建议进行以下修改：

@RequestMapping("/forward")
@ResponseBody
- public static void forward(HttpServletRequest request, HttpServletResponse response) {
+ public void forward(HttpServletRequest request, HttpServletResponse response) {
    String url = request.getParameter("url");
+    // 验证URL是否为内部路径，防止路径遍历攻击
+    if (url == null || url.contains("..") || !url.startsWith("/")) {
+        try {
+            response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
+            response.getWriter().write("Invalid URL path");
+            return;
+        } catch (IOException e) {
+            // 记录日志
+        }
+        return;
+    }
    RequestDispatcher rd = request.getRequestDispatcher(url);
    try {
        rd.forward(request, response);
    } catch (Exception e) {
-        e.printStackTrace();
+        // 记录日志
+        try {
+            response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+            response.getWriter().write("Error forwarding request");
+        } catch (IOException ex) {
+            // 记录日志
+        }
    }
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@RequestMapping("/forward")
@ResponseBody
public static void forward(HttpServletRequest request, HttpServletResponse response) {
String url = request.getParameter("url");
RequestDispatcher rd = request.getRequestDispatcher(url);
try {
rd.forward(request, response);
} catch (Exception e) {
e.printStackTrace();
}
}
@RequestMapping("/forward")
@ResponseBody
public void forward(HttpServletRequest request, HttpServletResponse response) {
String url = request.getParameter("url");
// 验证URL是否为内部路径，防止路径遍历攻击
if (url == null || url.contains("..") || !url.startsWith("/")) {
try {
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
response.getWriter().write("Invalid URL path");
return;
} catch (IOException e) {
// 记录日志
}
return;
}
RequestDispatcher rd = request.getRequestDispatcher(url);
try {
rd.forward(request, response);
} catch (Exception e) {
// 记录日志
try {
response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
response.getWriter().write("Error forwarding request");
} catch (IOException ex) {
// 记录日志
}
}
}



/**
* Safe code of sendRedirect.
* http://localhost:8080/urlRedirect/sendRedirect/sec?url=http://www.baidu.com
*/
@RequestMapping("/sendRedirect/sec")
@ResponseBody
public void sendRedirect_seccode(HttpServletRequest request, HttpServletResponse response)
throws IOException {
String url = request.getParameter("url");
if (SecurityUtil.checkURL(url) == null) {
response.setStatus(HttpServletResponse.SC_FORBIDDEN);
response.getWriter().write("url forbidden");
return;
}
response.sendRedirect(url);

Check failure

Code scanning / SonarCloud

HTTP request redirections should not be open to forging attacks High

Change this code to not perform redirects based on user-controlled data. See more on SonarQube Cloud
}
}