Formatting, usage concerns

[ameliaikeda]

While I love the idea of SecureHeaders being adoptable everywhere, you're not using PSR-1/PSR-4, you've not got this package on composer, and the vast majority of PHP code that is going to be updated will not be able to simply just include it.

In addition, it'd need to be modified so that all the major frameworks currently in use (wordpress/laravel/symfony2/drupal) can actually include it at the correct stages of their lifecycles without needing to modify core code. That one's far more tricky.

A few things you could do to cut down on work:

• Use StyleCI (free for open source) for automatic formatting
  • This is great for moving to PSR-2 (or at least PSR-1, but modern PHP is leaning towards namespaces + PascalCase classes + camelCase methods, etc)
• Autoload files via PSR-4 using Composer
• Add your package to Packagist so people can instantly use it.

People haven't really done the download package / require files / use library approach in several years for newer projects at this point, and it'd be a shame for this package to just die from lack of use.

[beejhuff]

I tend to do most of my PHP work in the Magento community and was getting ready to raise this concern myself (so glad I checked for existing issues before posting...)

The project looks interesting & I've forked it myself for further study...I'm digging into some research on other similar types of work in the Magento space and will provide whatever input / assistance I can after that. Great work and thanks for sharing, Aidan!

[aidantwoods]

@ameliaikeda thanks for the feedback!

• Use StyleCI (free for open source) for automatic formatting
  • This is great for moving to PSR-2 (or at least PSR-1, but modern PHP is leaning towards namespaces + PascalCase classes + camelCase methods, etc)

As I understand it, the PSR-2 standard would only require me to adjust method names? (Personally I prefer the clarity of under_scores to camelCase).
I did anticipate people disagreeing with me on that – which is why most of the key public functions use a single word.

Is there any case toward renaming my private methods and variables for this change, or would adjusting the public scope be sufficient? Renaming the public methods would be pretty easily done, probably most of the work will be adjusting the wiki.

• Autoload files via PSR-4 using Composer
• Add your package to Packagist so people can instantly use it.

These two were on my list of things I planned to get to eventually, it's good to know people are looking for them – will make this a priority!

[aidantwoods]

@ameliaikeda @beejhuff

In addition, it'd need to be modified so that all the major frameworks currently in use (wordpress/laravel/symfony2/drupal) can actually include it at the correct stages of their lifecycles without needing to modify core code. That one's far more tricky.

Any suggestions you have that would help the adoption within major frameworks is something I'll try my best to get done ASAP! 😄

Would be great to get this as easy to use as possible.

[ameliaikeda]

Private methods can generally be any format, really, but in actual practice in PHP with libraries, you may find it a good idea to mark your "private" code as protected instead, so it still remains extensible, and so people can hotfix/tweak methods locally. In doing that, though, everything becomes part of your public API, so it's up in the air at that point.

Personally I'd say go for consistency throughout everything, because then contributing is so much easier.

Any suggestions you have that would help the adoption within major frameworks is something I'll try my best to get done ASAP! 😄

I'd be happy to figure out a Laravel solution, since I have a fair bit of internals knowledge. It'd be easy enough to make a generic middleware for it.

[ameliaikeda]

Also, PSR-2 is far more involved than that but everyone just uses an automatic formatter 😛

[franzliedke]

Interesting library for sure! 👍

Not sure whether this belongs here, but the issue is titled "Usage concerns", so I may as well expand the discussion... The way the library is currently set up, it might be hard to integrate into any modern PHP framework. That's because they typically do not use PHP's global request context methods (such as header and setcookie) directly, but instead operate on various abstractions of HTTP requests and responses.

Most notably, this would probably be PSR-7 requests/responses or the equivalents from Symfony's HttpFoundation component.

So, my first suggestion would be to split the configuration. generation and actual writing of the headers / cookies into separate classes: you'd have a factory that is used for configuring the headers, a class that creates the appropriate HTTP headers and cookies (only the strings) from that configuration, and finally an adapter that actually writes them to either a HTTP response object, or PHP's global functions.

We could then create different adapters for integration with frameworks / other projects: I'd suggest three implementations, for PSR-7, Symfony, as well as the header/setcookie functions

