From 1729af8bb08a10e5809db86e79db5096d2adf730 Mon Sep 17 00:00:00 2001 From: Elijah Date: Thu, 6 Jun 2024 12:47:16 +0000 Subject: [PATCH 1/8] Disallow special characters in upload ID --- reflex/components/core/upload.py | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/reflex/components/core/upload.py b/reflex/components/core/upload.py index 1bcfa3b546..d78ef19276 100644 --- a/reflex/components/core/upload.py +++ b/reflex/components/core/upload.py @@ -3,6 +3,7 @@ from __future__ import annotations import os +import re from pathlib import Path from typing import Any, Callable, ClassVar, Dict, List, Optional, Union @@ -37,8 +38,25 @@ ) +def validate_upload_id(id_: str, fn_name): + """Check that only alphanumeric characters and underscores are present in an upload id. + + Args: + id_: The upload id. + fn_name: Name of caller to show when validation fails. + + Raises: + ValueError: If the string is not valid. + """ + pattern = re.compile(r"^[a-zA-Z0-9_]+$") + if not pattern.match(id_): + raise ValueError( + f"Invalid upload ID: '{id_}'. The upload id for `rx.{fn_name}` should contain only alphanumeric characters and underscores." + ) + + @CallableVar -def upload_file(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: +def upload_file(id_: str = DEFAULT_UPLOAD_ID, fn_name: str = "upload_file") -> BaseVar: """Get the file upload drop trigger. This var is passed to the dropzone component to update the file list when a @@ -46,10 +64,12 @@ def upload_file(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: Args: id_: The id of the upload to get the drop trigger for. + fn_name: Name of the caller to use in validation error message when the id string is invalid. Returns: A var referencing the file upload drop trigger. """ + validate_upload_id(id_, fn_name) return BaseVar( _var_name=f"e => setFilesById(filesById => ({{...filesById, {id_}: e}}))", _var_type=EventChain, @@ -67,8 +87,9 @@ def selected_files(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: Returns: A var referencing the list of selected file paths. """ + validate_upload_id(id_, "selected_files") return BaseVar( - _var_name=f"(filesById.{id_} ? filesById.{id_}.map((f) => (f.path || f.name)) : [])", + _var_name=f"(filesById[`{id_}`] ? filesById[`{id_}`].map((f) => (f.path || f.name)) : [])", _var_type=List[str], _var_data=upload_files_context_var_data, ) @@ -84,6 +105,7 @@ def clear_selected_files(id_: str = DEFAULT_UPLOAD_ID) -> EventSpec: Returns: An event spec that clears the list of selected files when triggered. """ + validate_upload_id(id_, "clear_selected_files") # UploadFilesProvider assigns a special function to clear selected files # into the shared global refs object to make it accessible outside a React # component via `call_script` (otherwise backend could never clear files). @@ -99,6 +121,7 @@ def cancel_upload(upload_id: str) -> EventSpec: Returns: An event spec that cancels the upload when triggered. """ + validate_upload_id(upload_id, "cancel_upload") return call_script(f"upload_controllers[{upload_id!r}]?.abort()") @@ -249,7 +272,9 @@ def create(cls, *children, **props) -> Component: if upload_props.get("on_drop") is None: # If on_drop is not provided, save files to be uploaded later. - upload_props["on_drop"] = upload_file(upload_props["id"]) + upload_props["on_drop"] = upload_file( + upload_props["id"], fn_name="upload_files" + ) else: on_drop = upload_props["on_drop"] if isinstance(on_drop, Callable): From d825a732db1616d6d823334278f399a79624a160 Mon Sep 17 00:00:00 2001 From: Elijah Date: Thu, 6 Jun 2024 12:51:14 +0000 Subject: [PATCH 2/8] pyi fix --- reflex/components/core/upload.pyi | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/reflex/components/core/upload.pyi b/reflex/components/core/upload.pyi index b9b4ad8d0d..45473d6a3f 100644 --- a/reflex/components/core/upload.pyi +++ b/reflex/components/core/upload.pyi @@ -8,6 +8,7 @@ from reflex.vars import Var, BaseVar, ComputedVar from reflex.event import EventChain, EventHandler, EventSpec from reflex.style import Style import os +import re from pathlib import Path from typing import Any, Callable, ClassVar, Dict, List, Optional, Union from reflex import constants @@ -29,8 +30,11 @@ from reflex.vars import BaseVar, CallableVar, Var, VarData DEFAULT_UPLOAD_ID: str upload_files_context_var_data: VarData +def validate_upload_id(id_: str, fn_name): ... @CallableVar -def upload_file(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: ... +def upload_file( + id_: str = DEFAULT_UPLOAD_ID, fn_name: str = "upload_file" +) -> BaseVar: ... @CallableVar def selected_files(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: ... @CallableEventSpec From 9b95a03b008a6dd7050e24dcfaa2ee4228c7658a Mon Sep 17 00:00:00 2001 From: Elijah Date: Thu, 6 Jun 2024 16:47:22 +0000 Subject: [PATCH 3/8] use square brackets notation instead --- reflex/components/core/upload.py | 38 ++++++++----------------------- reflex/components/core/upload.pyi | 6 +---- 2 files changed, 11 insertions(+), 33 deletions(-) diff --git a/reflex/components/core/upload.py b/reflex/components/core/upload.py index d78ef19276..4f6dab9463 100644 --- a/reflex/components/core/upload.py +++ b/reflex/components/core/upload.py @@ -3,7 +3,6 @@ from __future__ import annotations import os -import re from pathlib import Path from typing import Any, Callable, ClassVar, Dict, List, Optional, Union @@ -38,25 +37,8 @@ ) -def validate_upload_id(id_: str, fn_name): - """Check that only alphanumeric characters and underscores are present in an upload id. - - Args: - id_: The upload id. - fn_name: Name of caller to show when validation fails. - - Raises: - ValueError: If the string is not valid. - """ - pattern = re.compile(r"^[a-zA-Z0-9_]+$") - if not pattern.match(id_): - raise ValueError( - f"Invalid upload ID: '{id_}'. The upload id for `rx.{fn_name}` should contain only alphanumeric characters and underscores." - ) - - @CallableVar -def upload_file(id_: str = DEFAULT_UPLOAD_ID, fn_name: str = "upload_file") -> BaseVar: +def upload_file(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: """Get the file upload drop trigger. This var is passed to the dropzone component to update the file list when a @@ -64,14 +46,19 @@ def upload_file(id_: str = DEFAULT_UPLOAD_ID, fn_name: str = "upload_file") -> B Args: id_: The id of the upload to get the drop trigger for. - fn_name: Name of the caller to use in validation error message when the id string is invalid. Returns: A var referencing the file upload drop trigger. """ - validate_upload_id(id_, fn_name) + var_name = f"""e => setFilesById(filesById => {{ + const updatedFilesById = Object.assign({{}}, filesById); + updatedFilesById[`{id_}`] = e; + return updatedFilesById; + }}) + """ + return BaseVar( - _var_name=f"e => setFilesById(filesById => ({{...filesById, {id_}: e}}))", + _var_name=var_name, _var_type=EventChain, _var_data=upload_files_context_var_data, ) @@ -87,7 +74,6 @@ def selected_files(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: Returns: A var referencing the list of selected file paths. """ - validate_upload_id(id_, "selected_files") return BaseVar( _var_name=f"(filesById[`{id_}`] ? filesById[`{id_}`].map((f) => (f.path || f.name)) : [])", _var_type=List[str], @@ -105,7 +91,6 @@ def clear_selected_files(id_: str = DEFAULT_UPLOAD_ID) -> EventSpec: Returns: An event spec that clears the list of selected files when triggered. """ - validate_upload_id(id_, "clear_selected_files") # UploadFilesProvider assigns a special function to clear selected files # into the shared global refs object to make it accessible outside a React # component via `call_script` (otherwise backend could never clear files). @@ -121,7 +106,6 @@ def cancel_upload(upload_id: str) -> EventSpec: Returns: An event spec that cancels the upload when triggered. """ - validate_upload_id(upload_id, "cancel_upload") return call_script(f"upload_controllers[{upload_id!r}]?.abort()") @@ -272,9 +256,7 @@ def create(cls, *children, **props) -> Component: if upload_props.get("on_drop") is None: # If on_drop is not provided, save files to be uploaded later. - upload_props["on_drop"] = upload_file( - upload_props["id"], fn_name="upload_files" - ) + upload_props["on_drop"] = upload_file(upload_props["id"]) else: on_drop = upload_props["on_drop"] if isinstance(on_drop, Callable): diff --git a/reflex/components/core/upload.pyi b/reflex/components/core/upload.pyi index 45473d6a3f..b9b4ad8d0d 100644 --- a/reflex/components/core/upload.pyi +++ b/reflex/components/core/upload.pyi @@ -8,7 +8,6 @@ from reflex.vars import Var, BaseVar, ComputedVar from reflex.event import EventChain, EventHandler, EventSpec from reflex.style import Style import os -import re from pathlib import Path from typing import Any, Callable, ClassVar, Dict, List, Optional, Union from reflex import constants @@ -30,11 +29,8 @@ from reflex.vars import BaseVar, CallableVar, Var, VarData DEFAULT_UPLOAD_ID: str upload_files_context_var_data: VarData -def validate_upload_id(id_: str, fn_name): ... @CallableVar -def upload_file( - id_: str = DEFAULT_UPLOAD_ID, fn_name: str = "upload_file" -) -> BaseVar: ... +def upload_file(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: ... @CallableVar def selected_files(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: ... @CallableEventSpec From 01e367e537df3f910dfd5b530e840969861ea4c8 Mon Sep 17 00:00:00 2001 From: Elijah Date: Thu, 6 Jun 2024 16:57:07 +0000 Subject: [PATCH 4/8] fix unit tests --- tests/components/forms/test_uploads.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/tests/components/forms/test_uploads.py b/tests/components/forms/test_uploads.py index 9050bc0ddf..7a020e26c6 100644 --- a/tests/components/forms/test_uploads.py +++ b/tests/components/forms/test_uploads.py @@ -72,7 +72,12 @@ def test_upload_root_component_render(upload_root_component): assert upload["props"] == [ "id={`default`}", "multiple={true}", - "onDrop={e => setFilesById(filesById => ({...filesById, default: e}))}", + "onDrop={e => setFilesById(filesById => {\n" + " const updatedFilesById = Object.assign({}, filesById);\n" + " updatedFilesById[`default`] = e;\n" + " return updatedFilesById;\n" + " })\n" + " }", "ref={ref_default}", ] assert upload["args"] == ("getRootProps", "getInputProps") @@ -114,7 +119,12 @@ def test_upload_component_render(upload_component): assert upload["props"] == [ "id={`default`}", "multiple={true}", - "onDrop={e => setFilesById(filesById => ({...filesById, default: e}))}", + "onDrop={e => setFilesById(filesById => {\n" + " const updatedFilesById = Object.assign({}, filesById);\n" + " updatedFilesById[`default`] = e;\n" + " return updatedFilesById;\n" + " })\n" + " }", "ref={ref_default}", ] assert upload["args"] == ("getRootProps", "getInputProps") @@ -156,6 +166,11 @@ def test_upload_component_with_props_render(upload_component_with_props): "maxFiles={2}", "multiple={true}", "noDrag={true}", - "onDrop={e => setFilesById(filesById => ({...filesById, default: e}))}", + "onDrop={e => setFilesById(filesById => {\n" + " const updatedFilesById = Object.assign({}, filesById);\n" + " updatedFilesById[`default`] = e;\n" + " return updatedFilesById;\n" + " })\n" + " }", "ref={ref_default}", ] From b040f8ee4b9b9b2f8662db877a6e792c5ac354b5 Mon Sep 17 00:00:00 2001 From: Elijah Date: Thu, 6 Jun 2024 17:25:24 +0000 Subject: [PATCH 5/8] add test for special chars --- tests/components/forms/test_uploads.py | 29 ++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/components/forms/test_uploads.py b/tests/components/forms/test_uploads.py index 7a020e26c6..8fca2786b9 100644 --- a/tests/components/forms/test_uploads.py +++ b/tests/components/forms/test_uploads.py @@ -39,6 +39,19 @@ def upload_component(): return upload_component() +@pytest.fixture +def upload_component_id_special(): + def upload_component(): + return rx.upload( + rx.button("select file"), + rx.text("Drag and drop files here or click to select files"), + border="1px dotted black", + id="#spec!al-&%@)(*^&+_98ID", + ) + + return upload_component() + + @pytest.fixture def upload_component_with_props(): """A test upload component with props function. @@ -174,3 +187,19 @@ def test_upload_component_with_props_render(upload_component_with_props): " }", "ref={ref_default}", ] + + +def test_upload_component_id_with_special_chars(upload_component_id_special): + upload = upload_component_id_special.render() + + assert upload["props"] == [ + "id={`#spec!al-&%@)(*^&+_98ID`}", + "multiple={true}", + "onDrop={e => setFilesById(filesById => {\n" + " const updatedFilesById = Object.assign({}, filesById);\n" + " updatedFilesById[`#spec!al-&%@)(*^&+_98ID`] = e;\n" + " return updatedFilesById;\n" + " })\n" + " }", + "ref={ref__spec_al__98ID}", + ] From 46df6da139d941becd2593712adf5ba5c42ef76c Mon Sep 17 00:00:00 2001 From: Elijah Date: Sun, 9 Jun 2024 14:23:29 +0000 Subject: [PATCH 6/8] use Var._var_name_unwrapped to escape backticks --- reflex/components/core/upload.py | 18 ++++++++++++------ reflex/event.py | 1 - tests/components/forms/test_uploads.py | 6 +++--- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/reflex/components/core/upload.py b/reflex/components/core/upload.py index 4f6dab9463..d1d18b56bf 100644 --- a/reflex/components/core/upload.py +++ b/reflex/components/core/upload.py @@ -50,9 +50,10 @@ def upload_file(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: Returns: A var referencing the file upload drop trigger. """ + id_var = Var.create_safe(id_, _var_is_string=True) var_name = f"""e => setFilesById(filesById => {{ const updatedFilesById = Object.assign({{}}, filesById); - updatedFilesById[`{id_}`] = e; + updatedFilesById[{id_var._var_name_unwrapped}] = e; return updatedFilesById; }}) """ @@ -60,7 +61,7 @@ def upload_file(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: return BaseVar( _var_name=var_name, _var_type=EventChain, - _var_data=upload_files_context_var_data, + _var_data=VarData.merge(upload_files_context_var_data, id_var._var_data), ) @@ -74,10 +75,11 @@ def selected_files(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: Returns: A var referencing the list of selected file paths. """ + id_var = Var.create_safe(id_, _var_is_string=True) return BaseVar( - _var_name=f"(filesById[`{id_}`] ? filesById[`{id_}`].map((f) => (f.path || f.name)) : [])", + _var_name=f"(filesById[{id_var._var_name_unwrapped}] ? filesById[{id_var._var_name_unwrapped}].map((f) => (f.path || f.name)) : [])", _var_type=List[str], - _var_data=upload_files_context_var_data, + _var_data=VarData.merge(upload_files_context_var_data, id_var._var_data), ) @@ -94,7 +96,9 @@ def clear_selected_files(id_: str = DEFAULT_UPLOAD_ID) -> EventSpec: # UploadFilesProvider assigns a special function to clear selected files # into the shared global refs object to make it accessible outside a React # component via `call_script` (otherwise backend could never clear files). - return call_script(f"refs['__clear_selected_files']({id_!r})") + return call_script( + f"refs['__clear_selected_files']({Var.create_safe(id_, _var_is_string=True)._var_name_unwrapped!r})" + ) def cancel_upload(upload_id: str) -> EventSpec: @@ -106,7 +110,9 @@ def cancel_upload(upload_id: str) -> EventSpec: Returns: An event spec that cancels the upload when triggered. """ - return call_script(f"upload_controllers[{upload_id!r}]?.abort()") + return call_script( + f"upload_controllers[{Var.create_safe(upload_id, _var_is_string=True)._var_name_unwrapped!r}]?.abort()" + ) def get_upload_dir() -> Path: diff --git a/reflex/event.py b/reflex/event.py index 5d2a32190f..e63745cb28 100644 --- a/reflex/event.py +++ b/reflex/event.py @@ -386,7 +386,6 @@ def as_event_spec(self, handler: EventHandler) -> EventSpec: ) upload_id = self.upload_id or DEFAULT_UPLOAD_ID - spec_args = [ ( Var.create_safe("files", _var_is_string=False), diff --git a/tests/components/forms/test_uploads.py b/tests/components/forms/test_uploads.py index 8fca2786b9..f8856a20f7 100644 --- a/tests/components/forms/test_uploads.py +++ b/tests/components/forms/test_uploads.py @@ -46,7 +46,7 @@ def upload_component(): rx.button("select file"), rx.text("Drag and drop files here or click to select files"), border="1px dotted black", - id="#spec!al-&%@)(*^&+_98ID", + id="#spec!`al-_98ID", ) return upload_component() @@ -193,11 +193,11 @@ def test_upload_component_id_with_special_chars(upload_component_id_special): upload = upload_component_id_special.render() assert upload["props"] == [ - "id={`#spec!al-&%@)(*^&+_98ID`}", + r"id={`#spec!\`al-_98ID`}", "multiple={true}", "onDrop={e => setFilesById(filesById => {\n" " const updatedFilesById = Object.assign({}, filesById);\n" - " updatedFilesById[`#spec!al-&%@)(*^&+_98ID`] = e;\n" + " updatedFilesById[`#spec!\\`al-_98ID`] = e;\n" " return updatedFilesById;\n" " })\n" " }", From e559683d658d38a09d29cd4703c07d3ad906d6dd Mon Sep 17 00:00:00 2001 From: Elijah Date: Sun, 9 Jun 2024 15:03:04 +0000 Subject: [PATCH 7/8] fix broken tests --- reflex/components/core/upload.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/reflex/components/core/upload.py b/reflex/components/core/upload.py index d1d18b56bf..d499772983 100644 --- a/reflex/components/core/upload.py +++ b/reflex/components/core/upload.py @@ -96,9 +96,7 @@ def clear_selected_files(id_: str = DEFAULT_UPLOAD_ID) -> EventSpec: # UploadFilesProvider assigns a special function to clear selected files # into the shared global refs object to make it accessible outside a React # component via `call_script` (otherwise backend could never clear files). - return call_script( - f"refs['__clear_selected_files']({Var.create_safe(id_, _var_is_string=True)._var_name_unwrapped!r})" - ) + return call_script(f"refs['__clear_selected_files']({id_!r})") def cancel_upload(upload_id: str) -> EventSpec: From fb477f9f7ef9a188196b6a1eaef3ec979d541d49 Mon Sep 17 00:00:00 2001 From: Elijah Date: Tue, 11 Jun 2024 10:10:41 +0000 Subject: [PATCH 8/8] rebase on main --- reflex/event.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/reflex/event.py b/reflex/event.py index e63745cb28..c9799a527d 100644 --- a/reflex/event.py +++ b/reflex/event.py @@ -390,7 +390,8 @@ def as_event_spec(self, handler: EventHandler) -> EventSpec: ( Var.create_safe("files", _var_is_string=False), Var.create_safe( - f"filesById.{upload_id}", _var_is_string=False + f"filesById[{Var.create_safe(upload_id, _var_is_string=True)._var_name_unwrapped}]", + _var_is_string=False, )._replace(_var_data=upload_files_context_var_data), ), (