-
Notifications
You must be signed in to change notification settings - Fork 353
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 escaping of more than just tags #540
Labels
Comments
I agree it doesn't make sense for recursiveEscape to keep tags (in escaped
form) and yet discard attributes, given that it's all being converted to
text anyway.
…On Mon, Feb 21, 2022 at 8:50 AM Zsar ***@***.***> wrote:
The problem to solve
I would like the sanitised string to be displayed exactly as input.
Discarding text is not helpful, as it will be missing.
options.disallowedTagsMode = 'recursiveEscape' already exists to escape
tags instead of removing them. Attributes, etc. are still removed instead
of escaped though.
User Story:
1. We start with (mostly) everything disallowed.
2. User attempts to input HTML, sees the resulting text and
copy&pastes directly into a feature request to allow the
tag/attribute/style/whatever they want and do not get.
3. (If the requested element can be safely allowed) we copy&paste the
text directly into our mock project for TDD.
4. loop 2., 3.
5. Profit!
Proposed solution
I, personally, would like a simple switch options.disallowedEverythingMode:
'discard' | 'escape' | 'recursiveEscape' or similar.
Alternatives
Mirroring the lists and records of options akin to how the existing
options.disallowedTagsMode mirrors options.allowedTags is probably the
more sensible approach:
options.disallowedAttributesMode: 'discard' | 'escape' | 'recursiveEscape'
options.disallowedStylesMode: 'discard' | 'escape' | 'recursiveEscape'
<etc.>
Mayhap a synthetic "all the things" could be added as a function on top of
those:
function setDisallowedEverythingMode(mode: 'discard' | 'escape' |
'recursiveEscape') {/* set every flag to mode */}
Additional context
options: {
allowedAttributes: {},
allowedTags: [],
disallowedTagsMode: 'recursiveEscape',
nonTextTags: [],
selfClosing: [],
}
Input: "Cursed Project <img src=x
onerror=alert('AllYourBaseAreBelongToUs!');>"
Expected: Cursed Project <img src=x
onerror=alert('AllYourBaseAreBelongToUs!');>
Displayed: Cursed Project <img></img>
—
Reply to this email directly, view it on GitHub
<#540>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAH27MCVL3BBGDY2HF76RTU4I7J3ANCNFSM5O6U45NA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
--
THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER
APOSTROPHECMS | apostrophecms.com | he/him/his
|
This would make a good community contribution.
…On Tue, Feb 22, 2022 at 8:40 AM Tom Boutell ***@***.***> wrote:
I agree it doesn't make sense for recursiveEscape to keep tags (in escaped
form) and yet discard attributes, given that it's all being converted to
text anyway.
On Mon, Feb 21, 2022 at 8:50 AM Zsar ***@***.***> wrote:
> The problem to solve
>
> I would like the sanitised string to be displayed exactly as input.
> Discarding text is not helpful, as it will be missing.
> options.disallowedTagsMode = 'recursiveEscape' already exists to escape
> tags instead of removing them. Attributes, etc. are still removed instead
> of escaped though.
>
> User Story:
>
> 1. We start with (mostly) everything disallowed.
> 2. User attempts to input HTML, sees the resulting text and
> copy&pastes directly into a feature request to allow the
> tag/attribute/style/whatever they want and do not get.
> 3. (If the requested element can be safely allowed) we copy&paste the
> text directly into our mock project for TDD.
> 4. loop 2., 3.
> 5. Profit!
>
> Proposed solution
>
> I, personally, would like a simple switch options.disallowedEverythingMode:
> 'discard' | 'escape' | 'recursiveEscape' or similar.
> Alternatives
>
> Mirroring the lists and records of options akin to how the existing
> options.disallowedTagsMode mirrors options.allowedTags is probably the
> more sensible approach:
>
> options.disallowedAttributesMode: 'discard' | 'escape' | 'recursiveEscape'
> options.disallowedStylesMode: 'discard' | 'escape' | 'recursiveEscape'
> <etc.>
>
> Mayhap a synthetic "all the things" could be added as a function on top
> of those:
> function setDisallowedEverythingMode(mode: 'discard' | 'escape' |
> 'recursiveEscape') {/* set every flag to mode */}
> Additional context
>
> options: {
> allowedAttributes: {},
> allowedTags: [],
> disallowedTagsMode: 'recursiveEscape',
> nonTextTags: [],
> selfClosing: [],
> }
>
> Input: "Cursed Project <img src=x
> onerror=alert('AllYourBaseAreBelongToUs!');>"
> Expected: Cursed Project <img src=x
> onerror=alert('AllYourBaseAreBelongToUs!');>
> Displayed: Cursed Project <img></img>
>
> —
> Reply to this email directly, view it on GitHub
> <#540>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAH27MCVL3BBGDY2HF76RTU4I7J3ANCNFSM5O6U45NA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you are subscribed to this thread.Message
> ID: ***@***.***>
>
--
THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER
APOSTROPHECMS | apostrophecms.com | he/him/his
--
THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER
APOSTROPHECMS | apostrophecms.com | he/him/his
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
benelliott
added a commit
to benelliott/sanitize-html
that referenced
this issue
Jul 8, 2024
benelliott
added a commit
to benelliott/sanitize-html
that referenced
this issue
Jul 8, 2024
… disallowed tags to be retained Fixes apostrophecms#540
10 tasks
benelliott
added a commit
to benelliott/sanitize-html
that referenced
this issue
Jul 9, 2024
… disallowed tags to be retained Fixes apostrophecms#540
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The problem to solve
I would like the sanitised string to be displayed exactly as input. Discarding text is not helpful, as it will be missing.
options.disallowedTagsMode = 'recursiveEscape'
already exists to escape tags instead of removing them. Attributes, etc. are still removed instead of escaped though.User Story:
Proposed solution
I, personally, would like a simple switch
options.disallowedEverythingMode: 'discard' | 'escape' | 'recursiveEscape'
or similar.Alternatives
Mirroring the lists and records of
options
akin to how the existingoptions.disallowedTagsMode
mirrorsoptions.allowedTags
is probably the more sensible approach:Mayhap a synthetic "all the things" could be added as a function on top of those:
function setDisallowedEverythingMode(mode: 'discard' | 'escape' | 'recursiveEscape') {/* set every flag to mode */}
Additional context
Input:
"Cursed Project <img src=x onerror=alert('AllYourBaseAreBelongToUs!');>"
Expected:
Cursed Project <img src=x onerror=alert('AllYourBaseAreBelongToUs!');>
Displayed:
Cursed Project <img></img>
The text was updated successfully, but these errors were encountered: