search

aidantwoods / SecureHeaders

A PHP library aiming to make the use of browser security features more accessible.
MIT License
423 stars 23 forks source link

Ensure line-break is not OS dependent. #29

Closed lucasmichot closed 7 years ago

lucasmichot commented 7 years ago

Ensure line-break is not OS dependent.

franzliedke commented 7 years ago

Can you explain the use-case (or the bug this prevents)? As far as I remember, this adapter is only used in unit testing...

lucasmichot commented 7 years ago

It is, but tests are also run in Windows OS

franzliedke commented 7 years ago

Are you saying tests are not passing on Windows?

lucasmichot commented 7 years ago

I am personally not using Windows for my development, but this has come an issue in other situations. I am curious to see if they pass on Widows tho

franzliedke commented 7 years ago

From a quick check, we only used assertContains, never compared the full string.

aidantwoods commented 7 years ago

Although this adapter is only used for testing, it seems to me that the most correct thing to do would be to use "\r\n" in all cases so that we're following the standard HTTP/1.1 message format?

https://tools.ietf.org/html/rfc7230#section-3

aidantwoods commented 7 years ago

We never resolved this one, right? Any objections to switching from \n to \r\n for the string adapter? (we can write some logic into the tests to auto-convert what we've typed – since all files are using unix LF EOLs).

(this would have the benefit of being technically valid for output in a HTTP response – though I would be surprised if anything actually rejected headers because of LF instead of CRLF).