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

If cookie doesn't exist, an error is thrown #5

Closed
callumacrae opened this issue Mar 19, 2014 · 4 comments
Closed

If cookie doesn't exist, an error is thrown #5

callumacrae opened this issue Mar 19, 2014 · 4 comments

Comments

@callumacrae
Copy link

In my opinion, a better behaviour would be to just fail the verification.

@odino
Copy link
Contributor

odino commented Mar 20, 2014

Hi @callumacrae!

I quite dont get the issue, meaning that this library doesnt deal with cookies, the README just shows you a very simple / basic example

Yes, for sure you will need to check if the cookie exists. For example, we use Sf2 and in our code the JWS validation is fired after checking the cookie exists:

if ($cookies->has('identity')) {
    // do stuff
}

@callumacrae
Copy link
Author

I didn't quite get the issue at the time, either!

What I think I meant is that I think that the library should not throw a fatal error if it is given an invalid token, it should stay quiet (or possibly throw a NOTICE if you really must), and then return false on ->verify().

@odino
Copy link
Contributor

odino commented Mar 23, 2014

mmm, it should already act exactly like this, can you write a small test case? Or maybe add a failing test?

@callumacrae
Copy link
Author

I can't get your tests to run (phpunit doesn't seem to be doing anything), but the following code is failing and I don't think it should be:

$jws = JWS::load('invalid');
$jws->verify($publicKey);

I've worked around it for when the token doesn't exist, but if the token is simply invalid I have no way of telling. It's actually making the server throw a 500, so I'm not sure what is happening there (and it's failing on multiple servers).

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

Successfully merging a pull request may close this issue.

3 participants