public inbox for pdm-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Thomas Ellmenreich" <t.ellmenreich@proxmox.com>
To: "Shan Shaji" <s.shaji@proxmox.com>, <pdm-devel@lists.proxmox.com>
Subject: Re: [PATCH proxmox 3/4] api-types: add a scheme generic URL regex
Date: Wed, 19 Aug 2026 15:18:58 +0200	[thread overview]
Message-ID: <DKSY7YEFY5PK.31I7O8F289AQC@proxmox.com> (raw)
In-Reply-To: <DKSWLW1BI17F.TP4EI7F3RFC9@proxmox.com>

Thank you so much for your extensive testing, especially for using a mobile
device as well.

Personally, I would tackle the possible changes to PVE and PBS in a
separate patch series and on their own mailing lists.

On Wed Aug 19, 2026 at 2:03 PM CEST, Shan Shaji wrote:
> Hi,
>
> I have tested this again when checking the app open ID [0] series with the custom
> scheme and I was wrong about optionaly allowing to not have the
> host part. Sorry!
>
> So, when the redirect_uri is specified without the host part (com.proxmox.app.openid://)
> it will generate an authorization URL but keycloak will fail to parse it. The login
> prompt won't be shown. So, the behavior for PDM I believe is correct. Perhaps, we might need
> to update the validation for PVE and PBS as well? I am also not sure why it was relaxed
> there in first place. 
>
> Anyways, the changes inside this series  looks fine to me. I am getting the
> authorization url correctly. So, please consider this: 
>
> ```
> curl -o - --data '{"realm":"proxmox","redirect-url":"com.proxmox.app.openid://openid-callback"}' \\
> --header 'Content-Type: application/json;charset=UTF-8' \\
> -k https://10.12.0.11:8443/api2/extjs/access/openid/auth-url | jq
> ```
>
> - [0]  https://lore.proxmox.com/pve-devel/20260810144713.75806-1-haque@azharul.com/T/#u
>
> Reviewed-by: Shan Shaji <s.shaji@proxmox.com>
> Tested-by: Shan Shaji <s.shaji@proxmox.com>
>
> On Tue Aug 18, 2026 at 5:08 PM CEST, Shan Shaji wrote:
>> Hi,
>>
>> I have tested the changes and went through the code. AFAICT,
>> everything looks good except for one thing.
>>
>> Without the changes, when sending a curl request, I got the error
>> as mentioned in the ticket. After applying the fix, the
>> "redirect-url" regex validation works when using the same test URLs from
>> the test and I am getting a valid "data" response. However, when
>> using the redirect URIs without the host part (e.g: pveauth://, proxman://),
>> the validation still fails.
>>
>> AFAIK, in Android and iOS, since the host part is not mandatory for
>> custom schemes, IMHO it would be better to support the case where
>> the host part is optional as well. That is, as we discussed off
>> list: for http(s) URLs, the host part is mandatory, and for custom
>> URLs, the host part doesn't have to be mandatory.
>>
>> On Thu Aug 13, 2026 at 5:05 PM CEST, Thomas Ellmenreich wrote:
>>> The current URL regex, HTTP_URL_REGEX, is specific to the HTTP and HTTPS
>>> schemes. This patch creates a new regex that is scheme-generic and thus
>>> allows any other valid scheme.
>>>
>>> It also adds tests that cover some basic URL edge cases.
>>>
>>> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
>>> ---
>>>  pbs-api-types/src/lib.rs        |  1 +
>>>  proxmox-schema/src/api_types.rs | 40 ++++++++++++++++++++++++++++++++-
>>>  2 files changed, 40 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
>>> index 21ef733d..924f356b 100644
>>> --- a/pbs-api-types/src/lib.rs
>>> +++ b/pbs-api-types/src/lib.rs
>>> @@ -44,6 +44,7 @@ pub use proxmox_schema::api_types::HTTP_URL_SCHEMA;
>>>  pub use proxmox_schema::api_types::MULTI_LINE_COMMENT_SCHEMA;
>>>  pub use proxmox_schema::api_types::NODE_SCHEMA;
>>>  pub use proxmox_schema::api_types::SINGLE_LINE_COMMENT_FORMAT;
>>> +pub use proxmox_schema::api_types::URL_SCHEMA;
>>>  pub use proxmox_schema::api_types::{
>>>      BLOCKDEVICE_DISK_AND_PARTITION_NAME_SCHEMA, BLOCKDEVICE_NAME_SCHEMA,
>>>  };
>>
>>>>snip<<





  reply	other threads:[~2026-08-19 13:19 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-13 15:05 [PATCH datacenter-manager/proxmox 0/4] fix #7747: OpenID: allow non HTTP scheme in redirect URL Thomas Ellmenreich
2026-08-13 15:05 ` [PATCH proxmox 1/4] api-types: reorganise unit tests Thomas Ellmenreich
2026-08-13 15:05 ` [PATCH proxmox 2/4] api-types: refactor HTTP_URL_REGEX construction Thomas Ellmenreich
2026-08-13 15:05 ` [PATCH proxmox 3/4] api-types: add a scheme generic URL regex Thomas Ellmenreich
2026-08-18 15:08   ` Shan Shaji
2026-08-18 15:14     ` Shan Shaji
2026-08-19 12:03     ` Shan Shaji
2026-08-19 13:18       ` Thomas Ellmenreich [this message]
2026-08-13 15:05 ` [PATCH datacenter-manager 4/4] fix #7747: openid: allow non HTTP schemes in redirect URL Thomas Ellmenreich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DKSY7YEFY5PK.31I7O8F289AQC@proxmox.com \
    --to=t.ellmenreich@proxmox.com \
    --cc=pdm-devel@lists.proxmox.com \
    --cc=s.shaji@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal