From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 519A51FF396 for ; Thu, 6 Jun 2024 12:18:28 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E2546D0D6; Thu, 6 Jun 2024 12:18:58 +0200 (CEST) Date: Thu, 6 Jun 2024 12:18:24 +0200 From: Gabriel Goller To: Dominik Csapak Message-ID: <20240606101824.mhn43k2dqplospkz@luna.proxmox.com> References: <20240604125014.210321-1-g.goller@proxmox.com> <9a5e685e-d935-414a-93c7-6b3b46bda866@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <9a5e685e-d935-414a-93c7-6b3b46bda866@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.062 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH widget-toolkit/proxmox-backup v2 0/5] fix #5463: add optional consent banner before login X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Cc: Proxmox Backup Server development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 ) 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 "" 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: 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