[pve-devel] [PATCH v2 manager 1/3] vzdump: notes-template: avoid escaping meta-characters upon replace

[pve-devel] [PATCH v2 manager 1/3] vzdump: notes-template: avoid escaping meta-characters upon replace

[Fabian Ebner]

which is caused by the quoting operators \Q...\E. The actual intention
was to avoid such surprises.

Fixes: 413bb432 ("partially close #438: vzdump: support setting notes-template")
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * Add follow-ups to further improve behavior.

 PVE/VZDump.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 22fdb324..9af2de3d 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -90,7 +90,7 @@ my $generate_notes = sub {
     $notes_template =~ s/\\(.)/$unescape->($1)/eg;
 
     my $vars = join('|', keys $info->%*);
-    $notes_template =~ s/\{\{($vars)\}\}/\Q$info->{$1}\E/g;
+    $notes_template =~ s/\{\{($vars)\}\}/$info->{$1}/g;
 
     die "unexpected variable name '$1'" if $notes_template =~ m/\{\{([^\s]+)\}\}/;
 
-- 
2.30.2

[pve-devel] [PATCH v2 manager 2/3] vzdump: notes-template: improve check for unknown variable

[Fabian Ebner]

so that '{{foo}}{{bar}}' is not detected as being an unknown variable
named 'foo}}{{bar', but as 'foo' (and 'bar').

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 9af2de3d..05cc1bec 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -92,7 +92,7 @@ my $generate_notes = sub {
     my $vars = join('|', keys $info->%*);
     $notes_template =~ s/\{\{($vars)\}\}/$info->{$1}/g;
 
-    die "unexpected variable name '$1'" if $notes_template =~ m/\{\{([^\s]+)\}\}/;
+    die "unexpected variable name '$1'" if $notes_template =~ m/\{\{([^\s}]+)\}\}/;
 
     return $notes_template;
 };
-- 
2.30.2

[pve-devel] applied: [PATCH v2 manager 2/3] vzdump: notes-template: improve check for unknown variable

[Thomas Lamprecht]

Am 5/3/22 um 13:17 schrieb Fabian Ebner:
> so that '{{foo}}{{bar}}' is not detected as being an unknown variable
> named 'foo}}{{bar', but as 'foo' (and 'bar').
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/VZDump.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index 9af2de3d..05cc1bec 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -92,7 +92,7 @@ my $generate_notes = sub {
>      my $vars = join('|', keys $info->%*);
>      $notes_template =~ s/\{\{($vars)\}\}/$info->{$1}/g;
>  
> -    die "unexpected variable name '$1'" if $notes_template =~ m/\{\{([^\s]+)\}\}/;
> +    die "unexpected variable name '$1'" if $notes_template =~ m/\{\{([^\s}]+)\}\}/;

applied, but added an trailing \n to the error above to avoid the perl file context
ugliness, thanks.

[pve-devel] [PATCH/RFC v2 manager 3/3] vzdump: notes-template: replace unknown variable with error string

[Fabian Ebner]

rather than not setting the notes at all. They still can contain
useful information, and it likely is surprising to users to lose all
of the note when an unkown variable (or simply mistyped) is provided.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump.pm | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 05cc1bec..79dcd62b 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -71,7 +71,7 @@ sub run_command {
 }
 
 my $generate_notes = sub {
-    my ($notes_template, $task) = @_;
+    my ($notes_template, $task, $logfd) = @_;
 
     my $info = {
 	cluster => PVE::Cluster::get_clinfo()->{cluster}->{name},
@@ -89,10 +89,14 @@ my $generate_notes = sub {
 
     $notes_template =~ s/\\(.)/$unescape->($1)/eg;
 
-    my $vars = join('|', keys $info->%*);
-    $notes_template =~ s/\{\{($vars)\}\}/$info->{$1}/g;
+    my $replace_var = sub {
+	my ($var) = @_;
+	return $info->{$var} if exists($info->{$var});
+	debugmsg('warn', "notes-template: unknown variable '$var'", $logfd);
+	return "<unknown variable '$var'>";
+    };
 
-    die "unexpected variable name '$1'" if $notes_template =~ m/\{\{([^\s}]+)\}\}/;
+    $notes_template =~ s/\{\{([^\s}]+)\}\}/$replace_var->($1)/eg;
 
     return $notes_template;
 };
@@ -1045,7 +1049,7 @@ sub exec_backup_task {
 
 	    if ($opts->{'notes-template'} && $opts->{'notes-template'} ne '') {
 		debugmsg('info', "adding notes to backup", $logfd);
-		my $notes = eval { $generate_notes->($opts->{'notes-template'}, $task); };
+		my $notes = eval { $generate_notes->($opts->{'notes-template'}, $task, $logfd); };
 		if (my $err = $@) {
 		    debugmsg('warn', "unable to add notes - $err", $logfd);
 		} else {
-- 
2.30.2

Re: [pve-devel] [PATCH/RFC v2 manager 3/3] vzdump: notes-template: replace unknown variable with error string

[Thomas Lamprecht]

Am 5/3/22 um 13:18 schrieb Fabian Ebner:
> rather than not setting the notes at all. They still can contain
> useful information, and it likely is surprising to users to lose all
> of the note when an unkown variable (or simply mistyped) is provided.
> 

This feels like approaching the problem from the wrong end to me, why not check on add?

For jobs that's relatively simple and for manual VZDump runs we could explicitly do the
check to, e.g. something like (for jobs only & untested):


----8<----
diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 5d36789a..35482bc7 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -206,6 +206,18 @@ __PACKAGE__->register_method({
 
        $param->{enabled} = 1 if !defined($param->{enabled});
 
+       if (my $tmpl = $param->{'notes-template'}) {
+           my $problematic = [];
+           while($tmpl =~ /\{\{([^\s{}]+)\}\}/g) {
+               my $var = $1;
+               push @$problematic, "'$var' at char " . ((pos $tmpl) - length($var))
+                   if $var !~ /^(cluster|guestname|node|vmid)$/;
+           }
+           if (scalar(@$problematic)) {
+               raise_param_exc({ 'notes-template' => "found unknown variables: " . join(', ', @$problematic) });
+           }
+       }
+
        # autogenerate id for api compatibility FIXME remove with 8.0
        my $id = extract_param($param, 'id') // UUID::uuid();

[pve-devel] applied: [PATCH v2 manager 1/3] vzdump: notes-template: avoid escaping meta-characters upon replace

[Thomas Lamprecht]

Am 5/3/22 um 13:17 schrieb Fabian Ebner:
> which is caused by the quoting operators \Q...\E. The actual intention
> was to avoid such surprises.
> 
> Fixes: 413bb432 ("partially close #438: vzdump: support setting notes-template")
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes from v1:
>     * Add follow-ups to further improve behavior.
> 
>  PVE/VZDump.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!