mikebridge commented on PR #41550:
URL: https://github.com/apache/superset/pull/41550#issuecomment-5129270387
## Self-review: two independent review panels (14 lenses)
Ahead of asking for review I ran two separate panels over this work — one
scoped to this PR's own 23 commits (`sc-111185-deletion-retention..HEAD`:
react, preset, superset-committer, css, python, tidy-first) and one over the
full stack `master...HEAD` (clean-code, continuous-delivery, css, DDD, preset,
react, superset-committer, tidy-first). Reconciling them turned up findings
neither panel would have caught alone, so I'm posting the merged result rather
than waiting.
**Status: I'm holding this PR in draft until the two blockers below are
fixed.** Note that none of this affects #41549 — the dangerous composition
described below does not exist there, and that PR remains merge-ready.
Nothing here is deployed or merged: neither `superset/commands/purge.py` nor
`superset/commands/deletion_retention/force_purge.py` is on master, so this
describes unmerged code only.
---
### BLOCKER 1 — Purge resolves a UUID across *every* soft-delete model,
discarding the route's type-scoped authorization
`superset/commands/purge.py:56` →
`superset/commands/deletion_retention/force_purge.py:52`
`PurgeArchivedCommand.validate()` authorizes correctly: it resolves through
the route's binding DAO (chart route → `ChartDAO`), confirms the row is
soft-deleted, and calls `raise_for_editorship` on it. But `run()` then discards
that validated model and passes only the raw UUID string to
`ForcePurgeCommand`, whose `_resolve()` iterates
`SoftDeleteMixin._registered_subclasses` and returns the **first** UUID match
in **any** model — live or soft-deleted, visibility filter bypassed.
UUID uniqueness is per-table, and the import APIs accept caller-supplied
UUIDs. So a user who can import a chart bearing a target dashboard's UUID,
archive that chart, and call `POST /api/v1/chart/<uuid>/purge` can have the
dashboard resolved and irreversibly hard-deleted instead — with authorization
only ever checked against the chart they legitimately own. Registration order
decides which object dies.
This sits squarely inside the `SECURITY.md` test: a principal performing an
action the role-and-capability matrix does not entitle them to.
Worth noting *why* this was missed until now, because it's instructive:
`ForcePurgeCommand` is fine as written in #41549, where its only callers are
the CLI and the retention Celery task — operator-grade entry points, trusted
under the threat model. This PR introduces the first caller that accepts an
end-user-supplied UUID. The flaw exists only in the *composition*, which is
precisely the seam a per-PR review does not see.
**Fix direction:** carry the validated entity (or at minimum its model
class) from `validate()` into the purge; resolve only within that model;
re-check authorization on the resolved row under lock. A regression test with
two rows of different types sharing one UUID would pin it.
### BLOCKER 2 — Retention-days config key does not exist, so the
recoverability copy is dead and the flag-on E2E fails
`superset/views/base.py:119` +
`superset-frontend/src/utils/softDeleteCopy.ts:36`
Both name `SUPERSET_SOFT_DELETE_RETENTION_DAYS`. The actual config key is
`SOFT_DELETE_RETENTION_DAYS` (`superset/config.py:994`, default 30).
`app.config.get()` therefore always returns `None`,
`getSoftDeleteRetentionDays()` always returns `0`, and the "you can recover it
there within N days" clause never renders on any stock deployment.
This is fallout from the `SUPERSET_SOFT_DELETE_*` → `SOFT_DELETE_*` rename
on #41549 that the frontend never followed. It escaped because
`getSoftDeleteRetentionDays()` and `archiveConfirmDescription()` have no unit
tests, and because this PR's own `delete-modal.spec.ts` asserts `/recover it
there within \d+ days/` but *skips* rather than fails in default CI where
`SOFT_DELETE` is off — so the coverage that would have caught it is dormant
exactly where it was needed.
Fix should surface the **effective** window (which
`superset/commands/deletion_retention/window.py` can override live via
`SharedKey`), not the raw config default.
---
### HIGH
| # | Finding | Location |
|---|---|---|
| 1 | **Purge answers `200 OK` when nothing was deleted.**
`ForcePurgeCommand` returns `{'purged': False, 'reason':
'blocked'\|'not_found'}` for a report-referenced object or a lost race;
`PurgeArchivedCommand.run()` discards that dict and the route returns 200
unconditionally. The UI then removes the row while the object still exists. Map
non-purged outcomes to typed errors. | `commands/purge.py:56` |
| 2 | **Select-then-act race.** Authorization and the `deleted_at` check
happen in `validate()`; `ForcePurgeCommand` then rolls back, re-resolves, and
purges with `enforce_window=False`, which skips the archived re-check. A
restore landing in between means a **live** object is hard-deleted. Needs a
locked-row re-assertion of both archive state and authorization. |
`commands/purge.py:66-85` |
| 3 | **Purge audience is narrower than restore while the code claims
parity.** `validate()` omits `skip_base_filter=True` where
`BaseRestoreCommand.validate()` passes it deliberately, so an editor who lost
datasource access can restore their trash but gets 404 on purge. Fails closed,
but the docstring, OpenAPI text and the inline comment "matching the restore
path" are all wrong. Either match restore or document the divergence — and test
the non-admin-editor case either way. | `commands/purge.py:67-72` |
| 4 | **Blanket `except Exception` → 422.** DB outage, programming error and
policy block all collapse into one generic 422, and because the binding
exception is constructed with no args the logged/returned message describes the
*new* empty error rather than the cause. | `commands/purge.py:59-64` |
| 5 | **Disabling `SOFT_DELETE` hides the UI and scheduler but leaves the
irreversible purge APIs callable.** Either gate the routes on the flag, or add
a dedicated permanent-purge kill switch. (#42469 set the precedent by gating
restore on `ENABLE_VERSIONING_CAPTURE`.) | `charts/api.py:794` + siblings |
| 6 | **Archived name links very likely 404.** Chart/dashboard names link to
explore/dashboard pages, but soft-deleted GETs 404 — as this PR's own
stale-restore E2E documents. No test follows the link, and the docs describe it
as a preview. Drop/disable until restore, or teach the target pages to serve
archived rows to the restore audience. | `ArchivedList/index.tsx:215-229` |
### MEDIUM (10)
Archive shell gated on `Chart can_read` only, locking dashboard/dataset-only
users out of their own trash (`views/archived_assets.py:42`) · row actions
invisible to keyboard users — `.actions` reveals on `:hover` with no
`:focus-within` (`ArchivedList/index.tsx:90`) · restore stays enabled while its
request is in flight, so repeat activation reports failure after success
(`ArchivedList/index.tsx:157`) · bulk-archive toast drops `errMsg` (missing
`%s`) (`DashboardList/index.tsx:353`) · `SoftDeleteBinding.dao: Any` and
`validate() -> Any` · audit `actor` falls back to the literal string `"user"`
on a branch `@protect()` makes unreachable · `TestChartArchiveListing` lacks
the leak-proof `tearDown` its dashboard/dataset siblings have · ~60-line purge
route triplicated across three APIs (`SoftDeleteApiMixin` is the natural home)
· `ArchivedList` combines fetching, mutations, filters, columns and toast
policy in one component · docs say "Restore" where the UI says "Recover",
omit permanent deletion entirely, and describe the gate as "owners and admins"
where it is editorship.
### LOW (~20)
i18n and copy (the `→` glyph outside its translation unit, "Recently
archived" vs "Recently Archived" casing, `messages.pot` extracted for backend
only, `item(s)` pluralisation), UI polish (time-range cutoffs frozen at mount,
type filter not URL-synced, icon spacing and touch targets, hover-only
tooltip), code health (unused module logger, bootstrap-conf cast, PEP 257 first
line, a "passes today" comment of mine that breaks the timeless-comment
convention), and test hardening (purge tests assert only row absence; `403|404`
disjunctions weaken the RBAC pins; `softDeleteCopy` copy branches untested —
which is how blocker 2 escaped). Happy to paste the full list if useful.
---
### Out of scope for this PR
The full-stack panel also raised items in `purge_cascade.py`,
`tasks/deletion_retention.py`, the Celery config and `audit.py` — those belong
to #41549 and are tracked as follow-ups (sc-115342, sc-115343, sc-115409,
sc-115410) rather than reopening an approved PR. One process question it raised
that I can't settle myself: whether the retention policy, purge APIs and
migration are covered by an existing SIP or need their own.
--
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]
