Skip to content
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

Escape urls to avoid xss #11092

Merged
merged 1 commit into from
Aug 29, 2017
Merged

Escape urls to avoid xss #11092

merged 1 commit into from
Aug 29, 2017

Conversation

ceeram
Copy link
Contributor

@ceeram ceeram commented Aug 24, 2017

Escape urls to avoid xss

@ceeram ceeram self-assigned this Aug 24, 2017
@markstory markstory added this to the 3.5.1 milestone Aug 24, 2017
@@ -315,6 +315,9 @@ protected function _formatAttribute($key, $value, $escape = true)
}
$truthy = [1, '1', true, 'true', $key];
$isMinimized = isset($this->_compactAttributes[$key]);
if(!preg_match_all('/\A(\w|[.-])+\z/', $key)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this is supposed to be if ( with a space.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need preg_match_all or would preg_match work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, im no regex hero, if you think preg_match would do, i can change that

@codecov-io
Copy link

codecov-io commented Aug 24, 2017

Codecov Report

Merging #11092 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11092      +/-   ##
============================================
+ Coverage     94.86%   94.92%   +0.06%     
- Complexity    12838    13060     +222     
============================================
  Files           437      437              
  Lines         32733    33062     +329     
============================================
+ Hits          31051    31383     +332     
+ Misses         1682     1679       -3
Impacted Files Coverage Δ Complexity Δ
src/View/Helper/HtmlHelper.php 98.7% <ø> (ø) 131 <0> (ø) ⬇️
src/View/Helper/UrlHelper.php 98.82% <100%> (ø) 36 <0> (ø) ⬇️
src/View/StringTemplate.php 98.95% <100%> (+0.02%) 39 <0> (+1) ⬆️
src/Http/ActionDispatcher.php 100% <0%> (ø) 21% <0%> (+2%) ⬆️
src/Controller/Controller.php 99.53% <0%> (+0.07%) 102% <0%> (+25%) ⬆️
src/Http/ServerRequest.php 99.65% <0%> (+0.16%) 432% <0%> (+194%) ⬆️
src/Cache/CacheRegistry.php 100% <0%> (+4%) 11% <0%> (ø) ⬇️
src/Cache/CacheEngine.php 93.61% <0%> (+4.25%) 19% <0%> (ø) ⬇️

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 fcb8736...0c88f63. Read the comment docs.

@ceeram
Copy link
Contributor Author

ceeram commented Aug 24, 2017

fixed cs issue and replaced preg_match_all with preg_match

@markstory markstory merged commit da84f00 into master Aug 29, 2017
@markstory markstory deleted the html-helper-escape-urls branch August 29, 2017 00:45
@dereuromark
Copy link
Member

This causes a major regression in all HtmlHelper generated img URLs that contain query strings:
https://travis-ci.org/dereuromark/cakephp-geo/builds/270748293?utm_source=github_status&utm_medium=notification
Those URLs have now double encoded query string params:

<img src=".../staticmap?size=300x300&amp;amp;format=png&amp;amp;mobile=false&amp;amp;zoom=13&amp;amp;maptype=roadmap" alt="Karte"/>

We need a 3.5.2 to fix this before too many people update and have their apps completely broken.
Maybe also issue a warning in the 3.5.1 release notes not to upgrade to this and skip to the next patch release instead.

@markstory
Copy link
Member

I think it is relevant to mention that your plugin was HTML encoding URLs before passing them to assetUrl methods in UrlHelper, and that is the source of double encoding.

@dereuromark
Copy link
Member

Indeed. It was properly handling it on its own. I guess people doing similar handling will have to fix their code now to adhere to the new security standard of this patch release.

markstory added a commit that referenced this pull request Nov 15, 2017
Protocol relative URLs were missed from the changes in #11092 as they
are handled by a different code branch.
markstory added a commit that referenced this pull request Nov 15, 2017
Protocol relative URLs were missed from the changes in #11092 as they
are handled by a different code branch.
@cakephp cakephp deleted a comment from lorenzo Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants