[pve-devel] [PATCH proxmox 1/2] notify: webhook: gotify: set Content-Length header

[pve-devel] [PATCH proxmox 1/2] notify: webhook: gotify: set Content-Length header

[Lukas Wagner]

To quote from RFC 9110 [1]:

  A user agent SHOULD send Content-Length in a request when
  the method defines a meaning for enclosed content and it
  is not sending Transfer-Encoding. For example, a user agent
  normally sends Content-Length in a POST request even when
  the value is 0 (indicating empty content).
  A user agent SHOULD NOT send a Content-Length header field
  when the request message does not contain content and the
  method semantics do not anticipate such data.

It seemed like our HTTP client lib did not set the header
automatically, which is why we should do it manually.

While most services seemed to have worked fine without setting
the header, some Microsoft services seem to require it
to accept the webhook request [2].

[1] https://datatracker.ietf.org/doc/html/rfc9110#name-content-length
[2] https://forum.proxmox.com/threads/158827

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/src/endpoints/gotify.rs  |  4 ++++
 proxmox-notify/src/endpoints/webhook.rs | 19 ++++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/proxmox-notify/src/endpoints/gotify.rs b/proxmox-notify/src/endpoints/gotify.rs
index 3e977131..e154daab 100644
--- a/proxmox-notify/src/endpoints/gotify.rs
+++ b/proxmox-notify/src/endpoints/gotify.rs
@@ -136,6 +136,10 @@ impl Endpoint for GotifyEndpoint {
                 format!("Bearer {}", self.private_config.token),
             ),
             ("X-Gotify-Key".into(), self.private_config.token.clone()),
+            (
+                http::header::CONTENT_LENGTH.to_string(),
+                body.len().to_string(),
+            ),
         ]);
 
         let proxy_config = context()
diff --git a/proxmox-notify/src/endpoints/webhook.rs b/proxmox-notify/src/endpoints/webhook.rs
index 34dbac54..604777c7 100644
--- a/proxmox-notify/src/endpoints/webhook.rs
+++ b/proxmox-notify/src/endpoints/webhook.rs
@@ -35,7 +35,7 @@ pub(crate) const WEBHOOK_TYPENAME: &str = "webhook";
 const HTTP_TIMEOUT: Duration = Duration::from_secs(10);
 
 #[api]
-#[derive(Serialize, Deserialize, Clone, Copy, Default)]
+#[derive(Serialize, Deserialize, Clone, Copy, Default, PartialEq)]
 #[serde(rename_all = "kebab-case")]
 /// HTTP Method to use.
 pub enum HttpMethod {
@@ -347,6 +347,23 @@ impl WebhookEndpoint {
             builder = builder.header(header.name.clone(), value);
         }
 
+        // From https://datatracker.ietf.org/doc/html/rfc9110#name-content-length :
+        //
+        // A user agent SHOULD send Content-Length in a request when the method
+        // defines a meaning for enclosed content and it is not sending
+        // Transfer-Encoding. For example, a user agent normally sends
+        // Content-Length in a POST request even when the value is 0 (indicating
+        // empty content). A user agent SHOULD NOT send a Content-Length header
+        // field when the request message does not contain content and the
+        // method semantics do not anticipate such data.
+        //
+        // -> send the header always, unless we do a get with no body (which is the expected case
+        // for GET)
+        let content_length = body.as_bytes().len();
+        if !(self.config.method == HttpMethod::Get && content_length == 0) {
+            builder = builder.header(http::header::CONTENT_LENGTH, content_length);
+        }
+
         let request = builder
             .body(body)
             .map_err(|err| self.mask_secret_in_error(err))
-- 
2.39.5



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

[pve-devel] [PATCH proxmox 2/2] notify: gotify: use constant from http crate for 'Authorization' header

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/Cargo.toml              | 2 +-
 proxmox-notify/src/endpoints/gotify.rs | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
index ddbaacd7..5a54c4a1 100644
--- a/proxmox-notify/Cargo.toml
+++ b/proxmox-notify/Cargo.toml
@@ -41,7 +41,7 @@ proxmox-uuid = { workspace = true, features = ["serde"] }
 default = ["sendmail", "gotify", "smtp", "webhook"]
 mail-forwarder = ["dep:mail-parser", "dep:proxmox-sys", "proxmox-sendmail/mail-forwarder"]
 sendmail = ["dep:proxmox-sys", "dep:base64", "dep:proxmox-sendmail"]
-gotify = ["dep:proxmox-http"]
+gotify = ["dep:proxmox-http", "dep:http"]
 pve-context = ["dep:proxmox-sys"]
 pbs-context = ["dep:proxmox-sys"]
 smtp = ["dep:lettre"]
diff --git a/proxmox-notify/src/endpoints/gotify.rs b/proxmox-notify/src/endpoints/gotify.rs
index e154daab..3e12a60e 100644
--- a/proxmox-notify/src/endpoints/gotify.rs
+++ b/proxmox-notify/src/endpoints/gotify.rs
@@ -132,7 +132,7 @@ impl Endpoint for GotifyEndpoint {
             .map_err(|err| Error::NotifyFailed(self.name().to_string(), err.into()))?;
         let extra_headers = HashMap::from([
             (
-                "Authorization".into(),
+                http::header::AUTHORIZATION.to_string(),
                 format!("Bearer {}", self.private_config.token),
             ),
             ("X-Gotify-Key".into(), self.private_config.token.clone()),
-- 
2.39.5



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

[pve-devel] applied: [PATCH proxmox 1/2] notify: webhook: gotify: set Content-Length header

[Thomas Lamprecht]

Am 21.03.25 um 10:56 schrieb Lukas Wagner:
> To quote from RFC 9110 [1]:
> 
>   A user agent SHOULD send Content-Length in a request when
>   the method defines a meaning for enclosed content and it
>   is not sending Transfer-Encoding. For example, a user agent
>   normally sends Content-Length in a POST request even when
>   the value is 0 (indicating empty content).
>   A user agent SHOULD NOT send a Content-Length header field
>   when the request message does not contain content and the
>   method semantics do not anticipate such data.
> 
> It seemed like our HTTP client lib did not set the header
> automatically, which is why we should do it manually.
> 
> While most services seemed to have worked fine without setting
> the header, some Microsoft services seem to require it
> to accept the webhook request [2].
> 
> [1] https://datatracker.ietf.org/doc/html/rfc9110#name-content-length
> [2] https://forum.proxmox.com/threads/158827
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  proxmox-notify/src/endpoints/gotify.rs  |  4 ++++
>  proxmox-notify/src/endpoints/webhook.rs | 19 ++++++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
>

applied both patches, thanks!

FWIW, as it was already encoded in the commit message for posterity I'd
have been fine with the comment being a bit shorter, e.g., the link to
the RFC and the last line, but it did not bother me to care amending the
patch and it's not a clear-cut, or at least subjective, so just a nit.


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

Re: [pve-devel] applied: [PATCH proxmox 1/2] notify: webhook: gotify: set Content-Length header

[Lukas Wagner]

On  2025-03-25 19:41, Thomas Lamprecht wrote:
> Am 21.03.25 um 10:56 schrieb Lukas Wagner:
>> To quote from RFC 9110 [1]:
>>
>>   A user agent SHOULD send Content-Length in a request when
>>   the method defines a meaning for enclosed content and it
>>   is not sending Transfer-Encoding. For example, a user agent
>>   normally sends Content-Length in a POST request even when
>>   the value is 0 (indicating empty content).
>>   A user agent SHOULD NOT send a Content-Length header field
>>   when the request message does not contain content and the
>>   method semantics do not anticipate such data.
>>
>> It seemed like our HTTP client lib did not set the header
>> automatically, which is why we should do it manually.
>>
>> While most services seemed to have worked fine without setting
>> the header, some Microsoft services seem to require it
>> to accept the webhook request [2].
>>
>> [1] https://datatracker.ietf.org/doc/html/rfc9110#name-content-length
>> [2] https://forum.proxmox.com/threads/158827
>>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> ---
>>  proxmox-notify/src/endpoints/gotify.rs  |  4 ++++
>>  proxmox-notify/src/endpoints/webhook.rs | 19 ++++++++++++++++++-
>>  2 files changed, 22 insertions(+), 1 deletion(-)
>>
>>
> 
> applied both patches, thanks!
> 
> FWIW, as it was already encoded in the commit message for posterity I'd
> have been fine with the comment being a bit shorter, e.g., the link to
> the RFC and the last line, but it did not bother me to care amending the
> patch and it's not a clear-cut, or at least subjective, so just a nit.

The brief quote from the RFC gives good context on *why* the change should be done
in a self-contained way without having to go to the RFC text and search for the correct
paragraph. IMO it definitely makes sense to have it in the commit message.

-- 
- Lukas



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

Re: [pve-devel] applied: [PATCH proxmox 1/2] notify: webhook: gotify: set Content-Length header

[Thomas Lamprecht]

Am 26.03.25 um 10:54 schrieb Lukas Wagner:
> On  2025-03-25 19:41, Thomas Lamprecht wrote:
>> FWIW, as it was already encoded in the commit message for posterity I'd
>> have been fine with the comment being a bit shorter, e.g., the link to
>> the RFC and the last line, but it did not bother me to care amending the
>> patch and it's not a clear-cut, or at least subjective, so just a nit.
> 
> The brief quote from the RFC gives good context on *why* the change should be done
> in a self-contained way without having to go to the RFC text and search for the correct
> paragraph. IMO it definitely makes sense to have it in the commit message.

Yes, that's why I explicitly state that it makes sense in the *commit*
message, but that doing that is enough compared to also having the full
thing permanently as *comment* in the code like you did...
Because if we would do that for all implementation details the code
will get unreadable quickly.


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

Re: [pve-devel] applied: [PATCH proxmox 1/2] notify: webhook: gotify: set Content-Length header

[Lukas Wagner]

On  2025-03-26 11:11, Thomas Lamprecht wrote:
> Am 26.03.25 um 10:54 schrieb Lukas Wagner:
>> On  2025-03-25 19:41, Thomas Lamprecht wrote:
>>> FWIW, as it was already encoded in the commit message for posterity I'd
>>> have been fine with the comment being a bit shorter, e.g., the link to
>>> the RFC and the last line, but it did not bother me to care amending the
>>> patch and it's not a clear-cut, or at least subjective, so just a nit.
>>
>> The brief quote from the RFC gives good context on *why* the change should be done
>> in a self-contained way without having to go to the RFC text and search for the correct
>> paragraph. IMO it definitely makes sense to have it in the commit message.
> 
> Yes, that's why I explicitly state that it makes sense in the *commit*
> message, but that doing that is enough compared to also having the full
> thing permanently as *comment* in the code like you did...
> Because if we would do that for all implementation details the code
> will get unreadable quickly.
> 

Apparently my brain failed to parse and distinguish "commit" and "comment" successfully.

Sorry for the confusion.

-- 
- Lukas



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