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

Allow adding allowed values to existing attributes #65

Closed
DanielSiepmann opened this issue Oct 12, 2021 · 6 comments · Fixed by #66
Closed

Allow adding allowed values to existing attributes #65

DanielSiepmann opened this issue Oct 12, 2021 · 6 comments · Fixed by #66
Labels
enhancement New feature or request

Comments

@DanielSiepmann
Copy link

I've come to code where custom onclick is added to a-Tags.
TYPO3 already ads an onclick to allow list for a-Tags.

I could not find any way to add another allowed value to the list.
Looks like the code is too robust to allow me to add or alter the already configured behaviour.
I only could remove the already build a-Tag and build it from scratch / from reading original values excluding the onclick and rebuilding it again?
Do I overlook something here? How should a developer use the library in such situation, where a framework already adds configuration?

That's our code which is just not called, because TYPO3 onclick value already does not match. Further values are not checked anymore. I couldn't find a way to check whether at least one value matches.

class DefaultSanitizerBuilder extends Typo3DefaultSanitizerBuilder
{
    protected function createBasicTags(): array
    {
        $tags = parent::createBasicTags();
        // Allow onlick="econdaTrackMarker();" in addition to TYPO3 native onclick code.
        $tags['a']->getAttr('onclick')->addValues(new RegExpAttrValue('#^econdaTrackMarker\(#'));
        return $tags;
    }
}
@DanielSiepmann DanielSiepmann changed the title Allow adding allowed values to attributes Allow adding allowed values to existing attributes Oct 12, 2021
@ohader
Copy link
Member

ohader commented Oct 12, 2021

In TYPO3 v11 this has been removed - onclick is allowed for openPic( in earlier TYPO3 versions.

In this particular case all values-definitions for attr onclick need to match. The missing piece in the TYPO3 core is this: https://review.typo3.org/c/Packages/TYPO3.CMS/+/71560

Adding new Tag::removeAttr($name) and (maybe) Attr::setFlags($flags) methods to this library seems to be useful as well.

@ohader ohader added the enhancement New feature or request label Oct 12, 2021
@ohader
Copy link
Member

ohader commented Oct 12, 2021

Thinking a bit about that, it probably makes sense to deprecate MATCH_FIRST_VALUE and introduce MATCH_ALL_VALUES - in most cases values should be additive like OR.

@DanielSiepmann
Copy link
Author

Thanks for the fast response.
We can actually remove the TYPO3 code via composer patch in our specific use case.
But others might run into those issues as well. Your suggestions both sound useful. setFlags() feels easier to solve the issue, but bad from current design perspective. While removeAttr() feels good compared to current design, but complicated to solve the issue.

Your proposed Change to TYPO3 should already solve the job anyway, so no need to extend the API :)

@DanielSiepmann
Copy link
Author

Thinking a bit about that, it probably makes sense to deprecate MATCH_FIRST_VALUE and introduce MATCH_ALL_VALUES - in most cases values should be additive like OR.

Would love that. Would be easier to understand and probably reflect real world more :)

ohader added a commit to ohader/html-sanitizer that referenced this issue Oct 12, 2021
Prior to this change, using the first match out of multiple
`AttrValueInterface` items had to be declared explicitly using
flag `Attr::MATCH_FIRST_VALUE` - thus, all values had to match.

With this change, only the first match is considered (implicit
flag `Attr::MATCH_FIRST_VALUE`). To declare that all value items
have to match, new `Attr::MATCH_ALL_VALUES` has been introduced.

Deprecations:
* `\TYPO3\HtmlSanitizer\Behavior\Attr::MATCH_FIRST_VALUE`
* `\TYPO3\HtmlSanitizer\Behavior\Attr::shallMatchFirstValue()`

New additions:
* `\TYPO3\HtmlSanitizer\Behavior\Attr::MATCH_ALL_VALUES`
* `\TYPO3\HtmlSanitizer\Behavior\Attr::shallMatchAllValues()`

Fixes: TYPO3#65
@ohader
Copy link
Member

ohader commented Oct 12, 2021

@DanielSiepmann Looking forward to receiving your feedback on PR #66 (which makes mentioned core change above obsolete)

ohader added a commit to ohader/html-sanitizer that referenced this issue Oct 12, 2021
Prior to this change, using the first match out of multiple
`AttrValueInterface` items had to be declared explicitly using
flag `Attr::MATCH_FIRST_VALUE` - thus, all values had to match.

With this change, only the first match is considered (implicit
flag `Attr::MATCH_FIRST_VALUE`). To declare that all value items
have to match, new `Attr::MATCH_ALL_VALUES` has been introduced.

Deprecations:
* `\TYPO3\HtmlSanitizer\Behavior\Attr::MATCH_FIRST_VALUE`
* `\TYPO3\HtmlSanitizer\Behavior\Attr::shallMatchFirstValue()`

New additions:
* `\TYPO3\HtmlSanitizer\Behavior\Attr::MATCH_ALL_VALUES`
* `\TYPO3\HtmlSanitizer\Behavior\Attr::shallMatchAllValues()`

Fixes: TYPO3#65
ohader added a commit that referenced this issue Oct 12, 2021
Prior to this change, using the first match out of multiple
`AttrValueInterface` items had to be declared explicitly using
flag `Attr::MATCH_FIRST_VALUE` - thus, all values had to match.

With this change, only the first match is considered (implicit
flag `Attr::MATCH_FIRST_VALUE`). To declare that all value items
have to match, new `Attr::MATCH_ALL_VALUES` has been introduced.

Deprecations:
* `\TYPO3\HtmlSanitizer\Behavior\Attr::MATCH_FIRST_VALUE`
* `\TYPO3\HtmlSanitizer\Behavior\Attr::shallMatchFirstValue()`

New additions:
* `\TYPO3\HtmlSanitizer\Behavior\Attr::MATCH_ALL_VALUES`
* `\TYPO3\HtmlSanitizer\Behavior\Attr::shallMatchAllValues()`

Fixes: #65
@AEHluis
Copy link

AEHluis commented Jan 10, 2022

Adding new Tag::removeAttr($name) and (maybe) Attr::setFlags($flags) methods to this library seems to be useful as well.

Do you have any idea if and when that will be added?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants