Re: [Bug-wget] Implementing draft to update RFC6265

[Tim Ruehsen]

> > 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() ?

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."

Tim

On Wednesday 27 January 2016 14:32:51 Kushagra Singh wrote:
> I believe I am checking for the macro HAVE_SSL whenever required, did I
> miss it out anywhere? I'll have a look again.
> 
> Kush
> 
> On 27/01/2016 2:30 pm, "Tim Ruehsen" <address@hidden> wrote:
> > Hi Kushagra,
> > 
> > 
> > I made a few tests with and without UTF-8 locale - and they all succeeded
> > here.
> > 
> > (@Darshit: Could you reproduce the test failures outside Travis ?)
> > 
> > What about the '#ifdef HAVE_SSL' ? Don't we need the check always ?
> > Imagine you have some secure cookies saved and then try a non-SSL version
> > of
> > Wget. Should the secure cookies eventually be overwritten in this case ?
> > 
> > > As for testing the code, should I follow the way its being done in
> > > Test-*.py files?
> > 
> > Yes, please.
> > 
> > Tim
> > 
> > On Wednesday 27 January 2016 05:12:12 Kushagra Singh wrote:
> > > Hi,
> > > 
> > > You're absolutely right, I have merged the commits into one patch, and
> > > removed the trailing whitespaces in the patch. Please find it attached,
> > > I
> > > hope it's okay now.
> > > 
> > > I have fixed the error which was breaking the build when configured with
> > > "--without-ssl". I can't figure out why the build fails on the other two
> > > cases. I am unable to replicate them as well. The tests which are
> > > failing
> > > in the build are Test-cookie and Test-cookie-401, both of which run
> > > successfully when I execute make check. Am I missing something here?
> > > 
> > > As for testing the code, should I follow the way its being done in
> > > Test-*.py files?
> > > 
> > > Thanks,
> > > Kush
> > > 
> > > On Sun, Jan 24, 2016 at 6:14 PM, Darshit Shah <address@hidden> wrote:
> > > > Hi Kushagra,
> > > > 
> > > > Thanks for the patches! A couple of remarks on your patches before I
> > > > dive into the code:
> > > > 
> > > > 1. Please merge your changes into fewer, more logical patches. Making
> > > > small patches when working locally is a good idea, but when you submit
> > > > them for merging, they should be reorganized into logical changes. In
> > > > the case of your patches, one adds the new function, and the next
> > > > patch removes it. We don't need this in the git history.
> > > > 2. Your patches have trailing whitespace in them. Please configure
> > > > your editor to either alert you about this, or fix it silently. Most
> > > > good text editors have these features. You can even configure your Git
> > > > to complain if you try to commit anything with whitespace errors.
> > > > Trailing whitespaces can be a major pain when merging larger patches
> > > > or branches in the future.
> > > > 3. A good patch would also contain a test case that fails before. but
> > > > passes after the patch is applied. This way we can verify the
> > > > correctness of the patch and ensure that no regressions occur in the
> > > > future.
> > > > 
> > > > Apart from these, the build failed on Travis with the --without-ssl
> > > > configure option. That is an issue worth looking into.
> > > > The other two builds on travis failed inside the multi-byte locale
> > > > tests. It's probably some subtle bug, but we'll have to fix that as
> > > > well.
> > > > 
> > > > The code itself looks good. Will provide a deeper review once the
> > > > larger, more obvious issues have been sorted.
> > > > 
> > > > 
> > > > On 24 January 2016 at 12:38, Kushagra Singh
> > > > 
> > > > <address@hidden> wrote:
> > > > > I have added the first two out the three recommendations in the
> > 
> > draft.
> > 
> > > > The
> > > > 
> > > > > third one is relevant when cookies have to be removed in case the
> > 
> > total
> > 
> > > > > number of cookies hit a predefined upper bound, I'm not sure whether
> > 
> > we
> > 
> > > > > do that in wget?
> > > > > 
> > > > > As you mentioned, I had to change some method prototypes to get the
> > 
> > uri
> > 
> > > > > scheme. I made sure that I replaced all instances of those function
> > > > > calls
> > > > > with the right call. The tests run fine, so hopefully I haven't
> > 
> > broken
> > 
> > > > > anything.
> > > > > 
> > > > > I am attaching the patch files, please review them.
> > > > > 
> > > > > Thanks,
> > > > > Kush
> > > > > 
> > > > > On Sun, Jan 24, 2016 at 4:39 AM, Darshit Shah <address@hidden>
> > 
> > wrote:
> > > > >> On 23 January 2016 at 23:36, Kushagra Singh
> > > > >> 
> > > > >> <address@hidden> wrote:
> > > > >> > Thanks a lot for the help!
> > > > >> > 
> > > > >> > I've made some progress, but have a couple of more questions
> > > > >> > 
> > > > >> > - I can't manage to find the http-only-flag in the cookie struct,
> > 
> > do
> > 
> > > > we
> > > > 
> > > > >> not
> > > > >> 
> > > > >> > store this?
> > > > >> 
> > > > >> Since Wget supports only HTTP, this is not required. The HttpOnly
> > > > >> attribute prevents access to script code, but since Wget never
> > > > >> executes them it is not necessary at all. Although, it may be a
> > > > >> good
> > > > >> idea to explicitly store the flag for Wget saves the cookies to a
> > > > >> file. Maybe, we should add this.
> > > > >> 
> > > > >> > - The draft asks to check whether the "scheme" component of the
> > > > >> > "request-uri" denotes a secure protocol or not. Currently I am
> > > > 
> > > > checking
> > > > 
> > > > >> > using "#ifdef HAVE_SSL". I am not sure whether this is the right
> > 
> > way
> > 
> > > > to
> > > > 
> > > > >> do
> > > > >> 
> > > > >> > so, since having SSL with wget does not necessarily mean that the
> > > > 
> > > > current
> > > > 
> > > > >> > connection is secure.
> > > > >> 
> > > > >> Ideally, a code base should have as few #ifdef statements as
> > 
> > possible.
> > 
> > > > >> They make reading the code very difficult for a human. That said,
> > > > >> in
> > > > >> this scenario it is the absolute wrong technique. You will want to
> > > > >> access the scheme from the request URI. Find a way to access this
> > > > >> information, you may need to change some method prototypes to make
> > > > >> this happen.
> > > > >> 
> > > > >> > - To check whether there exists a cookie whose domain,
> > 
> > domain-matches
> > 
> > > > the
> > > > 
> > > > >> > domain of a new cookie, we should iterate through the chains
> > 
> > returned
> > 
> > > > by
> > > > 
> > > > >> > find_chains_of_host right?
> > > > >> 
> > > > >> That ought to work, I think.
> > > > >> 
> > > > >> > Regards,
> > > > >> > Kush
> > > > >> 
> > > > >> --
> > > > >> Thanking You,
> > > > >> Darshit Shah
> > > > 
> > > > --
> > > > Thanking You,
> > > > Darshit Shah

signature.asc
Description: This is a digitally signed message part.