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

fix post_link, asset_link when title contains <,>," charaters #3704

Merged
merged 6 commits into from Sep 21, 2019

Conversation

@dailyrandomphoto
Copy link
Contributor

dailyrandomphoto commented Sep 6, 2019

What does it do?

When the title contains <,>," characters, it will cause some errors on browsers.

e.g.
the titles are

  • this is a title with <a tag>.
  • this is a title with " class="badname
- {% post_link test1 %}
- {% post_link test2 %}

generates to

<li><a href="/posts/test1/" title="this is a title with <a tag>.">this is a title with </a><a tag="">.</a></li>
<li><a href="/posts/test2/" title="this is a title with " class="badname">this is a title with " class="badname</a>
</li>

hexo-2019090602

Solution:

use util.escapeHTML function escape the titles.

how to use post_link with escape option:

/**
 * Post link tag
 *
 * Syntax:
 *   {% post_link slug [title] [escape] %}
 */
escape = true or omit; // escape
escape = false; // do not escape

e.g.

# escape title
- {% post_link test1 %}
- {% post_link test1 '"special" custom <title>' %}
- {% post_link test1 true %}
- {% post_link test1 '"special" custom <title>' true %}

# do not escape title
- {% post_link test2 false %}
- {% post_link test2 '<b>bold custom title</b>' false %}

How to test

git clone -b fix-post_link-title https://github.com/dailyrandomphoto/hexo.git
cd hexo
npm install
npm test

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 6, 2019

Coverage Status

Coverage increased (+0.009%) to 97.233% when pulling f6318c3 on dailyrandomphoto:fix-post_link-title into d403b70 on hexojs:master.

lib/plugins/tag/asset_link.js Outdated Show resolved Hide resolved
@@ -44,6 +44,10 @@ describe('asset_link', () => {
assetLink('bar Hello world').should.eql('<a href="/foo/bar" title="Hello world">Hello world</a>');
});

