You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "hainenber (via GitHub)" <gi...@apache.org> on 2026/07/12 13:04:49 UTC
[PR] fix(sec): redact sensitive data in debug log for failed import validation [superset]
hainenber opened a new pull request, #41974:
URL: https://github.com/apache/superset/pull/41974
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
fix(sec): redact sensitive data in debug log for failed import validation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Fixes https://github.com/apache/superset/security/code-scanning/2565
I'm aware that only people with admin permission to Superset installation would be able to toggle on debug logging for troubleshooting sensitive data. However, per security posture, it would be highly risky to have these data to be visible since there can be others seeing as well, i.e. enterprise or vendor logging system. So let's mask these data away.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sec): redact sensitive data in debug log for failed import validation [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on code in PR #41974:
URL: https://github.com/apache/superset/pull/41974#discussion_r3566404068
##########
superset/commands/importers/v1/utils.py:
##########
@@ -230,7 +231,21 @@
prefix,
exc.messages,
)
- logger.debug("Config content that failed validation: %s", config)
+ # Mask sensitive data before logging out for debug
+ if config:
+ redacted_config = json.redact_sensitive(
+ config,
+ {
+ "$.password",
+ "$.ssh_tunnel.password",
+ "$.ssh_tunnel.private_key",
+ "$.ssh_tunnel.private_key_password",
+ "$.masked_encrypted_extra",
+ },
+ )
+ logger.debug(
+ "Config content that failed validation: %s.", redacted_config
Review Comment:
all the known secrets have been redacted via `json.redact_sensitive` function. Is that not enough?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sec): redact sensitive data in debug log for failed import validation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41974:
URL: https://github.com/apache/superset/pull/41974#issuecomment-5086185362
Closing in favor of #42399, which redacts the config values by logging just the field names instead of trying to allowlist/mask specific keys. Thanks for the PR, appreciate you catching this one.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sec): redact sensitive data in debug log for failed import validation [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41974:
URL: https://github.com/apache/superset/pull/41974#discussion_r3566354920
##########
superset/commands/importers/v1/utils.py:
##########
@@ -149,6 +149,7 @@ def load_configs(
prefix = file_name.split("/")[0]
schema = schemas.get(f"{prefix}/")
if schema:
+ config = None
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly introduced local variable so its optional shape is clear during validation and exception handling. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This new local variable is later used as a dictionary-like YAML config but is initialized as None without any type annotation. The custom rule requires type hints on relevant variables that can be annotated, so this is a real violation.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9bdbf2f88b534e499a5fb136a5871afd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9bdbf2f88b534e499a5fb136a5871afd&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/commands/importers/v1/utils.py
**Line:** 152:152
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced local variable so its optional shape is clear during validation and exception handling.
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%2F41974&comment_hash=63b4d0ccde93ed1e7c034bf120322050f36e502a45fad34baa779f8d691a46fd&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41974&comment_hash=63b4d0ccde93ed1e7c034bf120322050f36e502a45fad34baa779f8d691a46fd&reaction=dislike'>π</a>
##########
tests/unit_tests/commands/importers/v1/utils_test.py:
##########
@@ -117,3 +124,62 @@ def test_warning_count_excludes_preexisting_nulls(self) -> None:
call_args = mock_logger.warning.call_args[0]
assert call_args[1] == 2 # 2 out-of-bounds, 1 pre-existing null
+
+
+class TestLoadConfigs:
+ def test_load_configs_caught_exception_and_log_redacted_configs(self) -> None:
+ logging.basicConfig(level=logging.DEBUG)
+ from superset.commands.importers.v1.utils import (
+ load_configs,
+ )
+
+ with (
+ patch("superset.db") as mock_db,
+ patch("yaml.safe_load") as mock_yaml_safe_load,
+ patch("superset.commands.importers.v1.utils.logger") as mock_logger,
+ ):
+ mock_yaml_safe_load.return_value = {
+ "masked_encrypted_extra": json.dumps(
+ {"oauth2_client_info": {"secret": "MASKED"}}
+ ),
+ "ssh_tunnel": {},
+ }
+ mock_db.session.query.return_value = {"uuid-1": "SECRET_TO_BE_SEEN"}
+
+ class RaiseValidationSchema(Schema):
+ def load(self, value):
+ raise ValidationError("Exception when validating config")
Review Comment:
**Suggestion:** Add explicit type annotations to the inner schema `load` method parameters and return type so the new method is fully typed. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
The new inner `RaiseValidationSchema.load` method omits type hints on both the `value` parameter and the return type, which matches the Python type-hint rule for modified code.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=939df68989bb438e99da62922ab82afb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=939df68989bb438e99da62922ab82afb&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:** tests/unit_tests/commands/importers/v1/utils_test.py
**Line:** 149:151
**Comment:**
*Custom Rule: Add explicit type annotations to the inner schema `load` method parameters and return type so the new method is fully typed.
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%2F41974&comment_hash=efc08f28594b6cfb4997a9a8b0028993027afd6a5c1f45a803f0a11f30fb04f0&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41974&comment_hash=efc08f28594b6cfb4997a9a8b0028993027afd6a5c1f45a803f0a11f30fb04f0&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sec): redact sensitive data in debug log for failed import validation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #41974: fix(sec): redact sensitive data in debug log for failed import validation
URL: https://github.com/apache/superset/pull/41974
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sec): redact sensitive data in debug log for failed import validation [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41974:
URL: https://github.com/apache/superset/pull/41974#discussion_r3566359056
##########
tests/unit_tests/commands/importers/v1/utils_test.py:
##########
@@ -117,3 +124,62 @@ def test_warning_count_excludes_preexisting_nulls(self) -> None:
call_args = mock_logger.warning.call_args[0]
assert call_args[1] == 2 # 2 out-of-bounds, 1 pre-existing null
+
+
+class TestLoadConfigs:
+ def test_load_configs_caught_exception_and_log_redacted_configs(self) -> None:
+ logging.basicConfig(level=logging.DEBUG)
Review Comment:
**Suggestion:** Calling `logging.basicConfig` inside a unit test mutates global logging configuration for the whole test process and can leak state into unrelated tests. Remove this call or confine logging level changes to a local logger/mocked logger to avoid cross-test interference. [logic error]
<details>
<summary><b>Severity Level:</b> Minor π§Ή</summary>
```mdx
β οΈ Test mutates global logging configuration shared by suite.
β οΈ Later tests may see unexpected debug-level logging output.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open `tests/unit_tests/commands/importers/v1/utils_test.py` and locate
`TestLoadConfigs.test_load_configs_caught_exception_and_log_redacted_configs` starting at
line 129.
2. At line 131 inside this test, observe the call to
`logging.basicConfig(level=logging.DEBUG)` before any mocks or assertions are set up.
3. In Pythonβs logging module, `logging.basicConfig` configures the process-wide root
logger; calling it here mutates global logging settings (level and handlers) for all
subsequent code, including other tests in the same pytest run.
4. Run the test file with pytest; this test will execute and permanently adjust the root
logging configuration, so later tests inherit debug-level logging and any handlers
configured here, creating shared state that can affect logging behavior or noise outside
this specific test.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=db21446de3f84f9e92863ce39e78490a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=db21446de3f84f9e92863ce39e78490a&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:** tests/unit_tests/commands/importers/v1/utils_test.py
**Line:** 131:131
**Comment:**
*Logic Error: Calling `logging.basicConfig` inside a unit test mutates global logging configuration for the whole test process and can leak state into unrelated tests. Remove this call or confine logging level changes to a local logger/mocked logger to avoid cross-test interference.
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%2F41974&comment_hash=d7a2528141b84dfb0357e40594cbf94388f5b51841dfc81190928506d3a55616&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41974&comment_hash=d7a2528141b84dfb0357e40594cbf94388f5b51841dfc81190928506d3a55616&reaction=dislike'>π</a>
##########
tests/unit_tests/commands/importers/v1/utils_test.py:
##########
@@ -117,3 +124,62 @@ def test_warning_count_excludes_preexisting_nulls(self) -> None:
call_args = mock_logger.warning.call_args[0]
assert call_args[1] == 2 # 2 out-of-bounds, 1 pre-existing null
+
+
+class TestLoadConfigs:
+ def test_load_configs_caught_exception_and_log_redacted_configs(self) -> None:
+ logging.basicConfig(level=logging.DEBUG)
+ from superset.commands.importers.v1.utils import (
+ load_configs,
+ )
+
+ with (
+ patch("superset.db") as mock_db,
Review Comment:
**Suggestion:** The DB patch targets `superset.db`, but `load_configs` uses the module-local import from `superset.commands.importers.v1.utils`. Because the function is imported before entering the patch context, this mock does not intercept database calls and the test can hit the real DB/session instead of an isolated fake. Patch the symbol where it is used (`superset.commands.importers.v1.utils.db`) so the test is deterministic. [incorrect variable usage]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
β Unit test touches real database session instead of mock.
β οΈ Test behavior depends on external database state and contents.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open `superset/superset/commands/importers/v1/utils.py` and note the module-level
import `from superset import db` at line 27, and the use of `db.session.query(...).all()`
inside `load_configs` at lines 121-143.
2. Open `tests/unit_tests/commands/importers/v1/utils_test.py` and locate
`TestLoadConfigs.test_load_configs_caught_exception_and_log_redacted_configs` starting at
line 129, where `load_configs` is imported from `superset.commands.importers.v1.utils` at
lines 132-134 (triggering the module import and binding its local `db` name).
3. In the same test, observe the patch context at lines 136-139, which patches
`"superset.db"` (line 137) and `"superset.commands.importers.v1.utils.logger"`. The patch
replaces the `superset.db` attribute but does not modify the already-imported `db` symbol
in `superset.commands.importers.v1.utils`.
4. Run this test (e.g., `pytest
tests/unit_tests/commands/importers/v1/utils_test.py::TestLoadConfigs::test_load_configs_caught_exception_and_log_redacted_configs`);
during `load_configs` execution, the `db.session.query(...).all()` calls in `utils.py`
still hit the real SQLAlchemy `db` object, not the mocked `mock_db`, so the test interacts
with the live database session instead of an isolated fake.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5b9a094367794cf7a707a13a85dcb2bf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5b9a094367794cf7a707a13a85dcb2bf&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:** tests/unit_tests/commands/importers/v1/utils_test.py
**Line:** 137:137
**Comment:**
*Incorrect Variable Usage: The DB patch targets `superset.db`, but `load_configs` uses the module-local import from `superset.commands.importers.v1.utils`. Because the function is imported before entering the patch context, this mock does not intercept database calls and the test can hit the real DB/session instead of an isolated fake. Patch the symbol where it is used (`superset.commands.importers.v1.utils.db`) so the test is deterministic.
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%2F41974&comment_hash=807f821bd9c5180fd0d266d4c3b57fd35235990ab4aaa73550a6b3429991f638&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41974&comment_hash=807f821bd9c5180fd0d266d4c3b57fd35235990ab4aaa73550a6b3429991f638&reaction=dislike'>π</a>
##########
tests/unit_tests/commands/importers/v1/utils_test.py:
##########
@@ -117,3 +124,62 @@ def test_warning_count_excludes_preexisting_nulls(self) -> None:
call_args = mock_logger.warning.call_args[0]
assert call_args[1] == 2 # 2 out-of-bounds, 1 pre-existing null
+
+
+class TestLoadConfigs:
+ def test_load_configs_caught_exception_and_log_redacted_configs(self) -> None:
+ logging.basicConfig(level=logging.DEBUG)
+ from superset.commands.importers.v1.utils import (
+ load_configs,
+ )
+
+ with (
+ patch("superset.db") as mock_db,
+ patch("yaml.safe_load") as mock_yaml_safe_load,
+ patch("superset.commands.importers.v1.utils.logger") as mock_logger,
+ ):
+ mock_yaml_safe_load.return_value = {
+ "masked_encrypted_extra": json.dumps(
+ {"oauth2_client_info": {"secret": "MASKED"}}
+ ),
+ "ssh_tunnel": {},
+ }
+ mock_db.session.query.return_value = {"uuid-1": "SECRET_TO_BE_SEEN"}
Review Comment:
**Suggestion:** The mocked query return is a plain dict, but production code calls `.all()` on each `db.session.query(...)` result. This mock does not match the query API contract and will fail with an attribute error once the DB patch target is corrected. Return a mock object (or chain) that implements `.all()` with the expected tuple rows for each query. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
β DB mock would crash when `.all()` is invoked.
β οΈ Prevents isolating `load_configs` database queries via mocking.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Inspect `load_configs` in `superset/superset/commands/importers/v1/utils.py` (lines
108-143) and note that it builds several dictionaries by calling
`db.session.query(...).all()` at lines 121-124, 126-129, 131-135, and 138-142, expecting
the query object to implement an `.all()` method returning iterable rows.
2. In `tests/unit_tests/commands/importers/v1/utils_test.py`, locate the same test at
lines 129-185 and see that it patches `"superset.db"` at line 137 and configures the DB
mock at line 147 with `mock_db.session.query.return_value = {"uuid-1":
"SECRET_TO_BE_SEEN"}`, i.e., a plain dict.
3. A plain dict does not provide an `.all()` method, so a correctly wired mock that
replaces the `db` used in `load_configs` would cause `db.session.query(...).all()` in
`utils.py` to call `.all()` on that dict and raise `AttributeError` instead of returning
rows.
4. This demonstrates that the testβs DB mock does not conform to the query API contract
used by `load_configs`; it is currently unused due to the wrong patch target (see
suggestion 1), but as soon as the patch is corrected to
`superset.commands.importers.v1.utils.db`, the mismatch will immediately break the test.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=795b936e62734fa0a68af8dbfb9c7626&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=795b936e62734fa0a68af8dbfb9c7626&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:** tests/unit_tests/commands/importers/v1/utils_test.py
**Line:** 147:147
**Comment:**
*Api Mismatch: The mocked query return is a plain dict, but production code calls `.all()` on each `db.session.query(...)` result. This mock does not match the query API contract and will fail with an attribute error once the DB patch target is corrected. Return a mock object (or chain) that implements `.all()` with the expected tuple rows for each query.
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%2F41974&comment_hash=3e108cc4814f774beabe23cdd42071b1e732e8453ac88d7cf2766f96620d2d94&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41974&comment_hash=3e108cc4814f774beabe23cdd42071b1e732e8453ac88d7cf2766f96620d2d94&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sec): redact sensitive data in debug log for failed import validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41974:
URL: https://github.com/apache/superset/pull/41974#discussion_r3566361081
##########
tests/unit_tests/commands/importers/v1/utils_test.py:
##########
@@ -117,3 +124,62 @@ def test_warning_count_excludes_preexisting_nulls(self) -> None:
call_args = mock_logger.warning.call_args[0]
assert call_args[1] == 2 # 2 out-of-bounds, 1 pre-existing null
+
+
+class TestLoadConfigs:
+ def test_load_configs_caught_exception_and_log_redacted_configs(self) -> None:
+ logging.basicConfig(level=logging.DEBUG)
+ from superset.commands.importers.v1.utils import (
+ load_configs,
+ )
+
+ with (
+ patch("superset.db") as mock_db,
+ patch("yaml.safe_load") as mock_yaml_safe_load,
+ patch("superset.commands.importers.v1.utils.logger") as mock_logger,
+ ):
+ mock_yaml_safe_load.return_value = {
+ "masked_encrypted_extra": json.dumps(
+ {"oauth2_client_info": {"secret": "MASKED"}}
+ ),
+ "ssh_tunnel": {},
+ }
+ mock_db.session.query.return_value = {"uuid-1": "SECRET_TO_BE_SEEN"}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect mock causes AttributeError</b></div>
<div id="fix">
Mock at line 147 sets `return_value` to a dict, but production code calls `.all()` on this result (`utils.py:123`). SQLAlchemy's `.all()` returns a list of tuples, not a dict. Fix: use `mock_db.session.query.return_value.all.return_value = [("uuid-1", "SECRET_TO_BE_SEEN")]`. This test would pass in isolation only if the exception is caught before reaching the query code path, masking the real bug.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
mock_db.session.query.return_value = {"uuid-1": "SECRET_TO_BE_SEEN"}
mock_db.session.query.return_value.all.return_value = [("uuid-1", "SECRET_TO_BE_SEEN")]
````
</div>
</details>
</div>
<small><i>Code Review Run #a65b05</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sec): redact sensitive data in debug log for failed import validation [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41974:
URL: https://github.com/apache/superset/pull/41974#issuecomment-4951293853
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41974?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.99%. Comparing base ([`49dc0ac`](https://app.codecov.io/gh/apache/superset/commit/49dc0acd827ae59845dc079e980a0ff701114396?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6e2769f`](https://app.codecov.io/gh/apache/superset/commit/6e2769fb480edafbc1fe1d9a1eb8b6f0604603e1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/41974?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/importers/v1/utils.py](https://app.codecov.io/gh/apache/superset/pull/41974?src=pr&el=tree&filepath=superset%2Fcommands%2Fimporters%2Fv1%2Futils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL3V0aWxzLnB5) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (49dc0ac) and HEAD (6e2769f). Click for more details.
>
> <details><summary>HEAD has 20 uploads less than BASE</summary>
>
>| Flag | BASE (49dc0ac) | HEAD (6e2769f) |
>|------|------|------|
>|hive|3|1|
>|python|18|2|
>|presto|3|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41974 +/- ##
==========================================
- Coverage 64.87% 55.99% -8.89%
==========================================
Files 2742 2742
Lines 153378 153381 +3
Branches 35168 35169 +1
==========================================
- Hits 99509 85880 -13629
- Misses 51962 66675 +14713
+ Partials 1907 826 -1081
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41974/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/41974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.06% <0.00%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/41974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/41974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/41974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.06% <0.00%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/41974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.11% <0.00%> (-18.29%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/41974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/41974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41974?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sec): redact sensitive data in debug log for failed import validation [superset]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #41974:
URL: https://github.com/apache/superset/pull/41974#discussion_r3566355478
##########
superset/commands/importers/v1/utils.py:
##########
@@ -230,7 +231,21 @@
prefix,
exc.messages,
)
- logger.debug("Config content that failed validation: %s", config)
+ # Mask sensitive data before logging out for debug
+ if config:
+ redacted_config = json.redact_sensitive(
+ config,
+ {
+ "$.password",
+ "$.ssh_tunnel.password",
+ "$.ssh_tunnel.private_key",
+ "$.ssh_tunnel.private_key_password",
+ "$.masked_encrypted_extra",
+ },
+ )
+ logger.debug(
+ "Config content that failed validation: %s.", redacted_config
Review Comment:
## CodeQL / Clear-text logging of sensitive information
This expression logs [sensitive data (password)](1) as clear text.
This expression logs [sensitive data (password)](2) as clear text.
This expression logs [sensitive data (password)](3) as clear text.
This expression logs [sensitive data (password)](4) as clear text.
This expression logs [sensitive data (password)](5) as clear text.
This expression logs [sensitive data (password)](6) as clear text.
This expression logs [sensitive data (password)](7) as clear text.
This expression logs [sensitive data (password)](8) as clear text.
This expression logs [sensitive data (password)](9) as clear text.
This expression logs [sensitive data (password)](10) as clear text.
This expression logs [sensitive data (password)](11) as clear text.
This expression logs [sensitive data (password)](12) as clear text.
This expression logs [sensitive data (password)](13) as clear text.
This expression logs [sensitive data (password)](14) as clear text.
This expression logs [sensitive data (password)](15) as clear text.
This expression logs [sensitive data (password)](16) as clear text.
This expression logs [sensitive data (password)](17) as clear text.
This expression logs [sensitive data (password)](18) as clear text.
This expression logs [sensitive data (password)](19) as clear text.
This expression logs [sensitive data (password)](20) as clear text.
This expression logs [sensitive data (password)](21) as clear text.
This expression logs [sensitive data (password)](22) as clear text.
This expression logs [sensitive data (password)](23) as clear text.
[Show more details](https://github.com/apache/superset/security/code-scanning/2567)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sec): redact sensitive data in debug log for failed import validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41974:
URL: https://github.com/apache/superset/pull/41974#issuecomment-4951281136
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sec): redact sensitive data in debug log for failed import validation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41974:
URL: https://github.com/apache/superset/pull/41974#issuecomment-4997264274
Thanks for this, @hainenber. Bito's thread on the test mock is still open and looks right: `utils.py` does `from superset import db`, so patching `superset.db` in the test doesn't touch what `load_configs` actually calls, and `db.session.query.return_value` is set to a plain `dict` where the code expects `.all()`. As written the db-sourced secret path (the reason for the `SECRET_TO_BE_SEEN` setup) never actually gets exercised.
Also CodeQL is still failing on this file with a new high-severity clear-text-logging alert, and nobody answered your question in that thread about whether the redaction covers it. Want to close that loop, since this is a sec fix landing on top of an open security alert?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org