This might seem like overkill, but IMO it would greatly help in a) integratability (I might have made that word up), b) maintainability and c) testability.

If you're interested, I would gladly help in transitioning the package. :)

[svenluijten]

I wholeheartedly agree with @franzliedke! This package has a lot of potential but lacks the standards most of the PHP community has adopted, so it will probably be overlooked pretty quickly 😃

To expand on the automatic formatter @ameliaikeda mentioned, this one is used the most, but styleci can fix all of that after you push to GitHub with a pull request, if you prefer that.

[svenluijten]

Also, concerning PSR-2; I was hesitant at first too, but taking on a standard really lowers the barrier of entry for new contributors to open source projects like this one as well. After having worked with it a couple of weeks, I couldn't do without 😄

[aidantwoods]

Keep the feedback coming!

@franzliedke

So, my first suggestion would be to split the configuration. generation and actual writing of the headers / cookies into separate classes...
... We could then create different adapters for integration with frameworks / other projects: I'd suggest three implementations, for PSR-7, Symfony, as well as the header/setcookie functions

I was considering creating a SecureHeadersCore kind of class a while back actually. I wonder how much actually needs doing though:

SecureHeaders has its own setters for headers, but it actually extracts headers (and cookies) from PHPs internal list and then passes them off to the setters as part of an import (see:

SecureHeaders/SecureHeaders.php

Lines 789 to 848 in 14d30d8

	private function import_headers()
	{
	if ($this->headers_as_string)
	{
	$this->allow_imports = false;
	return;
	}
	# first grab any headers out of already set PHP headers_list
	$headers = $this->preg_match_array(
	'/^([^:]+)[:][ ](.*)$/i',
	headers_list(),
	1,
	2
	);
	# delete them (we'll set them again later)
	header_remove();
	# if any, add these to our internal header list
	foreach ($headers as $header)
	{
	$this->add_header($header[0], $header[1]);
	}
	$this->allow_imports = false;
	}
	private function import_csp($header_value, $report_only)
	{
	$this->assert_types(
	array(
	'string' => array($header_value),
	'bool' => array($report_only)
	)
	);
	$directives = $this->deconstruct_header_value(
	$header_value,
	'content-security-policy'
	);
	$csp = array();
	foreach ($directives as $directive => $source_string)
	{
	$sources = explode(' ', $source_string);
	if ( ! empty($sources) and ! is_bool($source_string))
	{
	$csp[$directive] = $sources;
	}
	else
	{
	$csp[] = $directive;
	}
	}
	$this->csp($csp, $report_only);
	}

)

I guess what I'm saying here is that, provided the headers are loaded into PHPs list when ->done is called (this can be done implicitly on output ). Policies/cookies/other headers are all extracted and put in the correct place within SecureHeaders at this point.

In fact, if you've already configured csp in SecureHeaders for example, and SecureHeaders finds a CSP header – it will perform a merge on the two policies. So the two modes are already pretty compatible.

(For the full chain of events when done is called, check out done in code, and the wiki )

Going back to:

... We could then create different adapters for integration with frameworks / other projects: I'd suggest three implementations, for PSR-7, Symfony, as well as the header/setcookie functions

Would be an easy change!

The setcookie function isn't actually used by SecureHeaders, but there are two calls to the header function (both within the private function send_headers). It should be an easy change to swap that out to call a function that is implementable by a latter class that could sit on top of a new SecureHeadersCore class (class that generates the headers).

Do you mind opening a separate issue for splitting up the class as you've suggested?

I'll use this as a general progress issue to keep track of the separate usability concerns.

@ameliaikeda, @beejhuff, @svenluijten

Looks like the consensus here is to move over to PSR-2 naming conventions. I'll definitely implement this for public methods, and protected variables.

The private stuff I can move over a bit later (without breaking backwards compatibility too).

[svenluijten]

I don't think you should worry too much about backwards compatibility. Simply tag a new major release (1.0.0, perhaps?). That way you wouldn't be breaking semver either.

[aidantwoods]

@ameliaikeda @beejhuff @svenluijten

How's this looking?

26335cf#diff-f050c9357fa31d46256fb41c20cbf3bf

[aidantwoods]

