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

Simple URL sanitization #137

Merged
merged 1 commit into from
Mar 1, 2023
Merged

Simple URL sanitization #137

merged 1 commit into from
Mar 1, 2023

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Feb 21, 2023

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 aiidalab/aiidalab#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.

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
that says "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
aiidalab/aiidalab#329
until the root cause is fixed.
@danielhollas danielhollas marked this pull request as ready for review February 22, 2023 14:20
@unkcpz
Copy link
Member

unkcpz commented Feb 22, 2023

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
Copy link
Contributor Author

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

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Sure, just ask me to review it once you finish.

@danielhollas
Copy link
Contributor Author

danielhollas commented Feb 22, 2023

@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.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

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]?

@danielhollas
Copy link
Contributor Author

danielhollas commented Mar 1, 2023 via email

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

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

From the perspective of the user, yes. I approve it.

@unkcpz unkcpz merged commit 9698cd1 into main Mar 1, 2023
@unkcpz unkcpz deleted the sanitize-external-url branch March 1, 2023 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants