Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Dangerous link detected error after upgrading to Dash 2.15.0 #2764

Closed
emilhe opened this issue Feb 18, 2024 · 11 comments · Fixed by #2833
Closed

Dangerous link detected error after upgrading to Dash 2.15.0 #2764

emilhe opened this issue Feb 18, 2024 · 11 comments · Fixed by #2833
Assignees
Labels
bug something broken sev-1 blocker

Comments

@emilhe
Copy link
Contributor

emilhe commented Feb 18, 2024

dash                      2.15.0
dash-bootstrap-components 1.5.0
dash-core-components      2.0.0
dash-extensions           1.0.12
dash-html-components      2.0.0
dash-iconify              0.1.2
dash-mantine-components   0.12.1
dash-table                5.0.0

After upgrading to Dash 2.15.0, I have apps that are now breaking with Dangerous link detected errors, which are emitted when I use Iframe components to display embedded data, e.g. PDF files. Here is a small example,

import base64
import requests
from dash import Dash, html

# Get a sample PDF file.
r = requests.get("https://www.w3.org/WAI/ER/tests/xhtml/testfiles/resources/pdf/dummy.pdf")
bts = r.content
# Encode PDF file as base64 string.
encoded_string = base64.b64encode(bts).decode("ascii")
src = f"data:application/pdf;base64,{encoded_string}"
# Make a small example app.
app = Dash()
app.layout = html.Iframe(id="embedded-pdf", src=src, width="100%", height="100%")

if __name__ == '__main__':
    app.run_server()

I would expect the app would continue to work, displaying the PDF, like it did in previous versions.

I guess the issue is related to the fixing of XSS vulnerabilities as mentioned in #2743 . However, I am not sure why it should be considered a vulnerability to display an embedded PDF file.

@emilhe emilhe changed the title PDF embedded in IFrame throws "Dangerous link detected" error in Dash Dangerous link detected error after upgrading to Dash 2.15.0 Feb 18, 2024
@AnnMarieW
Copy link
Collaborator

@Coding-with-Adam Coding-with-Adam added bug something broken sev-1 blocker labels Feb 20, 2024
@ndrezn
Copy link
Member

ndrezn commented Mar 14, 2024

Maybe for components which had sanitization added to prevent the XSS issue should have a new disable_validation= property which if flagged True, would revert to the previous behaviour, but would default to False with the current behaviour.

I am hesitant to introduce specific exceptions for example only allowing data:, or in essence manually overriding parts of our URL sanitizer.

@AnnMarieW
Copy link
Collaborator

A disable_validation prop would be helpful.

@alexcjohnson had mentioned handling the URL sanitation on the Dash framework level rather than the component level -- then it would apply to all components including custom components or libraries such as dbc and dmc. Is that still on the radar?

@emilhe
Copy link
Contributor Author

emilhe commented Mar 26, 2024

@ndrezn I like the idea of the disable_validation flag with a default value of False. That protects the (vast) majority of users, while maintining the option to (consciously) opt out to enable specific usecases.

@ndrezn
Copy link
Member

ndrezn commented Mar 27, 2024

So to summarize we have a few options to tackle this issue:

  1. Disable validation at the component level
  • After some thought & more conversation with @alexcjohnson, I think this is the wrong solution. It basically puts us back to square one: Given an app with a mechanism to save a layout statefully, a bad actor could add a dcc.Link(disable_validation=...) component to the layout before saving, meaning we're back to not protecting at all against XSS.
  1. Disable validation at the framework level
  • This avoids the issue above, because a bad actor can't change the setting at the app configuration level via the layout, but it means that apps with this setting might be unsafe (in @emilhe's PDF example, now this app is vulnerable because we might want to prevent any other links besides the one with the embedded PDF from working).
  1. More fine-grained validation to avoid warnings for safe links (which often, data: is safe)
  • We might be able to further scan the URL string to check whether to allow it or now. Blanket preventing data: might be overkill, essentially. We're using https://github.com/braintree/sanitize-url as the mechanism to check for bad URLs rather than something custom, which is quite popular, and which wholesale prevents javascript:, vbscript:, and data: URLs. It might be worth checking with the maintainers of this package to see if they have considered more fine-grained control.
  1. Retain current behaviour. Maybe we don't want users using data: at all!
  • We might just restrict the use of data: in Dash apps. Argument being, because our choice of URL sanitizer says "Don't do it!", the Dash framework should follow suit. But maybe we need a more permissive URL sanitizer if we want to ensure data: is supported.

