rusackas commented on PR #44003:
URL: https://github.com/apache/superset/pull/44003#issuecomment-5588488616
Went through Bito's additional suggestions and sha174n's inline review,
pushed a follow-up commit (72ccf98) addressing what checked out:
**Taken:**
- The `encrypted_extra` reuse gap in `update.py` (Bito's CWE-200 finding,
sha174n's more detailed version with a suggestion block) — real gap, took
sha174n's fix with two refinements (check `model.password is not None` instead
of re-deriving via the masked URI, and exclude an empty `"{}"`
`encrypted_extra` from being treated as a stored secret).
- `daos/database.py`'s duplicate base-filter logic —
`DatabaseDAO._apply_base_filter` exists upstream now (landed after this branch
was first opened), swapped the manual reimplementation for it.
- The SSH tunnel gating bug flagged on `validate.py` — real, and it turned
out to also exist in `test_connection.py` (same pattern, just not flagged
there). Both now gate the tunnel's own password unmask on whether the tunnel
endpoint itself changed, not the combined identity flag, so an unrelated
`engine_params` edit no longer spuriously breaks a legitimate tunneled
connection test.
- The `field_name` misattribution on `DatabaseUpdateUnsafeRebindError` —
parameterized it so the SSH-tunnel-only rebind case attributes its 422
correctly.
**Not taken:** the three "narrow `engine_params` comparison to host/port
only" suggestions (`update.py`, `test_connection.py`, `importers/v1/utils.py`).
The false-positive concern is real, but the fix as proposed is a regression
risk as-is — Postgres has `hostaddr` as a second way to redirect the
destination independent of `host`, and I haven't audited whether an equivalent
exists across the other engine specs this code touches. Narrowing the
comparison without that audit could reopen the exact bug this PR closes for
some driver. Left the conservative whole-dict comparison in place; the
usability cost seems like a reasonable follow-up issue for someone to do that
audit properly rather than something to rush into a security-relevant PR.
Regression tests added for everything taken above, each verified to fail
without its fix and pass with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
