Merge pull request #1546 from plotly/cb-request-validation

validate that callback request "outputs" field matches callback

Author: alexcjohnson

CHANGELOG.md

@@ -5,10 +5,10 @@ This project adheres to [Semantic Versioning](https://semver.org/).
 ## UNRELEASED
 
 ### Changed
+- [#1531](https://github.com/plotly/dash/pull/1531) Update the format of the docstrings to make them easier to read in the reference pages of Dash Docs and in the console.  This also addresses [#1205](https://github.com/plotly/dash/issues/1205)
 
-- [#1531](https://github.com/plotly/dash/pull/1531).  Updates the format of the docstrings to make them easier to read in
-  the reference pages of Dash Docs and in the console.  This also addresses [#1205](https://github.com/plotly/dash/issues/1205)
-
+### Fixed
+- [#1546](https://github.com/plotly/dash/pull/1546) Validate callback request `outputs` vs `output` to avoid a perceived security issue.
 
 ## [1.19.0] - 2021-01-19
 

dash/_validate.py

@@ -111,6 +111,26 @@ def validate_id_string(arg):
         )
 
 
+def validate_output_spec(output, output_spec, Output):
+    """
+    This validation is for security and internal debugging, not for users,
+    so the messages are not intended to be clear.
+    `output` comes from the callback definition, `output_spec` from the request.
+    """
+    if not isinstance(output, (list, tuple)):
+        output, output_spec = [output], [output_spec]
+    elif len(output) != len(output_spec):
+        raise exceptions.CallbackException("Wrong length output_spec")
+
+    for outi, speci in zip(output, output_spec):
+        speci_list = speci if isinstance(speci, (list, tuple)) else [speci]
+        for specij in speci_list:
+            if not Output(specij["id"], specij["property"]) == outi:
+                raise exceptions.CallbackException(
+                    "Output does not match callback definition"
+                )
+
+
 def validate_multi_return(outputs_list, output_value, callback_id):
     if not isinstance(output_value, (list, tuple)):
         raise exceptions.InvalidCallbackReturnValue(

dash/dash.py

@@ -27,7 +27,7 @@
 
 from .fingerprint import build_fingerprint, check_fingerprint
 from .resources import Scripts, Css
-from .dependencies import handle_callback_args
+from .dependencies import handle_callback_args, Output
 from .development.base_component import ComponentRegistry
 from .exceptions import PreventUpdate, InvalidResourceError, ProxyError
 from .version import __version__
@@ -1004,6 +1004,7 @@ def wrap_func(func):
             @wraps(func)
             def add_context(*args, **kwargs):
                 output_spec = kwargs.pop("outputs_list")
+                _validate.validate_output_spec(output, output_spec, Output)
 
                 # don't touch the comment on the next line - used by debugger
                 output_value = func(*args, **kwargs)  # %% callback invoked %%

tests/integration/callbacks/test_malformed_request.py

@@ -0,0 +1,59 @@
+import requests
+
+
+import dash_core_components as dcc
+import dash_html_components as html
+import dash
+from dash.dependencies import Input, Output
+
+
+def test_cbmf001_bad_output_outputs(dash_thread_server):
+    app = dash.Dash(__name__)
+    app.layout = html.Div(
+        [
+            dcc.Input(id="i", value="initial value"),
+            html.Div(html.Div([1.5, None, "string", html.Div(id="o1")])),
+        ]
+    )
+
+    @app.callback(Output("o1", "children"), [Input("i", "value")])
+    def update_output(value):
+        return value
+
+    dash_thread_server(app)
+
+    # first a good request
+    response = requests.post(
+        dash_thread_server.url + "/_dash-update-component",
+        json=dict(
+            output="o1.children",
+            outputs={"id": "o1", "property": "children"},
+            inputs=[{"id": "i", "property": "value", "value": 9}],
+            changedPropIds=["i.value"],
+        ),
+    )
+    assert response.status_code == 200
+    assert '"o1": {"children": 9}' in response.text
+
+    # now some bad ones
+    outspecs = [
+        {"output": "o1.nope", "outputs": {"id": "o1", "property": "nope"}},
+        {"output": "o1.children", "outputs": {"id": "o1", "property": "nope"}},
+        {"output": "o1.nope", "outputs": {"id": "o1", "property": "children"}},
+        {"output": "o1.children", "outputs": {"id": "nope", "property": "children"}},
+        {"output": "nope.children", "outputs": {"id": "nope", "property": "children"}},
+    ]
+    for outspeci in outspecs:
+        response = requests.post(
+            dash_thread_server.url + "/_dash-update-component",
+            json=dict(
+                inputs=[{"id": "i", "property": "value", "value": 9}],
+                changedPropIds=["i.value"],
+                **outspeci
+            ),
+        )
+        assert response.status_code == 500
+        assert "o1" not in response.text
+        assert "children" not in response.text
+        assert "nope" not in response.text
+        assert "500 Internal Server Error" in response.text

tests/integration/renderer/test_iframe.py

@@ -1,5 +1,3 @@
-from multiprocessing import Value
-
 import dash
 from dash.dependencies import Input, Output
 from dash.exceptions import PreventUpdate
@@ -9,7 +7,6 @@
 
 def test_rdif001_sandbox_allow_scripts(dash_duo):
     app = dash.Dash(__name__)
-    call_count = Value("i")
 
     N_OUTPUTS = 50
 
@@ -26,7 +23,6 @@ def update_output(n_clicks):
         if n_clicks is None:
             raise PreventUpdate
 
-        call_count.value += 1
         return ["{}={}".format(i, i + n_clicks) for i in range(N_OUTPUTS)]
 
     @app.server.after_request
@@ -39,6 +35,11 @@ def apply_cors(response):
 
     dash_duo.start_server(app)
 
+    dash_duo.find_element("#btn").click()
+    dash_duo.wait_for_element("#output-0").text == "0=1"
+
+    assert dash_duo.get_logs() == []
+
     iframe = """
         <!DOCTYPE html>
     <html>
@@ -53,8 +54,9 @@ def apply_cors(response):
 
     dash_duo.driver.switch_to.frame(0)
 
-    dash_duo.wait_for_element("#output-0")
-    dash_duo.wait_for_element_by_id("btn").click()
-    dash_duo.wait_for_element("#output-0").text == "0=1"
+    assert dash_duo.get_logs() == []
+
+    dash_duo.find_element("#btn").click()
+    dash_duo.wait_for_text_to_equal("#output-0", "0=1")
 
     assert dash_duo.get_logs() == []