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

[Dominik Csapak]

On 6/6/24 13:25, Gabriel Goller wrote:
> 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.

i don't understand completely, what escaping when we save base64 in the config?

if users write nonbase64 there it'll be an error on parsing/decoding?

> 
> 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.

would we have to encode the single quote though? if they
are surrounded by double quotes they shouldn't make a problem?
i guess '\' would make more problems...

the handlebars encoding can be turned off (see the link in my description)
imho we should do that in any case since that's rather unexpected
and asymmetric behaviour

> 
> 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?

yeah the name should probably reflect that, @thomas any opinion regarding
base64 ?


> 
>>>> * 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 also what we should use to display the string in the dialog btw

> 
>>>> * 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 :)
> 
  i doubt that ^^