amaannawab923 commented on PR #45515:
URL: https://github.com/apache/superset/pull/45515#issuecomment-6287228690
## Please do not adopt this approach — security review found two
load-bearing controls broken
A review pass since opening this draft found problems serious enough to
report before anyone spends time on the code. Both stem from the same root
decision — letting a contributed page claim a free-form top-level path — and I
would rather state them than have a reviewer find them.
### 1. The host-route collision guard compares strings against rule
templates, so every parameterised host route is unguarded
`_register_extension_pages` builds its guard set as `{str(rule) for rule in
current_app.url_map.iter_rules()}`. For a parameterised route that string is
the *template* — `/dashboard/<dashboard_id_or_slug>/`, `/embedded/<uuid>`,
`/api/v1/database/<pk>/schemas/`. A manifest path is always literal, so it can
never be string-equal to one, the guard passes, and Werkzeug then prefers the
*static* rule when matching.
Reproduced with host rules registered first, in the same order
`init_views()` runs before `init_extensions()`:
```
GET /superset/dashboard/sales/ -> 200 extension shell (host dashboard
bypassed)
GET /api/v1/database/1/schemas/ -> 200 extension shell (host rule
bypassed)
GET /api/v1/chart/1 -> 308 -> extension shell
GET /api/v1/chart/2 -> 200 host JSON (other ids
unaffected)
```
The `/<path:subpath>` deep-link route compounds it: a page at
`/api/v1/database/1/` shadows every sub-route for that id.
### 2. The same hole reaches the OAuth callback, and Werkzeug's
slash-redirect carries the query string
`/oauth-authorized/<provider>` is FAB's `AuthOAuthView` callback. A page
declaring `path: "/oauth-authorized/google/"` passes validation (only lowercase
letters and hyphens) and passes the collision guard for the reason above:
```
GET /oauth-authorized/google?code=...&state=abc
-> 308 Location: /oauth-authorized/google/?code=...&state=abc
-> 200 extension shell, with the code in window.location.search
```
The host callback never runs. At minimum this is a one-line install-time
denial of service on authentication for that provider. Whether the
authorization code is also readable by the extension's own bundle depends on
whether the page's permission resolves to something an anonymous caller holds,
which in turn depends on `PUBLIC_ROLE_LIKE`; that path is suspected but not
confirmed end to end.
### 3. Routing the page through `add_view_no_menu` hands an extension
control of a FAB `class_permission_name`
`add_permissions_view` treats the registering view's `base_permissions` as
the complete action list for that resource and deletes any permission on it
that is not in the list. With a host view owning `Database`
(`can_read`/`can_write`/`can_export`) and then a page view declaring `{"index":
"read"}` on the same resource:
```
before: ['can_export', 'can_read', 'can_write']
after: ['can_read', 'can_write'] # can_export deleted
```
`can_write` survived only because a role still referenced it. Separately,
the justification written into `view.py` is wrong: FAB grants a newly created
permission to Admin immediately, and `SupersetSecurityManager._is_gamma_pvm` is
default-allow, so an invented resource name reaches Gamma and Alpha on the next
`superset init` — the opposite of what the docstring claims.
### 4. The client-side route is ungated and never learns about a server-side
refusal
`App.tsx` mounts every registered page unconditionally. The declared
permission gates cold loads only; in-app navigation renders the component
regardless. And all three backend refusals are logged server-side only, so a
page the server refused still mounts in the browser — in-app navigation works,
refresh 404s.
---
### Direction
These are not independent bugs to patch. Three of the four would be
structurally impossible if contributed pages lived under a host-owned namespace
derived from the extension id, rather than claiming free-form top-level paths —
which is also what the comparable systems do, and what
`/api/v1/extensions/<publisher>/<name>/...` already does for extension assets
in this codebase.
I am reworking the design along those lines rather than patching, and will
update this PR when it is ready. Independently of the namespace question, the
collision check should ask the url_map whether a path *matches*
(`MapAdapter.match`, treating `RequestRedirect` as a match) rather than
comparing strings, and the path pattern should be ASCII-only — `str.isalnum()`
is Unicode-aware, so fullwidth and Roman-numeral homoglyphs of host paths are
currently admitted.
Not marking this ready for review until that is done.
--
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]
