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 leancloud counter security bug #137

Merged
merged 17 commits into from
Feb 27, 2018
Merged

Fix leancloud counter security bug #137

merged 17 commits into from
Feb 27, 2018

Conversation

LEAFERx
Copy link
Contributor

@LEAFERx LEAFERx commented Feb 12, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines.
  • Tests for the changes have been added (for bug fixes / features).
    • Muse | Mist have been tested.
    • Pisces | Gemini have been tested.
  • Docs have been added / updated (for bug fixes / features).

PR Type

What kind of change does this PR introduce?

  • Bugfix.
  • Feature.
  • Code style update (formatting, local variables).
  • Refactoring (no functional changes, no api changes).
  • Build related changes.
  • CI related changes.
  • Documentation content changes.
  • Other... Please describe:

What is the current behavior?

Issue Number(s): #25

@sli1989
Copy link
Collaborator

sli1989 commented Feb 12, 2018

😂

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Feb 12, 2018

@sli1989
这是为了配合我编写的插件而做的改动
中文文档已经写好,正在修饰当中
请用gitter或者邮箱联系我,我发给你文档链接(暂时不宜公开)

_config.yml Outdated
leancloud_visitors:
enable: false
security: false
Copy link
Member

Choose a reason for hiding this comment

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

Add short readme for dependencies here.

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Feb 13, 2018

the link on the readme is not available now.
i will release the doc as soon as this pr is merged.

Copy link
Member

@wafer-li wafer-li left a comment

Choose a reason for hiding this comment

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

Is it better to default to true of security

_config.yml Outdated
leancloud_visitors:
enable: false
security: false
Copy link
Member

Choose a reason for hiding this comment

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

Why not default to true?

Copy link
Member

Choose a reason for hiding this comment

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

Because need to install plugin separately.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I mean just like the gitmint, default to true, and ask the user to install the plugin.

Security is important, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Important, but LC can work without security plugin.

}
});
{% if theme.leancloud_visitors.security %}
console.log('Counter not initialized!');
Copy link
Member

@ivan-nginx ivan-nginx Feb 15, 2018

Choose a reason for hiding this comment

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

Ok, i see what here is security option just add warning in console log. There is some my 5 cents on it:

  1. Replace console.log with hexo.log.warn for better warning message.
  2. security can be set to true by default. So, if user wan't to install security plugin, he can turn off security option. If he don't know about security (newbie in NexT) and just turn on LC option, he will seen this warning message with following instructions to install plugin.
  3. Warning message should be informative. Instead of Counter not initialized! something like ATTENTION! LeanCloud counter have security bug, for solve it see here: https://github.com/theme-next/hexo-leancloud-counter-security.

OR

We can just cut out security option and add in NexT config comments under leancloud options what there are some security issues and can be solved under the plugin link. For now option — it's just console warning, no more. Who know about this — will know, option will be useless, i think. 1 excess interation in swig. That's what i suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but this file is excuted in browser where there is no hexo.log.warn

Copy link
Member

Choose a reason for hiding this comment

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

Oh! So, why this warn needed if user will not see it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because if they turn on the security but not install the plugin and follow the instruction to config th e Leancloud background, the counter will not work at all

Copy link
Member

Choose a reason for hiding this comment

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

So, stay as it? False by default?


Also, need to provide some badges for this repo. NPM link, version, compatability, etc.

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Feb 15, 2018

now the error msg on browser is like this:
1
2
@ivan-nginx

@ivan-nginx
Copy link
Member

Yeah, it's good for now! Well done! 👍

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Feb 15, 2018

so the config is default to true now.
anything else?

@ivan-nginx
Copy link
Member

ivan-nginx commented Feb 15, 2018

Em.. i think need to false. But other guys sugessted to true it. Need to make a decision about it.


anything else?

Also, need to provide some badges for this repo. NPM link, version, compatability, etc.


Post not found: https://leaferx.online/2018/02/11/lc-security/
Maybe better to add in NexT docs this info?
P.S. Later all this docs will be moved to main theme-next.org site.

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Feb 15, 2018

badges added.
i haven't release the doc yet because this pr is nor merged.

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Feb 15, 2018

Now that docs has been released.

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Feb 15, 2018

Question need discussion: what default value of security should be set

@ivan-nginx
Copy link
Member

Guys, can we please to deside this option?

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Feb 26, 2018

@wafer-li @tsanie @maple3142 @ivan-nginx
can we decide what default value of security should be set?

@wafer-li
Copy link
Member

@LEAFERx @tsanie @maple3142 @ivan-nginx

What about making a vote?

I still vote for default to true, if you agree with me, 👍 with this comment, otherwise 👎 with this comment.

@ivan-nginx ivan-nginx merged commit 8b2e72a into theme-next:master Feb 27, 2018
@ivan-nginx ivan-nginx modified the milestones: v6.0.5, v6.0.6 Feb 27, 2018
@ivan-nginx
Copy link
Member

@sli1989 @wafer-li @tsanie @maple3142 anybody of u can today translate from CN docs to EN docs?

@sli1989
Copy link
Collaborator

sli1989 commented Mar 15, 2018

It's better for @LEAFERx to do it...😅

@ivan-nginx
Copy link
Member

@sli1989 moved to here.

@stevenjoezhang stevenjoezhang changed the title fix leancloud counter security bug Fix leancloud counter security bug Sep 15, 2019
tongluyang pushed a commit to tongluyang/hexo-theme-next that referenced this pull request Nov 19, 2019
fix leancloud counter security bug
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