Prevent possible XSS via changeDebugUrl #519
Conversation
@snake14 This was a fix for XSS vulnerability report which I have shared in the slack channel you can check that |
@AltamashShaikh Thank you. I see it now. |
API.php
Outdated
| { | ||
| $this->accessValidator->checkWriteCapability($idSite); | ||
| if (!filter_var($url, FILTER_VALIDATE_URL)) { |
There was a problem hiding this comment.
I'm actually wondering if using FILTER_VALIDATE_URL only might cause problems here. It does not check if the protocol is "safe". It could also be ssh:// or similar. So we could additionally check preg_match('/^https?:\/\//'), or do you think other protocols are needed there as well?
There was a problem hiding this comment.
@sgiehl Added stripos($url, 'http') check to ensure valid protocol is present.
There was a problem hiding this comment.
the problem with stripos === false is, that you don't check if it's in the beginning. So something like ssh://http.webhost.com would pass here... So guess you would need to compare with !== 0
There was a problem hiding this comment.
Updated and added few testcases too
API.php
Outdated
| { | ||
| $this->accessValidator->checkWriteCapability($idSite); | ||
| if (!filter_var($url, FILTER_VALIDATE_URL) || stripos($url, 'http') !== 0) { |
There was a problem hiding this comment.
fyi @AltamashShaikh does it make sense to also look at UrlHelper::isLookLikeSafeurl("") here additionally? it includes a few additional checks. There's also isLookLikeurl("") which may be interesting too not sure.
There was a problem hiding this comment.
@tsteur Ideally the 'stripos($url, 'http') === 0' should fix most of the cases but adding both the methods makes no harm.
There was a problem hiding this comment.
👍 that's what I thought. Better be safe. Also in the future someone might add more changes to core and we won't remember to add them here as well meaning then we're covered.
Description:
Changes to fix possible XSS via changeDebugUrl
Review