secureCookie configuration option for cookie secure flag

[mberkowski]

Related to #54 this adds a configurable option to set a secure cookie flag. In the issue we discussed naming it httpsOnly but that may cause confusion with the httpOnly cookie option. It is instead named secureCookie but is easily changed.

It has been added with a false value in the example config, and defaults to false if omitted from the config.

Adding a test required also creating a helper method to retrieve header string values to match in assertions. According to the setcookie() docs, PHP will only set the secure flag if HTTPS is actually present, so the tests explicitly set $_SERVER['HTTPS'] = on. Accordingly that has been nulled out in the setUp() method.

[codecov-io]

Current coverage is 70.71% (diff: 100%)

Merging #55 into master will increase coverage by 0.66%

@@             master        #55   diff @@
==========================================
  Files             2          2          
  Lines           177        181     +4   
  Methods          10         10          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            124        128     +4   
  Misses           53         53          
  Partials          0          0          

Powered by Codecov. Last update 7344ed9...5007eb1

[mebjas]

@mberkowski could you do minor changes as mentioned in review? so that we could merge & close on this one?

[mberkowski]

@mebjas I'm sorry - which changes suggested in review? I have not seen additional comment threads, aside from the code coverage report. I'm unfamiliar with the workflow around codecov.io, if there's a place within it I should be looking for review notes.

[mebjas]

https://github.com/mebjas/CSRF-Protector-PHP/pull/55/files - you'll find review comments here

[mberkowski]

Interesting. No PR review comments are visible to me, nor any change requests. Docs suggest anyone with read access can comment, and I have the option to comment myself, but any comments on the PR are not publicly visible or I would have acted on them.

[mebjas]

@mberkowski
That's strange, I never used this earlier either, so let's leave it. Here's what I suggested

• libs/config.sample.php, you have added a secure cookie flag, please give information about in in readme in same path.
• test/csrfprotector_test.php: L28 -> Nice, could you write function documentation for both this one and checkHeader might come in handy to others. just as brief as testSecureCookie() one
• libs/csrf/csrfprotector.php: L329 -> Leaving empty domain & path is standard practice in such cases? when we need to set https only flag?

Line numbers are according to your push. Hope this workaround works :D

[mberkowski]

Apparently review comments don't become public until you hit "submit review" from the green review dropdown.

Docs are updated as requested. Sorry I missed the second README in libs/.

Regarding the setcookie() defaults - the documentation on it is nonspecific, but both those parameters have default values of the empty string "". For $path that means the current directory and is sort of discussed in the docs. There's no explicit mention of how "" is applied to $domain but I did test that the explicit empty string sets a host cookie identically to the omitted value.

In both cases, the result is the same as before my commit because the PHP defaults are "". To be honest, I don't think I have ever previously set $secure without also explicitly setting the others, but my aim here was to keep exactly the behavior you already had there accepting the "" defaults implicitly.

[mebjas]

Nice, could you write function documentation for both this one and checkHeader might come in handy to others. just as brief as testSecureCookie() one

[mebjas]

Leaving empty domain & path is standard practice in such cases? when we need to set https only flag?

[mebjas]

Give a description about this field in libs/README.md

[mebjas]

Thanks :)