Skip to content

feat(plugin-git): sanitize commit messages #436

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

Merged
merged 4 commits into from
Apr 16, 2025
Merged

Conversation

pengzhanbo
Copy link
Member

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Provide a description in this PR that addresses what the PR is solving. If this PR is going to solve an existing issue, please reference the issue (e.g. close #123).

What is the purpose of this pull request?

  • Bug fix
  • New feature
  • Other

Description

In previous versions, the submission information was rendered using innerHTML, but this could potentially contain illegal HTML structures. Therefore, it was modified to use innerText for rendering. However, this led to issues where content such as #issue within the submission information could not be rendered correctly.

Therefore, use rehype to sanitize commit messages, ensuring that the commit message HTML strings are clean and safe.

Ref:

rehype
rehype-sanitize

Screenshots

Before

After

@pengzhanbo pengzhanbo requested a review from Mister-Hope April 16, 2025 03:13
@Mister-Hope
Copy link
Member

Mister-Hope commented Apr 16, 2025

I am not familiar with these tool chain, but I have used dom-purify.

If HTML MUST be supported here, the sanitized process requires further discussion.

A general example can be found at https://github.com/walinejs/waline/blob/main/packages/server/src/service/markdown/xss.js

For example:

  • form tags like <form> <input> and media tags shall be forbidden, for commit msg, I believe only a small whitelist of tags shall be supported.
  • Anything related to style must be forbidden, inducing ID, class and style
  • Other attributes that uses may collect data or affecting site behavior shall also be blocked.

Copy link
Member

@Mister-Hope Mister-Hope left a comment

Choose a reason for hiding this comment

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

I believe some unit test shall be added in this PR.

We can trust the sanitize process, but we shall focus on other things like style inject.

@pengzhanbo
Copy link
Member Author

Currently, in commit messages, a whitelist mechanism only supports the a, code, em, and strong tags. Among these, the a tag only supports the href, target, and rel attributes, and there is also validation for the value of the href attribute.

@pengzhanbo pengzhanbo requested a review from Mister-Hope April 16, 2025 05:27
@Mister-Hope Mister-Hope merged commit b00e290 into main Apr 16, 2025
27 checks passed
@Mister-Hope Mister-Hope deleted the git-sanitize-commit branch April 16, 2025 07:53
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.

2 participants