-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix(link): allow all non-whitespace chars in urls #757
Conversation
LGTM but does this comply with the back? Currently it's that:
Not exactly sure what it includes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dialexo Do you think we would expose to possible security threats by allowing all non-whitespace characters in urls ?
From the front-end POV I can at least think of social-engineered attacks similar to https://medium.com/@bobbyrsec/the-dangers-of-googles-zip-tld-5e1e675e59a5 but it's debatable whether it's an actual issue in Graasp. IMO all weaknesses can be exploited, so we should consider it a security risk.
Since the method should return a boolean, can you use the native URL
object to perform validation? See https://snyk.io/blog/secure-javascript-url-validation/. If you want to restrict the URL to specific regexes, I think you should do both (however have a look at the regex used in the above article). Note that the URL
method also only works against these attacks if you manually drop the undesirable bits.
In the next example, you should drop username
and password
, this however means that we'd then never allow URLs that contain usernames and passwords.
> new URL("https://github%2Ecom%E2%88%95kubernetes%E2%88%95kubernetes%E2%88%95archive%E2%88%95refs%E2%88%95tags%E2%88%[email protected]/")
URL {
href: 'https://github%2Ecom%E2%88%95kubernetes%E2%88%95kubernetes%E2%88%95archive%E2%88%95refs%E2%88%95tags%E2%88%[email protected]/',
origin: 'https://v1271.zip',
protocol: 'https:',
username: 'github%2Ecom%E2%88%95kubernetes%E2%88%95kubernetes%E2%88%95archive%E2%88%95refs%E2%88%95tags%E2%88%95',
password: '',
host: 'v1271.zip',
hostname: 'v1271.zip',
port: '',
pathname: '/',
search: '',
searchParams: URLSearchParams {},
hash: ''
}
>
Another example (not sure if this is still relevant though especially on Chrome) is using the javascript:
URL construct to perform arbitrary XSS. One could craft an URL which executes the passed JS code, but still matches the regex. See https://stackoverflow.com/questions/4163879/call-javascript-function-from-url-address-bar
Lastly if you replicate the change in the server you are also opening up to more possibilities in case another injection vulnerability is present further down the server request. For instance, a server which would extract the path or query string part of the URL to use it in some command (e.g. CLI or SQL) without proper validation / sanitization would allow in this regex passing arbitrary SQL or CLI operators (such as ; echo "pwned"
). So defense in depth would tell us to not accept such URLs.
So the bottom line is I think, if you only make the change on the front-end, use the URL
constructor validation (which should take care of acceptable characters in each respective part) and drop what's not desirable.
Should this be included in your PR? #248 |
@dialexo @spaenleh what's the state of this? |
c09cff4
to
1af3d0f
Compare
This PR proposes to simplify the validation of urls to allow some urls that currently fail. See #756
Test urls
@dialexo Do you think we would expose to possible security threats by allowing all non-whitespace characters in urls ?