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

[Darshit Shah]

On 27 January 2016 at 10:00, 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 ?)

Interestingly, no. But Travis seems to fail repeatedly. I'll try to
set up a similar environment next week (too many things to finish this
week) to see if it comes from a environment related bug.
The patch doesn't seem to do anything that should affect only the
multi-byte characters in the URL. Or does it miss out on handling them
correctly somewhere? I'll review the patch again later today to see if
that's the case.

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

I would argue that Wget should still refuse to overwrite a secure
cookie. The #ifdef guards need to be smartly placed to allow this.
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.
>
>> 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



-- 
Thanking You,
Darshit Shah