Make WordPress Core

Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#11643 closed defect (bug) (fixed)

Invalid code in wp-login.php

Reported by: hakre's profile hakre Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: General Keywords: has-patch needs-testing
Focuses: Cc:

Description

This just does not makes any sense:

  if ( !$secure_cookie && whatever() && ... )
    secure_cookie = false;

Found in line 480-481 in wp-login.php while reviewing #9207. Fix is simple, will attach a patch.

Attachments (1)

11643.patch (591 bytes) - added by hakre 14 years ago.

Download all attachments as: .zip

Change History (9)

@hakre
14 years ago

#1 @Denis-de-Bernardy
14 years ago

Isn't it there in order to allow for a non-ssl admin while enforcing ssl on the login form?

#2 @dd32
14 years ago

  • Keywords needs-testing added; tested removed

Isn't it there in order to allow for a non-ssl admin while enforcing ssl on the login form?

Looks like it.. But its not doing its job if thats the case. I'm wondering if that was supposed to set it to true myself.

hakre: Can you please explain under what conditions patches have been tested before adding the tested keyword?

needs-testing: Need to verify all the cases are covered with regard to Login https...

#3 follow-up: @hakre
14 years ago

If I put testing there it at least means that the code was executed with the patch applied.

For these SSL related tickets I'm additionally working on a special secure wordpress setup with it's own confgiguration. I keep that on trunk plus the one or other patch all the time. When done I hopefully can finish the documentation properly as well.

#4 in reply to: ↑ 3 @ryan
14 years ago

Replying to hakre:

If I put testing there it at least means that the code was executed with the patch applied.

That makes the tested keyword pretty much useless.

What that line is saying is that if the user was redirected to an ssl login page from a non ssl admin link and that secure login is required but secure admin is not, then don't require a secure cookie. This way the user can POST their login creds over https but not be forced to visit the admin via https. Yes, some people want this. You can do this in gmail too, for example. Also, there is a difference between $secure_cookie = false and $secure_cookie = .

From what I see, the code does what is intended and removing that line breaks a currently supported scenario that is widely used on wordpress.com, for one.

A comment describing all of that is much needed though, as the code is impenetrable. I'll add something.

#5 @ryan
14 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [12665]) Document impenetrable logic in login secure cookie and redirect handler. fixes #11643

#6 @ryan
14 years ago

(In [12666]) Typo fix in comment. see #11643

#7 @dd32
14 years ago

That doesnt really explain the line anymore...

if ( var is false &....
 then set var to false

It'll never be run if $secure_cookie is true, or for that matter, anything non-null/non-false/non-empty.

If $secure_cookie is true, Then none of the rest will be run, and it'll never be set to false. If $secure_cookie is false, then it may get set to false again.

I can see what the logic is intending, I just dont see how it can ever work with !$secure_cookie at the start.

#8 @dd32
14 years ago

ryan: Also, there is a difference between $secure_cookie = false and $secure_cookie = .

..So safely ignore my last comment there, I completely missed the fact that it was possible for that to be non-bool.

Note: See TracTickets for help on using tickets.