Thalhammer / jwt-cpp

A header only library for creating and validating json web tokens in c++
https://thalhammer.github.io/jwt-cpp/
MIT License
855 stars 233 forks source link

traits/nlohmann-json using json::parse with exceptions without catch, an uncaught exception is generated when handling bad input. #293

Open lijl44 opened 1 year ago

lijl44 commented 1 year ago

What happened?

There is the following functions in the include/jwt-cpp/traits/nlohmann-json/traits.h header file.

static bool parse(json& val, std::string str) {
            val = json::parse(str.begin(), str.end());
            return true;
        }

This function uses json::parse with exceptions, an uncaught exception is generated when handling bad input.

Could you please refer to the following nlohmann-json official example? Thank you.

// parse with exceptions
try
{
    json j = json::parse(text);
}
catch (json::parse_error& e)
{
    std::cout << e.what() << std::endl;
}

// parse without exceptions
json j = json::parse(text, nullptr, false);

if (j.is_discarded())
{
    std::cout << "the input is invalid JSON" << std::endl;
}
else
{
    std::cout << "the input is valid JSON: " << j << std::endl;
}

How To Reproduce?

No response

Version

0.6.0

What OS are you seeing the problem on?

Linux

What compiler are you seeing the problem on?

GCC

Relevant log output

No response

Code of Conduct

prince-chrismc commented 1 year ago

This library is not exception free so could you elaborate on what the advantages of this would be?

This would be suppressing a useful error case for users with malformed token as well. I am curious what pros you see to this?

lijl44 commented 1 year ago

In the expected case, jwt-cpp throws an invalid_json_exception exception when parsing json fails. struct invalid_json_exception : public std::runtime_error

In fact, I failed when I tried to catch std::runtime_error because a nlohmann::json::parse_error exception was thrown instead. class parse_error : public exception class exception : public std::exception

Obviously, the actual exception thrown does not match the expected one. I think a change to this would be beneficial to make jwt-cpp's behavior consistent across json libs, thanks.

Thalhammer commented 1 year ago

I am curious what pros you see to this?

I think the bigger issue is that using the error_code overloads might still throw exceptions in this case.

prince-chrismc commented 1 year ago

Yes but the traits could easily be modified.

However we'd need to do this for every single json library where some might not support this. Then how do we expose that?

I think as a header-only library we do not care how the JSON libraries are compiled or setup. https://github.com/nlohmann/json/blob/a0c1318830519eac027a31edec1a99ce1ae5670e/docs/mkdocs/docs/home/exceptions.md?plain=1#L29 could easily be setup with the current implementation to have the desired effect?

I work on a package manager so in my head if you need to set something on a dependency it's not the responsibility of the upstream projects to do that for you.

Especially for the "user provided override" there's no obvious we could handle that? 🤔

lijl44 commented 1 year ago

I agree with you, in fact I'm just giving feedback since nlohmann-json is a commonly used json lib.

prince-chrismc commented 1 year ago

Oh this is a good discussion let's keep it open