Skip to content
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

PSR-7 support, or else array of headers to be returned #5

Closed
Ocramius opened this issue Jan 3, 2016 · 13 comments
Closed

PSR-7 support, or else array of headers to be returned #5

Ocramius opened this issue Jan 3, 2016 · 13 comments

Comments

@Ocramius
Copy link
Contributor

Ocramius commented Jan 3, 2016

This repo provides very useful functionality, but directly messes with global state via the header function.

Hereby I suggest one of either:

  • being able to inject CSP headers in a PSR-7 request (wouldn't become a dependency: just a dev-dependency or a mocked one)
  • provide the list of headers as an array (mainly because there is more than one header, according to
    \header($which.': '.$this->compiled);
    if ($legacy) {
    // Add deprecated headers for compatibility with old clients
    \header('X-'.$which.': '.$this->compiled);
    $which = $this->reportOnly
    ? 'X-Webkit-CSP-Report-Only'
    : 'X-Webkit-CSP';
    \header($which.': '.$this->compiled);
    )
@paragonie-scott
Copy link
Member

Something like this?

function injectCSPHeader(\Psr\Http\Message\MessageInterface $message, $legacy = false)
{
    if ($this->needsCompile) {
        $this->compile();
    }
    // Are we doing a report-only header?
    $which = $this->reportOnly 
        ? 'Content-Security-Policy-Report-Only'
        : 'Content-Security-Policy';
    $message->withAddedHeader($which, $this->compiled);
    if ($legacy) {
        // Add deprecated headers for compatibility with old clients
        $message->withAddedHeader('X-'.$which, $this->compiled);
        $which = $this->reportOnly 
            ? 'X-Webkit-CSP-Report-Only'
            : 'X-Webkit-CSP';
        $message->withAddedHeader($which, $this->compiled);
    }
    return $message;
}

@Ocramius
Copy link
Contributor Author

Ocramius commented Jan 3, 2016

@paragonie-scott either that or simply return the array of headers

Note that $message is immutable, therefore every with call returns a new object (referring to the snippet above)

@paragonie-scott
Copy link
Member

84ad3c5

How does that look?

@Ocramius
Copy link
Contributor Author

Ocramius commented Jan 3, 2016

@paragonie-scott looks good: is it covered by tests?

@paragonie-scott
Copy link
Member

The PSR-7 part isn't, yet. I don't really use PSR-7 anywhere directly so I'll need to find a way to add a unit test without adding a dependency to e.g. Guzzle.

@Ocramius
Copy link
Contributor Author

Ocramius commented Jan 3, 2016

@paragonie-scott $this->getMock('Name\Of\Interface', ['the', 'list', 'of', 'methods', 'that', 'are', 'expected', 'to', 'exist'])

@paragonie-scott
Copy link
Member

https://github.com/paragonie/csp-builder/blob/master/src/CSPBuilder.php#L269

This will break when it returns null.

@Ocramius
Copy link
Contributor Author

Ocramius commented Jan 3, 2016

@paragonie-scott you can use $this->getMock()->expects(self::exactly(2))->method('withFoo')->willReturnSelf();

@paragonie-scott
Copy link
Member

Expectation failed for method name is equal to string:withHeader when invoked 2 time(s).
Method was expected to be called 2 times, actually called 0 times.

.PHP Fatal error: Class Mock_MessageInterface_a0173770 contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Psr\Http\Message\MessageInterface::withHeader) in /mnt/share/csp-builder/vendor/phpunit/phpunit-mock-objects/src/Framework/MockObject/Generator.php(305) : eval()'d code on line 270

I've never used mocking before, and I have no idea what I'm even doing.

@Ocramius
Copy link
Contributor Author

Ocramius commented Jan 3, 2016

@paragonie-scott I'll send a PR :-)

Ocramius added a commit to Ocramius/csp-builder that referenced this issue Jan 3, 2016
Ocramius added a commit to Ocramius/csp-builder that referenced this issue Jan 3, 2016
@Ocramius
Copy link
Contributor Author

Ocramius commented Jan 3, 2016

@paragonie-scott see #6

Ocramius added a commit to Ocramius/csp-builder that referenced this issue Jan 3, 2016
@paragonie-scott
Copy link
Member

Has this been adequately addressed in the latest release?

@Ocramius
Copy link
Contributor Author

Looks like this was done in 1.2.0, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants