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

Notifications: think more about how to render them safely #11022

Closed
humitos opened this issue Jan 10, 2024 · 1 comment · Fixed by #11024
Closed

Notifications: think more about how to render them safely #11022

humitos opened this issue Jan 10, 2024 · 1 comment · Fixed by #11024
Assignees
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code

Comments

@humitos
Copy link
Member

humitos commented Jan 10, 2024

@stsewd mentioned a few times that we need to escape all the values we render in the notifications that comes from the user. Due to that, we implement this small trick to run escape() on each of them: #10996

However, we are always passing the instance the notification is attached to (User, Project, Organization, Build, etc) to the Message class to have access to all of fields from it. Due to that, there are some fields like project.name where the user has control and we have to escape() somehow.

I see two possible paths here:

  1. change the rendering engine from regular Python .format()s + trick to call escape() on all the fields and use something like Jinja or regular Django Template Engine where we can call {{ project.name|escape }}
  2. write an improve for the current rendering engine that it's always safe. Maybe using .format_map with a custom dict class that call escape() on __getitem__ (https://docs.python.org/3/library/stdtypes.html#str.format_map)
@humitos humitos added Improvement Minor improvement to code Accepted Accepted issue on our roadmap labels Jan 10, 2024
@stsewd
Copy link
Member

stsewd commented Jan 10, 2024

I think using Django's template engine should be fine, since it escapes all values by default.

humitos added a commit that referenced this issue Jan 10, 2024
We dealt a few times when rendering notifications using simple Python's
`.format()` and we changed our mind to use Django's engine instead.

Closes #11022
@github-project-automation github-project-automation bot moved this to Planned in 📍Roadmap Jan 10, 2024
@humitos humitos moved this from Planned to Needs review in 📍Roadmap Jan 10, 2024
@humitos humitos self-assigned this Jan 10, 2024
humitos added a commit that referenced this issue Jan 11, 2024
* Notifications: use `Template`'s Django engine to render them

We dealt a few times when rendering notifications using simple Python's
`.format()` and we changed our mind to use Django's engine instead.

Closes #11022

* Revert #11018 and use `instance.name` again

* Update tests
@github-project-automation github-project-automation bot moved this from Needs review to Done in 📍Roadmap Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants