codeant-ai-for-open-source[bot] commented on code in PR #40282:
URL: https://github.com/apache/superset/pull/40282#discussion_r3271677709
##########
superset/dashboards/api.py:
##########
@@ -1058,11 +1058,15 @@ def put_colors(self, pk: int) -> Response:
return self.response_400(message=error.messages)
try:
+ dry_run = parse_boolean_string(request.args.get("dry_run",
"false"))
mark_updated = parse_boolean_string(
request.args.get("mark_updated", "true")
)
- UpdateDashboardColorsConfigCommand(pk, item, mark_updated).run()
- response = self.response(200)
+ if dry_run:
+ response = self.response(200)
+ else:
+ UpdateDashboardColorsConfigCommand(pk, item,
mark_updated).run()
+ response = self.response(200)
Review Comment:
**Suggestion:** The `dry_run` branch skips
`UpdateDashboardColorsConfigCommand.run()`, which is where dashboard existence
and ownership checks happen. That causes `dry_run=true` requests to return 200
even for non-existent dashboards or dashboards the caller is not allowed to
modify. Keep the same validation/authorization path for dry runs (for example,
add a dry-run mode to the command that runs validation but skips persistence).
[security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dry-run colors endpoint returns 200 for nonexistent dashboard IDs.
- ❌ Dry-run bypasses DashboardDAO existence and raise_for_ownership
validations.
- ⚠️ Clients may accept invalid dashboard colors config as valid.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with this PR code and note the dashboard colors update
endpoint
`put_colors` defined in `superset/dashboards/api.py` at lines 10-85 of the
snippet
returned by Read (header "Showing lines 1000 to 1199"), exposed via
`@expose("/<pk>/colors", methods=("PUT",))` inside `DashboardRestApi`.
2. Send an HTTP `PUT` request to
`/api/v1/dashboard/<pk>/colors?dry_run=true` with a valid
JSON body (so Marshmallow validation in `put_colors` at `api.py` lines 56-59
passes), but
choose `<pk>` to be a dashboard ID that does not exist in the database (or
one the current
user does not own).
3. The request enters `DashboardRestApi.put_colors` (`api.py` lines 56-85);
inside the
second `try:` block at lines 61-70, `dry_run` is parsed as `True`, so the
branch at lines
66-70 executes: `if dry_run: response = self.response(200)` and the method
returns without
ever invoking `UpdateDashboardColorsConfigCommand(pk, item,
mark_updated).run()`.
4. Because `UpdateDashboardColorsConfigCommand.run()`
(`superset/commands/dashboard/update.py` lines 40-63) calls
`super().validate()` which in
turn runs `UpdateDashboardCommand.validate` (`update.py` lines 42-88) to (a)
look up the
dashboard via `DashboardDAO.find_by_id(self._model_id)` and raise
`DashboardNotFoundError()` if missing, and (b) enforce ownership via
`security_manager.raise_for_ownership(self._model)` raising
`DashboardForbiddenError()` on
failure, skipping `run()` in the dry-run branch means none of these checks
execute, no
exception is raised, and the client receives HTTP 200 instead of the
expected 404/403 for
nonexistent or unauthorized dashboards.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9ec964742a52469090a45215c4578745&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9ec964742a52469090a45215c4578745&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 1065:1069
**Comment:**
*Security: The `dry_run` branch skips
`UpdateDashboardColorsConfigCommand.run()`, which is where dashboard existence
and ownership checks happen. That causes `dry_run=true` requests to return 200
even for non-existent dashboards or dashboards the caller is not allowed to
modify. Keep the same validation/authorization path for dry runs (for example,
add a dry-run mode to the command that runs validation but skips persistence).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40282&comment_hash=77a3af389bc28dd4a77b36e3e571d065c0844ce81cf4efd2ee195e905d784255&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40282&comment_hash=77a3af389bc28dd4a77b36e3e571d065c0844ce81cf4efd2ee195e905d784255&reaction=dislike'>👎</a>
--
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]
