You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/06/02 00:23:49 UTC
[PR] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
rusackas opened a new pull request, #40647:
URL: https://github.com/apache/superset/pull/40647
### SUMMARY
This changes a **shipped default**: `SMTP_SSL_SERVER_AUTH` now defaults to `True` (previously `False`) in `superset/config.py`.
When STARTTLS or implicit SSL is used for outbound email (alerts & reports), Superset builds an SSL context via `ssl.create_default_context()` only when `SMTP_SSL_SERVER_AUTH` is truthy (see `superset/utils/core.py`). With the old `False` default, the SMTP server's TLS certificate was **not** validated against the system CA store. Defaulting to `True` verifies the mail server's identity out of the box, generic transport hardening for email notifications.
**Back-compat escape hatch:** the setting is unchanged in shape and remains fully overridable. Operators who rely on a self-signed or otherwise untrusted certificate can restore the prior behavior with:
```python
SMTP_SSL_SERVER_AUTH = False
```
An `UPDATING.md` entry under `## Next` documents the default change and the opt-out (recommending adding the cert/CA to the system trust store as the preferred fix).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (config default).
### TESTING INSTRUCTIONS
- Unit test added: `tests/unit_tests/config_test.py::test_smtp_ssl_server_auth_defaults_to_true` asserts the default is `True`. Ran locally: passes.
- Manual: configure SMTP with STARTTLS against a server with a CA-trusted cert and send a test report β delivery succeeds with validation on. Against an untrusted cert, delivery fails until `SMTP_SSL_SERVER_AUTH = False` is set.
### ADDITIONAL INFORMATION
- [ ] 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
> Note: this PR changes a shipped default. The back-compat escape hatch (`SMTP_SSL_SERVER_AUTH = False`) and an `UPDATING.md` entry are included.
π€ Generated with [Claude Code](https://claude.com/claude-code)
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4612384727
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4674152395
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4597665525
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40647?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 55.92%. Comparing base ([`41da35e`](https://app.codecov.io/gh/apache/superset/commit/41da35e9db016106f183764b6687a02580833da3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f545068`](https://app.codecov.io/gh/apache/superset/commit/f5450686fb4f571e333b292cc684bb4f4f0e0f53?dropdown=coverage&el=desc&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 (41da35e) and HEAD (f545068). Click for more details.
>
> <details><summary>HEAD has 13 uploads less than BASE</summary>
>
>| Flag | BASE (41da35e) | HEAD (f545068) |
>|------|------|------|
>|python|12|3|
>|unit|3|1|
>|hive|2|1|
>|presto|2|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40647 +/- ##
==========================================
- Coverage 63.94% 55.92% -8.03%
==========================================
Files 2658 2658
Lines 143011 143011
Branches 32866 32866
==========================================
- Hits 91454 79984 -11470
- Misses 49994 62339 +12345
+ Partials 1563 688 -875
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40647/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/40647/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.75% <100.00%> (ΓΈ)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40647/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/40647/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/40647/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.36% <100.00%> (ΓΈ)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/40647/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `42.68% <100.00%> (-17.28%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40647/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/40647/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ΓΈ> (ΓΈ)` | |
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 Sentry](https://app.codecov.io/gh/apache/superset/pull/40647?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] chore(config): default SMTP_SSL_SERVER_AUTH to True [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 #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3464142723
##########
tests/unit_tests/config_test.py:
##########
@@ -312,3 +312,123 @@ def test_full_setting(
assert dttm_col.is_dttm
assert dttm_col.python_date_format == "epoch_s"
assert dttm_col.expression == "CAST(dttm as INTEGER)"
+
+
+def test_smtp_ssl_server_auth_defaults_to_true() -> None:
+ """
+ The shipped default for SMTP_SSL_SERVER_AUTH validates the SMTP server's
+ TLS certificate. Operators can still opt out by overriding it to False.
+ """
+ from superset import config
+
+ assert config.SMTP_SSL_SERVER_AUTH is True
+
+
+def _smtp_config(**overrides: Any) -> dict[str, Any]:
+ """
+ Build a minimal SMTP config dict for ``send_mime_email`` tests, with
+ plaintext transport defaults; keyword ``overrides`` replace any key.
+ """
+ config = {
+ "SMTP_HOST": "localhost",
+ "SMTP_PORT": 25,
+ "SMTP_USER": "",
+ "SMTP_PASSWORD": "",
+ "SMTP_STARTTLS": False,
+ "SMTP_SSL": False,
+ "SMTP_SSL_SERVER_AUTH": True,
+ }
+ config.update(overrides)
+ return config
+
+
+def test_send_mime_email_ssl_server_auth_passes_context(
+ mocker: MockerFixture,
+) -> None:
+ """
+ With SMTP_SSL and SMTP_SSL_SERVER_AUTH enabled, ``send_mime_email`` builds a
+ default SSL context and threads it through to ``smtplib.SMTP_SSL`` so the
+ server certificate is validated.
+ """
+ from email.mime.multipart import MIMEMultipart
+
+ from superset.utils import core as utils
+
+ create_default_context = mocker.patch(
+ "superset.utils.core.ssl.create_default_context"
+ )
+ smtp_ssl = mocker.patch("smtplib.SMTP_SSL")
+ smtp = mocker.patch("smtplib.SMTP")
+
+ utils.send_mime_email(
+ "from",
+ ["to"],
+ MIMEMultipart(),
+ _smtp_config(SMTP_SSL=True, SMTP_SSL_SERVER_AUTH=True),
+ dryrun=False,
+ )
+
+ create_default_context.assert_called_once_with()
+ assert not smtp.called
+ smtp_ssl.assert_called_once_with(
+ "localhost", 25, context=create_default_context.return_value
+ )
Review Comment:
**Suggestion:** This assertion is missing the `timeout` keyword argument, but `send_mime_email` always calls `smtplib.SMTP_SSL(..., timeout=...)` (defaulting to 30 when not provided). The mock expectation will fail even when SSL context behavior is correct. Include the expected timeout (or assert on specific kwargs) so the test validates the intended contract instead of failing on a signature mismatch. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Test `test_send_mime_email_ssl_server_auth_passes_context` always fails.
- β οΈ CI for email-notification changes can be blocked by failure.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run the unit test `test_send_mime_email_ssl_server_auth_passes_context` in
`tests/unit_tests/config_test.py:345-376` (e.g. `pytest
tests/unit_tests/config_test.py::test_send_mime_email_ssl_server_auth_passes_context`).
2. Inside the test, `utils.send_mime_email(...)` from `superset/utils/core.py:927-973` is
called with `_smtp_config(SMTP_SSL=True, SMTP_SSL_SERVER_AUTH=True)` so `smtp_ssl` is
truthy and `smtp_ssl_server_auth` is True.
3. In `send_mime_email` the code at `superset/utils/core.py:941-960` computes
`smtp_timeout = config.get("SMTP_TIMEOUT", 30)` and then calls
`smtplib.SMTP_SSL(smtp_host, smtp_port, context=ssl_context, timeout=smtp_timeout)` (line
955-958), which with the test config becomes `SMTP_SSL("localhost", 25, context=<ctx>,
timeout=30)`.
4. Back in the test, the assertion at `tests/unit_tests/config_test.py:371-375` executes
`smtp_ssl.assert_called_once_with("localhost", 25,
context=create_default_context.return_value)`, which omits the `timeout` keyword argument;
pytest raises an `AssertionError` because the recorded mock call includes `timeout=30`, so
the test fails even though the SSL context behavior is correct.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b2519db7d433413ca0207caa56ce8b6c&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=b2519db7d433413ca0207caa56ce8b6c&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/config_test.py
**Line:** 373:375
**Comment:**
*Api Mismatch: This assertion is missing the `timeout` keyword argument, but `send_mime_email` always calls `smtplib.SMTP_SSL(..., timeout=...)` (defaulting to 30 when not provided). The mock expectation will fail even when SSL context behavior is correct. Include the expected timeout (or assert on specific kwargs) so the test validates the intended contract instead of failing on a signature mismatch.
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%2F40647&comment_hash=9b3bb1e2006eca6c59efab88e28e727d223853272641ca559112ad1932eb2351&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40647&comment_hash=9b3bb1e2006eca6c59efab88e28e727d223853272641ca559112ad1932eb2351&reaction=dislike'>π</a>
##########
tests/unit_tests/config_test.py:
##########
@@ -312,3 +312,123 @@ def test_full_setting(
assert dttm_col.is_dttm
assert dttm_col.python_date_format == "epoch_s"
assert dttm_col.expression == "CAST(dttm as INTEGER)"
+
+
+def test_smtp_ssl_server_auth_defaults_to_true() -> None:
+ """
+ The shipped default for SMTP_SSL_SERVER_AUTH validates the SMTP server's
+ TLS certificate. Operators can still opt out by overriding it to False.
+ """
+ from superset import config
+
+ assert config.SMTP_SSL_SERVER_AUTH is True
+
+
+def _smtp_config(**overrides: Any) -> dict[str, Any]:
+ """
+ Build a minimal SMTP config dict for ``send_mime_email`` tests, with
+ plaintext transport defaults; keyword ``overrides`` replace any key.
+ """
+ config = {
+ "SMTP_HOST": "localhost",
+ "SMTP_PORT": 25,
+ "SMTP_USER": "",
+ "SMTP_PASSWORD": "",
+ "SMTP_STARTTLS": False,
+ "SMTP_SSL": False,
+ "SMTP_SSL_SERVER_AUTH": True,
+ }
+ config.update(overrides)
+ return config
+
+
+def test_send_mime_email_ssl_server_auth_passes_context(
+ mocker: MockerFixture,
+) -> None:
+ """
+ With SMTP_SSL and SMTP_SSL_SERVER_AUTH enabled, ``send_mime_email`` builds a
+ default SSL context and threads it through to ``smtplib.SMTP_SSL`` so the
+ server certificate is validated.
+ """
+ from email.mime.multipart import MIMEMultipart
+
+ from superset.utils import core as utils
+
+ create_default_context = mocker.patch(
+ "superset.utils.core.ssl.create_default_context"
+ )
+ smtp_ssl = mocker.patch("smtplib.SMTP_SSL")
+ smtp = mocker.patch("smtplib.SMTP")
+
+ utils.send_mime_email(
+ "from",
+ ["to"],
+ MIMEMultipart(),
+ _smtp_config(SMTP_SSL=True, SMTP_SSL_SERVER_AUTH=True),
+ dryrun=False,
+ )
+
+ create_default_context.assert_called_once_with()
+ assert not smtp.called
+ smtp_ssl.assert_called_once_with(
+ "localhost", 25, context=create_default_context.return_value
+ )
+
+
+def test_send_mime_email_starttls_server_auth_passes_context(
+ mocker: MockerFixture,
+) -> None:
+ """
+ With STARTTLS and SMTP_SSL_SERVER_AUTH enabled, ``send_mime_email`` builds a
+ default SSL context and threads it through to ``starttls`` so the server
+ certificate is validated.
+ """
+ from email.mime.multipart import MIMEMultipart
+
+ from superset.utils import core as utils
+
+ create_default_context = mocker.patch(
+ "superset.utils.core.ssl.create_default_context"
+ )
+ smtp = mocker.patch("smtplib.SMTP")
+
+ utils.send_mime_email(
+ "from",
+ ["to"],
+ MIMEMultipart(),
+ _smtp_config(SMTP_STARTTLS=True, SMTP_SSL_SERVER_AUTH=True),
+ dryrun=False,
+ )
+
+ create_default_context.assert_called_once_with()
+ smtp.return_value.starttls.assert_called_once_with(
+ context=create_default_context.return_value
+ )
+
+
+def test_send_mime_email_server_auth_disabled_skips_context(
+ mocker: MockerFixture,
+) -> None:
+ """
+ When SMTP_SSL_SERVER_AUTH is disabled no SSL context is built and ``None`` is
+ passed through, preserving the opt-out (certificate validation skipped).
+ """
+ from email.mime.multipart import MIMEMultipart
+
+ from superset.utils import core as utils
+
+ create_default_context = mocker.patch(
+ "superset.utils.core.ssl.create_default_context"
+ )
+ smtp_ssl = mocker.patch("smtplib.SMTP_SSL")
+
+ utils.send_mime_email(
+ "from",
+ ["to"],
+ MIMEMultipart(),
+ _smtp_config(SMTP_SSL=True, SMTP_SSL_SERVER_AUTH=False),
+ dryrun=False,
+ )
+
+ assert not create_default_context.called
+ smtp_ssl.assert_called_once_with("localhost", 25, context=None)
Review Comment:
**Suggestion:** This assertion also omits the `timeout` keyword, but `send_mime_email` invokes `SMTP_SSL` with `timeout` in all paths. As written, the test will fail despite correctly verifying that server-auth is disabled. Add the timeout expectation (or inspect call kwargs) to match the real function contract. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Test `test_send_mime_email_server_auth_disabled_skips_context` fails.
- β οΈ Weakens verification of SMTP SSL server-auth opt-out.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run the unit test `test_send_mime_email_server_auth_disabled_skips_context` in
`tests/unit_tests/config_test.py:409-434` (e.g. `pytest
tests/unit_tests/config_test.py::test_send_mime_email_server_auth_disabled_skips_context`).
2. The test calls `utils.send_mime_email(...)` from `superset/utils/core.py:927-973` with
`_smtp_config(SMTP_SSL=True, SMTP_SSL_SERVER_AUTH=False)`, so `smtp_ssl` is truthy and
`smtp_ssl_server_auth` is False.
3. In `send_mime_email`, `smtp_timeout = config.get("SMTP_TIMEOUT", 30)` is computed at
`superset/utils/core.py:941-945`; since `smtp_ssl_server_auth` is False, `ssl_context` is
set to `None` at line 954, and then `smtplib.SMTP_SSL(smtp_host, smtp_port,
context=ssl_context, timeout=smtp_timeout)` is invoked at lines 955-958, i.e.
`SMTP_SSL("localhost", 25, context=None, timeout=30)` in this test.
4. The assertion at `tests/unit_tests/config_test.py:433-434` executes
`smtp_ssl.assert_called_once_with("localhost", 25, context=None)`, which omits the
`timeout` keyword argument present in the actual mock call, causing pytest to raise an
`AssertionError` and the test to fail despite correctly skipping SSL context creation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=09430e40b593419ca0866089a53cb997&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=09430e40b593419ca0866089a53cb997&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/config_test.py
**Line:** 434:434
**Comment:**
*Api Mismatch: This assertion also omits the `timeout` keyword, but `send_mime_email` invokes `SMTP_SSL` with `timeout` in all paths. As written, the test will fail despite correctly verifying that server-auth is disabled. Add the timeout expectation (or inspect call kwargs) to match the real function contract.
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%2F40647&comment_hash=66494096deaffd91d80db90f8a22934646a5901bfb6780a533d86048a2e4d6c2&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40647&comment_hash=66494096deaffd91d80db90f8a22934646a5901bfb6780a533d86048a2e4d6c2&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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4672668915
@aminghadersohi Good catch β fixed in fc6fe3a. The comment now correctly references `ssl.Purpose.SERVER_AUTH`, which is what `ssl.create_default_context()` defaults to and the correct purpose for Superset acting as an SMTP client verifying the mail server's certificate.
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #40647:
URL: https://github.com/apache/superset/pull/40647
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4662511984
Having secure default config is great! But IMO, from user's POV, this should land on major version, i.e. v7. Wdyt?
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3384816877
##########
tests/unit_tests/config_test.py:
##########
@@ -312,3 +312,13 @@ def test_full_setting(
assert dttm_col.is_dttm
assert dttm_col.python_date_format == "epoch_s"
assert dttm_col.expression == "CAST(dttm as INTEGER)"
+
+
+def test_smtp_ssl_server_auth_defaults_to_true() -> None:
+ """
+ The shipped default for SMTP_SSL_SERVER_AUTH validates the SMTP server's
+ TLS certificate. Operators can still opt out by overriding it to False.
+ """
+ from superset import config
+
+ assert config.SMTP_SSL_SERVER_AUTH is True
Review Comment:
Good call β addressed in the latest push. I added behavior-level unit tests in `tests/unit_tests/config_test.py` alongside the default-assertion test (which I kept):
- `test_send_mime_email_ssl_server_auth_passes_context`: with `SMTP_SSL=True` and `SMTP_SSL_SERVER_AUTH=True`, mocks `superset.utils.core.ssl.create_default_context` and `smtplib.SMTP_SSL`, then asserts `create_default_context()` is called once and its return value is threaded through as `SMTP_SSL(..., context=<that context>)`.
- `test_send_mime_email_starttls_server_auth_passes_context`: same idea for the STARTTLS path β asserts the context is passed to `smtp.starttls(context=<that context>)`.
- `test_send_mime_email_server_auth_disabled_skips_context`: the opt-out path β `create_default_context` is not called and `context=None` is passed through.
So the enabled path is now covered directly (create_default_context called + context object passed to `SMTP_SSL`/`starttls`), rather than only asserting the module-level default.
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4663308031
<!-- Bito Reply -->
The observation is accurate. The current unit test `test_smtp_ssl_server_auth_defaults_to_true` only verifies the module-level configuration value, and the integration test `test_send_mime_ssl` explicitly disables the feature, leaving the enabled path untested. To improve coverage, a new test case should be added that mocks `ssl.create_default_context` and verifies it is called when `SMTP_SSL_SERVER_AUTH` is `True` (the default).
**tests/unit_tests/config_test.py**
```
def test_smtp_ssl_server_auth_defaults_to_true() -> None:
"""
The shipped default for SMTP_SSL_SERVER_AUTH validates the SMTP server's
TLS certificate. Operators can still opt out by overriding it to False.
"""
from superset import config
assert config.SMTP_SSL_SERVER_AUTH is True
```
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4664666762
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4786148613
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3383303151
##########
tests/unit_tests/config_test.py:
##########
@@ -312,3 +312,13 @@ def test_full_setting(
assert dttm_col.is_dttm
assert dttm_col.python_date_format == "epoch_s"
assert dttm_col.expression == "CAST(dttm as INTEGER)"
+
+
+def test_smtp_ssl_server_auth_defaults_to_true() -> None:
+ """
+ The shipped default for SMTP_SSL_SERVER_AUTH validates the SMTP server's
+ TLS certificate. Operators can still opt out by overriding it to False.
+ """
+ from superset import config
+
+ assert config.SMTP_SSL_SERVER_AUTH is True
Review Comment:
NIT: asserts the module-level default but not the behavior. No test confirms `ssl.create_default_context()` is called and its return value passed to `SMTP_SSL`/`starttls` when `SMTP_SSL_SERVER_AUTH=True`. The integration test for `test_send_mime_ssl` opts *out* of the new default (line 211) rather than covering the enabled path.
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on code in PR #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3382755760
##########
UPDATING.md:
##########
@@ -34,6 +34,18 @@ The embedded dashboard page now validates the origin of incoming `postMessage` e
Enforcement only applies when the Allowed Domains list is non-empty. If the list is empty (the default), any origin is accepted, so there is no behavior change for embeds that did not configure Allowed Domains.
+### SMTP server certificate validation enabled by default
+
+`SMTP_SSL_SERVER_AUTH` now defaults to `True` (previously `False`). With this default, STARTTLS/SSL connections to the configured SMTP server validate the server's TLS certificate against the system trusted CA store. This makes outbound email (alerts and reports) verify the mail server's identity out of the box.
Review Comment:
```suggestion
`SMTP_SSL_SERVER_AUTH` now defaults to `True` (previously `False`). With this default, STARTTLS/SSL connections to the configured SMTP server validate the server's TLS certificate against the system's trusted CA store. This makes outbound email (alerts and reports) verify the mail server's identity out of the box.
```
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3416916056
##########
tests/integration_tests/email_tests.py:
##########
@@ -37,9 +37,18 @@
class TestEmailSmtp(SupersetTestCase):
+ SMTP_CONFIG_KEYS = ("SMTP_SSL", "SMTP_SSL_SERVER_AUTH", "SMTP_STARTTLS")
+
def setUp(self):
+ self._original_smtp_config = {
+ key: current_app.config[key] for key in self.SMTP_CONFIG_KEYS
+ }
current_app.config["SMTP_SSL"] = False
+ def tearDown(self):
Review Comment:
Good call β added `-> None` to `tearDown` (and `setUp` for consistency) in the latest push.
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4665517961
@hainenber - I was discussing the same with @villebro
I'm on the fence about it needing to be held back for V7 or not. It's arguably what people SHOULD be defaulting to, so it really catches people that have a "bad" config. I'm of the opinion that an UPDATING.md entry is sufficient here, but I'm OK with the call either way. @sadpandajoe @michael-s-molina @eschutho might have opinions here too.
I'm OK with either decision. If we want to switch the label to the `hold:next-major` variant, that's cool... still needs a review though ;)
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [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 #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3416582065
##########
tests/integration_tests/email_tests.py:
##########
@@ -37,9 +37,18 @@
class TestEmailSmtp(SupersetTestCase):
+ SMTP_CONFIG_KEYS = ("SMTP_SSL", "SMTP_SSL_SERVER_AUTH", "SMTP_STARTTLS")
+
def setUp(self):
+ self._original_smtp_config = {
+ key: current_app.config[key] for key in self.SMTP_CONFIG_KEYS
+ }
current_app.config["SMTP_SSL"] = False
+ def tearDown(self):
Review Comment:
**Suggestion:** Add a return type annotation to `tearDown` so the new method is fully typed. [custom_rule]
**Severity Level:** Minor β οΈ
<details>
<summary><b>Why it matters? π€ </b></summary>
`tearDown` is a newly added Python method and it has no return type annotation. The custom rule requires new Python code to be fully typed, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=94a892108b6d4449ad5636d13b88787d&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=94a892108b6d4449ad5636d13b88787d&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/integration_tests/email_tests.py
**Line:** 48:48
**Comment:**
*Custom Rule: Add a return type annotation to `tearDown` 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%2F40647&comment_hash=efa31f8d326bddde64a661058dc2e1dbf05fc505c970c2646a89095052214fd1&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40647&comment_hash=efa31f8d326bddde64a661058dc2e1dbf05fc505c970c2646a89095052214fd1&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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4597576269
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4599000393
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] chore(config): default SMTP_SSL_SERVER_AUTH to True [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 #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3464131827
##########
tests/integration_tests/email_tests.py:
##########
@@ -37,9 +37,18 @@
class TestEmailSmtp(SupersetTestCase):
- def setUp(self):
+ SMTP_CONFIG_KEYS = ("SMTP_SSL", "SMTP_SSL_SERVER_AUTH", "SMTP_STARTTLS")
+
+ def setUp(self) -> None:
+ self._original_smtp_config = {
+ key: current_app.config[key] for key in self.SMTP_CONFIG_KEYS
+ }
current_app.config["SMTP_SSL"] = False
Review Comment:
**Suggestion:** Add an inline docstring to this newly added setup method describing that it snapshots and overrides SMTP-related config keys for each test run. [custom_rule]
**Severity Level:** Minor β οΈ
<details>
<summary><b>Why it matters? π€ </b></summary>
This is a newly added Python method and it has no docstring. The custom rule requires newly added Python functions and classes to include inline docstrings, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2d553b05af3e4eb2bc449d870a2bc0b4&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=2d553b05af3e4eb2bc449d870a2bc0b4&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/integration_tests/email_tests.py
**Line:** 42:46
**Comment:**
*Custom Rule: Add an inline docstring to this newly added setup method describing that it snapshots and overrides SMTP-related config keys for each test run.
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%2F40647&comment_hash=3a68caadba4bfccdbb24a7946bf2d9244d06143768935076cbfb4824375b59dd&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40647&comment_hash=3a68caadba4bfccdbb24a7946bf2d9244d06143768935076cbfb4824375b59dd&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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3402404548
##########
tests/integration_tests/email_tests.py:
##########
@@ -208,6 +208,7 @@ def test_send_mime(self, mock_smtp, mock_smtp_ssl):
@mock.patch("smtplib.SMTP")
def test_send_mime_ssl(self, mock_smtp, mock_smtp_ssl):
current_app.config["SMTP_SSL"] = True
+ current_app.config["SMTP_SSL_SERVER_AUTH"] = False
Review Comment:
Good catch... added a setUp/tearDown save-and-restore for the SMTP config keys this class mutates, so nothing leaks to later tests.
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4690941879
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4713606692
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4689112244
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3401891920
##########
tests/unit_tests/config_test.py:
##########
@@ -312,3 +312,119 @@ def test_full_setting(
assert dttm_col.is_dttm
assert dttm_col.python_date_format == "epoch_s"
assert dttm_col.expression == "CAST(dttm as INTEGER)"
+
+
+def test_smtp_ssl_server_auth_defaults_to_true() -> None:
+ """
+ The shipped default for SMTP_SSL_SERVER_AUTH validates the SMTP server's
+ TLS certificate. Operators can still opt out by overriding it to False.
+ """
+ from superset import config
+
+ assert config.SMTP_SSL_SERVER_AUTH is True
+
+
+def _smtp_config(**overrides: Any) -> dict[str, Any]:
+ config = {
+ "SMTP_HOST": "localhost",
+ "SMTP_PORT": 25,
+ "SMTP_USER": "",
+ "SMTP_PASSWORD": "",
+ "SMTP_STARTTLS": False,
+ "SMTP_SSL": False,
+ "SMTP_SSL_SERVER_AUTH": True,
+ }
+ config.update(overrides)
+ return config
Review Comment:
Good call... added a docstring to `_smtp_config` in the latest push.
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4791270072
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3464199914
##########
tests/unit_tests/config_test.py:
##########
@@ -312,3 +312,123 @@ def test_full_setting(
assert dttm_col.is_dttm
assert dttm_col.python_date_format == "epoch_s"
assert dttm_col.expression == "CAST(dttm as INTEGER)"
+
+
+def test_smtp_ssl_server_auth_defaults_to_true() -> None:
+ """
+ The shipped default for SMTP_SSL_SERVER_AUTH validates the SMTP server's
+ TLS certificate. Operators can still opt out by overriding it to False.
+ """
+ from superset import config
+
+ assert config.SMTP_SSL_SERVER_AUTH is True
+
+
+def _smtp_config(**overrides: Any) -> dict[str, Any]:
+ """
+ Build a minimal SMTP config dict for ``send_mime_email`` tests, with
+ plaintext transport defaults; keyword ``overrides`` replace any key.
+ """
+ config = {
+ "SMTP_HOST": "localhost",
+ "SMTP_PORT": 25,
+ "SMTP_USER": "",
+ "SMTP_PASSWORD": "",
+ "SMTP_STARTTLS": False,
+ "SMTP_SSL": False,
+ "SMTP_SSL_SERVER_AUTH": True,
+ }
+ config.update(overrides)
+ return config
+
+
+def test_send_mime_email_ssl_server_auth_passes_context(
+ mocker: MockerFixture,
+) -> None:
+ """
+ With SMTP_SSL and SMTP_SSL_SERVER_AUTH enabled, ``send_mime_email`` builds a
+ default SSL context and threads it through to ``smtplib.SMTP_SSL`` so the
+ server certificate is validated.
+ """
+ from email.mime.multipart import MIMEMultipart
+
+ from superset.utils import core as utils
+
+ create_default_context = mocker.patch(
+ "superset.utils.core.ssl.create_default_context"
+ )
+ smtp_ssl = mocker.patch("smtplib.SMTP_SSL")
+ smtp = mocker.patch("smtplib.SMTP")
+
+ utils.send_mime_email(
+ "from",
+ ["to"],
+ MIMEMultipart(),
+ _smtp_config(SMTP_SSL=True, SMTP_SSL_SERVER_AUTH=True),
+ dryrun=False,
+ )
+
+ create_default_context.assert_called_once_with()
+ assert not smtp.called
+ smtp_ssl.assert_called_once_with(
+ "localhost", 25, context=create_default_context.return_value
+ )
Review Comment:
Good catch β fixed; #41250 added `timeout` to the `SMTP_SSL` call after these tests were written. Updated the assertion to expect `timeout=30`.
##########
tests/unit_tests/config_test.py:
##########
@@ -312,3 +312,123 @@ def test_full_setting(
assert dttm_col.is_dttm
assert dttm_col.python_date_format == "epoch_s"
assert dttm_col.expression == "CAST(dttm as INTEGER)"
+
+
+def test_smtp_ssl_server_auth_defaults_to_true() -> None:
+ """
+ The shipped default for SMTP_SSL_SERVER_AUTH validates the SMTP server's
+ TLS certificate. Operators can still opt out by overriding it to False.
+ """
+ from superset import config
+
+ assert config.SMTP_SSL_SERVER_AUTH is True
+
+
+def _smtp_config(**overrides: Any) -> dict[str, Any]:
+ """
+ Build a minimal SMTP config dict for ``send_mime_email`` tests, with
+ plaintext transport defaults; keyword ``overrides`` replace any key.
+ """
+ config = {
+ "SMTP_HOST": "localhost",
+ "SMTP_PORT": 25,
+ "SMTP_USER": "",
+ "SMTP_PASSWORD": "",
+ "SMTP_STARTTLS": False,
+ "SMTP_SSL": False,
+ "SMTP_SSL_SERVER_AUTH": True,
+ }
+ config.update(overrides)
+ return config
+
+
+def test_send_mime_email_ssl_server_auth_passes_context(
+ mocker: MockerFixture,
+) -> None:
+ """
+ With SMTP_SSL and SMTP_SSL_SERVER_AUTH enabled, ``send_mime_email`` builds a
+ default SSL context and threads it through to ``smtplib.SMTP_SSL`` so the
+ server certificate is validated.
+ """
+ from email.mime.multipart import MIMEMultipart
+
+ from superset.utils import core as utils
+
+ create_default_context = mocker.patch(
+ "superset.utils.core.ssl.create_default_context"
+ )
+ smtp_ssl = mocker.patch("smtplib.SMTP_SSL")
+ smtp = mocker.patch("smtplib.SMTP")
+
+ utils.send_mime_email(
+ "from",
+ ["to"],
+ MIMEMultipart(),
+ _smtp_config(SMTP_SSL=True, SMTP_SSL_SERVER_AUTH=True),
+ dryrun=False,
+ )
+
+ create_default_context.assert_called_once_with()
+ assert not smtp.called
+ smtp_ssl.assert_called_once_with(
+ "localhost", 25, context=create_default_context.return_value
+ )
+
+
+def test_send_mime_email_starttls_server_auth_passes_context(
+ mocker: MockerFixture,
+) -> None:
+ """
+ With STARTTLS and SMTP_SSL_SERVER_AUTH enabled, ``send_mime_email`` builds a
+ default SSL context and threads it through to ``starttls`` so the server
+ certificate is validated.
+ """
+ from email.mime.multipart import MIMEMultipart
+
+ from superset.utils import core as utils
+
+ create_default_context = mocker.patch(
+ "superset.utils.core.ssl.create_default_context"
+ )
+ smtp = mocker.patch("smtplib.SMTP")
+
+ utils.send_mime_email(
+ "from",
+ ["to"],
+ MIMEMultipart(),
+ _smtp_config(SMTP_STARTTLS=True, SMTP_SSL_SERVER_AUTH=True),
+ dryrun=False,
+ )
+
+ create_default_context.assert_called_once_with()
+ smtp.return_value.starttls.assert_called_once_with(
+ context=create_default_context.return_value
+ )
+
+
+def test_send_mime_email_server_auth_disabled_skips_context(
+ mocker: MockerFixture,
+) -> None:
+ """
+ When SMTP_SSL_SERVER_AUTH is disabled no SSL context is built and ``None`` is
+ passed through, preserving the opt-out (certificate validation skipped).
+ """
+ from email.mime.multipart import MIMEMultipart
+
+ from superset.utils import core as utils
+
+ create_default_context = mocker.patch(
+ "superset.utils.core.ssl.create_default_context"
+ )
+ smtp_ssl = mocker.patch("smtplib.SMTP_SSL")
+
+ utils.send_mime_email(
+ "from",
+ ["to"],
+ MIMEMultipart(),
+ _smtp_config(SMTP_SSL=True, SMTP_SSL_SERVER_AUTH=False),
+ dryrun=False,
+ )
+
+ assert not create_default_context.called
+ smtp_ssl.assert_called_once_with("localhost", 25, context=None)
Review Comment:
Fixed here too β added `timeout=30` to the assertion.
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on code in PR #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3382755760
##########
UPDATING.md:
##########
@@ -34,6 +34,18 @@ The embedded dashboard page now validates the origin of incoming `postMessage` e
Enforcement only applies when the Allowed Domains list is non-empty. If the list is empty (the default), any origin is accepted, so there is no behavior change for embeds that did not configure Allowed Domains.
+### SMTP server certificate validation enabled by default
+
+`SMTP_SSL_SERVER_AUTH` now defaults to `True` (previously `False`). With this default, STARTTLS/SSL connections to the configured SMTP server validate the server's TLS certificate against the system trusted CA store. This makes outbound email (alerts and reports) verify the mail server's identity out of the box.
Review Comment:
```suggestion
`SMTP_SSL_SERVER_AUTH` now defaults to `True` (previously `False`). With this default, STARTTLS/SSL connections to the configured SMTP server validate the server's TLS certificate against the system's trusted CA store. This makes outbound email (alerts and reports) verify the mail server's identity out of the box.
```
--
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] chore(config): default SMTP_SSL_SERVER_AUTH to True [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40647:
URL: https://github.com/apache/superset/pull/40647#issuecomment-4666448358
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] chore(config): default SMTP_SSL_SERVER_AUTH to True [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 #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3400911999
##########
tests/unit_tests/config_test.py:
##########
@@ -312,3 +312,119 @@ def test_full_setting(
assert dttm_col.is_dttm
assert dttm_col.python_date_format == "epoch_s"
assert dttm_col.expression == "CAST(dttm as INTEGER)"
+
+
+def test_smtp_ssl_server_auth_defaults_to_true() -> None:
+ """
+ The shipped default for SMTP_SSL_SERVER_AUTH validates the SMTP server's
+ TLS certificate. Operators can still opt out by overriding it to False.
+ """
+ from superset import config
+
+ assert config.SMTP_SSL_SERVER_AUTH is True
+
+
+def _smtp_config(**overrides: Any) -> dict[str, Any]:
+ config = {
+ "SMTP_HOST": "localhost",
+ "SMTP_PORT": 25,
+ "SMTP_USER": "",
+ "SMTP_PASSWORD": "",
+ "SMTP_STARTTLS": False,
+ "SMTP_SSL": False,
+ "SMTP_SSL_SERVER_AUTH": True,
+ }
+ config.update(overrides)
+ return config
Review Comment:
**Suggestion:** Add a short docstring to this new helper function to describe the default SMTP config it builds and how `overrides` are applied. [custom_rule]
**Severity Level:** Minor β οΈ
<details>
<summary><b>Why it matters? π€ </b></summary>
This is a newly added Python function and it does not include a docstring. The custom rule explicitly requires newly added functions to be documented inline, so the suggestion matches a real violation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a0912e6a2c0d47f3bfa2748380db440f&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=a0912e6a2c0d47f3bfa2748380db440f&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/config_test.py
**Line:** 327:338
**Comment:**
*Custom Rule: Add a short docstring to this new helper function to describe the default SMTP config it builds and how `overrides` are applied.
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%2F40647&comment_hash=af88b347ae47277bca55570b53bc72b0c31f732d4478accc95afe744445695b8&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40647&comment_hash=af88b347ae47277bca55570b53bc72b0c31f732d4478accc95afe744445695b8&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] chore(config): default SMTP_SSL_SERVER_AUTH to True [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 #40647:
URL: https://github.com/apache/superset/pull/40647#discussion_r3401936077
##########
tests/integration_tests/email_tests.py:
##########
@@ -208,6 +208,7 @@ def test_send_mime(self, mock_smtp, mock_smtp_ssl):
@mock.patch("smtplib.SMTP")
def test_send_mime_ssl(self, mock_smtp, mock_smtp_ssl):
current_app.config["SMTP_SSL"] = True
+ current_app.config["SMTP_SSL_SERVER_AUTH"] = False
Review Comment:
**Suggestion:** This test mutates global `current_app.config` and never restores `SMTP_SSL_SERVER_AUTH`, which makes later tests order-dependent when they rely on the default value. Save the original value and restore it in a `finally` block (or use a fixture/`patch.dict`) so this test remains isolated and cannot leak state to other tests. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ Later email tests may see wrong TLS verification default.
- β οΈ Test suite may become order-dependent and intermittently flaky.
- β οΈ Future tests using defaults risk unexpected SMTP behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Note that a single global Flask app instance is created for all integration tests in
`tests/integration_tests/test_app.py:28-32` via `app = create_app(...)`, and
`SupersetTestCase.create_app()` returns this same `app` for each test case in
`tests/integration_tests/base_tests.py:111-127`, so `current_app.config` mutations persist
across tests in the same process.
2. In `tests/integration_tests/email_tests.py:39-42`, `TestEmailSmtp.setUp()` only resets
`current_app.config["SMTP_SSL"] = False` and does not touch `SMTP_SSL_SERVER_AUTH`,
meaning whatever value this key had from a previous test is carried into the next one.
3. The test `TestEmailSmtp.test_send_mime_ssl` at
`tests/integration_tests/email_tests.py:209-223` explicitly sets
`current_app.config["SMTP_SSL_SERVER_AUTH"] = False` on line 211 and never restores it to
its prior value, unlike other tests in the same file that save and restore mutated config
(e.g. `EMAIL_HEADER_MUTATOR` at lines 69-92 and `SMTP_USER`/`SMTP_PASSWORD` at lines 7-23
in the later `test_send_mime_noauth`).
4. Since the default for `SMTP_SSL_SERVER_AUTH` is defined as `True` in
`superset/config.py:1696-1709`, any later test in the same process that expects this
default (and does not override the key in its own setup) will instead see the leaked
`False` value from `test_send_mime_ssl`, making the suite order-dependent and causing
future tests that rely on the default TLS verification behavior in
`superset/utils/core.py:930-32` to behave differently based solely on execution order.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aacc82d9a75b4feabfce0786adeae2f1&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=aacc82d9a75b4feabfce0786adeae2f1&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/integration_tests/email_tests.py
**Line:** 211:211
**Comment:**
*Logic Error: This test mutates global `current_app.config` and never restores `SMTP_SSL_SERVER_AUTH`, which makes later tests order-dependent when they rely on the default value. Save the original value and restore it in a `finally` block (or use a fixture/`patch.dict`) so this test remains isolated and cannot leak state to other tests.
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%2F40647&comment_hash=e52c8813e169c81ad73c784b0fbecf5926d972cb3ae92d210d103d6b7cd00b1c&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40647&comment_hash=e52c8813e169c81ad73c784b0fbecf5926d972cb3ae92d210d103d6b7cd00b1c&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