mirror of
https://github.com/comfyanonymous/ComfyUI.git
synced 2026-07-03 13:19:23 +08:00
security: address review feedback on GHSA-779p fixes
- Fix Windows CI failure in test_get_annotated_filepath: compare against os.path.abspath(...) to match the intentional abspath normalization added by the traversal hardening (abspath prepends the drive letter on Windows). - origin_check: narrow the bare `except:` in is_loopback() to ValueError so genuine interrupts aren't swallowed (review nit). - origin_check: guard .port access in is_cross_origin_forbidden() so a malformed/out-of-range port (e.g. Origin: http://127.0.0.1:99999) fails closed with a 403 instead of surfacing an uncaught 500 in the middleware. - server /view: escape backslash/quote in the Content-Disposition filename (RFC 6266 quoted-string) so a filename containing a double quote can't malform the response header.
This commit is contained in:
parent
ae4fcaaf41
commit
e4eb7f2698
@ -587,10 +587,15 @@ class PromptServer():
|
|||||||
# bare filename= hint does not force a download per
|
# bare filename= hint does not force a download per
|
||||||
# RFC 6266, so we only attach it on the dangerous branch
|
# RFC 6266, so we only attach it on the dangerous branch
|
||||||
# to avoid breaking inline display of legitimate images.
|
# to avoid breaking inline display of legitimate images.
|
||||||
disposition = f"filename=\"{filename}\""
|
# Escape backslash/quote per RFC 6266 quoted-string so a
|
||||||
|
# filename containing a double quote (which passes the
|
||||||
|
# ".."/leading-slash filter above) can't break out of the
|
||||||
|
# header's quoted-string and malform the disposition.
|
||||||
|
safe_filename = filename.replace("\\", "\\\\").replace('"', '\\"')
|
||||||
|
disposition = f"filename=\"{safe_filename}\""
|
||||||
if folder_paths.is_dangerous_content_type(content_type):
|
if folder_paths.is_dangerous_content_type(content_type):
|
||||||
content_type = 'application/octet-stream'
|
content_type = 'application/octet-stream'
|
||||||
disposition = f"attachment; filename=\"{filename}\""
|
disposition = f"attachment; filename=\"{safe_filename}\""
|
||||||
|
|
||||||
return web.FileResponse(
|
return web.FileResponse(
|
||||||
file,
|
file,
|
||||||
|
|||||||
@ -53,8 +53,11 @@ def test_annotated_filepath():
|
|||||||
|
|
||||||
def test_get_annotated_filepath():
|
def test_get_annotated_filepath():
|
||||||
default_dir = "/default/dir"
|
default_dir = "/default/dir"
|
||||||
assert folder_paths.get_annotated_filepath("test.txt", default_dir) == os.path.join(default_dir, "test.txt")
|
# get_annotated_filepath now normalizes with os.path.abspath (part of the
|
||||||
assert folder_paths.get_annotated_filepath("test.txt [output]") == os.path.join(folder_paths.get_output_directory(), "test.txt")
|
# GHSA-779p traversal hardening), so compare against the normalized form —
|
||||||
|
# on Windows abspath also prepends the current drive letter.
|
||||||
|
assert folder_paths.get_annotated_filepath("test.txt", default_dir) == os.path.abspath(os.path.join(default_dir, "test.txt"))
|
||||||
|
assert folder_paths.get_annotated_filepath("test.txt [output]") == os.path.abspath(os.path.join(folder_paths.get_output_directory(), "test.txt"))
|
||||||
|
|
||||||
def test_add_model_folder_path_append(clear_folder_paths):
|
def test_add_model_folder_path_append(clear_folder_paths):
|
||||||
folder_paths.add_model_folder_path("test_folder", "/default/path", is_default=True)
|
folder_paths.add_model_folder_path("test_folder", "/default/path", is_default=True)
|
||||||
|
|||||||
@ -25,7 +25,10 @@ def is_loopback(host):
|
|||||||
return True
|
return True
|
||||||
else:
|
else:
|
||||||
return False
|
return False
|
||||||
except:
|
except ValueError:
|
||||||
|
# Not an IP literal (ip_address raises ValueError); fall through to DNS
|
||||||
|
# resolution below. Narrowed from a bare except so genuine interrupts
|
||||||
|
# (KeyboardInterrupt/SystemExit) aren't swallowed.
|
||||||
pass
|
pass
|
||||||
|
|
||||||
loopback = False
|
loopback = False
|
||||||
@ -64,11 +67,21 @@ def is_cross_origin_forbidden(host, origin):
|
|||||||
origin_domain = parsed.netloc.lower()
|
origin_domain = parsed.netloc.lower()
|
||||||
host_domain_parsed = urllib.parse.urlsplit('//' + host_domain)
|
host_domain_parsed = urllib.parse.urlsplit('//' + host_domain)
|
||||||
|
|
||||||
|
# A non-numeric or out-of-range port (e.g. Origin: http://127.0.0.1:99999)
|
||||||
|
# makes urllib raise ValueError on .port access. Treat a malformed port as a
|
||||||
|
# rejected request rather than letting it surface as an uncaught 500 in the
|
||||||
|
# middleware — it fails closed, consistent with the CSRF stance.
|
||||||
|
try:
|
||||||
|
origin_port = parsed.port
|
||||||
|
host_port = host_domain_parsed.port
|
||||||
|
except ValueError:
|
||||||
|
return True
|
||||||
|
|
||||||
loopback = is_loopback(host_domain_parsed.hostname)
|
loopback = is_loopback(host_domain_parsed.hostname)
|
||||||
|
|
||||||
if parsed.port is None: # if origin doesn't have a port strip it from the host to handle weird browsers, same for host
|
if origin_port is None: # if origin doesn't have a port strip it from the host to handle weird browsers, same for host
|
||||||
host_domain = host_domain_parsed.hostname
|
host_domain = host_domain_parsed.hostname
|
||||||
if host_domain_parsed.port is None:
|
if host_port is None:
|
||||||
origin_domain = parsed.hostname
|
origin_domain = parsed.hostname
|
||||||
|
|
||||||
if loopback and host_domain is not None and len(host_domain) > 0:
|
if loopback and host_domain is not None and len(host_domain) > 0:
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user