[pbs-devel] [PATCH proxmox-backup] tape: fix anchoring of datastore map regex

[pbs-devel] [PATCH proxmox-backup] tape: fix anchoring of datastore map regex

[Dominik Csapak]

was missing. In practice it was not doing any harm, since we always
checked for an existing datastore with the target name anyway, and
if the source did not exists, we could not restore from that anyway.

Both the source datastore on the tape, as well as the existing
datastores have to adhere to the correct regex.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 72e8d1ee..7f8c1f09 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -34,7 +34,7 @@ const_regex! {
     pub SNAPSHOT_PATH_REGEX = concat!(r"^", SNAPSHOT_PATH_REGEX_STR!(), r"$");
     pub GROUP_OR_SNAPSHOT_PATH_REGEX = concat!(r"^", GROUP_OR_SNAPSHOT_PATH_REGEX_STR!(), r"$");
 
-    pub DATASTORE_MAP_REGEX = concat!(r"(:?", PROXMOX_SAFE_ID_REGEX_STR!(), r"=)?", PROXMOX_SAFE_ID_REGEX_STR!());
+    pub DATASTORE_MAP_REGEX = concat!(r"^(:?", PROXMOX_SAFE_ID_REGEX_STR!(), r"=)?", PROXMOX_SAFE_ID_REGEX_STR!(), r"$");
 }
 
 pub const CHUNK_DIGEST_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&SHA256_HEX_REGEX);
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup] tape: fix anchoring of datastore map regex

[Thomas Lamprecht]

Am 29/03/2023 um 12:06 schrieb Dominik Csapak:
> was missing. In practice it was not doing any harm, since we always
> checked for an existing datastore with the target name anyway, and
> if the source did not exists, we could not restore from that anyway.
> 
> Both the source datastore on the tape, as well as the existing
> datastores have to adhere to the correct regex.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-api-types/src/datastore.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

for the record, I spearheaded this one, but change seems to be the same:

https://git.proxmox.com/?p=proxmox-backup.git;a=commitdiff;h=6477df8f89b94e1a708d1f88420832c150c7bdcc

Re: [pbs-devel] [PATCH proxmox-backup] tape: fix anchoring of datastore map regex

[Stefan Sterz]

On 3/29/23 12:06, Dominik Csapak wrote:
> was missing. In practice it was not doing any harm, since we always
> checked for an existing datastore with the target name anyway, and
> if the source did not exists, we could not restore from that anyway.
> 
> Both the source datastore on the tape, as well as the existing
> datastores have to adhere to the correct regex.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-api-types/src/datastore.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 72e8d1ee..7f8c1f09 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -34,7 +34,7 @@ const_regex! {
>      pub SNAPSHOT_PATH_REGEX = concat!(r"^", SNAPSHOT_PATH_REGEX_STR!(), r"$");
>      pub GROUP_OR_SNAPSHOT_PATH_REGEX = concat!(r"^", GROUP_OR_SNAPSHOT_PATH_REGEX_STR!(), r"$");
>  
> -    pub DATASTORE_MAP_REGEX = concat!(r"(:?", PROXMOX_SAFE_ID_REGEX_STR!(), r"=)?", PROXMOX_SAFE_ID_REGEX_STR!());
> +    pub DATASTORE_MAP_REGEX = concat!(r"^(:?", PROXMOX_SAFE_ID_REGEX_STR!(), r"=)?", PROXMOX_SAFE_ID_REGEX_STR!(), r"$");

small question: should the `(:?` be a non-capturing group (`(?:`) here
instead? or am i missing something? afaict this would allow colons at
the beginning of the source datastore, creating a similar problem as was
fixed here, i'd assume.

>  }
>  
>  pub const CHUNK_DIGEST_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&SHA256_HEX_REGEX);

Re: [pbs-devel] [PATCH proxmox-backup] tape: fix anchoring of datastore map regex

[Dominik Csapak]

On 3/29/23 13:50, Stefan Sterz wrote:
> On 3/29/23 12:06, Dominik Csapak wrote:
>> was missing. In practice it was not doing any harm, since we always
>> checked for an existing datastore with the target name anyway, and
>> if the source did not exists, we could not restore from that anyway.
>>
>> Both the source datastore on the tape, as well as the existing
>> datastores have to adhere to the correct regex.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   pbs-api-types/src/datastore.rs | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
>> index 72e8d1ee..7f8c1f09 100644
>> --- a/pbs-api-types/src/datastore.rs
>> +++ b/pbs-api-types/src/datastore.rs
>> @@ -34,7 +34,7 @@ const_regex! {
>>       pub SNAPSHOT_PATH_REGEX = concat!(r"^", SNAPSHOT_PATH_REGEX_STR!(), r"$");
>>       pub GROUP_OR_SNAPSHOT_PATH_REGEX = concat!(r"^", GROUP_OR_SNAPSHOT_PATH_REGEX_STR!(), r"$");
>>   
>> -    pub DATASTORE_MAP_REGEX = concat!(r"(:?", PROXMOX_SAFE_ID_REGEX_STR!(), r"=)?", PROXMOX_SAFE_ID_REGEX_STR!());
>> +    pub DATASTORE_MAP_REGEX = concat!(r"^(:?", PROXMOX_SAFE_ID_REGEX_STR!(), r"=)?", PROXMOX_SAFE_ID_REGEX_STR!(), r"$");
> 
> small question: should the `(:?` be a non-capturing group (`(?:`) here
> instead? or am i missing something? afaict this would allow colons at
> the beginning of the source datastore, creating a similar problem as was
> fixed here, i'd assume.

yep, you're right, should be '(?:' instead


at least 2 other regex also have this wrong:

----
pub BLOCKDEVICE_NAME_REGEX = r"^(:?(:?h|s|x?v)d[a-z]+)|(:?nvme\d+n\d+)$";
----

and

----
pub TAPE_RESTORE_SNAPSHOT_REGEX = concat!(r"^", PROXMOX_SAFE_ID_REGEX_STR!(), r"
----

will send a patch...
> 
>>   }
>>   
>>   pub const CHUNK_DIGEST_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&SHA256_HEX_REGEX);
> 

Re: [pbs-devel] [PATCH proxmox-backup] tape: fix anchoring of datastore map regex

[Thomas Lamprecht]

Am 29/03/2023 um 13:54 schrieb Dominik Csapak:
>> small question: should the `(:?` be a non-capturing group (`(?:`) here
>> instead? or am i missing something? afaict this would allow colons at
>> the beginning of the source datastore, creating a similar problem as was
>> fixed here, i'd assume.

good catch!

> yep, you're right, should be '(?:' instead
> 
> 
> at least 2 other regex also have this wrong:
> 
> ----
> pub BLOCKDEVICE_NAME_REGEX = r"^(:?(:?h|s|x?v)d[a-z]+)|(:?nvme\d+n\d+)$";
> ----
> 
> and
> 
> ----
> pub TAPE_RESTORE_SNAPSHOT_REGEX = concat!(r"^", PROXMOX_SAFE_ID_REGEX_STR!(), r"
> ----
> 
> will send a patch...

please also ensure that the capture.group access ID is still right then,
if used