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

Compilation errors (VS 2022 32-bit /W3 /SDL) #278

Open Kemp-J opened 1 year ago

Kemp-J commented 1 year ago

When building a project that uses the jwt-cpp header in Visual Studio 2022 with warning level 3 and SDL checks enabled there are several errors raised related to suspicious implicit casts. These can be easily solved with a few tweaks. Specifically:

In modern C++ both of the above points can be solved with auto in preference to specifying the types.

I was going to raise an issue about a common base class for exceptions, but it looks like someone got there first :)

prince-chrismc commented 1 year ago

Hmm, never heard of /sdl very cool see msvc getting way better.

I would love to see a PR with these fixes 🙏

Kemp-J commented 1 year ago

I'll see what I can do. I use git elsewhere but I'm quite new to using it with github :grimacing:

prince-chrismc commented 1 year ago

It's the same tbh, just create a fork (there a button top left) then git clone your copy, and the rest is the same. After you push you changes to a branch, this repo will have a "open pull request banner" if not the pull request tab has a "new" button :)

It sounds like a lot but the CLI will even output hints along the way with links!

Kemp-J commented 1 year ago

A followup issue - using nlohmann json with jwt-cpp has additional errors for 32-bit builds. The source of this is that is_substr_start_end_index_signature and is_substr_start_index_signature check for the existence of substr member functions on string_type, but they do so on the assumption that they will accept integer_type for the parameters. The integer_type for nlohmann json is int64_t, the substr function defined for std::string has size_t parameters, and thus the error as the compiler sees you effectively trying to pass int64_t values to a size_t parameter (unsigned int on 32-bit builds). Given that, as far as I can tell, you don't actually need substr to accept integer_type specifically, is it worth swapping the check to look for substr taking size_t?

prince-chrismc commented 1 year ago

Windows 32-bit?! I've not touched that in 8 years :) I have no idea.

Kemp-J commented 1 year ago

Yeah, I have a few things I have to build as 32-bit for... historical reasons.

I guess there's mainly a question of intent here. If there is a reason substr needs to take integer_type, because it's called with values directly from a token, then it'll need some creativity. I don't think your calls to substr are used like that though (pretty much just hardcoded values and ones calculated from other sources) so moving the check away from integer_type wouldn't cause any problems. If you have insight on anything I missed then I'd be happy to hear it, otherwise I'll roll that into my changes. Might as well have 32-bit working if it only requires a small tweak here and there :slightly_smiling_face: