unofficial mirror of guile-devel@gnu.org 
 help / color / mirror / Atom feed
From: Maxime Devos <maximedevos@telenet.be>
To: Ryan Raymond <rjraymond@oakland.edu>,
	 "guile-devel@gnu.org" <guile-devel@gnu.org>
Subject: RE: Allowing extended HTTP methods
Date: Thu, 28 Mar 2024 17:17:49 +0100	[thread overview]
Message-ID: <20240328171749.4GHn2C00Y4SDPei06GHosd@albert.telenet-ops.be> (raw)
In-Reply-To: <CAGvJ-HSO5EYWFjgemopELXG_Subno5d4hm6OqJ_x=vpWvPCG6A@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]

As mentioned previous e-mails, either:

• the docstring forgets to mention that it is the caller’s responsibility it is to check that ‘str’ is a valid method name (and you need to check/adjust the callers to do the check, of which I have seen n)
• or 'parse-http-method’ is missing a check that (string-length str)>0 and the characters of str belong to tchar (defined in the RFC).

Please do not ignore reviews when making PRs – disagreeing with some review(s) and then mentioning in the commit message/cover letter/comments/... as appropriate why you deviate from them is one thing, just acting as if they didn’t happen is another.

In the previous discussion I have heard an argument for not doing the check, which basically amounts to flexibility for sake of flexibility in spite of potential security problems and in spite of the third option (i.e. with both security and flexibility) of not allowing it by default and instead adding an argument somewhere to add a custom checker/list of extra allowed methods/... that defaults to what RFC specifies, but I haven’t heard an argument for not properly documenting that what parse-http-method parses aren’t HTTP methods so the callers may need to do extra validation. 

Also, the commit message needs to be changed to changelog format.

Also worth checking if the Texinfo documentation mentions parse-http-method, and if so, adjust it.

Best regards,
Maxime Devos.

[-- Attachment #2: Type: text/html, Size: 6016 bytes --]

      reply	other threads:[~2024-03-28 16:17 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-28  4:45 Allowing extended HTTP methods Ryan Raymond
2024-03-28 16:17 ` Maxime Devos [this message]

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

  List information: https://www.gnu.org/software/guile/

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

  git send-email \
    --in-reply-to=20240328171749.4GHn2C00Y4SDPei06GHosd@albert.telenet-ops.be \
    --to=maximedevos@telenet.be \
    --cc=guile-devel@gnu.org \
    --cc=rjraymond@oakland.edu \
    /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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).