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

[Gabriel Goller <g.goller@proxmox.com>]

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.

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


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



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel