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

[Gabriel Goller]

On 06.06.2024 14:09, Dominik Csapak wrote:
>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?

haven't tested it, but AFAICT handlebars escapes '=' to '=', so we
would need to "unespace", then decode the base64. (Which tbh won't be an
issue, but just wanted to mention it.)

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

yes, I guess we would want to catch that, log and error and return
nothing. (I don't think such an error message in the consent-banner is
approriate)

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

Hmm, are we sure that that won't cause any problems?

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

Agree.

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