In general the real risk is a specific class of applications (applications which allow you to save a portion of the layout as state, for example https://github.com/plotly/all-in-ai-demo-app -- observe what happens when you click/how "Save this chart" works).

There's also the reasonable point that for the original example, maybe using data: in the first place isn't a great way to e.g. display a PDF or embedded data. But data: also isn't going to be deprecated so we might need to allow it generally.

I'm going to open a ticket in https://github.com/braintree/sanitize-url to see if they're thinking about this problem.

@emilhe
Copy link
Contributor Author

emilhe commented Mar 27, 2024

While I am not expect in web technology, I get the impression looking at the literature that using data: to display media (e.g. PDFs) is not considered bad practice. I am curious; @ndrezn do you consider it bad practice? If yes, do you have an example (a conceptual description, a link to a resource, ...) of good practice realising the same functionality?

I see Dash being used for a great variety of projects, many of which that do not have strict security requirements (e.g. a scientific single-user app running locally). For such users, the proposed change is removing (potentially important) functionality to address concerns (e.g. the save-layout-state example) they'll never face. In general, I consider the ability to adopt standard web technology solutions a key strength of Dash. Hence, removing (some of) this flexibility at framework level (without the ability to opt out) feels like a step in the wrong direction to me - unless there is a really strong argument for doing so.

I of course understand the security concerns, and I appreciate the work being put into addressing them. However, with Dash being such a general framework, I believe that we should strive to keep as much flexibilty as possible. Even if that means enabling users to shoot them self in the foot, if they really want to.

@alexcjohnson
Copy link
Collaborator

The specific type of data: urls I’ve seen as exploitable are text/html, which unfortunately is exactly what we document in plotly/plotly.py#4559. That’s not to say there aren’t others that could be abused, we need to do a little more research. In fact it’s possible browsers have even closed the entire data: loophole, see eg https://security.stackexchange.com/questions/258306/how-is-object-tag-data-uri-xss-actually-xss - if so we can perhaps whitelist that whole protocol.

@ndrezn
Copy link
Member

ndrezn commented Mar 27, 2024

@AnnMarieW pointed me to the interesting perspective from Mozilla: https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-data-urls-firefox-59/

Whereas the following cases will be allowed:

  • User explicitly entering/pasting “data:…” into the address bar
  • Opening all plain text data files
  • Opening “data:image/*” in top-level window, unless it’s “data:image/svg+xml”
  • Opening “data:application/pdf” and “data:application/json”
  • Downloading a data: URL, e.g. ‘save-link-as’ of “data:…”

@BSd3v
Copy link
Contributor

BSd3v commented Apr 10, 2024

Hello all,

Curious as to your thoughts on this as a workaround?

https://community.plotly.com/t/dangerous-link-detected-error-in-dash-debug-window-after-upgrading-from-2-14-2-to-2-15-0/82311/8?u=jinnyzor

Basically, you have a flask template, which is setup to pull the data: back and display it. Flask templates are driven from the server and therefore should be safe to use.

Plus the template is outside the ecosystem of dash. So, it should be safe as far as saving layouts and other props as well.

In this example, the weight falls on the dev to make sure the template is rendering safe and secure files.


from dash import *

app = Dash(__name__)

server = app.server

@server.route('/testfile', methods=['GET'])
def testfile():
    return """<html><body style="width:100%; height:100%;"><iframe style="width:100%; height:100%; border:0;" scrolling="no"
    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=="/>
    </body></html>"""


app.layout = html.Iframe(src='./testfile', style={'height': '100%', 'width': '100%', 'overflow': 'auto'})

app.run(debug=True)

image

@BSd3v
Copy link
Contributor

BSd3v commented Apr 14, 2024

Another possible concern would be what this person used as the workaround.

https://community.plotly.com/t/dangerous-link-detected-error-in-dash-debug-window-after-upgrading-from-2-14-2-to-2-15-0/82311/14?u=jinnyzor

This adjusted the srcDoc. Does this potentially expose the same vulnerability?

@ndrezn
Copy link
Member

ndrezn commented Apr 15, 2024

@T4rk1n -- I think the strategy will be to use Mozilla's heuristic for detecting safe URLs and using that custom strategy rather than sanitize-url. We can plan @ libraries meeting this week but we should get this resolved. Curious if you have other thoughts in advance or other strategies you'd recommend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken sev-1 blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants