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

[Darshit Shah]

Looks good now. Would like to see tests for the same though.

On 29 January 2016 at 09:19, Kushagra Singh
<address@hidden> wrote:
> Hi,
>
>
>
> On Thu, Jan 28, 2016 at 2:02 AM, Darshit Shah <address@hidden> wrote:
>>
>> On 27 January 2016 at 20:52, Kushagra Singh
>> <address@hidden> wrote:
>> >
>> >
>> > On Wed, Jan 27, 2016 at 5:06 PM, Tim Ruehsen <address@hidden> wrote:
>> >>
>> >> > > What about the '#ifdef HAVE_SSL' ? Don't we need the check always ?
>> >>
>> >> Sorry for my irritating text. What I tried to ask/say was "Do we need
>> >> the
>> >> #ifdef in cookie_handle_set_cookie() at all ?".
>> >>
>> >> And btw, do we need it in parse_set_cookie() ?
>> >>
>> >
>> > I think it is required in parse_set_cookie(). It does not create a
>> > secure
>> > only cookie in case the connection is insecure. Now this can happen
>> > because
>> > of two reasons, (i) communication over simple HTTP despite wget
>> > configured
>> > with SSL, (ii) wget configured with the "--without-ssl" option. The log
>> > output in both the cases should be different, right?
>>
>> I don't see the point of it. Why should it be any different? In fact,
>> why does the end user, who probably installed a distro-packaged
>> version of Wget care about the configure options?
>> Every #ifdef statement you add increases the complexity of the code
>> since it changes what portion of the code is compiled. As long as the
>> connection is insecure, Wget refuses to set a secure cookie, period.
>> Don't overengineer the situation.
>>
>
> I have made the changes accordingly, not checking using preprocessor
> directives now
>
>> >
>> >> Darshit said it with clearer words (and I agree with him):
>> >> "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."
>> >>
>> >
>> > In this case, then can we simply remove the #ifdef check, and and the if
>> > else statement check whether (scheme == SCHEME_HTTP) and not (scheme !=
>> > SCHEME_HTTPS), since they would essentially mean the same. This should
>> > take
>> > care of the problem you mention. I have attached a patch with these
>> > changes.
>>
>> Seems okay to me right now.
>>
>> Please prefer to not move functions around. Adding a prototype to the
>> top of the file is a better option. Moving a function around like this
>> causes things like git blame to not work very well.
>>
>> On a sidenote, I think the find_chains_of_host() method could use some
>> refactoring.
>>
>> One is that count_char could use a library function like strchr()
>> instead of trying to run a pass manually.
>
>
> Something like the way I have done in this patch?
>
>>
>> Apart from that, I think some parts could use help from Libpsl.
>> @Tim: When progressing to less specific domains, I think Libpsl could
>> provide a way to test if we're moving into a new TLD?
>>
>> There's also the section with a while(1) loop that exits using a
>> simple condition and a break statement. This is bad programming
>> practice. We should avoid using such a pattern since it obscures the
>> actual loop condition. Maybe we can refactor it slightly?
>>
>> >
>> > A question about the way things are done in the Wget project, should I
>> > attach a patch that should be applied in continuation to the last patch
>> > I
>> > sent, or one generated by all the commits? The patch I have attached is
>> > the
>> > one generated of the last commit only.
>> >
>> You should resend the entire patch again. So that everyone has context
>> and we can simply apply the final version directly. There is no point
>> in keeping a history of personal edits on the master branch.
>> When you have a large change that can be logically split into
>> different commits, you should have multiple patches for each of the
>> commit. Think of it this way, once you're done implementing a feature
>> or a bug fix, what version of your code do you want the rest of the
>> world to see? The one where you made a hundred stupid errors and later
>> fixed it, or just the final clean version?
>
>
> That makes a lot of sense, thanks a lot for that!
>
>>
>> >
>> > Kush
>>
>>
>>
>> --
>> Thanking You,
>> Darshit Shah
>
>
>
> Thanks,
> Kush
>
>
> On Thu, Jan 28, 2016 at 2:02 AM, Darshit Shah <address@hidden> wrote:
>>
>> On 27 January 2016 at 20:52, Kushagra Singh
>> <address@hidden> wrote:
>> >
>> >
>> > On Wed, Jan 27, 2016 at 5:06 PM, Tim Ruehsen <address@hidden> wrote:
>> >>
>> >> > > What about the '#ifdef HAVE_SSL' ? Don't we need the check always ?
>> >>
>> >> Sorry for my irritating text. What I tried to ask/say was "Do we need
>> >> the
>> >> #ifdef in cookie_handle_set_cookie() at all ?".
>> >>
>> >> And btw, do we need it in parse_set_cookie() ?
>> >>
>> >
>> > I think it is required in parse_set_cookie(). It does not create a
>> > secure
>> > only cookie in case the connection is insecure. Now this can happen
>> > because
>> > of two reasons, (i) communication over simple HTTP despite wget
>> > configured
>> > with SSL, (ii) wget configured with the "--without-ssl" option. The log
>> > output in both the cases should be different, right?
>>
>> I don't see the point of it. Why should it be any different? In fact,
>> why does the end user, who probably installed a distro-packaged
>> version of Wget care about the configure options?
>> Every #ifdef statement you add increases the complexity of the code
>> since it changes what portion of the code is compiled. As long as the
>> connection is insecure, Wget refuses to set a secure cookie, period.
>> Don't overengineer the situation.
>>
>> >
>> >> Darshit said it with clearer words (and I agree with him):
>> >> "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."
>> >>
>> >
>> > In this case, then can we simply remove the #ifdef check, and and the if
>> > else statement check whether (scheme == SCHEME_HTTP) and not (scheme !=
>> > SCHEME_HTTPS), since they would essentially mean the same. This should
>> > take
>> > care of the problem you mention. I have attached a patch with these
>> > changes.
>>
>> Seems okay to me right now.
>>
>> Please prefer to not move functions around. Adding a prototype to the
>> top of the file is a better option. Moving a function around like this
>> causes things like git blame to not work very well.
>>
>> On a sidenote, I think the find_chains_of_host() method could use some
>> refactoring.
>>
>> One is that count_char could use a library function like strchr()
>> instead of trying to run a pass manually.
>> Apart from that, I think some parts could use help from Libpsl.
>> @Tim: When progressing to less specific domains, I think Libpsl could
>> provide a way to test if we're moving into a new TLD?
>>
>> There's also the section with a while(1) loop that exits using a
>> simple condition and a break statement. This is bad programming
>> practice. We should avoid using such a pattern since it obscures the
>> actual loop condition. Maybe we can refactor it slightly?
>>
>> >
>> > A question about the way things are done in the Wget project, should I
>> > attach a patch that should be applied in continuation to the last patch
>> > I
>> > sent, or one generated by all the commits? The patch I have attached is
>> > the
>> > one generated of the last commit only.
>> >
>> You should resend the entire patch again. So that everyone has context
>> and we can simply apply the final version directly. There is no point
>> in keeping a history of personal edits on the master branch.
>> When you have a large change that can be logically split into
>> different commits, you should have multiple patches for each of the
>> commit. Think of it this way, once you're done implementing a feature
>> or a bug fix, what version of your code do you want the rest of the
>> world to see? The one where you made a hundred stupid errors and later
>> fixed it, or just the final clean version?
>> >
>> > Kush
>>
>>
>>
>> --
>> Thanking You,
>> Darshit Shah
>
>



-- 
Thanking You,
Darshit Shah