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: external link ignore mailto: & javascript: #3812

Merged
merged 4 commits into from
Oct 28, 2019

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Oct 27, 2019

Close #3796

What does it do?

Fix #3796 (comment)

How to test

git clone -b external-link-exclude https://github.com/sukkaw/hexo.git
cd hexo
npm install
npm test

Screenshots

Pull request tasks

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

@SukkaW SukkaW requested a review from curbengh October 27, 2019 13:26
@coveralls
Copy link

coveralls commented Oct 27, 2019

Coverage Status

Coverage increased (+0.008%) to 97.272% when pulling ec465a5 on SukkaW:external-link-exclude into 4bc90de on hexojs:master.

@@ -37,7 +37,7 @@ function externalLinkFilter(data) {
config.external_link.field !== 'post') return;

data.content = data.content.replace(/<a.*?(href=['"](.*?)['"]).*?>/gi, (str, hrefStr, href) => {
if (/target=/gi.test(str) || !isExternal(href, config)) return str;
if (/target=/gi.test(str) || href.startsWith('mailto:') || href.startsWith('javascript:') || !isExternal(href, config)) return str;
Copy link
Contributor

Choose a reason for hiding this comment

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

there are countless of types (data:, vbscript: and so on), can test whether it's a link protocol (http/ftp) or not by testing whether URL(href).origin is null. See encodeURL().

Copy link
Member Author

@SukkaW SukkaW Oct 28, 2019

Choose a reason for hiding this comment

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

OK, I will change to that.


It required WHATWG URL API. I might mitgate external_link filter to use it in this PR.

@SukkaW SukkaW requested a review from curbengh October 28, 2019 05:14
@SukkaW SukkaW force-pushed the external-link-exclude branch from 98fd59a to ab23935 Compare October 28, 2019 05:15
Copy link
Contributor

@curbengh curbengh left a comment

Choose a reason for hiding this comment

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

need to handle some oddity in whatwg.


if (!data.protocol || !sitehost) return false;
if (!data.protocol || !sitehost || !data.origin) return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

when there is no data.origin, it returns a string value of 'null', not null type.

!data.protocol should be removed, because it's not null even in javascript:foobar.

const url = new URL('javascript:foobar')
console.log(url.protocol)
// 'javascript:'

Copy link
Contributor

Choose a reason for hiding this comment

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

data variable will be string type if url is internal link.

you need to check the type of data first, otherwise data.origin will cause error if internal link.

if (!sitehost || typeof data === 'string') return false;
if (data.origin === 'null') return false;

const host = data.hostname;
const sitehost = parse(config.url).hostname || config.url;
const sitehost = urlObj(config.url).hostname || config.url;
Copy link
Contributor

Choose a reason for hiding this comment

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

urlObj(config.url).hostname would throw error if config.url doesn't have http.

To handle that case,

const sitehost = typeof urlObj(config.url) === 'object' ? urlObj(config.url).hostname : config.url;

Apply code suggestions from code review by @curbengh
@SukkaW SukkaW requested a review from curbengh October 28, 2019 06:29
@curbengh curbengh added this to the v4.1.0 milestone Oct 28, 2019
@@ -208,23 +208,27 @@ describe('External link - post', () => {
'# External link test',
'1. External link',
'<a href="https://hexo.io/">Hexo</a>',
'2. External link with "rel" Attribute',
'2. Link with hash (#), mailto: , javascript: shouldn\'t be processed',
'<a href="#top">Hexo</a>',
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@SukkaW SukkaW merged commit 91a4784 into hexojs:master Oct 28, 2019
@SukkaW
Copy link
Member Author

SukkaW commented Oct 28, 2019

I might implement is_external_link() helper in another PR, so that isExternal() can be used across both filters.

1v9 referenced this pull request in theme-next/hexo-theme-next Oct 30, 2019
curbengh added a commit to curbengh/hexo that referenced this pull request Nov 5, 2019
curbengh added a commit to curbengh/hexo that referenced this pull request Nov 5, 2019
thom4parisot pushed a commit to thom4parisot/hexo that referenced this pull request Jan 17, 2020
* fix: external link ignore mailto: & javascript:
Close hexojs#3796

* refactor: use startsWith instead of regex

* refactor(external_link): use whatwg url

* fix(external_link): handle whatwg url api
Apply code suggestions from code review by @curbengh
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

Successfully merging this pull request may close these issues.

hexo 4.0.0 會給 a 標簽 添加 target="_blank"
3 participants