bito-code-review[bot] commented on code in PR #42930:
URL: https://github.com/apache/superset/pull/42930#discussion_r3780786536
##########
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:
<!-- Bito Reply -->
The suggestion to add unit tests for `is_safe_ip` is appropriate. Since this
function is now a public utility used for security-critical SSRF protection,
explicit test coverage is necessary to ensure its logic correctly handles
various IP address types and edge cases, rather than relying solely on indirect
coverage.
##########
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:
<!-- Bito Reply -->
The update correctly addresses the reviewer's suggestion by capturing the
status code and adding an explicit assertion. This change ensures the test
verifies the HTTP status code, aligning with the project's testing standards.
**tests/unit_tests/extensions/storage/test_api.py**
```
body, status_code = ExtensionStorageRestApi().get_ephemeral(
"acme", "dashboard", "my-key"
)
assert status_code == 200
assert body.cache_control.no_store is True
```
##########
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:
<!-- Bito Reply -->
The observation that this duplication is pre-existing and widespread is
noted. Given that this PR is limited in scope and the suggested refactor would
be a file-wide change, it is reasonable to defer this improvement to a separate
task. You may choose to ignore this suggestion for now.
--
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]
