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

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

[Gabriel Goller]

The consent text is stored in the node.cfg config file and is encoded
using encodeURI [0] on the frontend. This allows us to support multi-line
strings and special characters. To easily edit the text a new edit-field 
called ProxmoxTextAreaField has been introduced. It supports editing and 
saving multi-line text and converting it to its URI-encoded representation.

The same implementation will be ported to pve and eventually pmg in 
the foreseeable future.

v2, thanks @Thomas, @Dominik:
 - remove consent.txt file, move to node.cfg config
 - add ui option to insert consent text
 - encode text with encodeURI/decodeURI

v1, thanks @Dominik:
 - embed consent text into index.html file instead of extra api request
 - removed decline button
 - added alwaysOnTop property to popup 

[0]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURI

widget-toolkit:

Gabriel Goller (2):
  window: add consent modal
  form: add support for multiline textarea

 src/Makefile               |  2 ++
 src/form/TextAreaField.js  | 60 ++++++++++++++++++++++++++++++++++++++
 src/grid/ObjectGrid.js     | 29 ++++++++++++++++++
 src/window/ConsentModal.js | 34 +++++++++++++++++++++
 4 files changed, 125 insertions(+)
 create mode 100644 src/form/TextAreaField.js
 create mode 100644 src/window/ConsentModal.js


backup:

Gabriel Goller (3):
  api: add consent api handler and config option
  ui: show consent banner before login
  docs: add section about consent banner

 docs/gui.rst                    |  7 +++++++
 src/api2/node/config.rs         |  8 ++++++++
 src/bin/proxmox-backup-proxy.rs | 11 ++++++++---
 src/config/node.rs              |  4 ++++
 www/LoginView.js                |  9 +++++++++
 www/config/NodeOptionView.js    |  6 ++++++
 www/index.hbs                   |  1 +
 7 files changed, 43 insertions(+), 3 deletions(-)


Summary over all repositories:
  11 files changed, 168 insertions(+), 3 deletions(-)

-- 
Generated by git-murpp 0.5.0


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

[pbs-devel] [PATCH widget-toolkit v2 1/5] window: add consent modal

[Gabriel Goller]

Add consentModal that gets displayed before the login. Simply shows the
text in a scrollable box and contains a single button "OK".

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 src/Makefile               |  1 +
 src/window/ConsentModal.js | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 src/window/ConsentModal.js

diff --git a/src/Makefile b/src/Makefile
index 0478251..3c2fd4b 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -87,6 +87,7 @@ JSSRC=					\
 	window/DiskSmart.js		\
 	window/ZFSDetail.js		\
 	window/Certificates.js		\
+	window/ConsentModal.js		\
 	window/ACMEAccount.js		\
 	window/ACMEPluginEdit.js	\
 	window/ACMEDomains.js		\
diff --git a/src/window/ConsentModal.js b/src/window/ConsentModal.js
new file mode 100644
index 0000000..16e862c
--- /dev/null
+++ b/src/window/ConsentModal.js
@@ -0,0 +1,34 @@
+Ext.define('Proxmox.window.ConsentModal', {
+    extend: 'Ext.window.Window',
+    alias: ['widget.pmxConsentModal'],
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    width: 600,
+    modal: true,
+    closable: false,
+    resizable: false,
+    alwaysOnTop: true,
+    title: gettext('Consent'),
+
+    items: [
+	{
+	    xtype: 'textareafield',
+	    cbind: {
+		value: '{consent}',
+	    },
+	    editable: false,
+	    width: 600,
+	    height: 400,
+	    scrollable: 'y',
+	},
+    ],
+    buttons: [
+	{
+	    handler: function() {
+		this.up('window').close();
+	    },
+	    text: gettext('OK'),
+	},
+    ],
+});
+
-- 
2.43.0



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

[pbs-devel] [PATCH widget-toolkit v2 2/5] form: add support for multiline textarea

[Gabriel Goller]

This adds support for a editable multiline textarea in the ObjectGrid.
Now we can add a textarea row, which will open a textarea popup, and
encode the multi-line text into an URI using encodeURI/decodeURI
[0].

[0]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURI

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 src/Makefile              |  1 +
 src/form/TextAreaField.js | 60 +++++++++++++++++++++++++++++++++++++++
 src/grid/ObjectGrid.js    | 29 +++++++++++++++++++
 3 files changed, 90 insertions(+)
 create mode 100644 src/form/TextAreaField.js

diff --git a/src/Makefile b/src/Makefile
index 3c2fd4b..aa81bef 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -31,6 +31,7 @@ JSSRC=					\
 	form/ExpireDate.js		\
 	form/IntegerField.js		\
 	form/TextField.js		\
+	form/TextAreaField.js		\
 	form/VlanField.js		\
 	form/DateTimeField.js		\
 	form/Checkbox.js		\
diff --git a/src/form/TextAreaField.js b/src/form/TextAreaField.js
new file mode 100644
index 0000000..a44279a
--- /dev/null
+++ b/src/form/TextAreaField.js
@@ -0,0 +1,60 @@
+Ext.define('Proxmox.form.field.Textareafield', {
+    extend: 'Ext.form.field.TextArea',
+    alias: ['widget.proxmoxtextareafield'],
+
+    config: {
+        skipEmptyText: true,
+        deleteEmpty: false,
+        trimValue: false,
+        editable: true,
+        width: 600,
+        height: 400,
+        scrollable: 'y',
+    },
+
+    setValue: function(value) {
+        // We want to edit the decoded version of the text
+        this.callParent([decodeURI(value)]);
+    },
+
+    processRawValue: function(value) {
+        // The field could contain multi-line values
+        return encodeURI(value);
+    },
+
+    getSubmitData: function() {
+        let me = this,
+            data = null,
+            val;
+        if (!me.disabled && me.submitValue && !me.isFileUpload()) {
+            val = me.getSubmitValue();
+            if (val !== null) {
+                data = {};
+                data[me.getName()] = val;
+            } else if (me.getDeleteEmpty()) {
+                data = {};
+                data.delete = me.getName();
+            }
+        }
+        return data;
+    },
+
+    getSubmitValue: function() {
+        let me = this;
+
+        let value = this.processRawValue(this.getRawValue());
+        if (me.getTrimValue() && typeof value === 'string') {
+            value = value.trim();
+        }
+        if (value !== '') {
+            return value;
+        }
+
+        return me.getSkipEmptyText() ? null: value;
+    },
+
+    setAllowBlank: function(allowBlank) {
+        this.allowBlank = allowBlank;
+        this.validate();
+    },
+});
diff --git a/src/grid/ObjectGrid.js b/src/grid/ObjectGrid.js
index b355d6d..c823e21 100644
--- a/src/grid/ObjectGrid.js
+++ b/src/grid/ObjectGrid.js
@@ -182,6 +182,35 @@ Ext.define('Proxmox.grid.ObjectGrid', {
 	};
     },
 
+    add_textareafield_row: function(name, text, opts) {
+	let me = this;
+
+	opts = opts || {};
+	me.rows = me.rows || {};
+
+	me.rows[name] = {
+	    required: true,
+	    defaultValue: opts.defaultValue,
+	    header: text,
+	    renderer: function(value) {
+		return decodeURI(value);
+	    },
+	    editor: {
+		xtype: 'proxmoxWindowEdit',
+		subject: text,
+		onlineHelp: opts.onlineHelp,
+		fieldDefaults: {
+		    labelWidth: opts.labelWidth || 600,
+		},
+		items: {
+		    xtype: 'proxmoxtextareafield',
+		    name: name,
+		},
+	    },
+	};
+    },
+
+
     editorConfig: {}, // default config passed to editor
 
     run_editor: function() {
-- 
2.43.0



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

[pbs-devel] [PATCH proxmox-backup v2 3/5] api: add consent api handler and config option

[Gabriel Goller]

Add consent_text option to the node.cfg config. Embed the value into
index.html file using handlebars.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 src/api2/node/config.rs         |  8 ++++++++
 src/bin/proxmox-backup-proxy.rs | 11 ++++++++---
 src/config/node.rs              |  4 ++++
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/src/api2/node/config.rs b/src/api2/node/config.rs
index 86a73cf8..19ede24b 100644
--- a/src/api2/node/config.rs
+++ b/src/api2/node/config.rs
@@ -67,6 +67,8 @@ pub enum DeletableProperty {
     Description,
     /// Delete the task-log-max-days property
     TaskLogMaxDays,
+    /// Delete the consent-text property
+    ConsentText,
 }
 
 #[api(
@@ -155,6 +157,9 @@ pub fn update_node_config(
                 DeletableProperty::TaskLogMaxDays => {
                     config.task_log_max_days = None;
                 }
+                DeletableProperty::ConsentText => {
+                    config.consent_text = None;
+                }
             }
         }
     }
@@ -198,6 +203,9 @@ pub fn update_node_config(
     if update.task_log_max_days.is_some() {
         config.task_log_max_days = update.task_log_max_days;
     }
+    if update.consent_text.is_some() {
+        config.consent_text = update.consent_text;
+    }
 
     crate::config::node::save_config(&config)?;
 
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 15444685..4d8542b6 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -49,12 +49,12 @@ use pbs_api_types::{
 
 use proxmox_rest_server::daemon;
 
-use proxmox_backup::auth_helpers::*;
 use proxmox_backup::server;
 use proxmox_backup::tools::{
     disks::{zfs_dataset_stats, DiskManage},
     PROXMOX_BACKUP_TCP_KEEPALIVE_TIME,
 };
+use proxmox_backup::{auth_helpers::*, config};
 
 use proxmox_backup::api2::pull::do_sync_job;
 use proxmox_backup::api2::tape::backup::do_tape_backup_job;
@@ -87,7 +87,7 @@ fn get_language(headers: &http::HeaderMap) -> String {
 
     match cookie_from_header(headers, "PBSLangCookie") {
         Some(cookie_lang) if exists(&cookie_lang) => cookie_lang,
-        _ => match proxmox_backup::config::node::config().map(|(cfg, _)| cfg.default_lang) {
+        _ => match config::node::config().map(|(cfg, _)| cfg.default_lang) {
             Ok(Some(default_lang)) if exists(&default_lang) => default_lang,
             _ => String::from(""),
         },
@@ -152,6 +152,10 @@ async fn get_index_future(env: RestEnvironment, parts: Parts) -> Response<Body>
 
     let theme = get_theme(&parts.headers);
 
+    let consent = config::node::config()
+        .ok()
+        .and_then(|config| config.0.consent_text)
+        .unwrap_or("".to_string());
     let data = json!({
         "NodeName": nodename,
         "UserName": user,
@@ -160,6 +164,7 @@ async fn get_index_future(env: RestEnvironment, parts: Parts) -> Response<Body>
         "theme": theme,
         "auto": theme == "auto",
         "debug": debug,
+        "consentText": consent,
     });
 
     let (ct, index) = match api.render_template(template_file, &data) {
@@ -387,7 +392,7 @@ fn make_tls_acceptor() -> Result<SslAcceptor, Error> {
     let key_path = configdir!("/proxy.key");
     let cert_path = configdir!("/proxy.pem");
 
-    let (config, _) = proxmox_backup::config::node::config()?;
+    let (config, _) = config::node::config()?;
     let ciphers_tls_1_3 = config.ciphers_tls_1_3;
     let ciphers_tls_1_2 = config.ciphers_tls_1_2;
 
diff --git a/src/config/node.rs b/src/config/node.rs
index 937beb3a..77f72073 100644
--- a/src/config/node.rs
+++ b/src/config/node.rs
@@ -225,6 +225,10 @@ pub struct NodeConfig {
     /// Maximum days to keep Task logs
     #[serde(skip_serializing_if = "Option::is_none")]
     pub task_log_max_days: Option<usize>,
+
+    /// Consent banner text
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub consent_text: Option<String>,
 }
 
 impl NodeConfig {
-- 
2.43.0



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

[pbs-devel] [PATCH proxmox-backup v2 4/5] ui: show consent banner before login

[Gabriel Goller]

Before showing the LoginView, check if we got a non-empty consent text
from the template. If there is a non-empty text, display it in a modal.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 www/LoginView.js             | 9 +++++++++
 www/config/NodeOptionView.js | 6 ++++++
 www/index.hbs                | 1 +
 3 files changed, 16 insertions(+)

diff --git a/www/LoginView.js b/www/LoginView.js
index d4d8e73e..96f97da7 100644
--- a/www/LoginView.js
+++ b/www/LoginView.js
@@ -20,6 +20,15 @@ Ext.define('PBS.LoginView', {
     controller: {
 	xclass: 'Ext.app.ViewController',
 
+	init: async function() {
+	    if (Proxmox.consentText !== "") {
+		Ext.create('Proxmox.window.ConsentModal', {
+		    autoShow: true,
+		    consent: Proxmox.consentText,
+		});
+	    }
+	},
+
 	submitForm: async function() {
 	    var me = this;
 	    var loginForm = me.lookupReference('loginForm');
diff --git a/www/config/NodeOptionView.js b/www/config/NodeOptionView.js
index ae6453fe..35938f9a 100644
--- a/www/config/NodeOptionView.js
+++ b/www/config/NodeOptionView.js
@@ -54,5 +54,11 @@ Ext.define('PBS.NodeOptionView', {
 	    deleteEmpty: true,
 	    renderer: Proxmox.Utils.render_language,
 	},
+	{
+	    xtype: 'textareafield',
+	    name: 'consent-text',
+	    text: gettext('Consent Text'),
+	    deleteEmpty: true,
+	},
     ],
 });
diff --git a/www/index.hbs b/www/index.hbs
index 824268e3..f1729cef 100644
--- a/www/index.hbs
+++ b/www/index.hbs
@@ -38,6 +38,7 @@
 	UserName: "{{ UserName }}",
 	defaultLang: "{{ language }}",
 	CSRFPreventionToken: "{{ CSRFPreventionToken }}",
+	consentText: decodeURI("{{ consentText }}"),
     };
     </script>
     <script type="text/javascript" src="/widgettoolkit/proxmoxlib.js"></script>
-- 
2.43.0



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

[pbs-devel] [PATCH proxmox-backup v2 5/5] docs: add section about consent banner

[Gabriel Goller]

Add short section on how to enable consent banner.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 docs/gui.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/docs/gui.rst b/docs/gui.rst
index 9547c73f..a2999bfd 100644
--- a/docs/gui.rst
+++ b/docs/gui.rst
@@ -40,6 +40,13 @@ Proxmox Backup Server supports various languages and authentication back ends
 .. note:: For convenience, you can save the username on the client side, by
   selecting the "Save User name" checkbox at the bottom of the window.
 
+Consent Banner
+^^^^^^^^^^^^^^
+
+A custom consent banner that has to be accepted before login can be
+configured in **Configuration -> Other -> General -> Consent Text**. If
+there is no content, the consent banner will not be displayed.
+
 
 GUI Overview
 ------------
-- 
2.43.0



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

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

[Dominik Csapak]

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 '"')
* 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>)
* it's not possible to delete the text again from the ui
* if it's deleted (by api or by hand) 'undefined' is rendered
* i really would like markdown support here too ;)


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

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

[Gabriel Goller]

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

Re: [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?


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

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

[Gabriel Goller]

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.

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.

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?

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



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

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

[Dominik Csapak]

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?

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

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

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

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

Re: [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 ^^
>


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

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

[Thomas Lamprecht]

Am 06/06/2024 um 14:09 schrieb Dominik Csapak:
>> 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 ?

We normally do not include the format in the property names, but it still
_might_ be OK to add it there.
Otherwise, the description can mention it and a validator should catch actual
invalid base64 though, as any space is invalid it would be quite obvious to
admins when the test how it looks.

Alternatively (just as idea, not as recommendation) you could make it rather
obvious by requiring a format, or encoding, like:

ui_consent: format=base64,value=...

or possibly already with the type in there:

consent: type=ui,format=base64,value=...


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

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

[Gabriel Goller]

On 06.06.2024 15:04, Thomas Lamprecht wrote:
>Am 06/06/2024 um 14:09 schrieb Dominik Csapak:
>>> 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 ?
>
>We normally do not include the format in the property names, but it still
>_might_ be OK to add it there.
>Otherwise, the description can mention it and a validator should catch actual
>invalid base64 though, as any space is invalid it would be quite obvious to
>admins when the test how it looks.
>
>Alternatively (just as idea, not as recommendation) you could make it rather
>obvious by requiring a format, or encoding, like:
>
>ui_consent: format=base64,value=...
>
>or possibly already with the type in there:
>
>consent: type=ui,format=base64,value=...

Hmm, idk, for me this looks like there are different formats and types
available—which we don't plan to add anytime soon.


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

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

[Gabriel Goller]

Submitted a new version!

Note: I left the config attribute as it is ('consent-text'), because I
thought the ending '==' is enough of an indicator that it is encoded
with base64. (I also mentioned it in the docs.)


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