Skip to content

Sanitize language switching URLs#1307

Merged
osma merged 4 commits intomasterfrom
issue1289-sanitize-lang-url
Apr 26, 2022
Merged

Sanitize language switching URLs#1307
osma merged 4 commits intomasterfrom
issue1289-sanitize-lang-url

Conversation

@osma
Copy link
Member

@osma osma commented Apr 26, 2022

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

  1. add some more unit tests for Request->getLangUrl method (to make sure all relevant cases are covered)
  2. sanitize the URL string returned by Request->getLangUrl so it won't contain special characters

The sanitizing will cause the "smuggled" URLs to stop working, for example http://example.com becomes http//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

  • phpUnit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if not, explain why below)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@osma osma added the bug label Apr 26, 2022
@osma osma added this to the 2.15 milestone Apr 26, 2022
@osma osma requested a review from joelit April 26, 2022 11:16
@osma osma self-assigned this Apr 26, 2022
@osma osma changed the title Issue1289 sanitize lang url Sanitize language switching URLs Apr 26, 2022
@codecov
Copy link

codecov bot commented Apr 26, 2022 •
edited
Loading

Codecov Report

Merging #1307 (fe9e356) into master (9b3aa64) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             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
Impacted Files Coverage Δ
model/Request.php 71.91% <100.00%> (+0.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b3aa64...fe9e356. Read the comment docs.

@osma
Copy link
Member Author

osma commented Apr 26, 2022

Note that this may restrict the range of available characters in vocabulary IDs. Currently only [a-zA-Z0-9/-] are allowed.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@osma
Copy link
Member Author

osma commented Apr 26, 2022

The last commit changed the sanitizing. Now it's more minimal and only strips colons from the URL, preventing the smuggling of absolute URLs. http://example.com becomes http//example.com. This has a minimal effect on vocabulary IDs since even before, it was in practice impossible to use colons in vocabulary IDs, they wouldn't work anyway.

joelit
joelit approved these changes Apr 26, 2022
View reviewed changes
Copy link
Contributor

@joelit joelit left a 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).

@osma osma merged commit e6ec026 into master Apr 26, 2022
@osma osma deleted the issue1289-sanitize-lang-url branch April 26, 2022 12:16
@Vainonen Vainonen mentioned this pull request Oct 31, 2023
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelit joelit joelit approved these changes

Assignees

@osma osma

Labels

bug

Projects

None yet

Milestone

2.15

Development

Successfully merging this pull request may close these issues.

YSO concept pages broken

2 participants

@osma @joelit