Skip to content

Commit 3a99ec8

Browse files
edoakesedoakes
authored
Cleanup dashboard test and header validation (#58648)
Getting rid of the excessive `while True` loops & timeouts in the tests (we already wait for the dashboard to be up). Also just cleaned up some comments and naming while I was poking around. --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
1 parent 3ff6ed3 commit 3a99ec8

File tree

3 files changed

+57
-88
lines changed

3 files changed

+57
-88
lines changed

‎python/ray/dashboard/http_server_head.py‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -305,12 +305,9 @@ async def browsers_no_post_put_middleware(request, handler):
305305
return await handler(request)
306306

307307
if (
308-
# A best effort test for browser traffic. All common browsers
309-
# start with Mozilla at the time of writing.
310-
(
311-
dashboard_optional_utils.is_browser_request(request)
312-
or dashboard_optional_utils.has_sec_fetch_headers(request)
313-
)
308+
# Deny mutating requests from browsers.
309+
# See `is_browser_request` for details of the check.
310+
dashboard_optional_utils.is_browser_request(request)
314311
and request.method in [hdrs.METH_POST, hdrs.METH_PUT]
315312
):
316313
return aiohttp.web.Response(

‎python/ray/dashboard/optional_utils.py‎

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -128,19 +128,17 @@ def _update_cache(task):
128128

129129

130130
def is_browser_request(req: Request) -> bool:
131-
"""Checks if a request is made by a browser like user agent.
132-
133-
This heuristic is very weak, but hard for a browser to bypass- eg,
134-
fetch/xhr and friends cannot alter the user-agent, but requests made with
135-
an http library can stumble into this if they choose to user a browser like
136-
user agent.
131+
"""Best-effort detection if the request was made by a browser.
132+
133+
Uses two heuristics:
134+
1) If the `User-Agent` header starts with 'Mozilla'. This heuristic is weak,
135+
but hard for a browser to bypass e.g., fetch/xhr and friends cannot alter the
136+
user agent, but requests made with an HTTP library can stumble into this if
137+
they choose to user a browser-like user agent. At the time of writing, all
138+
common browsers' user agents start with 'Mozilla'.
139+
2) If any of the `Sec-Fetch-*` headers are present.
137140
"""
138-
return req.headers["User-Agent"].startswith("Mozilla")
139-
140-
141-
def has_sec_fetch_headers(req: Request) -> bool:
142-
"""Checks for the existance of any of the sec-fetch-* headers"""
143-
return any(
141+
return req.headers.get("User-Agent", "").startswith("Mozilla") or any(
144142
h in req.headers
145143
for h in (
146144
"Sec-Fetch-Mode",
@@ -152,21 +150,17 @@ def has_sec_fetch_headers(req: Request) -> bool:
152150

153151

154152
def deny_browser_requests() -> Callable:
155-
"""Reject any requests that appear to be made by a browser"""
153+
"""Reject any requests that appear to be made by a browser."""
156154

157155
def decorator_factory(f: Callable) -> Callable:
158156
@functools.wraps(f)
159157
async def decorator(self, req: Request):
160-
if has_sec_fetch_headers(req):
161-
return Response(
162-
text="Browser requests not allowed",
163-
status=aiohttp.web.HTTPMethodNotAllowed.status_code,
164-
)
165158
if is_browser_request(req):
166159
return Response(
167160
text="Browser requests not allowed",
168161
status=aiohttp.web.HTTPMethodNotAllowed.status_code,
169162
)
163+
170164
return await f(self, req)
171165

172166
return decorator

‎python/ray/dashboard/tests/test_dashboard.py‎

Lines changed: 42 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -425,43 +425,32 @@ def dashboard_available():
425425
except Exception:
426426
return False
427427

428-
timeout_seconds = 30
429-
start_time = time.time()
430428
wait_for_condition(dashboard_available)
431-
while True:
432-
try:
433-
# Starting and getting jobs should be fine from API clients
434-
response = requests.post(
435-
webui_url + "/api/jobs/", json={"entrypoint": "ls"}
436-
)
437-
response.raise_for_status()
438-
response = requests.get(webui_url + "/api/jobs/")
439-
response.raise_for_status()
440429

441-
# Starting job should be blocked for browsers
442-
response = requests.post(
443-
webui_url + "/api/jobs/",
444-
json={"entrypoint": "ls"},
445-
headers={
446-
"User-Agent": (
447-
"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) "
448-
"AppleWebKit/537.36 (KHTML, like Gecko) "
449-
"Chrome/119.0.0.0 Safari/537.36"
450-
)
451-
},
430+
# Starting and getting jobs should be fine from API clients
431+
response = requests.post(webui_url + "/api/jobs/", json={"entrypoint": "ls"})
432+
response.raise_for_status()
433+
response = requests.get(webui_url + "/api/jobs/")
434+
response.raise_for_status()
435+
436+
# Starting job should be blocked for browsers
437+
response = requests.post(
438+
webui_url + "/api/jobs/",
439+
json={"entrypoint": "ls"},
440+
headers={
441+
"User-Agent": (
442+
"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) "
443+
"AppleWebKit/537.36 (KHTML, like Gecko) "
444+
"Chrome/119.0.0.0 Safari/537.36"
452445
)
453-
with pytest.raises(HTTPError):
454-
response.raise_for_status()
446+
},
447+
)
448+
with pytest.raises(HTTPError):
449+
response.raise_for_status()
455450

456-
# Getting jobs should be fine for browsers
457-
response = requests.get(webui_url + "/api/jobs/")
458-
response.raise_for_status()
459-
break
460-
except (AssertionError, requests.exceptions.ConnectionError) as e:
461-
logger.info("Retry because of %s", e)
462-
finally:
463-
if time.time() > start_time + timeout_seconds:
464-
raise Exception("Timed out while testing.")
451+
# Getting jobs should be fine for browsers
452+
response = requests.get(webui_url + "/api/jobs/")
453+
response.raise_for_status()
465454

466455

467456
@pytest.mark.skipif(
@@ -479,40 +468,29 @@ def dashboard_available():
479468
except Exception:
480469
return False
481470

482-
timeout_seconds = 30
483-
start_time = time.time()
484471
wait_for_condition(dashboard_available)
485-
while True:
486-
try:
487-
# Starting and getting jobs should be fine from API clients
488-
response = requests.post(
489-
webui_url + "/api/jobs/", json={"entrypoint": "ls"}
490-
)
491-
response.raise_for_status()
492-
response = requests.get(webui_url + "/api/jobs/")
493-
response.raise_for_status()
494472

495-
# Starting job should be blocked for browsers
496-
response = requests.post(
497-
webui_url + "/api/jobs/",
498-
json={"entrypoint": "ls"},
499-
headers={
500-
"User-Agent": ("Spurious User Agent"),
501-
"Sec-Fetch-Site": ("cross-site"),
502-
},
503-
)
504-
with pytest.raises(HTTPError):
505-
response.raise_for_status()
473+
# Starting and getting jobs should be fine from API clients
474+
response = requests.post(webui_url + "/api/jobs/", json={"entrypoint": "ls"})
475+
response.raise_for_status()
476+
response = requests.get(webui_url + "/api/jobs/")
477+
response.raise_for_status()
506478

507-
# Getting jobs should be fine for browsers
508-
response = requests.get(webui_url + "/api/jobs/")
509-
response.raise_for_status()
510-
break
511-
except (AssertionError, requests.exceptions.ConnectionError) as e:
512-
logger.info("Retry because of %s", e)
513-
finally:
514-
if time.time() > start_time + timeout_seconds:
515-
raise Exception("Timed out while testing.")
479+
# Starting job should be blocked for browsers
480+
response = requests.post(
481+
webui_url + "/api/jobs/",
482+
json={"entrypoint": "ls"},
483+
headers={
484+
"User-Agent": ("Spurious User Agent"),
485+
"Sec-Fetch-Site": ("cross-site"),
486+
},
487+
)
488+
with pytest.raises(HTTPError):
489+
response.raise_for_status()
490+
491+
# Getting jobs should be fine for browsers
492+
response = requests.get(webui_url + "/api/jobs/")
493+
response.raise_for_status()
516494

517495

518496
@pytest.mark.skipif(

0 commit comments

Comments
 (0)