[pbs-devel] [PATCH widget-toolkit/proxmox-backup v2 0/5] fix #5463: add optional consent banner before login

[Gabriel Goller]

On 06.06.2024 12:30, Dominik Csapak wrote:
>On 6/6/24 12:18, Gabriel Goller wrote:
>>Thanks for reviewing this!
>>
>>On 05.06.2024 15:22, Dominik Csapak wrote:
>>>did not look too closely at the code, but gave it a spin and found a few problems/
>>>have suggestions:
>>>
>>>* handlebars by default does html escaping (https://docs.rs/handlebars/latest/handlebars/#escaping)
>>> so any of the reserved characters will be wrong
>>> (namely as html escape sequence such as '"')
>>
>>Hmm yes, this is because encodeURI encodes all characters that
>>handlebars escapes, **except**:
>>  - "&"
>>  - "'"
>>  - "="
>>so these are the ones that currently don't work.
>>We could switch to encodeURIComponent, which also encodes the "&" and
>>the "=". This would only leave us with the "'", but we could just forbid
>>it using a validator and be done with it.
>>
>
>probably a dumb question, but why don't we use base64 encoding/decoding?
>the only downside to that is IMO that it's not readable in the config file itself,
>but that should be ok?
>
>then we don't have a problem with any "special" characters on reading, no?
>we just have to use 'htmlEncode' on the contents to show exactly what was saved
>(untested though ;) )
>

Hmm I though about the base64 encoding as well... It would be perfect
for saving/retrieving everything safely, but I don't know about the
representation in the config file. I think some users are still going to
edit this in the config (although it's a PITA escaping everything) and
that will be very difficult with base64.

To be honest, currently the esacaping with the encodeURIComponent works
quite well, the only thing we need to look out for is the single quite
("'"), because otherwise it gets encoded twice with encodeURIComponent
**and** handlbars. With base64 we also would need handle the "=", which
gets encoded by hanldebars.

I don't have any strong opinions though, so I'll let you decide... :)
If we do it, we should maybe rename the option to consent_test_base64 or
something, so that it's instantly visible that it's encoded?

>>>* that accidentally prevented code injection when directly editing the config file
>>> this is something we should do even if we assume that the text was set through the api
>>> just a simple search/replace of some specific characters such as "< etc. should be enough
>>>* there is still a code execution potential, namely on the rendering part of the config
>>> in configuration -> other (works e.g. by setting <svg onmouseover=alert(1)></svg>)
>>
>>Correct, this only works in the configuration menu though (not in the
>>consent banner before login). Added a validator that prohibits "<" and
>>">", so we should be ok. Again this is only the "preview" of the consent
>>text, so it shouldn't be too harmful. Regardless, we could also not
>>render this and just show the encoded version, but I think this works
>>fine now.
>>
>>Maybe we should also prohibit "<" and ">" on the api-side... Otherwise a
>>use could add "<svg onmouseover=alert(1)></svg>" with the api, and then
>>when opening the configuration ui, the alert would popup. So on the
>>server I would just check for "%3C" ("<") or "%3E" (">"). What do you
>>think?
>>
>
>that's easier done with 'Ext.htmlEncode', no need to reinvent the wheel

True. This makes it a lot easier, we also don't need the "<" and ">"
escaping!

>>>* it's not possible to delete the text again from the ui
>>>* if it's deleted (by api or by hand) 'undefined' is rendered
>>
>>Fixed this: a simple "skipEmptyText: false".
>>
>>>* i really would like markdown support here too ;)
>>
>>This is possible as all the markdown rendering is already present in
>>widget-toolkit!
>>
>>We just need to kinda rearrange the imports in index.hbs like this:
>>
>>     <script type="text/javascript">
>>     Proxmox = {
>>     Setup: { auth_cookie_name: 'PBSAuthCookie' },
>>     NodeName: "{{ NodeName }}",
>>     UserName: "{{ UserName }}",
>>     defaultLang: "{{ language }}",
>>     CSRFPreventionToken: "{{ CSRFPreventionToken }}",
>>     };
>>     </script>
>>     <script type="text/javascript" src="/widgettoolkit/proxmoxlib.js"></script>
>>     <script type="text/javascript">
>>     Proxmox.consentText = Proxmox.Markdown.parse(decodeURIComponent("{{ consentText }}"));
>>     </script>
>>
>>So that we have Proxmox.Markdown available. This worked for me, I hope
>>this doesn't have any other implications I don't know about :)
>>
>
>why here though? we can simply parse in in the constructor of the consent dialog?
>
>i'd generally leave it as original as it gets in the the 'Proxmox.consentText' variable and
>only parse/modify it when we show it?
>

oof, yeah I'm stupid :)