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

fix: ignore theme key when sanitizing strings for extra data #234

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Badmuts
Copy link
Contributor

@Badmuts Badmuts commented Jul 28, 2022

This fix ignores sanitization of the theme key in extraData. This causes a problem where theme filters don't work when a theme is used with the '&' character. This character gets escaped with the value &amp.

Another solution would be to sanitize the theme in the frontend widget so the api receives a sanitized theme like "Duurzaam &amp groen" instead of "Duurzaam & groen".

@Badmuts Badmuts requested a review from nlsvgtr July 28, 2022 09:52
@nlsvgtr
Copy link
Collaborator

nlsvgtr commented Aug 1, 2022

An even better solution would be to store the theme as id, not as a string.

@nlsvgtr
Copy link
Collaborator

nlsvgtr commented Aug 1, 2022

Sanitation is mostly a security measure, and the solution chosen here is a potential security issue: if theme is defined in site.config.ideas.extraData as a string a user can upload arbitrary executable code:

{
   "extraData": {
    "theme": "Theme with script <script>alert(1)</script>"
  }
}

@Badmuts
Copy link
Contributor Author

Badmuts commented Aug 22, 2022

Sanitation is mostly a security measure, and the solution chosen here is a potential security issue: if theme is defined in site.config.ideas.extraData as a string a user can upload arbitrary executable code:

{
   "extraData": {
    "theme": "Theme with script <script>alert(1)</script>"
  }
}

I agree. This seemed the simplest solution at the time. The best solution would be to use the generated theme id from apostrophe but this would not be backwards compatible.

Maybe we can make theme a special key in extraData so that it can only contain values (themes) that are defined in apostrophe. But this only allows for validation on the frontend since themes are not stored in the site config but in apostrophe.

@nlsvgtr
Copy link
Collaborator

nlsvgtr commented Aug 3, 2023

With the new tags system it should be possible to create a set of tags in the category 'theme' and use those. Frontend should then be updated of course. I'll add it to the Headless todo list.

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

Successfully merging this pull request may close these issues.

2 participants