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

[Darshit Shah]

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