it('title with tag', () => {

This comment has been minimized.

Copy link
@curbengh

curbengh Sep 10, 2019

Contributor

suggest should escape tag in title.

test/scripts/tags/post_link.js Outdated Show resolved Hide resolved
test/scripts/tags/post_link.js Outdated Show resolved Hide resolved
@curbengh curbengh added this to the v4.0.0 milestone Sep 10, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Sep 10, 2019

Just realized there is a possible breaking change with ejs theme layout, i.e. when user wants to markup the title, like

---
title: This is a <b>Bold</b> statement
---

, so need to display it in unescaped form. <%- post.title %>.

I suggest to escape in post_link tag plugin instead.

@curbengh curbengh removed this from the v4.0.0 milestone Sep 10, 2019
@dailyrandomphoto

This comment has been minimized.

Copy link
Contributor Author

dailyrandomphoto commented Sep 10, 2019

Just realized there is a possible breaking change with ejs theme layout, i.e. when user wants to markup the title, like

---
title: This is a <b>Bold</b> statement
---

, so need to display it in unescaped form. <%- post.title %>.

I suggest to escape in post_link tag plugin instead.

@curbengh post_link is a tag using in the Markup. ({% post_link test1 %})
To escape post_link tag, <%= post_link test1 %> or {%= post_link test1 %} does not work.

And, as I know most themes escape titles by default now. <%= post.title %>.
Users can't use markuped title without modify the themes.

@dailyrandomphoto dailyrandomphoto requested a review from curbengh Sep 10, 2019
@dailyrandomphoto

This comment has been minimized.

Copy link
Contributor Author

dailyrandomphoto commented Sep 10, 2019

@curbengh I have refactored this feature.
Keep the behavior of the original code,
use escape option to escape title.

how to use post_link with escape option:

/**
 * Post link tag
 *
 * Syntax:
 *   {% post_link slug [escape] [title] %}
 */
escape = true; // escape
escape = false or omit; // do not escape

e.g.

# escape title
- {% post_link test1 true %}
- {% post_link test1 true custom title %}

# do not escape title
- {% post_link test2 false %}
- {% post_link test2 false custom title %}
- {% post_link test2 %}
- {% post_link test2 custom title %}
@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Sep 11, 2019

what I meant was,

return `<a href="${ctx.config.root}${post.path}" title="${title}">${title}</a>`;

becomes

return `<a href="${ctx.config.root}${post.path}" title="${escapeHTML(title)}">${escapeHTML(title)}</a>`;

And, as I know most themes escape titles by default now. <%= post.title %>.

you're right. As such, having escape option is not necessary.
I agree with you that title should be escaped by default; have you tested whether <%= post.title %> will double escape (most probably not, just want to make sure)?

@dailyrandomphoto

This comment has been minimized.

Copy link
Contributor Author

dailyrandomphoto commented Sep 11, 2019

@curbengh

have you tested whether <%= post.title %> will double escape (most probably not, just want to make sure)?

Yes, <%= post.title %> will double escape title when post.title was escaped before in some filter or somewhere else.
e.g.

<b>Bold</b>
=>
&lt;b&gt;Bold&lt;&#x2F;b&gt;
=>
&amp;lt;b&amp;gt;Bold&amp;lt;&amp;#x2F;b&amp;gt;

Bold
=>
<b>Bold</b>
=>
&lt;b&gt;Bold&lt;&#x2F;b&gt;

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Sep 11, 2019

I see, then we can only escape in tag plugins. Another tag plugin that also need to escape is {% asset_img %}, but that can be done https://github.com/hexojs/hexo-util/blob/master/lib/html_tag.js (PR welcome 😄).

I suggest to remove escape option, I believe title should always be escaped (in the tag plugins).

Edit: text can be in unescape form, but value (of an attribute) should always be escaped. Refer to my comment below.

if (escape === 'true') {
attrTitle = title = escapeHTML(title);
} else {
attrTitle = escapeHTML(title);

This comment has been minimized.

Copy link
@curbengh

curbengh Sep 16, 2019

Contributor

In retrospect, I think it's fine to disable escaping the title, but attrTitle should always be escaped.

@curbengh curbengh added this to the v4.0.0 milestone Sep 16, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Sep 16, 2019

@dailyrandomphoto I would like to merge this as part of hexo v4, consider this PR high priority.

@dailyrandomphoto

This comment has been minimized.

Copy link
Contributor Author

dailyrandomphoto commented Sep 16, 2019

@curbengh
Instead of removing the escape option, I have changed default value of escape option to true.

Is there anything else I need to fix?

how to use post_link with escape option:

/**
 * Post link tag
 *
 * Syntax:
 *   {% post_link slug [title] [escape] %}
 */
escape = true or omit; // escape
escape = false; // do not escape

e.g.

# escape title
- {% post_link test1 %}
- {% post_link test1 '"special" custom <title>' %}
- {% post_link test1 true %}
- {% post_link test1 '"special" custom <title>' true %}

# do not escape title
- {% post_link test2 false %}
- {% post_link test2 '<b>bold custom title</b>' false %}

edit: escape option is the last parameter.


/**
* Asset link tag
*
* Syntax:
* {% asset_link slug [title] %}
* {% asset_link slug [escape] [title] %}

This comment has been minimized.

Copy link
@curbengh

curbengh Sep 17, 2019

Contributor

try to put (if possible, I'm not sure whether it would complicate things) escape as the last since it's a new parameter. applies to post.link (if viable).

let title = args.length ? args.join(' ') : asset.slug;
let attrTitle;
if (escape === 'true') {
attrTitle = title = escapeHTML(title);

This comment has been minimized.

Copy link
@curbengh

curbengh Sep 17, 2019

Contributor

if (escape === 'true') title = escapeHTML(title);, no need else.


return `<a href="${url.resolve(ctx.config.root, asset.path)}" title="${title}">${title}</a>`;
let title = args.length ? args.join(' ') : asset.slug;
let attrTitle;

This comment has been minimized.

Copy link
@curbengh

curbengh Sep 17, 2019

Contributor

const attrTitle = escapeHTML(title);

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Sep 17, 2019

Instead of removing the escape option, I have changed default value of escape option to true.

Thanks for your prompt respond.

@dailyrandomphoto

This comment has been minimized.

Copy link
Contributor Author

dailyrandomphoto commented Sep 17, 2019

@curbengh
the escape option is the last parameter now.

how to use post_link with escape option:

/**
 * Post link tag
 *
 * Syntax:
 *   {% post_link slug [title] [escape] %}
 */
escape = true or omit; // escape
escape = false; // do not escape

e.g.

# escape title
- {% post_link test1 %}
- {% post_link test1 '"special" custom <title>' %}
- {% post_link test1 true %}
- {% post_link test1 '"special" custom <title>' true %}

# do not escape title
- {% post_link test2 false %}
- {% post_link test2 '<b>bold custom title</b>' false %}
@curbengh curbengh mentioned this pull request Sep 21, 2019
@curbengh curbengh merged commit 53ebe22 into hexojs:master Sep 21, 2019
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.009%) to 97.233%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.