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

Proper output escaping #4

Closed
boonebgorges opened this issue Feb 22, 2016 · 6 comments
Closed

Proper output escaping #4

boonebgorges opened this issue Feb 22, 2016 · 6 comments

Comments

@boonebgorges
Copy link
Member

I noticed a number of places where user-generated content - like group names - is displayed on the front end, without being escaped. Let's fix this. Rules of thumb:

  • use esc_html() for text that's echoed in such a way that it's displayed as the content of a HTML element (eg: <h2><?php echo esc_html( $group->name ); ?></h2>)
  • use esc_attr() for text that needs to be sanitized for use in element attributes (eg <input name="foo" value="<?php echo esc_attr( $group->slug ); ?>">)
  • use esc_url() for URLs, pretty much whenever they need to be echoed to the screen (there are exceptions where it's better to use esc_url_raw() - see https://kovshenin.com/2012/esc_url-vs-esc_url_raw/)
@rjbaniel
Copy link
Contributor

Addressed here: 6eed651

@boonebgorges
Copy link
Member Author

Looking good

@modelm
Copy link
Contributor

modelm commented Dec 30, 2016

Because of this change 6eed651#diff-510c3663aa41187468817522a3a7c765R123 - when markup is in the content or body of a post, it is escaped in the duplicate posts resulting in this:

screen shot 2016-12-30 at 12 12 42 pm
screen shot 2016-12-30 at 12 12 37 pm

Is there a reason the escaping is necessary here or is this a bug?

@boonebgorges
Copy link
Member Author

Hi @modelm - Thanks for the ticket. It looks like a bug; I'd noted in that changeset that content should not be output-escaped on the way into the database, but it looks like it was never rolled out. See https://github.com/cuny-academic-commons/bp-multiple-forum-post/blob/master/bp-multiple-forum-post.php#L142

@rjbaniel Could you have a look? Be sure to test with rich-text content.

@modelm
Copy link
Contributor

modelm commented Dec 30, 2016

OK - I have already removed the esc_attr() for title & content in our fork, so if that's the only required change and you want a PR just let me know.

@boonebgorges
Copy link
Member Author

boonebgorges commented Dec 30, 2016 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants