GNU bug report logs - #10109
[PATCH] (web http): list-style headers do not validate

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 10109 in the body.
You can then email your comments to 10109 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Daniel Hartwig <mandyke <at> gmail.com>
To: submit <at> debbugs.gnu.org
Subject: [PATCH] (web http): list-style headers do not validate
Date: Wed, 23 Nov 2011 02:18:36 +0800

[Message part 1 (text/plain, inline)]

Package: guile
Version: 2.0.3
Tags: patch

Many of the list-style headers from (web http) do not validate
correctly.  The test suite only checks that the header's parse and
does not test the associated validators.

Attached is a very quick patch (0002) which exposes the failing
validators through the test-suite:

$ ./guile-test tests/web-http.test
Running tests/web-http.test
...
FAIL: tests/web-http.test: general headers: cache-control:
"no-transform" -> (no-transform)
FAIL: tests/web-http.test: general headers: cache-control:
"no-transform,foo" -> (no-transform foo)
FAIL: tests/web-http.test: general headers: cache-control: "no-cache"
-> (no-cache)
FAIL: tests/web-http.test: general headers: cache-control:
"no-cache=\"Authorization, Date\"" -> ((no-cache authorization date))
FAIL: tests/web-http.test: general headers: cache-control:
"private=\"Foo\"" -> ((private foo))
FAIL: tests/web-http.test: general headers: cache-control:
"no-cache,max-age=10" -> (no-cache (max-age . 10))
FAIL: tests/web-http.test: general headers: pragma: "no-cache" -> (no-cache)
FAIL: tests/web-http.test: general headers: pragma: "no-cache, foo" ->
(no-cache foo)
FAIL: tests/web-http.test: general headers: transfer-encoding: "foo,
chunked" -> ((foo) (chunked))
FAIL: tests/web-http.test: entity headers: allow: "foo, bar" -> (foo bar)
FAIL: tests/web-http.test: entity headers: content-encoding: "qux,
baz" -> (qux baz)
FAIL: tests/web-http.test: request headers: accept: "text/*;q=0.3,
text/html;q=0.7, text/html;level=1" -> ((text/* (q . 300)) (text/html
(q . 700)) (text/html (level . "1")))
FAIL: tests/web-http.test: request headers: authorization: "Basic
foooo" -> (basic . "foooo")
FAIL: tests/web-http.test: request headers: authorization: "Digest
foooo" -> (digest foooo)
FAIL: tests/web-http.test: request headers: expect: "100-continue,
foo" -> ((#{100-continue}#) (foo))
FAIL: tests/web-http.test: request headers: proxy-authorization:
"Basic foooo" -> (basic . "foooo")
FAIL: tests/web-http.test: request headers: proxy-authorization:
"Digest foooo" -> (digest foooo)
FAIL: tests/web-http.test: request headers: te: "trailers" -> ((trailers))
FAIL: tests/web-http.test: request headers: te: "trailers,foo" ->
((trailers) (foo))
FAIL: tests/web-http.test: response headers: accept-ranges: "foo,bar"
-> (foo bar)

Totals for this test run:
passes:                 60
failures:               20
...


The other patch (0001) corrects `http.scm' for some typos and missing logic,
after which the above failures no longer occur.

$ ./guile-test tests/web-http.test
Running tests/web-http.test
...

Totals for this test run:
passes:                 80
failures:               0
...


0001 (web http): fix validators for various list-style headers

* module/web/http.scm (default-val-validator): Valid with no value.
  (key-value-list?): Keys are always symbols, do not accept strings.
  (validate-param-list): Apply `valid?' to list elements.
  (validate-credentials): Validate param for Basic scheme, which
  is parsed as a string.
  (declare-symbol-list-header!): `list-of?' args were in wrong order.
  ("Cache-Control"): Replace `default-val-validator' with more
  specific procedure.
  ("Accept"): Validate on first param which has no value.
---
 module/web/http.scm |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)

[0001-web-http-fix-validators-for-various-list-style-heade.patch (text/x-patch, attachment)]

[0002-web-http-test.patch (text/x-patch, attachment)]

Message #10 received at 10109-done <at> debbugs.gnu.org (full text, mbox):

From: Andy Wingo <wingo <at> pobox.com>
To: Daniel Hartwig <mandyke <at> gmail.com>
Cc: ludo <at> gnu.org, 10109-done <at> debbugs.gnu.org
Subject: Re: bug#10109: [PATCH] (web http): list-style headers do not validate
Date: Wed, 23 Nov 2011 21:25:48 +0100

On Tue 22 Nov 2011 19:18, Daniel Hartwig <mandyke <at> gmail.com> writes:

> Many of the list-style headers from (web http) do not validate
> correctly.  The test suite only checks that the header's parse and
> does not test the associated validators.

I applied both of your patches.  Thank you very much!

In the future, please make your patches as separate git commits in your
repository.  Then do `git format-patch origin/stable-2.0..HEAD'.  Then
attach the generated files to a mail.  The advantage is that I don't
have to cut and paste your commit log, and I don't have to make special
effort to ensure that you are listed as the author in the commits.

Also, your first patch is probably at the limit of how big a patch we
can accept without getting you to assign copyright to the FSF.  If you
think you will send more patches in the future, it's probably a good
idea to start that process, if you are OK with that.  Email me privately
and I'll tell you how to do that.  Note that GNU has recently entered
the 20th century ;) by sending the forms via PDF.  You can submit them
electronically too, but only if you are a US resident.  Anyway, send me
an email if you are interested.

Thanks again for the patches!

Andy
-- 
http://wingolog.org/

Message #13 received at 10109 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Hartwig <mandyke <at> gmail.com>
To: 10109 <at> debbugs.gnu.org
Cc: Andy Wingo <wingo <at> pobox.com>
Subject: Re: bug#10109: [PATCH] (web http): list-style headers do not validate
Date: Sun, 27 Nov 2011 23:11:08 +0800

[Message part 1 (text/plain, inline)]

Hello again

My apologies for not noticing earlier, but I have spotted a couple
minor issues with both the previous patch and original code that are
corrected by the attached. All relate to the "Cache-Control" header:

- `max-stale' has optional value (previous code requires it)
- some directives do not have values (`no-store', etc.)
- there are `cache-extension' directives that may or may not have a value

Attached patch tidies this up, with explicit validation of all defined
directives, though it leaves open one issue with the cache-extension
directives:

;; cache-extension = token [ "=" ( token | quoted-string ) ]
scheme@(web http)> (read-header (open-input-string "Cache-Control:
foo=\"qstring\"\r\n"))
$102 = cache-control
$103 = ((foo . "qstring"))
scheme@(web http)> (write-header 'cache-control $103
(current-output-port)) (newline)
Cache-Control: foo=qstring

You see quotes around `qstring' are dropped, `parse-key-value-list'
appears inadequate to distinguish between a token and quoted-string
when passing values to the `val-parser'.  This looks like it will
raise itself in edge cases for some other headers.  Will file a new
bug if needed after investigation.


Regards

Daniel

On 23 November 2011 02:18, Daniel Hartwig <mandyke <at> gmail.com> wrote:
> Package: guile
> Version: 2.0.3
> Tags: patch
>
> Many of the list-style headers from (web http) do not validate
> correctly.  The test suite only checks that the header's parse and
> does not test the associated validators.
>
> Attached is a very quick patch (0002) which exposes the failing
> validators through the test-suite:
>
> $ ./guile-test tests/web-http.test
> Running tests/web-http.test
> ...
> FAIL: tests/web-http.test: general headers: cache-control:
> "no-transform" -> (no-transform)
> [...]

[0001-Extend-handling-of-Cache-Control-header.patch (text/x-patch, attachment)]

Message #16 received at 10109 <at> debbugs.gnu.org (full text, mbox):

From: Andy Wingo <wingo <at> pobox.com>
To: Daniel Hartwig <mandyke <at> gmail.com>
Cc: 10109 <at> debbugs.gnu.org
Subject: Re: bug#10109: [PATCH] (web http): list-style headers do not validate
Date: Thu, 22 Dec 2011 08:21:06 -0500

On Sun 27 Nov 2011 10:11, Daniel Hartwig <mandyke <at> gmail.com> writes:

> My apologies for not noticing earlier, but I have spotted a couple
> minor issues with both the previous patch and original code that are
> corrected by the attached. All relate to the "Cache-Control" header:
>
> - `max-stale' has optional value (previous code requires it)
> - some directives do not have values (`no-store', etc.)
> - there are `cache-extension' directives that may or may not have a value
>
> Attached patch tidies this up

Thanks, it was a great patch.  Applied and pushed.

> with explicit validation of all defined
> directives, though it leaves open one issue with the cache-extension
> directives:

I fixed this one, I think.

Happy hacking,

Andy
-- 
http://wingolog.org/

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.