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

Date validation does not work when using nlohmann traits #103

Closed
seghcder opened this issue Oct 18, 2020 · 4 comments · Fixed by #104
Closed

Date validation does not work when using nlohmann traits #103

seghcder opened this issue Oct 18, 2020 · 4 comments · Fixed by #104
Assignees
Labels

Comments

@seghcder
Copy link

Creating a token is fine...

    auto token = jwt::create<nlohmann_traits>()
            .set_issuer(jwt_issuer)
            .set_issued_at(std::chrono::system_clock::now())
            .set_expires_at(std::chrono::system_clock::now() + std::chrono::seconds{3600})
            .sign(jwt::algorithm::hs256{jwt_secret});

... however when running verify, nlohmann_traits throws a bad cast during the expiry check here -

static int64_t as_int(const json &val) {
if (val.type() != json::value_t::number_integer)
throw std::bad_cast();
return val.get<int64_t>();
}

It seems nlohmann::json is returning the val.type() as unsigned long long (uint64_t which is probably correct for a date). However, jwt-cpp is expecting a signed long long (int64_t).

A workaround is to also allow json::value_t::number_unsigned and then cast to the signed version. Shouldn't be an issue unless the expiry date is 300 billion years from now? :-)

Platform is MSVC 2019 on Windows 10 and nlohmann::json 3.8.0.

@prince-chrismc prince-chrismc self-assigned this Oct 18, 2020
@prince-chrismc
Copy link
Collaborator

Thanks for this report! I'll look into it shortly

prince-chrismc added a commit to prince-chrismc/jwt-cpp that referenced this issue Oct 18, 2020
@prince-chrismc
Copy link
Collaborator

prince-chrismc commented Oct 18, 2020

From https://travis-ci.com/github/prince-chrismc/jwt-cpp/builds/190847927

unknown file: Failure
C++ exception with description "std::bad_cast" thrown in the test body.
[ FAILED ] NlohmannTest.Bug103Expiration (0 ms)

It's a bug! 🐛

@seghcder
Copy link
Author

It's a bit of a weird one. The JSON schema doesn't really have the concept of unsigned integers. However, nlohmann::json is returning the value as an unsigned int. Does it do that for any positive integer, and any negative integer becomes a signed int?

I guess this is one challenge of schemaless formats (ref nlohmann/json#305 too.)

There's some discussion re JWT and JSON date issues here - https://stackoverflow.com/questions/49220571/json-web-token-and-the-year-2038-bug

One suggestion - perhaps separate the struct nlohmann_traits {... out into separate include? I extracted it from your test file and included it as a separate header for my project.

@prince-chrismc
Copy link
Collaborator

One suggestion - perhaps separate the struct nlohmann_traits {... out into separate include? I extracted it from your test file and included it as a separate header for my project.

Yep... needs a little refactor 🙊 It's also in the readme and an example which is not so pleasant to maintain 🤯

It's a bit of a weird one. The JSON schema doesn't really have the concept of unsigned integers. However, nlohmann::json is returning the value as an unsigned int. Does it do that for any positive integer, and any negative integer becomes a signed int?

That's been my experience with the library (though I rarely pass numbers in JSON), and it matches with the docs... https://github.com/nlohmann/json/blob/486812904f29a86f18c9fd78b34fecd6b25a6ae7/include/nlohmann/detail/value_t.hpp#L30-L32

JSON has only a "number" type which can be anything from a C++ perspective, in order to respect the precision I suspect nlohmann/json needs to use the closest C++ type and performs the implicit conversions when you "get" the value.

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

Successfully merging a pull request may close this issue.

2 participants