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

[Kushagra Singh]

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
>

0001-Added-recomendations-of-draft-west-leave-secure-cook.patch
Description: Text Data