bito-code-review[bot] commented on code in PR #42930:
URL: https://github.com/apache/superset/pull/42930#discussion_r3780493648
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate DatasetDAO mock setup</b></div>
<div id="fix">
DatasetDAO mock setup duplication found in jinja_context_test.py at lines
1083-1095, 1717-1732, 1768-1780, and 1770-1785. The mock configuration for
SqlMetric, Database, and SqlaTable is repeated. Recommend creating a shared
fixture in conftest.py.
</div>
</div>
<small><i>Code Review Run #315649</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for is_safe_ip</b></div>
<div id="fix">
`is_safe_ip` is a new public function used directly by
`superset/commands/dataset/importers/v1/utils.py` (line 88) for SSRF
protection, and its docstring is now cited in `is_safe_host` as the recommended
peer-address validation. It has zero dedicated unit tests — only indirect
coverage via mocked `is_safe_host` calls. Security-critical functions require
explicit test coverage to catch regressions.
</div>
</div>
<small><i>Code Review Run #315649</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/daos/test_tasks.py:
##########
@@ -395,16 +396,24 @@ def
test_remove_subscriber_not_subscribed(session_with_task: Session) -> None:
assert result is None
-def test_get_status(session_with_task: Session) -> None:
+def test_get_status(session_with_task: Session, mocker: MockerFixture) -> None:
"""Test get_status returns status string when task found by UUID"""
from superset.daos.tasks import TaskDAO
+ from superset.models.task_subscribers import TaskSubscriber
+
+ # get_status enforces the TaskFilter, so the polling user must be
+ # authenticated and subscribed to see the task.
+ mocker.patch("superset.tasks.filters.get_user_id",
return_value=TEST_USER_ID)
+ mocker.patch("superset.security_manager.is_admin", return_value=False)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect mock patch path</b></div>
<div id="fix">
The mock path `superset.security_manager` is incorrect — `security_manager`
is imported as a module attribute in `superset/tasks/filters.py`, so the patch
must reference it through that module's namespace:
`superset.tasks.filters.security_manager.is_admin`. The current path will raise
`AttributeError` at test runtime.
</div>
</div>
<small><i>Code Review Run #315649</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing status code assertion</b></div>
<div id="fix">
Test misses explicit HTTP status code assertion. BITO.md rule 12782 requires
status codes to be verified immediately after API calls. The response() method
returns 200 for this success path, but this test discards the status with
`_status_code`. All 29 other API tests in this file assert status codes
explicitly.
</div>
</div>
<small><i>Code Review Run #315649</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
