Skip to content

Related Posts: decode ellipsis at the end of a long excerpt#1282

Merged
xyu merged 3 commits intoAutomattic:masterfrom
jeherve:fix/related-posts-excerpts-ellipsis
May 22, 2015
Merged

Related Posts: decode ellipsis at the end of a long excerpt#1282
xyu merged 3 commits intoAutomattic:masterfrom
jeherve:fix/related-posts-excerpts-ellipsis

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented Nov 8, 2014

screen shot 2014-11-08 at 5 00 59 pm

Reported here:
https://wordpress.org/support/topic/related-posts-hellip-typo-on-hover?replies=3

@xyu My patch works, but I wonder if there would be a better way of doing it, and if my patch could cause things to break, somehow?

@jeherve jeherve added [Feature] Related Posts Bug When a feature is broken and / or not performing as intended labels Nov 8, 2014
@jeherve jeherve added this to the 3.3 milestone Nov 8, 2014
@xyu
Copy link
Copy Markdown
Member

xyu commented Nov 8, 2014

The only thing I'm concerned here is just decoding like this may end up opening up some XSS vulnerabilities. I need to think about this a bit.

Sent from my iPhone

On Nov 8, 2014, at 11:01, Jeremy Herve notifications@github.com wrote:

Reported here:
https://wordpress.org/support/topic/related-posts-hellip-typo-on-hover?replies=3

@xyu My patch works, but I wonder if there would be a better way of doing it, and if my patch could cause things to break, somehow?

You can merge this Pull Request by running

git pull https://github.com/jeherve/jetpack fix/related-posts-excerpts-ellipsis
Or view, comment on, or merge it at:

#1282

Commit Summary

Related Posts: decode ellipsis at the end of a long excerpt
File Changes

M modules/related-posts/jetpack-related-posts.php (2)
Patch Links:

https://github.com/Automattic/jetpack/pull/1282.patch
https://github.com/Automattic/jetpack/pull/1282.diff
—
Reply to this email directly or view it on GitHub.

xyu added a commit to xyu/jetpack that referenced this pull request Dec 2, 2014
@xyu
Related Posts: Bring UI in line with WordPress.com version
825ff4c 
Some niceties / UI tweaks for related posts:

* Limit excerpts to 5 lines max (resolves Automattic#1249)
* Use UTF-8 ellipsis to prevent double encoding issue (Automattic#1282)
* Make entire excerpt clickable
@xyu xyu mentioned this pull request Dec 2, 2014
Merged
@lezama lezama modified the milestones: 3.4, 3.3 Dec 4, 2014
@jeherve jeherve closed this Jan 13, 2015
@jeherve jeherve reopened this Jan 13, 2015
@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Jan 13, 2015

@xyu bd1241da924f791f90227bb74191cfc765f05e6f works nicely the ellipsis at the end of the excerpt, but the problem remains for other special characters in the excerpt:

screen shot 2015-01-13 at 3 02 53 pm

My Pull Request fixes the problem, but as you mentioned:

Decoding like this may end up opening up some XSS vulnerabilities

Can you think of another solution?

Reported here:
https://wordpress.org/support/topic/related-posts-hellip-typo-on-hover?replies=14#post-6426194

@samhotchkiss samhotchkiss modified the milestones: 3.5, 3.4, vFuture Jan 28, 2015
@samhotchkiss samhotchkiss added the [Status] Needs Review This PR is ready for review. label Apr 13, 2015
@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Apr 24, 2015

@xyu Do you think we could merge this in the next release?

@jeherve jeherve modified the milestones: 3.5.1, vFuture Apr 24, 2015
@samhotchkiss samhotchkiss modified the milestones: 3.5.1, 3.6 Apr 29, 2015
@dereksmart dereksmart removed the [Status] Needs Review This PR is ready for review. label May 13, 2015
@xyu
Related Posts: Make sure we escape the excerpt
90bd54f 
fa0ba20 started calling `html_entity_decode()` on the excerpt so we should
escape it when outputing to avoid potential XSS.
@xyu
Copy link
Copy Markdown
Member

xyu commented May 21, 2015

I'm probably doing it wrong but...

I just made a PR for this change at: jeherve#1, basically we should escape on output now that we are calling html_entity_decode() on the server side.

@jeherve
Merge pull request #1 from xyu/PR1282
129d3e0 
Related Posts: Make sure we escape the excerpt
@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented May 21, 2015

@xyu That looks good to me!

@jeherve jeherve added the [Status] Needs Review This PR is ready for review. label May 21, 2015
xyu added a commit that referenced this pull request May 22, 2015
@xyu
Merge pull request #1282 from jeherve/fix/related-posts-excerpts-elli…
c4dca47 
…psis

Related Posts: decode ellipsis at the end of a long excerpt
@xyu xyu merged commit c4dca47 into Automattic:master May 22, 2015
@xyu xyu added [Status] In Progress [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] In Progress [Status] Ready to Merge Go ahead, you can push that green button! labels May 22, 2015
@jeherve jeherve deleted the fix/related-posts-excerpts-ellipsis branch June 1, 2015 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@xyu xyu

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Related Posts

Projects

None yet

Milestone

3.6

Development

Successfully merging this pull request may close these issues.

6 participants

@jeherve @xyu @lezama @samhotchkiss @georgestephanis @dereksmart