kubeflow / dashboard

Kubeflow Central Dashboard is the web interface for Kubeflow
Apache License 2.0
3 stars 2 forks source link

Kubeflow centraldashboard double escapes characters in iframe links #34

Open timcharper opened 2 years ago

timcharper commented 2 years ago

/kind bug

What steps did you take and what happened: I've embedded mlflow in the kubeflow central dashboard. Certain links, like metrics, include quotation characters. When I click the link, the resulting location gets redirected with the quotes mangled and double escaped, first to %22, then to %2522. The nonsense URL is unparseable and causes mlflow to break.

What did you expect to happen: I expect characters in URLs to not be double escaped

Anything else you would like to add: The problem fundamentally stems from iron-location's assumptions that it's hash property is already decoded. In the defined getter, we see that it takes care to decode it:

https://github.com/PolymerElements/iron-location/blob/8bdb632adbdf5de8d16e33ef55d1c732ea373750/iron-location.js#L89

However, in this kubeflow function _iframePageChanged, we pull the window location raw without decoding it:

https://github.com/kubeflow/kubeflow/blob/9b6076d7b11bbae2a5de1eeb3eee7d36741f23aa/components/centraldashboard/public/components/main-page.js#L348

This value eventually gets bound to App-Location:

https://github.com/kubeflow/kubeflow/blob/9b6076d7b11bbae2a5de1eeb3eee7d36741f23aa/components/centraldashboard/public/components/main-page.pug#L13

Which consequently gets bound to Iron-Location:

https://github.com/PolymerElements/app-route/blob/1a5e93ead6c7f0cfb1971a1abc322a49a649b687/app-location.js#L77

In the end, we get an encoded hash value in iron-location, and then iron-location re-encodes it, again, leading to the double-escaped URL character:

https://github.com/PolymerElements/iron-location/blob/8bdb632adbdf5de8d16e33ef55d1c732ea373750/iron-location.js#L223

To fix this, we need to take care to not leak in encoded url values to Polymer App-Location / Iron-Location Components. Putting a window.decodeURIComponent around the hash location extraction in _iframePageChanged should resolve the issue.

Environment:

juliusvonkohout commented 1 year ago

/close

There has been no activity for a long time. Please reopen if necessary. Please also consult the Kubeflow slack channel for support questions.

google-oss-prow[bot] commented 1 year ago

@juliusvonkohout: Closing this issue.

In response to [this](https://github.com/kubeflow/dashboard/issues/34): >/close > >There has been no activity for a long time. Please reopen if necessary. >Please also consult the Kubeflow slack channel for support questions. Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
thesuperzapper commented 6 months ago

There is an open PR https://github.com/kubeflow/kubeflow/pull/6564

google-oss-prow[bot] commented 1 week ago

@timcharper: The label(s) kind/bug cannot be applied, because the repository doesn't have them.

In response to [this](https://github.com/kubeflow/dashboard/issues/34): >/kind bug > >**What steps did you take and what happened:** >I've embedded mlflow in the kubeflow central dashboard. Certain links, like metrics, include quotation characters. When I click the link, the resulting location gets redirected with the quotes mangled and double escaped, first to %22, then to %2522. The nonsense URL is unparseable and causes mlflow to break. > >**What did you expect to happen:** >I expect characters in URLs to not be double escaped > >**Anything else you would like to add:** >The problem fundamentally stems from iron-location's assumptions that it's hash property is already decoded. In the defined getter, we see that it takes care to decode it: > >https://github.com/PolymerElements/iron-location/blob/8bdb632adbdf5de8d16e33ef55d1c732ea373750/iron-location.js#L89 > >However, in this kubeflow function `_iframePageChanged`, we pull the window location raw without decoding it: > >https://github.com/kubeflow/kubeflow/blob/9b6076d7b11bbae2a5de1eeb3eee7d36741f23aa/components/centraldashboard/public/components/main-page.js#L348 > >This value eventually gets bound to App-Location: > >https://github.com/kubeflow/kubeflow/blob/9b6076d7b11bbae2a5de1eeb3eee7d36741f23aa/components/centraldashboard/public/components/main-page.pug#L13 > >Which consequently gets bound to Iron-Location: > >https://github.com/PolymerElements/app-route/blob/1a5e93ead6c7f0cfb1971a1abc322a49a649b687/app-location.js#L77 > >In the end, we get an encoded `hash` value in iron-location, and then iron-location re-encodes it, again, leading to the double-escaped URL character: > >https://github.com/PolymerElements/iron-location/blob/8bdb632adbdf5de8d16e33ef55d1c732ea373750/iron-location.js#L223 > >To fix this, we need to take care to not leak in encoded url values to Polymer App-Location / Iron-Location Components. Putting a `window.decodeURIComponent` around the hash location extraction in `_iframePageChanged` should resolve the issue. > >**Environment:** > >- Kubeflow version: (version number can be found at the bottom left corner of the Kubeflow dashboard): 1.25.1 >- kfctl version: (use `kfctl version`): ? >- Kubernetes platform: kind >- Kubernetes version: (use `kubectl version`): 1.23 >- OS (e.g. from `/etc/os-release`): Ubuntu 22.04 > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
andreyvelich commented 1 week ago

/transfer dashboard