Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Do not try to request thumbnails with non-integer widths #3031

Merged

Conversation

spantaleev
Copy link
Contributor

Issue described in element-hq/element-web#9690.

With certain window.devicePixelRatio values
(e.g. 1.5789473684210527), the calculated thumb width/height
would be a non-integer value.

Passing such values to client.mxcUrlToHttp() causes it to
generate URLs to the thumbnail API with non-integer values.
As per the spec, non-integer values are forbidden for that API and a
400 HTTP response is returned (Query parameter b'width' must be an integer).

Fixing matrix-js-sdk's mxcUrlToHttp() to sanitize such values
would also be a good idea and likely fix more than just matrix-react-sdk
and riot-web. Still, it feels like matrix-react-sdk should play nice
as well, and not request thumbnails for weird widths/heights.

Signed-off-by: Slavi Pantaleev [email protected]

Issue described in element-hq/element-web#9690.

With certain `window.devicePixelRatio` values
(e.g.  `1.5789473684210527`), the calculated thumb width/height
would be a non-integer value.

Passing such values to `client.mxcUrlToHttp()` causes it to
generate URLs to the thumbnail API with non-integer values.
As per the spec, non-integer values are forbidden for that API and a
400 HTTP response is returned (`Query parameter b'width' must be an
integer`).

Fixing matrix-js-sdk's `mxcUrlToHttp()` to sanitize such values
would also be a good idea and likely fix more than just matrix-react-sdk
and riot-web. Still, it feels like matrix-react-sdk should play nice
as well, and not request thumbnails for weird widths/heights.

Signed-off-by: Slavi Pantaleev <[email protected]>
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@turt2live turt2live merged commit 6174cd2 into matrix-org:develop May 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants