Skip to content

Commit

Permalink
minor #44814 [HtmlSanitizer] Some minor changes in the config API (ja…
Browse files Browse the repository at this point in the history
…viereguiluz)

This PR was squashed before being merged into the 6.1 branch.

Discussion
----------

[HtmlSanitizer] Some minor changes in the config API

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

First of all, thanks to @tgalopin for this superb contribution 🙇

This PR makes 3 little changes:

(1) Fix two minor typos

(2) Rename `allowAllStaticElements()` as `allowStaticElements()` to be consistent with the rest of methods, which don't include the `All` word.

(3) A proposal to change this default value:

```diff
-public function allowElement(string $element, array|string $allowedAttributes = []): static
+public function allowElement(string $element, array|string $allowedAttributes = '*'): static
```

In my opinion, when you want to allow some element, most of the times you want to allow the standard attributes on them too. So, the following should allow `<div>` and their standard attributes:

```php
->allowElement('div')
```

Forcing to write it as `->allowElement('div', '*')` seems cumbersome. The previous behavior (forbid all attributes) would now be like this:

```php
->allowElement('div', [])
```

Commits
-------

84470efbaa [HtmlSanitizer] Some minor changes in the config API
  • Loading branch information
fabpot committed Jan 7, 2022
2 parents a57e7b9 + ae23fc8 commit b67d0e7
Show file tree
Hide file tree
Showing 5 changed files with 5 additions and 5 deletions.
2 changes: 1 addition & 1 deletion HtmlSanitizerConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public function __construct()
* All scripts will be removed but the output may still contain other dangerous
* behaviors like CSS injection (click-jacking), CSS expressions, ...
*/
public function allowAllStaticElements(): static
public function allowStaticElements(): static
{
$elements = array_merge(
array_keys(W3CReference::HEAD_ELEMENTS),
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ $config = (new HtmlSanitizerConfig())
// standard. All scripts will be removed but the output may still contain
// other dangerous behaviors like CSS injection (click-jacking), CSS
// expressions, ...
->allowAllStaticElements()
->allowStaticElements()

// Allow the "div" element and no attribute can be on it
->allowElement('div')
Expand Down
2 changes: 1 addition & 1 deletion Tests/HtmlSanitizerAllTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ private function createSanitizer(): HtmlSanitizer
{
return new HtmlSanitizer(
(new HtmlSanitizerConfig())
->allowAllStaticElements()
->allowStaticElements()
->allowLinkHosts(['trusted.com', 'external.com'])
->allowMediaHosts(['trusted.com', 'external.com'])
->allowRelativeLinks()
Expand Down
2 changes: 1 addition & 1 deletion TextSanitizer/StringSanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ final class StringSanitizer
// "&#34;" is shorter than "&quot;"
'&quot;',

// Fix several potential issues in how browsers intepret attributes values
// Fix several potential issues in how browsers interpret attributes values
'+',
'=',
'@',
Expand Down
2 changes: 1 addition & 1 deletion Visitor/DomVisitor.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ final class DomVisitor
private array $elementsConfig;

/**
* Registry of attributes to forcefuly set on nodes, index by element and attribute.
* Registry of attributes to forcefully set on nodes, index by element and attribute.
*
* @var array<string, array<string, string>>
*/
Expand Down

0 comments on commit b67d0e7

Please sign in to comment.