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

[Dominik Csapak]

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 ;) )

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


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