Re: [PATCH proxmox v2 1/2] router: compile time check privilege path parameters for existence

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Mon, Aug 31, 2026 at 09:08:47AM +0200, Dominik Csapak wrote:
> In an API privilege, path components can be interpolated from
> parameters, e.g. 'foo/{bar}'. If this parameter does not exist,
> the api privilege check fails and the client gets a 403 error back.
> 
> Instead of only checking this at runtime, check the existence of the
> parameter in the schema during compilation since the schemas need to
> be const anyway. In case of 'additional_properties', we can only
> check it during runtime, but currently there are no such cases for the
> API where a dynamic parameter would be used in the privilege path.
> (And it does not make sense to not statically define such a parameter)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  proxmox-router/src/router.rs | 209 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 208 insertions(+), 1 deletion(-)
> 
> diff --git a/proxmox-router/src/router.rs b/proxmox-router/src/router.rs
> index fea47ba6..ca48dcba 100644
> --- a/proxmox-router/src/router.rs
> +++ b/proxmox-router/src/router.rs
> @@ -16,7 +16,7 @@ use proxmox_http::Body;
>  use serde::Serialize;
>  use serde_json::Value;
>  
> -use proxmox_schema::{ObjectSchema, ParameterSchema, ReturnType, Schema};
> +use proxmox_schema::{AllOfSchema, ObjectSchema, OneOfSchema, ParameterSchema, ReturnType, Schema};
>  
>  use super::Permission;
>  use crate::RpcEnvironment;
> @@ -831,6 +831,131 @@ impl std::fmt::Debug for ApiMethod {
>      }
>  }
>  
> +// const helpers to check privilege parameters
> +
> +const fn byte_slice_eq(a: &[u8], b: &[u8]) -> bool {

We should consider adding a helper crate for such things.
We already have a byte slice compare function in `proxmox-schema`.

> +    if a.len() != b.len() {
> +        return false;
> +    }
> +    let mut i = 0;
> +    while i < a.len() {
> +        if a[i] != b[i] {
> +            return false;
> +        }
> +        i += 1;
> +    }
> +    true
> +}
> +
> +const fn object_schema_has_parameter(object: &ObjectSchema, name: &[u8]) -> bool {
> +    // additional properties are not statically known, so any name could exist
> +    if object.additional_properties {
> +        return true;
> +    }
> +    let mut i = 0;
> +    while i < object.properties.len() {
> +        if byte_slice_eq(object.properties[i].0.as_bytes(), name) {
> +            return true;
> +        }
> +        i += 1;
> +    }
> +    false
> +}
> +
> +const fn all_of_schema_has_parameter(all_of: &AllOfSchema, name: &[u8]) -> bool {
> +    let mut i = 0;
> +    while i < all_of.list.len() {
> +        if schema_has_parameter(all_of.list[i], name) {
> +            return true;
> +        }
> +        i += 1;
> +    }
> +    false
> +}
> +
> +const fn one_of_schema_has_parameter(one_of: &OneOfSchema, name: &[u8]) -> bool {
> +    if byte_slice_eq(one_of.type_property_entry.0.as_bytes(), name) {
> +        return true;
> +    }
> +    let mut i = 0;
> +    while i < one_of.list.len() {
> +        if schema_has_parameter(one_of.list[i].1, name) {
> +            return true;
> +        }
> +        i += 1;
> +    }
> +    false
> +}
> +
> +const fn schema_has_parameter(schema: &Schema, name: &[u8]) -> bool {
> +    match schema {
> +        Schema::Object(object) => object_schema_has_parameter(object, name),
> +        Schema::AllOf(all_of) => all_of_schema_has_parameter(all_of, name),
> +        Schema::OneOf(one_of) => one_of_schema_has_parameter(one_of, name),
> +        _ => false,
> +    }
> +}
> +
> +const fn parameter_exists(parameters: ParameterSchema, name: &[u8]) -> bool {
> +    match parameters {
> +        ParameterSchema::Object(object) => object_schema_has_parameter(object, name),
> +        ParameterSchema::AllOf(all_of) => all_of_schema_has_parameter(all_of, name),
> +        ParameterSchema::OneOf(one_of) => one_of_schema_has_parameter(one_of, name),
> +    }
> +}
> +
> +// mirrors the splitting done by check_api_permission: a component can contain multiple '/'
> +// separated parts, each of which may be a '{name}' parameter reference
> +const fn check_privilege_path_components(component: &str, parameters: ParameterSchema) {
> +    let bytes = component.as_bytes();

We should immediately return if `bytes.len() < 3`, see below ↓

> +    let mut component_start = 0;
> +    let mut pos = 0;
> +    while pos <= bytes.len() {
> +        if pos == bytes.len() || bytes[pos] == b'/' {
> +            let component_len = pos - component_start;
> +            let component_end = pos - 1;

Technically the above can underflow on an empty string where it would be
`0 - 1`.
Not an actual issue, but since we only care about components which are
names enclosed in braces, we could skip this entire function if the
string is not at least 3 characters, because an empty `{}` also does not
make sense.
So we could also add a more informative error message for
`if component_len == 2`, and turn the `>= 2` below into a `> 2`.

> +            if component_len >= 2 && bytes[component_start] == b'{' && bytes[component_end] == b'}'
> +            {
> +                let name = bytes
> +                    .split_at(component_end)
> +                    .0
> +                    .split_at(component_start + 1)
> +                    .1;
> +                if !parameter_exists(parameters, name) {
> +                    panic!(
> +                        "privilege path references a parameter that does not exist in the method's parameter schema"

↑ needs line wrapping

> +                    );
> +                }
> +            }
> +            component_start = pos + 1;
> +        }
> +        pos += 1;
> +    }
> +}
> +
> +const fn assert_path_parameters_exist(perm: &Permission, parameters: ParameterSchema) {
> +    match perm {
> +        Permission::WithParam(_, permission) => {

↑ The first entry is a parameter name - shouldn't we check this one,
too? (The content of that parameter is considered to be the user name
to check `permission` against.)

> +            assert_path_parameters_exist(permission, parameters)
> +        }
> +        Permission::Privilege(path, _, _) => {

↑ nit: would pluralize this to "paths"

> +            let mut i = 0;
> +            while i < path.len() {
> +                check_privilege_path_components(path[i], parameters);
> +                i += 1;
> +            }
> +        }
> +        Permission::And(permissions) | Permission::Or(permissions) => {
> +            let mut i = 0;
> +            while i < permissions.len() {
> +                assert_path_parameters_exist(permissions[i], parameters);
> +                i += 1;
> +            }
> +        }
> +        _ => (),
> +    }
> +}
> +
>  impl ApiMethod {
>      pub const fn new_full(handler: &'static ApiHandler, parameters: ParameterSchema) -> Self {
>          Self {
> @@ -890,11 +1015,19 @@ impl ApiMethod {
>          self
>      }
>  
> +    /// Set the access permissions.
> +    ///
> +    /// This asserts that every '{name}' parameter reference in `Privilege` permission paths
> +    /// exists in the method's parameter schema, since such a path could otherwise never match at
> +    /// runtime. Since API methods are usually built in a const context, a violation is a compile
> +    /// time error.
>      pub const fn access(
>          mut self,
>          description: Option<&'static str>,
>          permission: &'static Permission,
>      ) -> Self {
> +        assert_path_parameters_exist(permission, self.parameters);
> +
>          self.access = ApiAccess {
>              description,
>              permission,
> @@ -903,3 +1036,77 @@ impl ApiMethod {
>          self
>      }
>  }
> +
> +#[cfg(test)]
> +mod test {
> +    use super::*;
> +
> +    use proxmox_schema::StringSchema;
> +
> +    const STRING_SCHEMA: Schema = StringSchema::new("test").schema();
> +
> +    const PARAMETERS: ObjectSchema = ObjectSchema::new(
> +        "test parameters",
> +        &[
> +            ("bar", true, &STRING_SCHEMA),
> +            ("foo", false, &STRING_SCHEMA),
> +        ],
> +    );
> +
> +    const ADDITIONAL_PARAMETERS: ObjectSchema =
> +        ObjectSchema::new("test parameters", &[]).additional_properties(true);
> +
> +    // compile time check that valid parameter references are accepted
> +    const _: ApiMethod = ApiMethod::new_dummy(&PARAMETERS).access(
> +        None,
> +        &Permission::And(&[
> +            &Permission::Privilege(&["foo", "{foo}", "{bar}"], 1, true),
> +            &Permission::Privilege(&["foo", "{foo}/{bar}"], 1, true),
> +            &Permission::WithParam(
> +                "foo",
> +                &Permission::Or(&[&Permission::Privilege(&["foo", "{foo}"], 1, true)]),
> +            ),
> +        ]),
> +    );
> +
> +    #[test]
> +    #[should_panic(expected = "privilege path references a parameter")]
> +    fn missing_privilege_path_parameter() {
> +        let _ = ApiMethod::new_dummy(&PARAMETERS)
> +            .access(None, &Permission::Privilege(&["foo", "{baz}"], 1, true));
> +    }
> +
> +    #[test]
> +    #[should_panic(expected = "privilege path references a parameter")]
> +    fn missing_parameter_in_combined_component() {
> +        let _ = ApiMethod::new_dummy(&PARAMETERS).access(
> +            None,
> +            &Permission::Or(&[&Permission::Privilege(
> +                &["datastore", "{foo}/{baz}"],
> +                0b01,
> +                true,
> +            )]),
> +        );
> +    }
> +
> +    #[test]
> +    fn malformed_parameter_in_combined_component() {
> +        // should work, components are not enclosed in brackets properly so no interpolation should
> +        // be done
> +        let _ = ApiMethod::new_dummy(&PARAMETERS).access(
> +            None,
> +            &Permission::Or(&[&Permission::Privilege(&["foo", "bar/{baz/}"], 1, true)]),
> +        );
> +
> +        let _ = ApiMethod::new_dummy(&PARAMETERS).access(
> +            None,
> +            &Permission::Or(&[&Permission::Privilege(&["foo", "{bar/baz}/"], 1, true)]),
> +        );
> +    }
> +
> +    #[test]
> +    fn additional_properties_allow_any_parameter() {
> +        let _ = ApiMethod::new_dummy(&ADDITIONAL_PARAMETERS)
> +            .access(None, &Permission::Privilege(&["foo", "{baz}"], 1, true));
> +    }
> +}
> -- 
> 2.47.3
> 
> 
> 
> 
> 

--