Skip to content

[grid] Add config blocked-routes and specific blocked-delete-session in Router #15920

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.

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
merged 1 commit into from
Jun 20, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jun 19, 2025

User description

🔗 Related Issues

💥 What does this PR do?

1. Add a config to support #15808 (a flag to block session deletion - both on UI and API via route filter).

  • This is a synchronized way to prevent deleting any session proactively via UI or API endpoint.
  • Usage: Set CLI config --blocked-delete-session true when starting Router/Hub/Standalone component. (or similar config name in TOML)

2. Extend the feature for advanced users are able to define a list of routes to block.

  • Grid support list of endpoints (https://www.selenium.dev/documentation/grid/advanced_features/endpoints/), which include delete session, delete node, or drain a node.
  • For example, an advanced user who wants to block a few methods to route paths, they can define to this config.
  • Usage: Route to block in format 'METHOD:path'. Can be specified multiple times.
  • Example:

CLI options

--blocked-routes 'DELETE:/session/{session-id} --blocked-routes 'DELETE:/se/grid/distributor/node/{node-id}'

TOML configs

[router]
blocked-routes = ["DELETE:/session/{session-id}", "POST:/session"]

In logs, we can monitor the config apply

23:52:16.945 INFO [Hub.createHandlers] - Blocking 1 route(s): [DELETE:/session/{session-id}]
23:52:17.012 INFO [Hub.execute] - Started Selenium Hub 4.34.0-SNAPSHOT (revision Unknown): [http://172.18.0.2:4444⁠](http://172.18.0.2:4444/)

User who performs method to route path matched will see the response with status 403

curl -X DELETE http://localhost:4444/session/9da451a700131aac631c4547382661f9
Route blocked by configuration: DELETE /session/9da451a700131aac631c4547382661f9

From Router logs, we can also monitor how many blocked methods get hit

23:54:35.897 WARN [BlockedRoutesFilter.execute] - Blocked request: DELETE /session/9da451a700131aac631c4547382661f9 (matches blocked route: DELETE:/session/{session-id})

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement


Description

• Add configurable route blocking for Grid components
• Implement specific flag to block session deletion
• Support comma-separated list of blocked routes
• Add comprehensive test coverage for new functionality


Changes walkthrough 📝

Relevant files
Enhancement
5 files
Hub.java
Apply blocked routes filter to Hub                                             
+10/-0   
Standalone.java
Apply blocked routes filter to Standalone                               
+10/-0   
BlockedRoute.java
Create BlockedRoute model class                                                   
+101/-0 
BlockedRoutesFilter.java
Implement HTTP request filtering logic                                     
+86/-0   
RouterServer.java
Apply blocked routes filter to RouterServer                           
+8/-0     
Configuration changes
3 files
RouterFlags.java
Add CLI flags for route blocking                                                 
+21/-0   
RouterOptions.java
Parse blocked routes configuration                                             
+50/-0   
BUILD.bazel
Add test build configuration                                                         
+18/-0   
Tests
2 files
BlockedRoutesFilterTest.java
Test blocked routes filter functionality                                 
+213/-0 
RouterOptionsTest.java
Test router options configuration parsing                               
+272/-0 

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added B-grid Everything grid and server related C-java Java Bindings B-build Includes scripting, bazel and CI integrations labels Jun 19, 2025
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 19, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @SeleniumHQ SeleniumHQ deleted a comment from qodo-merge-pro bot Jun 20, 2025
    @SeleniumHQ SeleniumHQ deleted a comment from qodo-merge-pro bot Jun 20, 2025
    @VietND96
    Copy link
    Member Author

    /review

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 20, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 24e5e23)

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix ConnectFailure error when instantiating ChromeDriver
    • Address connection refused errors for subsequent ChromeDriver instances
    • Ensure first instance works without console errors

    1234 - Not compliant

    Non-compliant requirements:

    • Fix JavaScript execution in link's href on click() for Firefox
    • Ensure test case alerts work with version 2.48+ as they did with 2.47.1
    • Address regression in Firefox 42.0 compatibility
    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Path traversal prevention:
    The BlockedRoute class implements path normalization to prevent path traversal attacks using techniques like '../' sequences and URL encoding. However, the complexity of this logic (lines 144-192) requires careful review to ensure all attack vectors are covered. The URL decoding and path resolution could potentially be bypassed with sophisticated encoding techniques or edge cases not covered by the current implementation.

    ⚡ Recommended focus areas for review

    Security Concern

    The path normalization logic in normalizePath method is complex and handles security-sensitive operations like path traversal prevention. The URL decoding and path resolution logic should be thoroughly reviewed for edge cases and potential bypasses.

    private String normalizePath(String path) {
      if (path == null || path.isEmpty()) {
        return "/";
      }
    
      try {
        // URL decode the path to handle percent-encoded characters like %2F
        String decodedPath = URLDecoder.decode(path, StandardCharsets.UTF_8);
    
        // Normalize multiple consecutive slashes to single slashes
        String normalizedPath = decodedPath.replaceAll("/+", "/");
    
        // Split into segments and resolve path traversal
        String[] segments = normalizedPath.split("/");
        List<String> resolvedSegments = new ArrayList<>();
    
        for (String segment : segments) {
          if (segment.isEmpty() || ".".equals(segment)) {
            // Skip empty segments and current directory references
            continue;
          } else if ("..".equals(segment)) {
            // Go up one directory level
            if (!resolvedSegments.isEmpty()) {
              resolvedSegments.remove(resolvedSegments.size() - 1);
            } else {
              // Attempting to go above root - this is a security violation
              throw new IllegalArgumentException("Path traversal attack detected: " + path);
            }
          } else {
            // Add normal segment
            resolvedSegments.add(segment);
          }
        }
    
        // Reconstruct the path
        StringBuilder result = new StringBuilder();
        for (String segment : resolvedSegments) {
          result.append("/").append(segment);
        }
    
        // Ensure the result starts with / and handle empty path case
        String finalPath = result.toString();
        return finalPath.isEmpty() ? "/" : finalPath;
    
      } catch (Exception e) {
        // If URL decoding fails or any other error occurs, throw security exception
        throw new IllegalArgumentException("Invalid path format: " + path, e);
      }
    }
    Performance Risk

    The matchesPathPattern method performs string splitting and iteration for every request match check. This could impact performance under high load, especially with many blocked routes configured.

    private boolean matchesPathPattern(String pattern, String requestPath) {
      // Normalize both paths to prevent path traversal attacks
      String normalizedPattern = normalizePath(pattern);
      String normalizedRequestPath = normalizePath(requestPath);
    
      // Split both paths into segments
      String[] patternSegments = normalizedPattern.split("/", -1); // keep trailing empty segments
      String[] requestSegments = normalizedRequestPath.split("/", -1);
    
      // Paths must have the same number of segments
      if (patternSegments.length != requestSegments.length) {
        return false;
      }
    
      // Compare each segment
      for (int i = 0; i < patternSegments.length; i++) {
        String patternSegment = patternSegments[i];
        String requestSegment = requestSegments[i];
    
        // If both are empty (leading/trailing slash), continue
        if (patternSegment.isEmpty() && requestSegment.isEmpty()) {
          continue;
        }
        // If pattern segment is a path parameter (enclosed in {}), it matches any non-empty segment
        if (isPathParameter(patternSegment)) {
          if (requestSegment.isEmpty()) {
            return false;
          }
        } else {
          // For literal segments, they must match exactly
          if (!patternSegment.equals(requestSegment)) {
            return false;
          }
        }
      }
    
      return true;
    }
    Logic Issue

    The duplicate prevention logic for DELETE session route uses stream operations that create new lists unnecessarily. The logic could be simplified and made more efficient.

    if (blockedDeleteSession) {
      BlockedRoute deleteSessionRoute = new BlockedRoute("DELETE", "/session/{session-id}");
      // Only add if not already present
      if (routes.stream()
          .noneMatch(
              route ->
                  "DELETE".equals(route.getMethod())
                      && "/session/{session-id}".equals(route.getPath()))) {
        routes =
            Stream.concat(routes.stream(), Stream.of(deleteSessionRoute))
                .collect(Collectors.toList());
      }
    }

    Copy link
    Contributor

    @pujagani pujagani left a comment

    Choose a reason for hiding this comment

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

    Looks good to me

    …on` in Router
    
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @VietND96
    Copy link
    Member Author

    @pujagani thanks Puja for approval. In the last commit, I added a minor fix for AI review point "Path traversal vulnerability".
    /review

    @VietND96
    Copy link
    Member Author

    /review

    Copy link
    Contributor

    Persistent review updated to latest commit 24e5e23

    @SeleniumHQ SeleniumHQ deleted a comment from qodo-merge-pro bot Jun 20, 2025
    @VietND96 VietND96 merged commit 406427b into trunk Jun 20, 2025
    31 checks passed
    @VietND96 VietND96 deleted the blocked-routes branch June 20, 2025 05:19
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-build Includes scripting, bazel and CI integrations B-grid Everything grid and server related C-java Java Bindings Possible security concern Review effort 4/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants