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

Showing version number cause security concern #1873

Closed
3 tasks done
Haocen opened this issue Sep 14, 2017 · 18 comments
Closed
3 tasks done

Showing version number cause security concern #1873

Haocen opened this issue Sep 14, 2017 · 18 comments

Comments

@Haocen
Copy link
Contributor

Haocen commented Sep 14, 2017

I agree and want to create new issue


Expected behavior

Do not show theme version in footer.

Actual behavior

Showing theme version in footer.

Steps to reproduce the behavior

  1. N/A
  2. N/A
  3. N/A
  • Link to demo site with this issue: N/A
  • Link(s) to source code or any usefull link(s): N/A

NexT Information

NexT Version:

[x] Latest Master branch.
[] Latest Release version.
[] Old version - 

NexT Scheme:

[x] All schemes
[] Muse
[] Mist
[] Pisces
[] Gemini

Other Information

Although the site is static, sophisticate attack can still be used to inject malicious scripts if there is a bug in certain version of the theme.

@ivan-nginx
Copy link
Collaborator

In _config.yml:

copyright: false

Will not show copyright in footer, included NexT version too.

@Haocen
Copy link
Contributor Author

Haocen commented Sep 14, 2017

@ivan-nginx This will hide the whole copyright footer.

Let me explain it in the other way:

I believe showing version number of the theme does not help either the owner of the blog, nor the visitor of the blog.

Showing the link to the theme, on the other hand, help the visitor of the blog to find this theme.

The only case one want to know the version number is the owner of the blog try to fix a problem in the theme, which can be done by looking at the json since he already has the source of the theme on the build machine.

In conclusion, showing version number in the footer is rather useless.

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Sep 14, 2017

Two ways to expand this feature:

  1. Add option version true/false under the copyright option. It will be hide footer version only (json will visible in front-end and may be used by bots).
  2. Add same option, but it will hide both footer and json version vision.

What u think?

@Haocen
Copy link
Contributor Author

Haocen commented Sep 14, 2017

May I ask why we want it to be visible to bots?

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Sep 14, 2017

I don't want to make visible version for bots and this not actually for bots. I added version vision to fast resolve any issues, because peoples sometimes don't know wich version they use and create issues on old/wrong versions, for example.
And i'm don't thinking about bots or any security things before your issue. And u are right, need to add switch option to enable/disable version vision, but by default it must turned on like in PHP/Nginx/Apache/etc.

About JSON version — it can be used by any JS files included in src HexT dir, but for now it no used anywhere. I just added it on future.

Also, as a footer labels we talking here, need to add #1853 to footer copyright too and make all this things separated. Can u do this changes and create pull request?

For example, as is see this options:

-# Specify the date when the site was setup
-#since: 2015

-# icon between year and author @Footer
-authoricon: heart

-# Footer `powered-by` and `theme-info` copyright
-copyright: true

+footer:
+  # Specify the date when the site was setup.
+  #since: 2015

+  # Icon between year and copyright info.
+  icon: heart

+  # Copyright info about author or site [author | site].
+  copyright: author

+  # Hexo link (Powered by Hexo).
+  powered: true

+  # Theme & scheme info link (Theme - NexT.scheme).
+  theme: true

+  # NexT version info (vX.X.X).
+  version: true

+  #custom_text:

@Haocen
Copy link
Contributor Author

Haocen commented Sep 14, 2017

Will take a look, doesn't seem very complex.

Please assign the issues to me.

@Haocen
Copy link
Contributor Author

Haocen commented Sep 15, 2017

I compared whatever in the dev branch but it doesn't appear to be the same as you listed above.

@ivan-nginx are you refering to testing branch? Is this the branch I'm supposed to pull request against?

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Sep 15, 2017

master branch instead of testing or dev, u may create pull right in master. Releases — are releases.

it doesn't appear to be the same as you listed above

Yep, i write it in comments directly. Need just do it, it's not too hard i think. Just switches and options, that's easy.

@Haocen
Copy link
Contributor Author

Haocen commented Sep 15, 2017

-# icon between year and author @Footer
-authoricon: heart

This pending removal option does not appear to be in master, will omit instead.

@Haocen
Copy link
Contributor Author

Haocen commented Sep 17, 2017

It is unclear to me how this option should work:

  # Copyright info about author or site [author | site].
  copyright: author

So I simply ignored it.

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Sep 17, 2017

Mean here: © 2016 — 2017 Ivan.Nginx will be replaced by config.author or config.title or any custom text.

For example, i want to show © 2016 — 2017 AlmostOver.ru instead of © 2016 — 2017 Ivan.Nginx, i define:
copyright: AlmostOver.ru (custom string).

Or if i want to show author name (Ivan.Nginx) by how is default, i define there:
copyright: config.author (string from config).
It is possible to do?

@Haocen
Copy link
Contributor Author

Haocen commented Sep 17, 2017

@ivan-nginx this option seemed to be very ambiguous?
Sometimes it is considered a string, sometimes it is considered the name of a variable.

@ivan-nginx
Copy link
Collaborator

And if try to parse config. and if it find in whole string, then this must get string from config.
And if config. not found in string, well, this string will be used.
Possible?

@Haocen
Copy link
Contributor Author

Haocen commented Sep 17, 2017

How about this:

We make this option optional, if supplied, we consider it a string and override the config.author from Hexo main config.

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Sep 17, 2017

Or like that, yep. Seems good too.

+  # If value not defined, will be used `author` from Hexo main config.
+  copyright:

@Haocen
Copy link
Contributor Author

Haocen commented Sep 18, 2017

@ivan-nginx done.
PR updated.

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Sep 18, 2017

@Haocen well done! I'll check it later and make some changes (custom text, for example) and after that PR will merged. Good job and thank's for doing impact in NexT theme!

@ivan-nginx
Copy link
Collaborator

Merged #1886 with some improvements.
And thank u again.

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

No branches or pull requests

2 participants