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

[config] "allowed-elements", "allowed-markup" should replace, not amend default allowlists #751

Open
chendachao opened this issue Dec 13, 2021 · 7 comments · May be fixed by #1007
Open
Labels
needs-decision Architectural/Behavioral decision by maintainers needed question Something needs clarification. support Someone asking for support -> Should be moved to GitHub Discussions
Milestone

Comments

@chendachao
Copy link

I found an issue about the markup config, any suggestions would be appreciate.

Step1, add below code to isso server config,

options = strikethrough, superscript, autolink
allowed-elements = img, blockquote, br, code, del, em, h1, h2, h3, h4, h5, h6, hr, ins, ul, li, ol, p, pre, strong, table, tbody, td, th, thead
allowed-attributes = src

Step2, In the website where you refer isso as comment fn, add below comment:
<a href=http://evil.com>clickmetochangethispassword</a>

Result: the comment was successfully rendered.

Expect: transform the a markup.

@ix5
Copy link
Member

ix5 commented Dec 16, 2021

See server docs

allowed-elements
Additional HTML tags to allow in the generated output, comma-separated.
By default, only a, blockquote, br, code, del, em, h1, h2, h3, h4, h5, h6, hr, ins, li, ol, p, pre, strong, table, tbody, td, th, thead and ul are allowed.

I agree that the config should be named additional-allowed-elements to be more accurate (or replace instead of amend the default list).

The code for that is in isso/utils/html.py:
https://github.com/posativ/isso/blob/9e021cb6263e921ac519f52b82df3f66a297d3d1/isso/utils/html.py#L91-L93
https://github.com/posativ/isso/blob/9e021cb6263e921ac519f52b82df3f66a297d3d1/isso/utils/html.py#L20-L24

But the config has been working that way for quite some time and has been documented to work that way. If you wanted to change this behaviour, you'd most likely break existing setups.

@chendachao
Copy link
Author

chendachao commented Dec 21, 2021

Hi @ix5, so how could I setup the configurations to exclude markup a, both elements and allowed-elements can't work

@ix5
Copy link
Member

ix5 commented Dec 21, 2021

Edit isso/utils/html.py and remove the default lists:
https://github.com/posativ/isso/blob/9e021cb6263e921ac519f52b82df3f66a297d3d1/isso/utils/html.py#L20-L27

Change that to:

self.elements = elements
self.attributes = attributes

Note that this is only a temporary fix ("monkey-patching") that you have to apply every time you install/update isso. If you want to change the behaviour for everyone, you should submit a Pull Request to change it.

@ix5 ix5 mentioned this issue Dec 27, 2021
@ix5 ix5 added needs-decision Architectural/Behavioral decision by maintainers needed question Something needs clarification. support Someone asking for support -> Should be moved to GitHub Discussions labels Dec 27, 2021
@chendachao
Copy link
Author

Could you submit a pull request to fix it, cos I know nothing about python.

@ix5
Copy link
Member

ix5 commented Feb 6, 2022

@chendachao even if you do not know anything about python, you could be a big help in sketching how this functionality should work.

Here's an overview of what I think you should do to get this "fixed":

  1. Ping some people who you think should be consulted and discuss this with them
  2. Create two new properties, named element-allowlist and attribute-allowlist, that defifine all the allowed syntax, instead of amending the default
  3. Warn users who still use allowed-elements or allowed-attributes in the isso error log
  4. Write up a changelog item (CHANGES.rst) notifying users of this breaking change
  5. Suggest/decide whether this change should be part of the next isso release or rather wait until a major version bump

@ix5
Copy link
Member

ix5 commented Feb 6, 2022

An alternative approach would be to re-use the existing config key names (allowed-elements, allowed-attributes) and strip out the default allowed elements. Then add them to the default isso.conf.

That way, people who used the allowed-* config to define the only items they wanted to allow will not have to change anything and Isso would work the way they probably expected it to work (including you).

Most people would probably leave the default untouched (meaning they didn't overwrite the values in their own isso.conf), thus not being affected by this change.

The only negative impact would be people who understood the meaning of the config keys as they are currently specified in the docs (see server.rst )

@ix5 ix5 added this to the 0.13 milestone Feb 13, 2022
@chendachao
Copy link
Author

Sorry, I think it's irresponsible to make a PR if I don't understand the source code. Could you make that changes?

@ix5 ix5 changed the title Isso config [markup] can't work [config] "allowed-elements", "allowed-markup" should replace, not amend default allowlists Mar 16, 2022
@ix5 ix5 modified the milestones: 0.13, 0.14 May 24, 2022
pkvach added a commit to pkvach/isso that referenced this issue Apr 16, 2024
…utes

These changes provide full control over the management of "allowed-elements" and "allowed-attributes" through the configuration file.

Fixes isso-comments#751
pkvach added a commit to pkvach/isso that referenced this issue Apr 16, 2024
…utes

These changes provide full control over the management of "allowed-elements" and "allowed-attributes" through the configuration file.

Fixes isso-comments#751
pkvach added a commit to pkvach/isso that referenced this issue Apr 23, 2024
…utes

These changes provide full control over the management of "allowed-elements" and "allowed-attributes" through the configuration file.

Fixes isso-comments#751
pkvach added a commit to pkvach/isso that referenced this issue Apr 24, 2024
…utes

These changes provide full control over the management of "allowed-elements" and "allowed-attributes" through the configuration file.

Fixes isso-comments#751
pkvach added a commit to pkvach/isso that referenced this issue May 6, 2024
Added new configuration option "strictly-allowed-html-elements" to specify only allowed HTML tags in the generated output.

Fixes isso-comments#751
@ix5 ix5 modified the milestones: 0.14, 0.13.1 May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-decision Architectural/Behavioral decision by maintainers needed question Something needs clarification. support Someone asking for support -> Should be moved to GitHub Discussions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants