Re: [Bug-wget] [PATCH] Disable automatic wget headers.
Hi,
Sorry for the delay, I've been a little too busy with other things.
I've attached a testcase file containing two failing tests:
1. I disable the "Host" header, then set it manually, then I disable an
arbitrary header, the "Host" header gets disabled. This is incorrect.
2. I disabled the "Cookie" header, but it was still sent when the server sent a
Cookie. I assume something similar will happen if you disable the
"Authorization" header, but the server sends a auth request.
* adham elkarn [190506 11:26]:
> From: sulfastor
>
> Hi, Thank you again Darshit for your response. The RejectHeaderField
> rule rejects ANY header
> of the header field while RejectHeader rejects ONLY the specified full
> header.
> Since we wanted to be sure a header field is not sent to the server we
> wrote this rule.
>
> * doc/wget.texi: Added --disable-header documentation.
> * fuzz/wget_options_fuzzer.dict: Update with --disable-header inputs.
> * src/http.c (disabled_header): Checks for disabled headers
> (request_set_header): Doesn't let header to be set if disabled
> (gethttp): frees disabled header to let overriding
> * src/init.c (cmd_dis_header), (check_user_disabled_header) added new
> option disabled_headers.
> * src/main.c: added new option --disable-header, added help
> description
> * src/options.h: added new option --disable-header
> * src/utils.h (vec_remove_header)
> * src/utils.c (vec_remove_header) removes all header instances from
> vector
> * testenv/Makefile.am: Added new test files
> * testenv/server/http/http_server.py: Added new rule RejectHeaderField
> * testenv/conf/reject_header_field.py: Added new rule
> RejectHeaderField
> * testenv/README: Added help description for new rule
> * testenv/Test-disable-default-headers.py: Test without using --header
> * testenv/Test-disable-headers-after.py: Test using --header before
> --disable-header
> * testenv/Test-disable-headers-before.py: Test using --header after
> --disable-header
>
> Signed-off-by: sulfastor , adham elkarn
>
> ---
> doc/wget.texi | 15 +
> fuzz/wget_options_fuzzer.dict | 17 ++
> src/http.c | 28 -
> src/init.c | 48 ---
> src/main.c | 3 +
> src/options.h | 1 +
> src/utils.c | 43 +
> src/utils.h | 1 +
> testenv/Makefile.am | 3 +
> testenv/README | 4 ++
> testenv/Test-disable-default-headers.py | 74 +++
> testenv/Test-disable-headers-after.py | 80 +
> testenv/Test-disable-headers-before.py | 78
> testenv/conf/reject_header_field.py | 12
> testenv/server/http/http_server.py | 8 +++
> 15 files changed, 405 insertions(+), 10 deletions(-)
> create mode 100644 testenv/Test-disable-default-headers.py
> create mode 100644 testenv/Test-disable-headers-after.py
> create mode 100644 testenv/Test-disable-headers-before.py
> create mode 100644 testenv/conf/reject_header_field.py
>
> diff --git a/doc/wget.texi b/doc/wget.texi
> index 7eada2dd..6301cbf6 100644
> --- a/doc/wget.texi
> +++ b/doc/wget.texi
> @@ -1542,6 +1542,21 @@ wget --header="Host: foo.bar" http://localhost/
> In versions of Wget prior to 1.10 such use of @samp{--header} caused
> sending of duplicate headers.
>
> +@cindex disable header, choose
> +@item --disable-header=@var{list}
> +Specify comma-separated header fields to remove from the @sc{http}
> +request.
> +
> +@example
> +@group
> +wget --disable-header='Accept,User-Agent,Authorization' \
> +https://example.com/
> +@end group
> +@end example
> +
> +Specifying a header field with @samp{--header} after disabling it
> +will override it and include it in the @sc{http} request headers.
> +
> @cindex Content-Encoding, choose
> @item --compression=@var{type}
> Choose the type of compression to be used. Legal values are
> diff --git a/fuzz/wget_options_fuzzer.dict b/fuzz/wget_options_fuzzer.dict
> index 9a2dbd8e..12d54d60 100644
> --- a/fuzz/wget_options_fuzzer.dict
> +++ b/fuzz/wget_options_fuzzer.dict
> @@ -30,6 +30,22 @@
> "human"
> "csv"
> "json"
> +"Authorization"
> +"User-Agent"
> +"Referer"
> +"Cache-Control"
> +"Pragma"
> +"If-Modified-Since"
> +"Range"
> +"Accept"
> +"Accept-Encoding"
> +"Host"
> +"Connection"
> +"Proxy-Connection"
> +"Content-Type"
> +"Content-Length"
> +"Proxy-Authorization"
> +"Cookie"
> "accept="
> "accept-regex="
> "adjust-extension="
> @@ -66,6 +82,7 @@
> "delete-after="
> "directories="
> "directory-prefix="
> +"disable-header="
> "dns-caching="
> "dns-timeout="
> "domains="
> diff --git a/src/http.c b/src/http.c
>
Re: [Bug-wget] [PATCH] Disable automatic wget headers.
Thanks for the updated patch and for sticking around through my nitpicky
reviews.
I'd like to spend some more time reviewing this patch and testing it out.
So, a full review will likely have to wait till the coming weekend.
* adham elkarn [190506 11:26]:
> From: sulfastor
>
> Hi, Thank you again Darshit for your response. The RejectHeaderField
> rule rejects ANY header
> of the header field while RejectHeader rejects ONLY the specified full
> header.
> Since we wanted to be sure a header field is not sent to the server we
> wrote this rule.
>
> * doc/wget.texi: Added --disable-header documentation.
> * fuzz/wget_options_fuzzer.dict: Update with --disable-header inputs.
> * src/http.c (disabled_header): Checks for disabled headers
> (request_set_header): Doesn't let header to be set if disabled
> (gethttp): frees disabled header to let overriding
> * src/init.c (cmd_dis_header), (check_user_disabled_header) added new
> option disabled_headers.
> * src/main.c: added new option --disable-header, added help
> description
> * src/options.h: added new option --disable-header
> * src/utils.h (vec_remove_header)
> * src/utils.c (vec_remove_header) removes all header instances from
> vector
> * testenv/Makefile.am: Added new test files
> * testenv/server/http/http_server.py: Added new rule RejectHeaderField
> * testenv/conf/reject_header_field.py: Added new rule
> RejectHeaderField
> * testenv/README: Added help description for new rule
> * testenv/Test-disable-default-headers.py: Test without using --header
> * testenv/Test-disable-headers-after.py: Test using --header before
> --disable-header
> * testenv/Test-disable-headers-before.py: Test using --header after
> --disable-header
>
> Signed-off-by: sulfastor , adham elkarn
>
> ---
> doc/wget.texi | 15 +
> fuzz/wget_options_fuzzer.dict | 17 ++
> src/http.c | 28 -
> src/init.c | 48 ---
> src/main.c | 3 +
> src/options.h | 1 +
> src/utils.c | 43 +
> src/utils.h | 1 +
> testenv/Makefile.am | 3 +
> testenv/README | 4 ++
> testenv/Test-disable-default-headers.py | 74 +++
> testenv/Test-disable-headers-after.py | 80 +
> testenv/Test-disable-headers-before.py | 78
> testenv/conf/reject_header_field.py | 12
> testenv/server/http/http_server.py | 8 +++
> 15 files changed, 405 insertions(+), 10 deletions(-)
> create mode 100644 testenv/Test-disable-default-headers.py
> create mode 100644 testenv/Test-disable-headers-after.py
> create mode 100644 testenv/Test-disable-headers-before.py
> create mode 100644 testenv/conf/reject_header_field.py
>
> diff --git a/doc/wget.texi b/doc/wget.texi
> index 7eada2dd..6301cbf6 100644
> --- a/doc/wget.texi
> +++ b/doc/wget.texi
> @@ -1542,6 +1542,21 @@ wget --header="Host: foo.bar" http://localhost/
> In versions of Wget prior to 1.10 such use of @samp{--header} caused
> sending of duplicate headers.
>
> +@cindex disable header, choose
> +@item --disable-header=@var{list}
> +Specify comma-separated header fields to remove from the @sc{http}
> +request.
> +
> +@example
> +@group
> +wget --disable-header='Accept,User-Agent,Authorization' \
> +https://example.com/
> +@end group
> +@end example
> +
> +Specifying a header field with @samp{--header} after disabling it
> +will override it and include it in the @sc{http} request headers.
> +
> @cindex Content-Encoding, choose
> @item --compression=@var{type}
> Choose the type of compression to be used. Legal values are
> diff --git a/fuzz/wget_options_fuzzer.dict b/fuzz/wget_options_fuzzer.dict
> index 9a2dbd8e..12d54d60 100644
> --- a/fuzz/wget_options_fuzzer.dict
> +++ b/fuzz/wget_options_fuzzer.dict
> @@ -30,6 +30,22 @@
> "human"
> "csv"
> "json"
> +"Authorization"
> +"User-Agent"
> +"Referer"
> +"Cache-Control"
> +"Pragma"
> +"If-Modified-Since"
> +"Range"
> +"Accept"
> +"Accept-Encoding"
> +"Host"
> +"Connection"
> +"Proxy-Connection"
> +"Content-Type"
> +"Content-Length"
> +"Proxy-Authorization"
> +"Cookie"
> "accept="
> "accept-regex="
> "adjust-extension="
> @@ -66,6 +82,7 @@
> "delete-after="
> "directories="
> "directory-prefix="
> +"disable-header="
> "dns-caching="
> "dns-timeout="
> "domains="
> diff --git a/src/http.c b/src/http.c
> index 289d1101..24b16cf1 100644
> --- a/src/http.c
> +++ b/src/http.c
> @@ -88,6 +88,7 @@ static char *basic_authentication_encode (const char *,
> const char *);
> static bool known_authentication_scheme_p (const char *, const char *);
> static void ensure_extension
Re: [Bug-wget] [PATCH] Disable automatic wget headers.
Hi,
Comments inline
* adham elkarn [190504 17:41]:
> From: sulfastor
>
> Hi, Thank you Darshit for your feedback and code review.
> Those are the updates:
>
> * doc/wget.texi: Added --disable-header documentation.
> * fuzz/wget_options_fuzzer.dict: Update with --disable-header inputs.
> * src/http.c (disabled_header): Checks for disabled headers
> (request_set_header): Doesn't let header to be set if disabled
> (gethttp): frees disabled header to let overriding
> * src/init.c (cmd_dis_header), (check_user_disabled_header),
> (vec_remove_header): added new option disabled_headers.
> * src/main.c: added new option --disable-header, added help
> description
> * src/options.h: added new option --disable-header
> * testenv/Makefile.am: Added new test files
> * testenv/server/http/http_server.py: Added new rule RejectHeaderField
> * testenv/conf/reject_header_field.py: Added new rule
> RejectHeaderField
> * testenv/README: Added help description for new rule
> * testenv/Test-disable-default-headers.py: Test without using --header
> * testenv/Test-disable-headers-after.py: Test using --header before
> --disable-header
> * testenv/Test-disable-headers-before.py: Test using --header after
> --disable-header
>
> Signed-off-by: sulfastor , adham elkarn
>
> ---
> doc/wget.texi | 19 ++
> fuzz/wget_options_fuzzer.dict | 17 +
> src/http.c | 31 +
> src/init.c | 88 +
> src/main.c | 4 ++
> src/options.h | 1 +
> testenv/Makefile.am | 3 +
> testenv/README | 4 ++
> testenv/Test-disable-default-headers.py | 73
> testenv/Test-disable-headers-after.py | 77 ++
> testenv/Test-disable-headers-before.py | 77 ++
> testenv/conf/reject_header_field.py | 12
> testenv/server/http/http_server.py | 8 +++
> 13 files changed, 414 insertions(+)
> create mode 100644 testenv/Test-disable-default-headers.py
> create mode 100644 testenv/Test-disable-headers-after.py
> create mode 100644 testenv/Test-disable-headers-before.py
> create mode 100644 testenv/conf/reject_header_field.py
>
> diff --git a/doc/wget.texi b/doc/wget.texi
> index 7eada2dd..a43224a3 100644
> --- a/doc/wget.texi
> +++ b/doc/wget.texi
> @@ -1542,6 +1542,25 @@ wget --header="Host: foo.bar" http://localhost/
> In versions of Wget prior to 1.10 such use of @samp{--header} caused
> sending of duplicate headers.
>
> +@cindex disable header, choose
> +@item --disable-header=@var{header-field}
> +Remove @var{header-field} among the headers in each @sc{http} request.
> +
> +You may define more than one additional header field by specifying
> +@samp{--disable-header} more than once as in @samp{--header}.
> +
> +@example
> +@group
> +wget --disable-header='Accept' \
> + --disable-header='User-Agent' \
> + --disable-header='Accept-Encoding' \
> +https://example.com/
> +@end group
> +@end example
> +
> +Specifying a header field with @samp{--header} after disabling it
> +will override it and include it in the @sc{http} request headers.
> +
> @cindex Content-Encoding, choose
> @item --compression=@var{type}
> Choose the type of compression to be used. Legal values are
> diff --git a/fuzz/wget_options_fuzzer.dict b/fuzz/wget_options_fuzzer.dict
> index 9a2dbd8e..12d54d60 100644
> --- a/fuzz/wget_options_fuzzer.dict
> +++ b/fuzz/wget_options_fuzzer.dict
> @@ -30,6 +30,22 @@
> "human"
> "csv"
> "json"
> +"Authorization"
> +"User-Agent"
> +"Referer"
> +"Cache-Control"
> +"Pragma"
> +"If-Modified-Since"
> +"Range"
> +"Accept"
> +"Accept-Encoding"
> +"Host"
> +"Connection"
> +"Proxy-Connection"
> +"Content-Type"
> +"Content-Length"
> +"Proxy-Authorization"
> +"Cookie"
> "accept="
> "accept-regex="
> "adjust-extension="
> @@ -66,6 +82,7 @@
> "delete-after="
> "directories="
> "directory-prefix="
> +"disable-header="
> "dns-caching="
> "dns-timeout="
> "domains="
> diff --git a/src/http.c b/src/http.c
> index 289d1101..225be265 100644
> --- a/src/http.c
> +++ b/src/http.c
> @@ -88,6 +88,7 @@ static char *basic_authentication_encode (const char *,
> const char *);
> static bool known_authentication_scheme_p (const char *, const char *);
> static void ensure_extension (struct http_stat *, const char *, int *);
> static void load_cookies (void);
> +static bool disabled_header (char*);
>
> static bool cookies_loaded_p;
> static struct cookie_jar *wget_cookie_jar;
> @@ -236,6 +237,9 @@ request_set_header (struct request *req, const char
> *name, const char *value,
>struct request_header *hdr;
>int i;
>
> + if (disabled_header ((char*) name))
Why do you need to explicitly cast the
Re: [Bug-wget] [PATCH] Disable automatic wget headers.
Hi,
Thanks for working on the patch. I would really like to see something similar
for Wget2 as well.
A few comments on the patch:
1. "specificating" as used in the documentation is an archaic term that is no
longer used in modern language. Additionally, I am not confident that it is
used correctly here. The word "specifying" seems to be a better option and
is more consistent with the rest of the document.
2. I am in general not very happy with the overall design of the patch.
The current design puts undue burden and constant burden on the maintenance of
the codebase since we have to always remember to make the call to
`disabled_header()`. It would be a lot better if this was handled more or less
transparently. Say by adding the check to `request_set_header()`.
I get that adding the check to `request_set_header` is less efficient since
it is too lazy. However, it significantly reduces the maintenance overhead and
chance for bugs in the future.
3. Could you please add the function names as well to the git commit message as
is common in the GNU ChangeLog format?
4. I know that you were following the existing data in the documentation when
you used the http://fly.srk.fer.hr URL. However, it would be much better if
we could simply use https://example.com for these things. That domain was
designed specifically for use in examples and documentation. Over time, I
will remove mentions of all other domains from the documentation as well.
* adham elkarn [190504 10:13]:
> From: sulfastor
>
> Hi,
> This our update for the disable-header feature [bug #54769
> (https://savannah.gnu.org/bugs/?54769)].
> We took in account Dale Worley's comment to avoid regression since a header
> can be sent empty, although is rare.
>
> * doc/wget.texi: Added --disable-header documentation.
> * fuzz/wget_options_fuzzer.dict: Update with --disable-header inputs.
> * src/http.c: removed disabled headers before its creation
> * src/init.c: added new functions to check user disabled headers,
> disable headers, remove user headers
> * src/main.c: added new option disable-header, added help description
> * src/options.h: added new option disable-header
> * testenv/Makefile.am: Added new test files
> * testenv/server/http/http_server.py: Added new rule RejectHeaderField
> * testenv/conf/reject_header_field.py: Added new rule RejectHeaderField
> * testenv/README: Added help description for new rule
> * testenv/Test-disable-default-headers.py: Test without using --header
> * testenv/Test-disable-headers-after.py: Test using --header before
> --disable-header
> * testenv/Test-disable-headers-before.py: Test using --header after
> --disable-header
>
> Signed-off-by: sulfastor , adham elkarn
>
> ---
> doc/wget.texi | 19
> fuzz/wget_options_fuzzer.dict | 17 +++
> src/http.c | 140
> src/init.c | 88 +++
> src/main.c | 4 +
> src/options.h | 1 +
> testenv/Makefile.am | 3 +
> testenv/README | 4 +
> testenv/Test-disable-default-headers.py | 73
> testenv/Test-disable-headers-after.py | 77 +
> testenv/Test-disable-headers-before.py | 77 +
> testenv/conf/reject_header_field.py | 12 ++
> testenv/server/http/http_server.py | 8 ++
> 13 files changed, 476 insertions(+), 47 deletions(-)
> create mode 100644 testenv/Test-disable-default-headers.py
> create mode 100644 testenv/Test-disable-headers-after.py
> create mode 100644 testenv/Test-disable-headers-before.py
> create mode 100644 testenv/conf/reject_header_field.py
>
> diff --git a/doc/wget.texi b/doc/wget.texi
> index 7eada2dd..7a194bb2 100644
> --- a/doc/wget.texi
> +++ b/doc/wget.texi
> @@ -1542,6 +1542,25 @@ wget --header="Host: foo.bar" http://localhost/
> In versions of Wget prior to 1.10 such use of @samp{--header} caused
> sending of duplicate headers.
>
> +@cindex disable header, choose
> +@item --disable-header=@var{header-field}
> +Remove @var{header-field} among the headers in each @sc{http} request.
> +
> +You may define more than one additional header field by specifying
> +@samp{--disable-header} more than once as in @samp{--header}.
> +
> +@example
> +@group
> +wget --disable-header='Accept' \
> + --disable-header='User-Agent' \
> + --disable-header='Accept-Encoding' \
> + http://fly.srk.fer.hr/
> +@end group
> +@end example
> +
> +Specificating a header field with @samp{--header} after disabling it
> +will override it and include it in the @sc{http} request headers.
> +
> @cindex Content-Encoding, choose
> @item --compression=@var{type}
> Choose the type of compression to be used. Legal values are
> diff --git
Re: [Bug-wget] [PATCH] Disable automatic wget headers.
Hi.
We're working on the --disable-header feature.
We are taking into account the comment made by Dale Worley to avoid regression.
While implementing it we remarked that cmd_spec_header in init.c is just
appending any header read from the command line, allowing duplicates. Shouldn't
that be a string set? Or a dynamic array is used for performance? A set would
be useful to easily remove disabled headers.
Sulfastor
On Apr 28, 2019, at 21:19, Dale R. Worley wrote:
>>> From bug #54769 (https://savannah.gnu.org/bugs/?54769).
>> Some servers doesn't handle well some headers. A --disable-header option
>> will ensure a request header
>> will not be included on the request. In addition a empty header value in
>> --header="headername: " will also disable
>> the request header.
>
> I may have missed it, but I don't see any updates to the documentation
> files for the new option.
>
> Also, is there a possibility that "a empty header value in
> --header="headername: " will also disable the request header." will
> cause problems because someone wants to specify a header with an empty
> value? RFC 7230 (the definition of HTTP) section 3.2 ("Header Fields")
> contains this BNF:
>
> header-field = field-name ":" OWS field-value OWS
>
> field-name = token
> field-value= *( field-content / obs-fold )
>
> which says that empty field-value's are allowed. OTOH, I've never seen
> an empty value in a header field of an HTTP request.
>
> I suggest that removing the definition of "an empty header value in
> --header means to suppress the header" is the safest way to make this
> change.
>
> Dale
>
Re: [Bug-wget] [PATCH] Disable automatic wget headers.
Hi,
thanks for your contribution !
On a first (quick) glimpse, it looks pretty good to me.
There are just a few things you could improve:
- please use the same coding style as the existing code, e.g.
space after function names, space after 'if' and 'for', space after a
cast ().
+ if (!disabled_header("Authorisation") && auth_err == RETROK)
typo: "Authorization"
+ for (p = s; *p && *p != '\0' && !c_isspace (*p); p++)
checking "*p != '\0'" is redunndant, it is the same as *p
- please add documentation in doc/wget.texi
- please add one or more tests in testenv/
- please add "disable-header=" into fuzz/wget_options_fuzzer.dict plus
the used header names (fixed strings like "Authorization"). This is for
helping the fuzzer to generate input that covers the new code.
Did you think about corner cases, e.g. what happens when a user puts
disableheader=referer into .wgetrc and later wants to override it on the
command line via --header="Referer=..." ?
Regards, Tim
On 27.04.19 18:34, adham elkarn wrote:
> From: sulfastor
>
> Hello,
> We've worked on this features (bug #54769
> (https://savannah.gnu.org/bugs/?54769)) taking in account the comments made
> by Darshit some time ago. We would like you to review our changes and code.
> We thank you for your helping us improving our programming skills.
>
> * src/http.c: removed disabled headers before its creation
> * src/init.c: added new functions to check user disabled headers, disable
> headers
> * src/main.c: added new option disable-header, added help description
> * src/options.h: added new option disable-header
>
> From bug #54769 (https://savannah.gnu.org/bugs/?54769).
> Some servers doesn't handle well some headers. A --disable-header option will
> ensure a request header
> will not be included on the request. In addition a empty header value in
> --header="headername: " will also disable
> the request header.
>
> Signed-off-by: sulfastor , adham elkarn
>
> ---
> src/http.c| 137 ++
> src/init.c| 43
> src/main.c| 4 ++
> src/options.h | 1 +
> 4 files changed, 142 insertions(+), 43 deletions(-)
>
> diff --git a/src/http.c b/src/http.c
> index 289d1101..79cd2168 100644
> --- a/src/http.c
> +++ b/src/http.c
> @@ -88,6 +88,7 @@ static char *basic_authentication_encode (const char *,
> const char *);
> static bool known_authentication_scheme_p (const char *, const char *);
> static void ensure_extension (struct http_stat *, const char *, int *);
> static void load_cookies (void);
> +static bool disabled_header(char*);
>
> static bool cookies_loaded_p;
> static struct cookie_jar *wget_cookie_jar;
> @@ -152,6 +153,8 @@ struct request {
>int hcount, hcapacity;
> };
>
> +/* Forward decls. */
> +static bool request_remove_header (struct request*, const char*);
>
> /* Create a new, empty request. Set the request's method and its
> arguments. METHOD should be a literal string (or it should outlive
> @@ -245,6 +248,13 @@ request_set_header (struct request *req, const char
> *name, const char *value,
>return;
> }
>
> + /* A empty value is a disabled header; so remove it from the request */
> + if (!*value)
> +{
> + request_remove_header(req, name);
> + return;
> +}
> +
>for (i = 0; i < req->hcount; i++)
> {
>hdr = >headers[i];
> @@ -436,7 +446,7 @@ maybe_send_basic_creds (const char *hostname, const char
> *user,
>DEBUGP (("Host %s has not issued a general basic challenge.\n",
>quote (hostname)));
> }
> - if (do_challenge)
> + if (!disabled_header("Authorization") && do_challenge)
> {
>request_set_header (req, "Authorization",
>basic_authentication_encode (user, passwd),
> @@ -1770,23 +1780,29 @@ read_response_body (struct http_stat *hs, int sock,
> FILE *fp, wgint contlen,
>
> #ifdef __VMS
> #define SET_USER_AGENT(req) do { \
> - if (!opt.useragent)\
> -request_set_header (req, "User-Agent", \
> -aprintf ("Wget/%s (VMS %s %s)", \
> -version_string, vms_arch(), vms_vers()), \
> -rel_value); \
> - else if (*opt.useragent) \
> -request_set_header (req, "User-Agent", opt.useragent, rel_none); \
> + if(!disabled_header("User-Agent")) \
> +{\
> + if (!opt.useragent) \
> + request_set_header (req, "User-Agent", \
> + aprintf ("Wget/%s (VMS %s %s)", \
> +
