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

refactor: replace md5 with sha256 for commenter email hash #7092

Merged
merged 11 commits into from
Dec 9, 2024

Conversation

cryptochecktool
Copy link
Contributor

@cryptochecktool cryptochecktool commented Nov 28, 2024

What type of PR is this?

/kind feature
/kind improvement

What this PR does / why we need it:

本次PR对系统中用于电子邮件哈希的算法进行了升级。原先使用的是MD5算法,现在替换为了更安全的SHA-256算法。这一变更提高了数据的安全性,降低了电子邮件被破解的风险。

Which issue(s) this PR fixes:

未指定具体问题编号,但解决了潜在的安全隐患。

Special notes for your reviewer:

在替换哈希算法的过程中,我已经确保了代码的兼容性和性能。建议审查者在合并前进行全面的测试,以确保新算法的正确性和系统的稳定性。

Does this PR introduce a user-facing change?

增强评论邮箱哈希算法(SHA256)

@f2c-ci-robot f2c-ci-robot bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 28, 2024
@f2c-ci-robot f2c-ci-robot bot requested a review from guqing November 28, 2024 13:00
@f2c-ci-robot f2c-ci-robot bot added the kind/improvement Categorizes issue or PR as related to a improvement. label Nov 28, 2024
@f2c-ci-robot f2c-ci-robot bot requested a review from wan92hen November 28, 2024 13:00
@f2c-ci-robot f2c-ci-robot bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Nov 28, 2024
@cryptochecktool
Copy link
Contributor Author

对开发者说声很抱歉,我不太明白为什么我的PR总是会有do-not-merge/release-note-label-needed的标签。

@f2c-ci-robot f2c-ci-robot bot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 28, 2024
@ruibaby
Copy link
Member

ruibaby commented Nov 28, 2024

对开发者说声很抱歉,我不太明白为什么我的PR总是会有do-not-merge/release-note-label-needed的标签。

这是提示没有在 PR 内容里面填写 release-note,这是后续发布版本的时候用来生成变更日志的。详情可见 PR 模板:https://raw.githubusercontent.com/halo-dev/halo/refs/heads/main/.github/pull_request_template.md

我已经改了 PR 内容了,现在应该移除这个标签了。

@ruibaby
Copy link
Member

ruibaby commented Nov 28, 2024

这里生成 md5 字符串主要是为了支持评论者的 Gravatar 头像,目前 Gravatar 是支持 sha256 的,所以应该可以合并此 PR。

https://docs.gravatar.com/api/avatars/hash/

cc @halo-dev/sig-halo @ShiinaKin 帮忙看看是否会有其他副作用。

Copy link
Member

@JohnNiang JohnNiang left a comment

Choose a reason for hiding this comment

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

Hi @cryptochecktool , thank you for your contribution!

官方文档了解到,这里可能需要将 email 字符串转换成小写后,再进行 sha256 哈希。如果我忽略了什么,请随时提醒我。

@cryptochecktool
Copy link
Contributor Author

邮箱地址本身是不区分大小写的。等我晚上,我新增一下转换小写的步骤,谢谢提醒。

增加邮件进行小写转换
@cryptochecktool
Copy link
Contributor Author

已经修改了,增加了邮件小写

@ruibaby ruibaby requested a review from JohnNiang December 3, 2024 02:06
Copy link
Member

@JohnNiang JohnNiang left a comment

Choose a reason for hiding this comment

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

Hi @cryptochecktool , thank you for your contribution!

看起来不错。唯一的问题就是代码格式化的问题会导致 CI 检查失败。如何利用 IDEA 格式化代码,请参考这篇文档

@ruibaby ruibaby changed the title 使用sha256代替MD5算法 refactor: replace md5 with sha256 for commenter email hash Dec 4, 2024
@cryptochecktool
Copy link
Contributor Author

已经修改,符合check要求

@ruibaby
Copy link
Member

ruibaby commented Dec 6, 2024

有单元测试未通过,https://github.com/halo-dev/halo/pull/7092/checks

@JohnNiang
Copy link
Member

Hi @cryptochecktool , 感谢贡献!

我将尝试帮你解决代码格式化的问题。

Copy link

sonarqubecloud bot commented Dec 8, 2024

Copy link

codecov bot commented Dec 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.23%. Comparing base (eff73dc) to head (b618791).
Report is 77 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7092      +/-   ##
============================================
+ Coverage     56.99%   57.23%   +0.23%     
- Complexity     3999     4048      +49     
============================================
  Files           714      719       +5     
  Lines         24110    24352     +242     
  Branches       1585     1598      +13     
============================================
+ Hits          13742    13937     +195     
- Misses         9756     9798      +42     
- Partials        612      617       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@JohnNiang JohnNiang left a comment

Choose a reason for hiding this comment

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

/approve

Copy link

f2c-ci-robot bot commented Dec 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JohnNiang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@f2c-ci-robot f2c-ci-robot bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Dec 8, 2024
Copy link
Member

@ruibaby ruibaby left a comment

Choose a reason for hiding this comment

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

/lgtm

@f2c-ci-robot f2c-ci-robot bot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2024
@f2c-ci-robot f2c-ci-robot bot merged commit 0748ae4 into halo-dev:main Dec 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. kind/improvement Categorizes issue or PR as related to a improvement. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants