-
Notifications
You must be signed in to change notification settings - Fork 95
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
Sanitize language switching URLs #1307
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1307 +/- ##
============================================
+ Coverage 69.44% 69.45% +0.01%
Complexity 1657 1657
============================================
Files 32 32
Lines 4068 4070 +2
============================================
+ Hits 2825 2827 +2
Misses 1243 1243
Continue to review full report at Codecov.
|
Note that this may restrict the range of available characters in vocabulary IDs. Currently only |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
The last commit changed the sanitizing. Now it's more minimal and only strips colons from the URL, preventing the smuggling of absolute URLs. |
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.
LGTM - better to just break absolute URLs this way (instead of limiting the possibility of using non-alphanumeric characters in vocabulary IDs).
Reasons for creating this PR
See issue #1289 - it was possible to trick Skosmos to generate links to an arbitrary URL. This could be a potential security issue together with some social engineering (instruct users to click on the link).
Link to relevant issue(s), if any
Description of the changes in this PR
The sanitizing will cause the "smuggled" URLs to stop working, for example
http://example.com
becomeshttp//examplecom
and thus the links won't work anymore.Known problems or uncertainties in this PR
The language switching links on the 404 page generated for a page such as
/Skosmos/http://example.com
will be broken. It's no worse than before though, since they were already broken, now they are just less harmful.Checklist