Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow data urls #2833

Merged
merged 5 commits into from
Apr 22, 2024
Merged

Allow data urls #2833

merged 5 commits into from
Apr 22, 2024

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Apr 16, 2024

  • Allows data urls protocol in links.
  • Add window.dash_clientside.clean_url function to sanitize url, used between html and dcc.
    • Disable the sanitation by overriding clean_url in an asset file: window.dash_clientside.clean_url = (url) => url;

Fixes #2764

@T4rk1n T4rk1n requested a review from emilykl April 16, 2024 15:55
if (url === '') {
return url;
}
const cleaned = url
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to include the decodeHtmlCharacters step as in braintree/sanitize-url?

[
html.Img(
id="image",
src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAYUAAACBCAMAAAAYG1bYAAABC1BMVEX///8oLTMXHiUgJi0AAA4UGyMFERsjKC5MUFQAABFfYmUMFR74+PmbnZ/ExcbU1dWpqqva29wsMTeuoMn3N3ato8v5NHSpqM/yPnyxnMYbISgACxeJ1vTx8vLVaJ3SbaDOcqWH2Pb8MHDwQX6Oz+6E3vrm5+eChIaQkpUAAABCRkvHyMmxkL5ucXQ7P0Sur7G5urz8AGDYX5fS1+lnam1VWFzm4O17fYD10t/55+70krH0X47vf6XqyNrXj7bYg63gudHKwty3r9HQ6Pei1/L2tMjKcaXYz+Pk9PvZeKaWzOzWpcXS5vWp5vv/AGDu6vP0obv3usztZJTNwdvjscvBrM721+LqSISor9QSCNdVAAAJ+klEQVR4nO2ca0PbOBaGYzuyE2SHJNAB7JqytE0MNIm7Qwj0NlPa0p3OzjLTmXbn//+StWNJ1s2hLiH2hvN+aSPLjnweXY6OTmg0QCAQCAQCgUAgEAgEAoFAIBAIBAKBQCAQCAQCgUAgEAgEAoFAIBAIBAKB6q2LFy9fvjqXS1+/+enNz65UeP328vLtmVz13fuH79/JVUGl9OH54Q/b29uvhEL3l3/uP3r06Eq0+Me9va0HD3beilU/PXn48B9Pn/7rzlu6xjqfQ9jd3X7BlyYQ9h8dHBxcXXOFZ89SCjs7IoZPT1IKCQYYDd+v7cMf5hQe717kha8TCBmFX7mqe3uEwg5n8H8/oRR+W1mb104X/2EUPuSlbxiFq7zw+hml8PePeel7RuHp6lq9bkonJELh97z0Fx2FM0Zhh6PwCSjcXl+1Y+GnxWOBp/D+E1C4vQ5168LP2nUhp8Ct2bAuLEN/EB9pV3BV93U+0sfFPhK4qrfQi+eHyXZh+7NQeL2f7hcOrl4LpX8+m+8X/hYK3SfZfuHdnbd0rXX+OWHwh1z618H+1a/XUuHZ5dbW5Y9SofsuwfDb0nYLHc8blL1n4Hkd/RV34k1u26J7qFZgoqO43D3xETKDlu5Kz8YmtkpTve8aIiPRUSm7DZrpPXioudR3kivW8ZIad280N5uBumXu6c7JOX31ymaQXjGaMBhKyZ1bzTC9Mjd55vwmDYUOnl+xN5fVvvshoFAHAYW7Vxhy/w11vu1CCsk9oa6cUhiEmfIHKxT0D2gM7k9U3p2ZuJ/6oD2v1UcYGa2op9QpouDGo7GFsTUeqV4soWAghOdy2N0ShfDUxGN1Y9HtY1/nYK2lWokr4xx1etPAtObGcRIn/0SsU0DBjRIAc+/JsTCSEVEKTEFErkgUxmbaAukrG91m8mR8uqS3rLlOMq/Rsh3OXk57JEwGegonFuKNjCzRkAoFIyATj0ghtudfKe8esjpNZVyuWF8/fFAO/xtuZxKrk+j1x4/K4X9BVVldJNsqM3ifd+a1FLymdI/TFK8rFGjnFylEWb1AXKsnWR1ccr++ZF28TIPbjyUOE99G2JZmy+sve1tbD3YkDl1dVVUFFJI5hsOgoxAF6k1szkmlUqDbNJECaYE1Etp1nI3NQJ6oVir3kJz+Cxi89rxpaCpUpecLAoYoq4o3bviiIgrCpldDYaKBkBiNi9EpFEzaFpHCgDwp4Afupq00ogJ9Jqc8u4+5CbrXJu9j86GEL+zcmSvs0enCviF2KVCwEh/Joh/8fGVUKQxoU5LFHCGTLSpBPoKYj5S5SDiY0leRVueRnz2dH0inWTOUJX+lcvMTTy64HdHexS9lrvbEM/I1VXXiKPjmqBtPRia9NWDeo0qhRWA59rEXx9GxTTnmw5TuF2KifN6XKJB+bzh5qwakCN28sN2hvmpP/zdYP23nhfrT/ynrnc3F35RTsEfZK4czahUWlVYodMgs4jiEVMcibcsn8m/fO7eyxnIrMelv/mxx4+9YN1HgTKun0GIUOGA6MQp23tE9OlNTiyoUyFBw+qyvhlnYlRsM304hxjJ1MhyDav1UN6fAnW5qKXAzErc8l6YgdLuh1BVlCnRVwJwfRSeRJgVTIo5EGsuc1S7BMlXvXalePSer83+5Qi2FxpdnmtW5NAVL2KaRLzLJR5nCJLsJ6fYHaCJ+/hYKpCpzVsdZ6+2C89LVaZfkYHCJMAUUmKfKH0eXpeBHQikxC50RZAqjrCG+sHaGSBxAJSiExKMjHhbZzzvG4qavQO6rdNf2mYdQQKHh/pliuBRMUpaCFGUmbgtdL2UKWV+1pL1I1jxnTD6WiWyLzip5z3JHe3ck9/z8QiwpoJBUPTuTgsBlKWDx9NG1hSlHpmDwNmMaElOSj2Uo0J0QTj8MyMjA9QxsF1JQVZYCEt+YOCmmnoJriVCIiHtJH1XqlGeaPRKniwqhWdew9gopmIsohCIjqttQiEnEYsy+27BrmhywxhRIlkdaRqN7N0XAqtI6U/CI7U+Zm1ppNHWB1plCSHePpEHM1aqd1plCY0aiFuSfWripWtWGwvJ9pMRZFQ8s7Hq6qY07pSDtFxrfs1+ItPsFQzWnPh9pyt7OqK+b2rhTCpJFekvYO9NRph4R6Cl06BnFvNW1cVNduRcVU1CqlqVgloojkZMwMQLoSiE5Eq/GakiuIDfPyNNAauOmxi2ExqJtiijELRONxUm6dEzVESxKDILJR5kC7eZdzZNYIRlPGoMWUOCO/ao99M81CxKTO2jMG6eAwixIbOZgoWppCiY/EUfMec+knC+QlZQPqoZkGcjz4UkT1Ph0AYUQ0zbXxU3t0vfkDzr0FOjJmM93uu84a8s7dhxI9lPO2kieinXMyLtjMkvlP94h8SADyydmRdnC9IYsnFQDscHJJ0vpKbCEE/58sDwFIxhmJnVpplHeIxUKJOxjWH16gt+3lJ5PY6NGIPn+RRR6LIegHm5q7jzz51laClxV7mW/g4JhWsP4JB46FKvNTuPVHAwyGAwnmHY7ne6UJlg6/O/YRqxv96O4k3s9hZnzJGdBdoGrEusVwtZIS2GTVb0thWRSwxj77EM+w6kUcvYOwhjl+Uj87BOyZzvJk4PTgnykvD757rpEUyuiwMvhll5Nbp5n624KRE+t0+YvIurDFlHwJKegalVPwQluyFOdaTAoabFdISzR1OZs5yLR7UCZqSpS5RR8h59aXEel0Bi25ZsCdT6ftLmMfH3Odl6X5L/UxE2tggLirJUMhA0x7pAtxraYxh7na8i8pUiX5b7ZZ7sAo00eSpynpvgdPVKxLm5qBRSszjjws3scP+jL9oxTu/lyZko4a9LkYgs1hwU5pR7C2ZPz+WqEDWX62qT9ANXDTW1UQAElDxqOTWxjczzUhA8mvm1PVTOH3Q0H2zZ2NroLbNeZ9dMnc1afiR+TLx/RFaTaNG1Bq6cwN2I4GAyKsqQHRe5jcs/NnmUoPTjkfr/Z2Rgjm76a06/NUKiKQiXymha3JtUljpfqHlEIhZ/HBfWZj+4VhRPhcKcmsYtMq6NAf0u53PaXEEfBr/gXnbJWRyE7KvArDBrQH8X57dOaxI+oVkehMTkyfdyv8PdjJ23km8i2Z1X/wFzRCik0BtGw2t1q6A2jSV1iR7xWSQFUJKBQBwGFOggo1EFAoQ4CCnXQEincmNIKKpI+vWXEKCBtVc7tP2VV7dW1eu3EzhLb3I5yQg8Pff4vOjF7BzdWBZXThBytm0KAhyQpCGjY34syBXvTBGj4M8q3UdRMs4XxsRBxHljp4azfFiMOw3Za1W4JVXu+Oa9aryjl/51ONuzmsfzjLjfqN/2Rkn27YbeVqmHUP/JnMBJAIBAIBAKBQCAQCAQCgUAgEAgEAoFAIBAIBAKBQLfV/wD2/u2SfXLkQQAAAABJRU5ErkJggg==",
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it would be more readable to commit the image file and then read and base64 encode it inside the test

assert (
element.get_attribute(prop) == "about:blank"
), f"Failed prop: {element_id}.{prop}"
prop_value == "about:blank"
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are inlining the sanitization logic to our codebase, probably makes sense to add a few more tests along the lines of these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added more tests cases, some of the sanitization in that was done in the lib were not valid as dash prop.

@emilykl
Copy link
Contributor

emilykl commented Apr 16, 2024

LGTM at a high level. The workflow of overriding window.dash_clientside.clean_url to disable the sanitization feels a little weird... couldn't we just add another prop to the affected components to disable sanitization? How many places would we want to allow users to disable?

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Apr 16, 2024

LGTM at a high level. The workflow of overriding window.dash_clientside.clean_url to disable the sanitization feels a little weird... couldn't we just add another prop to the affected components to disable sanitization? How many places would we want to allow users to disable?

I just thought it would be convenient to have it available for multiple components library (dcc & html in this case), if a community library want to use the same clean url pattern for it's url it's now available. Overriding the function to disable it is an afterthought pattern that emerged from that, but I think it does well as a global disable feature that would be something only a user that really want to do that.

@ndrezn
Copy link
Member

ndrezn commented Apr 16, 2024

@emilykl re: prop on component level, see: #2764 (comment). We don't want to allow users to disable sanitization at the component level.

@T4rk1n T4rk1n requested a review from emilykl April 19, 2024 17:38
@emilykl
Copy link
Contributor

emilykl commented Apr 19, 2024

Understood re: why we don't want to disable sanitization at the component level. 👍

Copy link
Contributor

@emilykl emilykl left a comment

Choose a reason for hiding this comment

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

🚀

@ndrezn
Copy link
Member

ndrezn commented Apr 19, 2024

One thing I would like to validate before merging. We know this pattern is considered sufficient by Mozilla, but is there any documentation for other browsers (Safari, Chrome) that is in agreement? I started looking but did not find anything. If it exists I imagine it's buried in dev docs somewhere.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Apr 22, 2024

One thing I would like to validate before merging. We know this pattern is considered sufficient by Mozilla, but is there any documentation for other browsers (Safari, Chrome) that is in agreement? I started looking but did not find anything. If it exists I imagine it's buried in dev docs somewhere.

I found this for Safari: https://developer.apple.com/forums/thread/650034?answerId=624675022#624675022
Seems like it was the last one to allow it and no longer does as from 4 years ago.

Copy link
Member

@ndrezn ndrezn left a comment

Choose a reason for hiding this comment

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

💃

@T4rk1n T4rk1n merged commit 9a4a479 into dev Apr 22, 2024
3 checks passed
@T4rk1n T4rk1n deleted the fix/xss-data-url branch April 22, 2024 15:43
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dangerous link detected error after upgrading to Dash 2.15.0
3 participants