Simple URL sanitization#137
Conversation
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.
|
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. |
|
Well, I I think it a good practice to sanitize the URLs anyway. I'll see if I can do something more robust. |
unkcpz
left a comment
There was a problem hiding this comment.
Sure, just ask me to review it once you finish.
|
@unkcpz I wrote the following validation function using the urllib 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 FalseHowever, it looks like one cannot use external functions like this inside Jinja templates (at least not easily). |
unkcpz
left a comment
There was a problem hiding this comment.
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]?
|
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
<#137 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACIY64LPCIO2RLZ4F4VK3ODWZ4GLRANCNFSM6AAAAAAVCZDI2M>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
unkcpz
left a comment
There was a problem hiding this comment.
I'd prefer to just hide it tbh to keep the UI clean.
From the perspective of the user, yes. I approve it.
Currently, if the app URL is not found in app metadata, the
app.urlvariable (that we render on the single app page) is not None, but instead it is a stringWhen 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.