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

[Tim Ruehsen]

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