New release: https://github.com/aidantwoods/SecureHeaders/releases/tag/v1.0.0

On packagist: https://packagist.org/packages/aidantwoods/secureheaders

[ameliaikeda]

@aidantwoods you need a namespace if you're going to be using a package. I'd suggest Woods\SecureHeaders, so your primary class is Woods\SecureHeaders\SecureHeaders. Also, stick your code in src and autoload it using "psr-4": { "Woods\\SecureHeaders\\": "src/" }

[svenluijten]

@ameliaikeda You're right that that is common convention, but it's not actually required for a relatively small & simple package like this. Of course, once complexity gets to a point where a one-file package isn't enough, you might consider namespacing it and putting it in a src/ folder. For now though, I think it's just fine the way it is 😃

[lucasmichot]

Hey @aidantwoods great library, but hardly integrable within any modern framework as @franzliedke said
I would love to integrate that with Laravel

I feel it would definitely be worth changing the structure of the project.

[aidantwoods]

@lucasmichot
If you're wanting to use SecureHeaders alongside Laravel, you may be able to use ->doneOnOutput to have SecureHeaders apply itself (and anything you've configured via it) right before content is sent.
Provided Laravel has set its own headers and cookies at this point – SecureHeaders should be able to read them off PHPs internal list (that anything wanting to send headers/cookies will have to write to).

I'll get something pushed soon that splits SecureHeaders into more easily separable components as suggested by @franzliedke. Hopefully then it should just be a case of making adapters for particular frameworks 😄

[ameliaikeda]

@aidantwoods: you can't use any of the built-in PHP functions with most modern frameworks.

They generally have a concept of "header bags", "cookie jars" and so on that are all set in advance, and all of the processing/sending a response is done after your code has completed.

In order for this package to be used in Laravel (example only because I know it), you'd need the ability to pass in arbitrary cookies, and to pass in headers, as well as fetch headers/cookies back out of the state to transform back into a HeaderBag, and so on.

Also @svenluijten: a "simple" single class is still not an excuse to be using the global namespace ^^;

[svenluijten]

I know @ameliaikeda, but I also don't think we should force anyone to use something just because it's a convention. Using the global namespace can cause some conflicts, but it's fine for now. Maybe in a future refactor when it's actually becoming a problem, it can be fixed.

[lucasmichot]

#8 is a good start maybe

[lucasmichot]

Lots of PR have been merged today thanks to @aidantwoods
I believe the new branch 2.0 is a better starting point for adapters

I believed Symfony and Laravel might come very soon 💯 !

[aidantwoods]

@lucasmichot thanks again for all the good work with those PRs! 😄

[aidantwoods]

Since this issue is about usability, I guess this is still relevant 😉

Does anyone have any thoughts on the behaviour I've introduced into type exceptions, discussed here.

To summarise, functions have been designed to throw exceptions when given a type that they can't make any sense of. However, because throwing an exception causes output – PHP will send its current list of header immediately. This means that SecureHeaders no longer has the ability to modify headers, or even give PHP the ones that have already been configured successfully.

I didn't want to remove exceptions, I think it's important that the programmer is made aware of the error if they're available. However, there is always the possibility that these exceptions will happen outside of testing.

For that case I introduced an extension to the Exception class. If the exception is caught, it will behave as normal. However, if the exception is left uncaught, then the exception (before generating output) will call SecureHeaders' done method for you. This means that any previously configured headers (like CSP), and in particular any cookies that would be upgraded to include Secure and HttpOnly flags will be included in the list that PHP will send when output is generated (as well as everything under else performed by done, including safeMode).

I feel that this method best honours the programmer's intent by sending headers that they have already configured, and also by producing the exception to let them know that something is wrong (if they're available to see it).

@franzliedke has pointed out a few counter points to this, which you can read in full starting here. It would be valuable to get some more feedback on whether this should/should not be a feature/should be a feature but the default should be [x].

Thanks!

[aidantwoods]

Lots of work on this front, hopefully all summed up in https://github.com/aidantwoods/SecureHeaders/milestone/1

Let me know if you'd like to see anything added to that: open an Issue/PR :)

Planning to close this issue once that milestone is all complete – let me know of any objections to that!