8000 [v4.0] Add stricter ValidAt constraint · Issue #554 · lcobucci/jwt · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[v4.0] Add stricter ValidAt constraint #554

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.

Sign up for GitHub

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

Jump to bottom
Closed
Slamdunk opened this issue Nov 25, 2020 · 6 comments · Fixed by #555
Closed

[v4.0] Add stricter ValidAt constraint #554

Slamdunk opened this issue Nov 25, 2020 · 6 comments · Fixed by #555
Labels
Improvement
Milestone
4.1.0

Comments

@Slamdunk
Copy link
Collaborator
Slamdunk commented Nov 25, 2020 •
edited
Loading

Hi, it seems to me that the new ValidAt constraint is very dangerous:

jwt/test/unit/Validation/Constraint/ValidAtTest.php

Lines 218 to 225 in 6d8665c

public function assertShouldNotRaiseExceptionWhenTokenDoesNotHaveTimeClaims(): void
{
$token = $this->buildToken();
$constraint = new ValidAt($this->clock);
$constraint->assert($token);
$this->addToAssertionCount(1);
}

It creates a false sense of security, and it's easy to mess with: I've already had a successful token misused because no time claim was set.

I propose to rename ValidAt to LooseValidAt, and introduce a new StrictValidAt that requires all time claims to be set
@Slamdunk
Copy link
Collaborator Author
Slamdunk commented Nov 25, 2020 •
edited
Loading

DateTimeImmutable and NULL comparison returns true https://3v4l.org/J7OGi :

jwt/src/Token/Plain.php

Lines 67 to 75 in 6d8665c

public function hasBeenIssuedBefore(DateTimeInterface $now): bool
{
return $now >= $this->claims->get(RegisteredClaims::ISSUED_AT);
}
public function isMinimumTimeBefore(DateTimeInterface $now): bool
{
return $now >= $this->claims->get(RegisteredClaims::NOT_BEFORE);
}

Pretty dangerous too!

@Slamdunk Slamdunk mentioned this issue Nov 25, 2020
Merged
2 tasks
@Ocramius
Copy link
Collaborator

@Slamdunk can we turn it into a test case? Seems like an oversight that could have security implications

@Slamdunk
Copy link
Collaborator Author

I tried to get hasBeenIssuedBefore tested, but then I run into assertShouldNotRaiseExceptionWhenTokenDoesNotHaveTimeClaims test so I thought it was done on purpose with the reasoning: "If no time claim has been set, the token shall always be valid in time".

I'm totally against this, but wanted to wait @lcobucci for a reply before a PR

@lcobucci
Copy link
Owner

This constraint was created based in the behaviour we had in v3.x (nothing fails when claims aren't set).
Your concern is valid, though! It's a pity we didn't raise this point during the alpha stages.

We cannot rename it any more (BC-break) but can definitely create the strict one (which makes sure we have the claims before of performing the validations with the Token).

Does it make sense to you @Slamdunk?
8000

@Slamdunk
Copy link
Collaborator Author

We cannot rename it any more (BC-break)

We can proxy ValidAt to LooseValidAt by composition, and deprecated ValidAt within a MINOR release.

I'll PR everything soon if it's ok to you. A 4.1.0 can be released right-away without hassles I think

@lcobucci
Copy link
Owner

@Slamdunk sounds like a plan 👍

@Slamdunk Slamdunk mentioned this issue Nov 25, 2020
Merged
@lcobucci lcobucci added this to the 4.1.0 milestone Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Improvement
Projects
None yet
Milestone
4.1.0
Development

Successfully merging a pull request may close this issue.

Add StrictValidAt constraint, deprecate ValidAt to LooseValidAt
3 participants
@Slamdunk @Ocramius @lcobucci
0