Skip to content

Simple URL sanitization#137

Merged
unkcpz merged 1 commit into
mainfrom
sanitize-external-url
Mar 1, 2023
Merged

Simple URL sanitization#137
unkcpz merged 1 commit into
mainfrom
sanitize-external-url

Conversation

@danielhollas

@danielhollas danielhollas commented Feb 21, 2023

Copy link
Copy Markdown
Contributor

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
@danielhollas danielhollas requested a review from unkcpz February 22, 2023 14:20
@unkcpz

unkcpz commented Feb 22, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

@unkcpz unkcpz left a comment

Copy link
Copy Markdown
Member

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

danielhollas commented Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

@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 danielhollas requested a review from unkcpz February 24, 2023 12:52

@unkcpz unkcpz left a comment

Copy link
Copy Markdown
Member

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

danielhollas commented Mar 1, 2023 via email

Copy link
Copy Markdown
Contributor Author

@unkcpz unkcpz left a comment

Copy link
Copy Markdown
Member

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