Hi,
On Thu, Jan 28, 2016 at 2:02 AM, Darshit Shah <[email protected]> wrote: > On 27 January 2016 at 20:52, Kushagra Singh > <[email protected]> wrote: > > > > > > On Wed, Jan 27, 2016 at 5:06 PM, Tim Ruehsen <[email protected]> wrote: > >> > >> > > What about the '#ifdef HAVE_SSL' ? Don't we need the check always ? > >> > >> Sorry for my irritating text. What I tried to ask/say was "Do we need > the > >> #ifdef in cookie_handle_set_cookie() at all ?". > >> > >> And btw, do we need it in parse_set_cookie() ? > >> > > > > I think it is required in parse_set_cookie(). It does not create a secure > > only cookie in case the connection is insecure. Now this can happen > because > > of two reasons, (i) communication over simple HTTP despite wget > configured > > with SSL, (ii) wget configured with the "--without-ssl" option. The log > > output in both the cases should be different, right? > > I don't see the point of it. Why should it be any different? In fact, > why does the end user, who probably installed a distro-packaged > version of Wget care about the configure options? > Every #ifdef statement you add increases the complexity of the code > since it changes what portion of the code is compiled. As long as the > connection is insecure, Wget refuses to set a secure cookie, period. > Don't overengineer the situation. > > I have made the changes accordingly, not checking using preprocessor directives now > > >> Darshit said it with clearer words (and I agree with him): > >> "When a user loads a file backed cookie jar, they expect it to work > >> according to the RFC, irrespective of whether the client supports SSL > >> or not. And especially since support for this does not depend on the > >> actual linking of any SSL library, it shouldn't be hard to implement." > >> > > > > In this case, then can we simply remove the #ifdef check, and and the if > > else statement check whether (scheme == SCHEME_HTTP) and not (scheme != > > SCHEME_HTTPS), since they would essentially mean the same. This should > take > > care of the problem you mention. I have attached a patch with these > changes. > > Seems okay to me right now. > > Please prefer to not move functions around. Adding a prototype to the > top of the file is a better option. Moving a function around like this > causes things like git blame to not work very well. > > On a sidenote, I think the find_chains_of_host() method could use some > refactoring. > > One is that count_char could use a library function like strchr() > instead of trying to run a pass manually. > Something like the way I have done in this patch? > Apart from that, I think some parts could use help from Libpsl. > @Tim: When progressing to less specific domains, I think Libpsl could > provide a way to test if we're moving into a new TLD? > > There's also the section with a while(1) loop that exits using a > simple condition and a break statement. This is bad programming > practice. We should avoid using such a pattern since it obscures the > actual loop condition. Maybe we can refactor it slightly? > > > > > A question about the way things are done in the Wget project, should I > > attach a patch that should be applied in continuation to the last patch I > > sent, or one generated by all the commits? The patch I have attached is > the > > one generated of the last commit only. > > > You should resend the entire patch again. So that everyone has context > and we can simply apply the final version directly. There is no point > in keeping a history of personal edits on the master branch. > When you have a large change that can be logically split into > different commits, you should have multiple patches for each of the > commit. Think of it this way, once you're done implementing a feature > or a bug fix, what version of your code do you want the rest of the > world to see? The one where you made a hundred stupid errors and later > fixed it, or just the final clean version? > That makes a lot of sense, thanks a lot for that! > > > > Kush > > > > -- > Thanking You, > Darshit Shah > Thanks, Kush On Thu, Jan 28, 2016 at 2:02 AM, Darshit Shah <[email protected]> wrote: > On 27 January 2016 at 20:52, Kushagra Singh > <[email protected]> wrote: > > > > > > On Wed, Jan 27, 2016 at 5:06 PM, Tim Ruehsen <[email protected]> wrote: > >> > >> > > What about the '#ifdef HAVE_SSL' ? Don't we need the check always ? > >> > >> Sorry for my irritating text. What I tried to ask/say was "Do we need > the > >> #ifdef in cookie_handle_set_cookie() at all ?". > >> > >> And btw, do we need it in parse_set_cookie() ? > >> > > > > I think it is required in parse_set_cookie(). It does not create a secure > > only cookie in case the connection is insecure. Now this can happen > because > > of two reasons, (i) communication over simple HTTP despite wget > configured > > with SSL, (ii) wget configured with the "--without-ssl" option. The log > > output in both the cases should be different, right? > > I don't see the point of it. Why should it be any different? In fact, > why does the end user, who probably installed a distro-packaged > version of Wget care about the configure options? > Every #ifdef statement you add increases the complexity of the code > since it changes what portion of the code is compiled. As long as the > connection is insecure, Wget refuses to set a secure cookie, period. > Don't overengineer the situation. > > > > >> Darshit said it with clearer words (and I agree with him): > >> "When a user loads a file backed cookie jar, they expect it to work > >> according to the RFC, irrespective of whether the client supports SSL > >> or not. And especially since support for this does not depend on the > >> actual linking of any SSL library, it shouldn't be hard to implement." > >> > > > > In this case, then can we simply remove the #ifdef check, and and the if > > else statement check whether (scheme == SCHEME_HTTP) and not (scheme != > > SCHEME_HTTPS), since they would essentially mean the same. This should > take > > care of the problem you mention. I have attached a patch with these > changes. > > Seems okay to me right now. > > Please prefer to not move functions around. Adding a prototype to the > top of the file is a better option. Moving a function around like this > causes things like git blame to not work very well. > > On a sidenote, I think the find_chains_of_host() method could use some > refactoring. > > One is that count_char could use a library function like strchr() > instead of trying to run a pass manually. > Apart from that, I think some parts could use help from Libpsl. > @Tim: When progressing to less specific domains, I think Libpsl could > provide a way to test if we're moving into a new TLD? > > There's also the section with a while(1) loop that exits using a > simple condition and a break statement. This is bad programming > practice. We should avoid using such a pattern since it obscures the > actual loop condition. Maybe we can refactor it slightly? > > > > > A question about the way things are done in the Wget project, should I > > attach a patch that should be applied in continuation to the last patch I > > sent, or one generated by all the commits? The patch I have attached is > the > > one generated of the last commit only. > > > You should resend the entire patch again. So that everyone has context > and we can simply apply the final version directly. There is no point > in keeping a history of personal edits on the master branch. > When you have a large change that can be logically split into > different commits, you should have multiple patches for each of the > commit. Think of it this way, once you're done implementing a feature > or a bug fix, what version of your code do you want the rest of the > world to see? The one where you made a hundred stupid errors and later > fixed it, or just the final clean version? > > > > Kush > > > > -- > Thanking You, > Darshit Shah >
From f765337085bac0986c9f20dca96d484d55759f45 Mon Sep 17 00:00:00 2001 From: kush789 <[email protected]> Date: Fri, 29 Jan 2016 13:42:06 +0530 Subject: [PATCH] Added recommendations from draft-West --- src/cookies.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++-------- src/cookies.h | 5 ++++- src/http.c | 2 +- 3 files changed, 65 insertions(+), 11 deletions(-) diff --git a/src/cookies.c b/src/cookies.c index 81ecfa5..b2bfb54 100644 --- a/src/cookies.c +++ b/src/cookies.c @@ -122,6 +122,13 @@ struct cookie { same domain. */ }; +static int +find_chains_of_host (struct cookie_jar *jar, const char *host, + struct cookie *dest[]); + +static int +count_char (const char *string, char chr); + #define PORT_ANY (-1) /* Allocate and return a new, empty cookie structure. */ @@ -350,7 +357,8 @@ discard_matching_cookie (struct cookie_jar *jar, struct cookie *cookie) filled. */ static struct cookie * -parse_set_cookie (const char *set_cookie, bool silent) +parse_set_cookie (const char *set_cookie, enum url_scheme scheme, + bool silent) { const char *ptr = set_cookie; struct cookie *cookie = cookie_new (); @@ -439,8 +447,21 @@ parse_set_cookie (const char *set_cookie, bool silent) } else if (TOKEN_IS (name, "secure")) { - /* ignore value completely */ - cookie->secure = 1; + if (scheme == SCHEME_HTTP) + { + /* Deleting cookie since secure only flag is set but connection + is not secure. */ + if (!silent) + logprintf (LOG_NOTQUIET, + _("Trying to create secure only cookie, but connection is not secure.\nSet-Cookie : %s\n"), + quotearg_style (escape_quoting_style, set_cookie)); + delete_cookie (cookie); + return NULL; + } + else + /* Ignore value completely since secure is a value-less + attribute*/ + cookie->secure = 1; } /* else: Ignore unrecognized attribute. */ } @@ -461,6 +482,7 @@ parse_set_cookie (const char *set_cookie, bool silent) return NULL; } + #undef TOKEN_IS #undef TOKEN_NON_EMPTY @@ -707,9 +729,12 @@ check_path_match (const char *cookie_path, const char *path) void cookie_handle_set_cookie (struct cookie_jar *jar, const char *host, int port, - const char *path, const char *set_cookie) + const char *path, enum url_scheme scheme, + const char *set_cookie) { - struct cookie *cookie; + struct cookie *cookie, *old_cookie; + struct cookie **chains; + int chain_count, i; cookies_now = time (NULL); /* Wget's paths don't begin with '/' (blame rfc1808), but cookie @@ -717,7 +742,7 @@ cookie_handle_set_cookie (struct cookie_jar *jar, simply prepend slash to PATH. */ PREPEND_SLASH (path); - cookie = parse_set_cookie (set_cookie, false); + cookie = parse_set_cookie (set_cookie, scheme, false); if (!cookie) goto out; @@ -767,6 +792,29 @@ cookie_handle_set_cookie (struct cookie_jar *jar, } } + if ((cookie->secure == 0) && (scheme == SCHEME_HTTP)) + { + /* If an old cookie exists such that the all of the following are + true, then discard the new cookie. + + - The "domain" domain-matches the domain of the new cookie + - The "name" matches the "name" of the new cookie + - Secure-only flag of old cookie is set */ + + chains = alloca_array (struct cookie *, 1 + count_char (host, '.')); + chain_count = find_chains_of_host (jar, host, chains); + + for (i = 0; i < chain_count; i++) + for (old_cookie = chains[i]; old_cookie; old_cookie = old_cookie->next) + { + if (!cookie_expired_p(old_cookie) + && !(old_cookie->domain_exact + && 0 != strcasecmp (host, old_cookie->domain)) + && 0 == strcasecmp(old_cookie->attr, cookie->attr) + && 1 == old_cookie->secure) + goto out; + } + } /* Now store the cookie, or discard an existing cookie, if discarding was requested. */ @@ -795,9 +843,12 @@ count_char (const char *string, char chr) { const char *p; int count = 0; - for (p = string; *p; p++) - if (*p == chr) - ++count; + p = string; + while ((p = strchr(p, chr)) != NULL) + { + ++count; + p++; + } return count; } diff --git a/src/cookies.h b/src/cookies.h index b4281e1..d565b09 100644 --- a/src/cookies.h +++ b/src/cookies.h @@ -31,13 +31,16 @@ as that of the covered work. */ #ifndef COOKIES_H #define COOKIES_H +#include "url.h" + struct cookie_jar; struct cookie_jar *cookie_jar_new (void); void cookie_jar_delete (struct cookie_jar *); void cookie_handle_set_cookie (struct cookie_jar *, const char *, int, - const char *, const char *); + const char *, enum url_scheme, + const char *); char *cookie_header (struct cookie_jar *, const char *, int, const char *, bool); diff --git a/src/http.c b/src/http.c index 8916d2b..f7d42b2 100644 --- a/src/http.c +++ b/src/http.c @@ -3276,7 +3276,7 @@ gethttp (struct url *u, struct http_stat *hs, int *dt, struct url *proxy, { char *set_cookie; BOUNDED_TO_ALLOCA (scbeg, scend, set_cookie); cookie_handle_set_cookie (wget_cookie_jar, u->host, u->port, - u->path, set_cookie); + u->path, u->scheme, set_cookie); } } -- 1.9.1
