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

[Kushagra Singh]

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
>