Add denial of fetch headers

[richo-anyscale]

This causes the dashboard to be more thorough in it's attempts to deny browsers access to the job creation APIs

[gemini-code-assist]

Code Review

This pull request enhances security by adding a check for Sec-Fetch-* headers to better identify and block API requests originating from browsers. This is a solid improvement. My review includes suggestions to make the implementation more concise and robust, such as using more idiomatic Python, correcting the HTTP status code for denied requests, and parameterizing the new test to reduce code duplication and improve maintainability.

[gemini-code-assist]

The implementation of has_sec_fetch_headers can be made more concise and Pythonic by using the any() built-in function. This improves readability. Also, there's a typo ('existance') in the docstring, which should be 'existence', and the empty line can be removed for better formatting.

def has_sec_fetch_headers(req: Request) -> bool:
    """Checks for the existence of any of the sec-fetch-* headers."""
    return any(h in req.headers for h in ("Sec-Fetch-Mode", "Sec-Fetch-Dest", "Sec-Fetch-Site", "Sec-Fetch-User"))

[edoakes]

I was going to suggest the same thing -- well done, AI overlord!

[gemini-code-assist]

The new test test_deny_fetch_requests is very similar in structure to the existing test_browser_no_post_no_put test. To improve maintainability and avoid code duplication, you could parameterize this test to cover both scenarios (blocking by User-Agent and by Sec-Fetch-* headers). This would allow you to consolidate the logic and eventually remove the test_browser_no_post_no_put test.

@pytest.mark.parametrize(
    "headers_to_block",
    [
        {
            "User-Agent": (
                "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) "
                "AppleWebKit/537.36 (KHTML, like Gecko) "
                "Chrome/119.0.0.0 Safari/537.36"
            )
        },
        {"User-Agent": "Spurious User Agent", "Sec-Fetch-Site": "cross-site"},
    ],
)
def test_deny_browser_like_requests(
    headers_to_block, enable_test_module, ray_start_with_dashboard
):
    assert wait_until_server_available(ray_start_with_dashboard["webui_url"]) is True
    webui_url = ray_start_with_dashboard["webui_url"]
    webui_url = format_web_url(webui_url)

    timeout_seconds = 30
    start_time = time.time()
    while True:
        time.sleep(3)
        try:
            # Starting and getting jobs should be fine from API clients
            response = requests.post(
                webui_url + "/api/jobs/", json={"entrypoint": "ls"}
            )
            response.raise_for_status()
            response = requests.get(webui_url + "/api/jobs/")
            response.raise_for_status()

            # Starting job should be blocked for browsers
            response = requests.post(
                webui_url + "/api/jobs/",
                json={"entrypoint": "ls"},
                headers=headers_to_block,
            )
            with pytest.raises(HTTPError):
                response.raise_for_status()

            # Getting jobs should be fine for browsers
            response = requests.get(webui_url + "/api/jobs/")
            response.raise_for_status()
            break
        except (AssertionError, requests.exceptions.ConnectionError) as e:
            logger.info("Retry because of %s", e)
        finally:
            if time.time() > start_time + timeout_seconds:
                raise Exception("Timed out while testing.")

[edoakes]

I was going to suggest the same thing -- well done, AI overlord!

[edoakes]

what's the timeout & sleep needed for?

if we need to keep it, can refactor to use the wait_for_condition utility that is used in many other tests

[richo-anyscale]

I just cargo culted the other test, I can try to refactor them.

[richo-anyscale]

ok, so I did this refactor, but now I'm looking closer at it, I'm betting it's related to whatever we felt the retry logic was needed for (This is before I was around). I moved it out of the loop and we'll see if it becomes flaky.

[edoakes]

Looks like test is failing in CI: https://buildkite.com/ray-project/microcheck/builds/30837#019a7537-db13-4b75-97c6-0e20cc0c5969/632-2692

Please also sign off the commits to fix DCO build

[cursor]

Bug: Uncaught Timeout Error Prevents Proper Retries

The wait_for_condition call inside the retry loop raises RuntimeError on timeout, which isn't caught by the except clause that only handles AssertionError and ConnectionError. This causes the test to fail after 10 seconds even though the outer timeout is 30 seconds, preventing proper retry behavior when the dashboard is slow to start.

 

[richo-anyscale]

🙌