aiidalab / aiidalab-home

AiiDAlab Home App
MIT License
5 stars 9 forks source link

Simple URL sanitization #137

Closed danielhollas closed 1 year ago

danielhollas commented 1 year ago

Currently, if the app URL is not found in app metadata, the app.url variable (that we render on the single app page) is not None, but instead it is a string

"Field 'external_url' not present in app metadata".

When this string is rendered as a href target, it results in an invalid link leading to a 404 page.

Here's we just add a simple validation to check that the URL starts with http.

This is a hotfix solution to https://github.com/aiidalab/aiidalab/issues/329 until the root cause is fixed. Note that this bug affects AWB and a bunch of other apps. I still did not find out why it affects some apps and not the others.

unkcpz commented 1 year ago

The root comes that in the registry https://github.com/aiidalab/aiidalab-registry/blob/master/apps.yaml the metadata is specifically defined rather than read from the repo. I don't think we need to do anything in aiidalab or aiidalab-home but simply update the registry. For his PR itself, I think it is not necessary to check the URL, it can be other protocol, although very unlikely.

danielhollas commented 1 year ago

Well, I I think it a good practice to sanitize the URLs anyway. I'll see if I can do something more robust.

danielhollas commented 1 year ago

@unkcpz I wrote the following validation function using the urllib urlparse

def is_valid_app_url(url):
    allowed_schemes = ("http", "https")
    try:
        # https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlparse
        if urlparse(url).scheme in allowed_schemes:
            return True
    except Exception:
        return False
    else:
        return False

However, it looks like one cannot use external functions like this inside Jinja templates (at least not easily). I am sure this could be circumvented with more code, but I would prefer to stick with the current super-simple check. Anyways I do not think we want to link to anything else other than http or https protocols.

danielhollas commented 1 year ago

I'd prefer to just hide it tbh to keep the UI clean.

Dne st 1. 3. 2023 9:04 dop. uživatel Jusong Yu @.***> napsal:

@.**** requested changes on this pull request.

Okay, I think check the http is no problem. Do you think it is good to add an else with URL: [Not detect in app's metadata]?

— Reply to this email directly, view it on GitHub https://github.com/aiidalab/aiidalab-home/pull/137#pullrequestreview-1319343915, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACIY64LPCIO2RLZ4F4VK3ODWZ4GLRANCNFSM6AAAAAAVCZDI2M . You are receiving this because you authored the thread.Message ID: @.***>