rusackas commented on code in PR #42930:
URL: https://github.com/apache/superset/pull/42930#discussion_r3780785682
##########
superset/utils/network.py:
##########
@@ -44,6 +44,19 @@
PING_TIMEOUT = 5
+def is_safe_ip(ip: ipaddress.IPv4Address | ipaddress.IPv6Address) -> bool:
+ """
+ Return True if a single IP address is public and globally routable.
+
+ IPv4-mapped IPv6 addresses (e.g. ``::ffff:127.0.0.1``) are unwrapped so
+ they are checked against the IPv4 unsafe networks rather than bypassing
+ them.
+ """
+ if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped:
+ ip = ip.ipv4_mapped
+ return ip.is_global and not any(ip in net for net in _SSRF_UNSAFE_NETWORKS)
Review Comment:
Added direct unit tests for `is_safe_ip` in `test_network.py` (public,
loopback, RFC-1918, link-local, CGNAT, multicast, and IPv4-mapped IPv6 cases)
rather than relying only on the indirect coverage through `is_safe_host`.
##########
tests/unit_tests/extensions/storage/test_api.py:
##########
@@ -78,6 +78,31 @@ def test_ephemeral_get_delegates_to_dao(
)
+@patch("superset.extensions.storage.api.ExtensionEphemeralDAO")
+@patch("superset.extensions.storage.utils.get_extensions")
+def test_ephemeral_get_response_is_marked_no_store(
+ mock_get_ext: MagicMock, mock_dao: MagicMock, app: Flask
+) -> None:
+ """Stored values are scoped to the requesting user, so responses built via
+ `response()` must never be cached (e.g. by a shared/CDN cache)."""
+ mock_get_ext.return_value = {"acme.dashboard": MagicMock()}
+ Babel(app)
+ app.appbuilder = MagicMock()
+ app.appbuilder.sm.is_item_public.return_value = True
+ mock_dao.get_raw.return_value = (get_codec("json").encode({"data": 42}),
"json")
+
+ with app.test_request_context(
+ "/api/v1/extensions/acme/dashboard/storage/ephemeral/my-key"
+ ):
+ g.user = MagicMock(id=7)
+
+ body, _status_code = ExtensionStorageRestApi().get_ephemeral(
+ "acme", "dashboard", "my-key"
+ )
+
+ assert body.cache_control.no_store is True
Review Comment:
Renamed the discarded `_status_code` and added an explicit `assert
status_code == 200`.
##########
tests/unit_tests/jinja_context_test.py:
##########
@@ -1732,6 +1760,13 @@ def
test_metric_macro_embedded_user_skips_base_filter(mocker: MockerFixture) ->
mock_is_guest_user =
mocker.patch("superset.security_manager.is_guest_user")
mock_is_guest_user.return_value = True
+ # Dashboard-level guest scope is asserted separately; here the dataset is
+ # in scope so the test can focus on the base-filter bypass.
+ mocker.patch(
+ "superset.jinja_context.guest_user_can_access_dataset",
+ return_value=True,
+ )
Review Comment:
That duplication pattern is pre-existing throughout this file (dozens of
call sites), not something this PR introduces — this PR only adds 35 lines
here. A shared conftest fixture is a file-wide refactor beyond this PR's scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
