mganss / HtmlSanitizer

Cleans HTML to avoid XSS attacks
MIT License
1.52k stars 198 forks source link

Remove old constructor, add empty constructor #370

Closed vanillajonathan closed 1 year ago

vanillajonathan commented 1 year ago

This removes the old constructor in favor of the new constructor taking in an HtmlSanitizerOptions. It also adds a new empty constructor for convenience.

codecov[bot] commented 1 year ago

Codecov Report

Merging #370 (12b29d6) into master (07a8058) will decrease coverage by 0.08%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
- Coverage   94.51%   94.43%   -0.09%     
==========================================
  Files           6        6              
  Lines         839      826      -13     
  Branches       85       79       -6     
==========================================
- Hits          793      780      -13     
  Misses         34       34              
  Partials       12       12              
Impacted Files Coverage Δ
src/HtmlSanitizer/HtmlSanitizer.cs 92.93% <100.00%> (-0.24%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 07a8058...12b29d6. Read the comment docs.

vanillajonathan commented 1 year ago

@mganss The code coverage fails because it says it has less code coverage than before. On the diff on the codecov website it says the commit would remove code coverage for some lines but those are the lines that were removed. I don't know what's up with that. Maybe you could check it out?

mganss commented 1 year ago

@vanillajonathan The coverage change is negligible (0.09%). Don't worry about it.

vanillajonathan commented 1 year ago

Alright. Check the diff then, maybe @tiesont can do so too if he has any feedback.

This will be a breaking change.


+public HtmlSanitizer()
-public HtmlSanitizer(IEnumerable<string>? allowedTags = null, IEnumerable<string>? allowedSchemes = null,
-    IEnumerable<string>? allowedAttributes = null, IEnumerable<string>? uriAttributes = null, IEnumerable<string>? allowedCssProperties = null)
tiesont commented 1 year ago

Alright. Check the diff then, maybe @tiesont can do so too if he has any feedback.

I think I'm mostly an interested bystander, but thank you for the mention. I do use HtmlSanitizer pretty heavily, in a few projects, to sanitize any content I've received from users, so I've been following your updates as much as I can.

Without having used it yet, I really like the new options object and the (to me) simplified constructors. Basically, no complaints or nits from me.

vanillajonathan commented 1 year ago

@tiesont Thank you for your feedback!

@mganss Check it out! What do you think?

mganss commented 1 year ago

@vanillajonathan Sorry for the late response. LGTM. I'm wondering if all current use cases are supported in a non-surprising manner, especially the DI case discussed at #314 and #220, but AFAICT they are.

vanillajonathan commented 1 year ago

Thanks!

Yes, I believe the DI use cases are covered. Previously a workaround to deal with the DI injecting empty arrays were used, but now with the new DI-friendly parameterless constructor it always initializes the class with the default values just as before.

Removing the old parameters means this change is a breaking change though since it breaks the API and existing users needs to rewrite the code to use the other constructor which takes in the options parameter.

mganss commented 1 year ago

@vanillajonathan 🆗 Can this be merged now?

vanillajonathan commented 1 year ago

@mganss Yes.