Redirect: syntax highlight for $variable
#634
Conversation
This example was missing. On top of #634
| </div> | ||
| <code data-bind="text: redirect_to"></code> | ||
| <div class="sub header">{% trans "Redirect to" %}</div> | ||
| <code data-bind="html: redirect_to"></code> |
There was a problem hiding this comment.
This is likely vulnerable to XSS, we are dealing with user input here.
There was a problem hiding this comment.
Can we mark the user input as unsafe somehow while keeping the current pattern? I mean, calling something like unsafe(user_input) so all the HTML tags are converted to regular safe strings.
There was a problem hiding this comment.
Don't think knockout has a feature like that. This is probably better as a web component, so you don't need to build the HTML manually.
There was a problem hiding this comment.
I used DOMPurify.sanitize() which is the same we are already using for the notifications.
There was a problem hiding this comment.
What you need here is to escape the text, not sanitize it.
There was a problem hiding this comment.
Injecting HTML is still dangerous, it can be used to leak information from the page, steal credentials, etc. DOMPurify is being used as an additional layer for notifications, but all values are escaped in notifications.
There was a problem hiding this comment.
DOMPurify sanitizes HTML and prevents XSS attacks. You can feed DOMPurify with string full of dirty HTML and it will return a string (unless configured otherwise) with clean HTML. DOMPurify will strip out everything that contains dangerous HTML and thereby prevent XSS attacks and other nastiness.
(this is from the https://github.com/cure53/DOMPurify)
Whom are we protecting against? If the user adds HTML to the to_/from_ fields they are the only ones that are going to "evaluate" that HTML code --so, they will be "stealing" themselves. In any case, DOMPurify will prevent this to happen by making the HTML written to be clean.
There was a problem hiding this comment.
Again, this isn't just about being able to execute JS, it's about never allowing users to include arbitrary HTML in the dashboard. Injecting HMLT without executing JS is still a security risk that allows exfiltrating information. This page is shared across maintainers, so this isn't a self-injection vuln, but even there, we should never allow users to inject HTML into our pages.
There was a problem hiding this comment.
I think the approach here could be easily tweaked slightly. It is a bit janky you can input HTML into the field and it renders:
It's not necessary to sanitize the user input if we just treat it as text instead of HTML, so splitting up our HTML and the user input text would solve this.
Ideally, I wanted this to be a web component so that the same display could be used in the listing view as well. That can be a next step, though I had a POC kicking around in my stash for this too.
There was a problem hiding this comment.
I pushed up a commit that just splits the field up into a prefix and the user content. It's not a great pattern, we should probably just replace this all with a web component that renders everything consistently.
src/js/project/admin.js
Outdated
|
|
||
| this.redirect_from = ko.computed(() => { | ||
| var from_url = this.from_url(); | ||
| var from_url = DOMPurify.sanitize(this.from_url()); |
There was a problem hiding this comment.
Note, const here should be used instead of var, the value is immutable.
|
Cool! Are we ready to merge this PR? |
|
#633 is conflicting with this and could just be brought in here too, I've got a little energy today I'll attempt that update here as well. |
|
Actually, that seemed to already be added to the logic here too. This could use a look over still but should be safe otherwise. |
<img width="607" height="320" alt="Screenshot_2025-07-16_16-42-23" src="https://github.com/user-attachments/assets/3eb38c81-6c88-4772-a234-39dc62e55252" /> Closes #82 --------- Co-authored-by: Anthony Johnson <[email protected]>
Rebased on main. Build assets are up to date. - Fixes #645 --------- Co-authored-by: Manuel Kaufmann <[email protected]>
Closes #82