phpgt / csrf Goto Github PK

View Code? Open in Web Editor NEW

7.0 4.0 7.0 366 KB

Automatic protection from Cross-Site Request Forgery.

Home Page: https://www.php.gt/csrf

License: MIT License

PHP 100.00%

csrf csrf-protection csrf-protector csrf-tokens security php-security automatic-protection phpgt

csrf's People

Contributors

Stargazers

Watchers

Forkers

j4m3s gitter-badger g105b xinze testuser97-max nusaibsqli isabella232

csrf's Issues

Implement token time-outs

This would reduce the attack surface, countering the fact that far more tokens will be generated to allow AJAX forms to be permitted (feature #14).

At the moment, the ArrayTokenStore remains in-play for as long as the session survives.

@g105b any thoughts on the timeout? 30 mins maybe? Don't want to frustrate people who take a phone-call or take a long time filling out a form before they click a button.

Allow the length of token to be altered/ specified

Currently it's set at 64 characters, and there is no way for users to change it. Frankly, 6 characters would probably be more than sufficient and considerably quicker (though it's pretty snappy already...)

Add csrf token meta tag to <head>

Currently, javscript would have to parse the token out of a form somewhere in the page (assuming one exists). Adding a specific tag to the head (which is reliably in the same place) would simplify the javascript logic considerably.

Change namespace to Gt\Csrf for consistency.

Remember composer.json's autoload bit.

Limit the memory footprint on busy sites

Spawned from #16.

On busy sites with many forms per page, long session timeouts and many users the amount of memory used for storing all these tokens could become significant.

Obvious options to resolve include a timeout or a limit to the number of tokens in circulation per user.

The timeout is not optimal because it would have to be long (2 hours?) to allow users to take a call or be part way through filling out a form then doing something else etc

The fixed number of tokens seems more useful as it can be tuned to an appropriate depth for a given application - a number much smaller than the total number of tokens generated over a period of hours. HOWEVER, should a user leave one tab open then start working on another they could invalidate the tokens associated with the first page. This could be mitigated by setting the limit to a fairly high number (1,000?) but the risk would remain.

Thoughts @g105b?

SessionTokenStore

Idea of an extra TokenStore type that abstracts the need to get, set and check the session variables yourself.

Store POST data - do not use globals

Using global variables is discouraged in the StyleGuide

Instead, provide a method for setting the input data. Maybe allow array or InputData?

Make compatible with PHP.Gt/Dom's new Facade

The DOM repository is being refactored and will no longer directly extend the native DOMDocument. See the facade branch for more details.

Inappropriate copyright notice?

As an open source project I don't think it's right to mark code as copyright Brightflair Ltd - that's simply not the case for code written by others and will discourage contributions :)

One token per form

Rather than one token for all forms, this will allow ajax requests to use the already generated tokens rather than having to generate their own.

Should tokens be stored in meta tag when generating one token per form?

The last command in protectAndInject sets the meta's content to the last-generated token for the page. What about the other ones?

Dependabot

Dependabot needs to be tamed, as per PhpGt/WebEngine#568

TokenStore::verify does not need to return bool

It is never going to return false, as it should throw an exception if the token is invalid.

Leaving the bool return type could lead to confusion in how new implementations of TokenStore should behave.

"HTMLDocument" clash?

There is already the HTMLDocument within the DOM, so having a class of this name within the csrf package might be confusing?

Automate tests in CircleCI

Allow DOMDocument usage

I was working on a legacy project for a client that uses the DOMDocument to render the HTML. Using this repo on that project would involve rendering the DOMDocument into a HTMLDocument, then back again.

A few more type checks on $html in the HTMLDocumentProtector's constructor will be beneficial.

Use case: sending token as HTTP header?

When updating parts of the page using JavaScript, rather than having to parse the response for form tags and extract the input elements, it would be really useful to have the tokens provided by HTTP tokens.

Are there any security implications of doing this?
How would responses containing multiple forms be handled?

Update readme to show correct namespaces

Make scrutinizer pass

I can't tell what's going wrong. If you check the history of my circleci pull request you'll see it did pass once... but goodness knows why.

It appears to be failing to consolidate the results of the various steps.

Deprecation notices in PHP 7.1

ircmaxell/RandomLib uses mcrypt, which will be removed in PHP 7.2 for security reasons.

Already tracked here: ircmaxell/RandomLib#55

Possible solution: https://github.com/paragonie/random_compat

Only automate POST forms

Generating a CSRF token for GET forms is not needed.

One token per form

Rather than one token for all forms, this will allow ajax requests to use the already generated tokens rather than having to generate their own.

One token per form by default?

There are so many instances where multiple forms are output to the same response that I think it would be beneficial to have the PER_FORM flag as default.

However, this is still up for the decision of the implementor, in which case WebEngine can make this choice.

Leaving this issue here while I ponder, or if anyone else wants to share their thoughts.

Update test runner

Update to CircleCI 2.0, excuse to organise unit tests too.

Automate code quality checks on Scrutinizer

Use inbuilt crypto features of PHP 7.2

PHP 7.2 ships with inbuilt crypto features, such as cryptographically secure random byte generator (https://www.php.net/manual/en/function.random-bytes.php)

There is no need to depend on ircmaxel's fantastic library any more, and as such, we will no longer have to depend on packages such as:

    "symfony/console": "^3.4.17 || ^4.1.6",
    "symfony/event-dispatcher": "^3.0 || ^4.0",
    "symfony/filesystem": "^3.0 || ^4.0",
    "symfony/finder": "^3.0 || ^4.0",
    "symfony/options-resolver": "^3.0 || ^4.0",
    "symfony/polyfill-php70": "^1.0",
    "symfony/polyfill-php72": "^1.4",
    "symfony/process": "^3.0 || ^4.0",
    "symfony/stopwatch": "^3.0 || ^4.0"

@g105b it seems this is something only you can do

DatabaseTokenStore

This would use Gt\Database's QueryBuilder to ensure engine-agnostic queries are generated independent of whatever database engine the developer is using.