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

Validate rule guidance strictly. #4304

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Validate rule guidance strictly. #4304

merged 1 commit into from
Aug 30, 2024

Conversation

blkt
Copy link
Contributor

@blkt blkt commented Aug 28, 2024

Summary

This change uses bluemonday library to perform strict validation of guidance field of rule types. In case sanitized input is different from the input itself, rule type creation/update is rejected returning a meaningful error message.

The way input is sanitized is in line with the way it is rendered in the CLI by the glamour library.

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Manual tests.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@blkt blkt self-assigned this Aug 28, 2024
@blkt blkt force-pushed the issue-4286-strict branch from c3f6b52 to 1ff34d3 Compare August 28, 2024 11:20
@blkt
Copy link
Contributor Author

blkt commented Aug 28, 2024

Note: this might be overly strict in that characters like ' are mapped to ', which is otherwise acceptable. An alternative might be to use a custom policy to strip just HTML tags.

I'll put this back to draft and verify what options we have.

@blkt blkt marked this pull request as draft August 28, 2024 11:26
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

Good idea

@JAORMX
Copy link
Contributor

JAORMX commented Aug 28, 2024

this validation probably merits adding unit tests

@blkt blkt force-pushed the issue-4286-strict branch from 1ff34d3 to b4e86de Compare August 28, 2024 13:11
@coveralls
Copy link

coveralls commented Aug 28, 2024

Coverage Status

coverage: 53.936% (+0.3%) from 53.663%
when pulling afcff20 on issue-4286-strict
into a8b4f81 on main.

@blkt blkt force-pushed the issue-4286-strict branch 2 times, most recently from 9b6fd1a to 7104864 Compare August 29, 2024 14:53
@blkt blkt marked this pull request as ready for review August 29, 2024 14:54
@blkt blkt force-pushed the issue-4286-strict branch 2 times, most recently from 82fa702 to 6509b96 Compare August 29, 2024 16:35
This change uses `bluemonday` library to perform strict validation of
`guidance` field of rule types. In case sanitized input is different
from the input itself, rule type creation/update is rejected returning
a meaningful error message.

The way input is sanitized is in line with the way it is rendered in
the CLI by the `glamour` library.
@blkt blkt force-pushed the issue-4286-strict branch from 6509b96 to afcff20 Compare August 29, 2024 16:50
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

Love the focus on input validation. Thanks for doing this!

@blkt blkt merged commit d3523eb into main Aug 30, 2024
21 checks passed
@blkt blkt deleted the issue-4286-strict branch August 30, 2024 10:22
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.

3 participants