-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[2.0] Introduce adapter interface and implementations #17
Conversation
This simplifies the code of the SecureHeaders class quite a bit.
public function replace($name, $value = '') | ||
{ | ||
$this->headers[strtolower($name)] = $value; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I think I messed up the difference between add
and replace
here. add
should still add the header if it already exists, just add another equally-named header...
Changes are looking good, thanks for the great work! 😃 (Just a few tiny queries 😉):
Thanks again for all those changes so far! |
Thanks for the detailed feedback! I have stumbled across most of these already and was working on a follow-up PR after sending this one.
|
|
Also, just RE the capitalisation again – some clients (like IE11 😞) have been known to break when Header-Names-Dont-Look-Like-This. That's kind of why I made |
Hmm, extracting the type checking method is kind of tricky, as it is coupled to the This is not needed, in my humble opinion. If the type checks fail, the program should just fail completely, we don't need to try to get some of the behavior that we wanted. After all, we are trying to mirror PHP 7 here, right? |
Not "needed" as far as mirroring PHP7 – though IMO sending the headers that we know about is better than a failure causing headers not to be sent at all. (An enhancement on top of PHP 7 if you like). Also, since this class is really all about headers – having a misconfiguration prevent all headers being sent seems a bit, "suicidal" (lack of a better word). If things like CSP don't get sent then it could open the page up to script execution – supposing that an attacker could somehow force the exception. Not a perfect solution I know (ideally people should keep user input away from headers), but given PHP will serialise arrays and strings in Would it be completely unreasonable to suggest that SecureHeaders to pass its own instance along to the HeaderBag maybe? (Similar to what happens when a type error is thrown?). |
We're talking about a type error. I would argue that's a much worse thing to an app/website than a few headers not being sent. |
Well that's why we throw the exception – though the exception shouldn't become a vector for disabling CSP (for example), IMO. We're definitely not talking about ignoring the error – we're just sending everything that was okay up until that point. (If the response is completely lacking in headers, that can put the user in danger). |
Incidentally by sending the headers from |
A method called Again, these errors are meant to show when the developer makes an error - preventing this exception from being thrown at all is the developer's job. I don't agree that we are talking about a real vector here. |
P.S.: And, if anything, the problem about such a "vector" would be that the site breaks down, not that some headers do or do not get sent. |
Hmm... I'm not sure – I think the application responding in the most helpful way should be the goal? If the type error is left uncaught then the dev pretty clearly hasn't accounted for it – if we send the headers at this point then we get behaviour that is close to encountering an exception after using the built in I do agree that it is the devs fault, and they should catch the exception, but I think we can at least try to honour the developer's intent for things that were previously configured correctly? RE From the docs:
|
Ha, that's the nature of PHP. ;) IMO, the key point here is can these exceptions happen surprisingly in production? And given where the |
😉 I'd say that yes, it's possible one of these exceptions could happen surprisingly in production (especially on public scoped functions). Even just as a mechanism to cover self-inflicted failures I think it's valuable. If I push an update that casts a variable to the wrong type in a place I didn't test for – I'd rather the application generated an exception, than the application generated an exception and someone's CSP doesn't get deployed! (remember that SecureHeaders does things like cookie upgrades too – would rather these things happened even in the event of a type error, than throw an error that allows cookies to go out without the secure attribute). |
Note that this removes a "feature" of the SecureHeadersTypeError class that was able to send headers when being serialized as string. I disagree with Aidan about the best way forward, but for the sake of separation of concerns, this feature needs to be removed here. He will have to decide whether it should be added again on top of these classes. See the discussion starting in aidantwoods#17 (comment).
Okay, I just pushed two commits that extract the type assertion method into its own static helper class, and uses it from the If you can't live without it, it will have to be added back on top of these classes. Again, I'd strongly advocate against it. It violates separations of concerns, I would say it's unexpected, it uses the global PHP methods (so would not work well with adapters), and it is trying to be smart in cases where the developer has messed up (so badly that they should notice in development). And in any case, trying to convert any weirdly-typed input into expected types seems like more in line with classic dynamic typing as beloved and hated in PHP. And I'd still like to see that example of where a developer might get surprised by a sudden wrong type, for any of these methods. ;) As for your other feedback, I'll try to get it done in the next few days. |
Okay, casing of headers and the add/replace thing is now fixed. What's left is now dealing with |
Awesome! 😃 Changes looking great there! Thanks for all the hard work.
Okay ;p setcookie('auth', 'supersecretauthenticationstring');
$headers = new SecureHeaders;
$headers->csp('default', 'none');
$headers->cspNonce('script');
$page = $_GET['redirect'];
$headers->header('Location', $page);
$headers->done();
Right now, already configured headers (like CSP), and automatic actions tied to If we take away this special exception handling, then you'd get an output like this:
Basically a free pass for an attacker on the network/one that can find XSS to do what they like with the page/cookies. Given that the type in this case is dictated by the user (thanks PHP 😉) – I can't see it being an obvious one to spot in development (bar a pentest – in which case it should be obvious). Perhaps we can make this functionality optional to solve both our concerns? Regarding whether it should default to on/off, I can open up another issue perhaps and we can see if anyone from #3 has any input? P.S. RE
there's no reason that the built in I do take your point that it would be unexpected, so perhaps configuration options would be a must if we were to keep this (though wouldn't an uncaught exception be just as unexpected?). |
*/ | ||
public function getHeaders() | ||
{ | ||
return new HeaderBag(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know StringHttpAdapter
isn't really meant to ever retrieve headers as an object, but just to keep this as a good implementation example for others, I guess this should be return new HeaderBag($this->headers);
, rather than a blank list of headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not really, as getHeaders()
is called before $this->headers
is ever populated.
@@ -23,7 +23,10 @@ public static function fromHeaderLines(array $lines) | |||
|
|||
foreach ($lines as $line) | |||
{ | |||
list($name, $value) = explode(': ', $line, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No guarantee that explode would return two values, and that space is optional but not ignored if present. Unfortunately I think that's slightly too conditional for a nice one liner, would be pleased if wrong though!
@@ -103,6 +106,6 @@ public function getValue() | |||
|
|||
public function __toString() | |||
{ | |||
return $this->name . ($this->value === '' ? '' : ': ' . $this->value); | |||
return $this->name . ':' .(empty($this->value) ? '' : ' ' . $this->value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much like PHPs type hinting – that type assertion function will allow null
to be passed (so you can see if the user left the default value in, without having to hard code check for the default). SecureHeaders defaults a header value to null if it isn't specified, so it's plausible that the value may be of null type (which should also set a header with no value).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both bugs fixed here were brought over from my code of course, just explaining the change here ;-)
$this->headerBag = $this->httpAdapter->getHeaders(); | ||
|
||
foreach ($this->headerBag->get() as $header) | ||
{ | ||
$this->addHeader($header->getName(), $header->getValue()); | ||
} | ||
|
||
# delete them (we'll set them again later) | ||
$this->headerBag->removeAll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This avoids the headerBag receiving headers that it already has when headers are sent back to it (though the httpAdapter should still (as it currently does) remove headers from its header source before sending it new ones – for the same duplication reasons).
I've replaced Test case for |
To me, it more and more looks like the better way to fix this trouble is by being liberal in what we accept: don't raise exceptions on invalid types, but convert them to the appropriate types where possible. In the example you posted, this would result in the string 'Array' - which obviously doesn't make sense, but would be the result of a programmer error anyway, so it's hard to expect good results anyway. ;) As for My plan is still to replace tl;dr This is done, right? |
👍 would love to see this done – I think the only thing that is really left to implement is the header value deconstruction (and changing the references in code to use the header bag, of course).
I was thinking about something along those lines too actually – would be useful if you wanted to say, use the string adapter to get the list of headers, and send them off to a framework/PHPs list.
Yup! That may be a good change – though I suppose if we don't have the adapter implemented in the constructor, then it would make it impossible for
Hmm, I did consider doing that – though I kind of already do accept as many types as I can where possible e.g. SecureHeaders/SecureHeaders.php Lines 593 to 603 in 180913f
For types where nothing reasonable can be inferred, if we don't throw an exception what is the alternative? Do we cast (leading to a strange result that may be hard for the programmer to track down the cause of), or do we silently fail (making it hard to track down why a function isn't doing anything)? I'd love to just check types myself and blame the programmer for anything bad that happens when they use the wrong one, but there are more than a few legitimate situations where PHP is particularly unhelpful with the so called "type-juggling", e.g. TL;DR Yup, I think this is done. I think I'd still like to add the custom exceptions back in, but that's maybe best left for later once the Give me a couple hours (should be able to get some final testing done within then) and will merge 😄 |
As discussed in #3, this PR starts by adding a HTTP adapter interface and a basic implementation using PHP's global functions a la
headers_list()
,header_remove()
andheader()
.This also opens the door for introducing a simple adapter implementation that simply collects all headers and returns them as a string - for use in tests, which - I believe - was the only use of the
headersAsString
mode.To prevent more tampering with actual HTTP responses than necessary, I also introduced a new
HeaderBag
class as a middleman that is added to / removed from before actually sending / removing all headers at once. Turns out, this is more or less what is already done with the internal$this->headers
array, so I will remove that array in a next PR once theHeaderBag
can completely fulfill its role.Hope you like it. :)