From: Gabriel Goller <g.goller@proxmox.com>
To: Dominik Csapak <d.csapak@proxmox.com>
Cc: Thomas Lamprecht <t.lamprecht@proxmox.com>,
Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH widget-toolkit/proxmox-backup v2 0/5] fix #5463: add optional consent banner before login
Date: Thu, 6 Jun 2024 14:56:45 +0200 [thread overview]
Message-ID: <20240606125645.rsymn3ns7qu6vile@luna.proxmox.com> (raw)
In-Reply-To: <01429bf0-df9a-402d-9089-e36a460b4da8@proxmox.com>
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 ^^
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
next prev parent reply other threads:[~2024-06-06 12:56 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-06-04 12:50 Gabriel Goller
2024-06-04 12:50 ` [pbs-devel] [PATCH widget-toolkit v2 1/5] window: add consent modal Gabriel Goller
2024-06-04 12:50 ` [pbs-devel] [PATCH widget-toolkit v2 2/5] form: add support for multiline textarea Gabriel Goller
2024-06-04 12:50 ` [pbs-devel] [PATCH proxmox-backup v2 3/5] api: add consent api handler and config option Gabriel Goller
2024-06-04 12:50 ` [pbs-devel] [PATCH proxmox-backup v2 4/5] ui: show consent banner before login Gabriel Goller
2024-06-04 12:50 ` [pbs-devel] [PATCH proxmox-backup v2 5/5] docs: add section about consent banner Gabriel Goller
2024-06-05 13:22 ` [pbs-devel] [PATCH widget-toolkit/proxmox-backup v2 0/5] fix #5463: add optional consent banner before login Dominik Csapak
2024-06-06 10:18 ` Gabriel Goller
2024-06-06 10:30 ` Dominik Csapak
2024-06-06 11:25 ` Gabriel Goller
2024-06-06 12:09 ` Dominik Csapak
2024-06-06 12:56 ` Gabriel Goller [this message]
2024-06-06 13:04 ` Thomas Lamprecht
2024-06-07 8:08 ` Gabriel Goller
2024-06-07 11:48 ` Gabriel Goller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240606125645.rsymn3ns7qu6vile@luna.proxmox.com \
--to=g.goller@proxmox.com \
--cc=d.csapak@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
--cc=t.lamprecht@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox