-
Notifications
You must be signed in to change notification settings - Fork 222
Conversation
- fixes "local variable 'params' referenced before assignment" when JSON is malformed - return "unsupported action" when JSON body is not an object - refactor to make if-statements more readable
Hello, I wanted to tackle this problem as well. I did it a bit differently.
This method would greatly benefit from a complete rewrite, but I didn't want to make a big change so I opted for the smallest change. Here's the diff if you are curious (I didn't write tests at the time): oakkitten@954dae6. |
Hey, I didn't know this is already being worked on. I'm not really that experienced in python anyway, so let's go with yours.
I'm not a security expert by any means, but I do I agree limiting the information from unauthorized hosts is indeed better. I think I failed to account for that one.
This may leak some implementation details that may not be necessarily useful for API consumers, especially for non-python users. A custom error message might be better. |
Oh I didn't mean to imply that this issue is mine or anything, I did touch it but never told anyone or even push it to my fork before I saw this PR. Just wanted to put in my two cents so to say.
Perhaps, but then Anki-Connect normally does output exception strings. And, end users most likely will never see this message, for whatever they use will probably use a cute and neat JSON library that never outputs bad JSON. And if they are constructing the JSON themselves, they are likely to enjoy the actual message even if they don't know Python, eh? |
I don't think I've ever seen a JSON library that doesn't barf out an exception with the slightest error, even with trailing commas. But that aside, speaking from my recent experience, I do tend to construct JSON manually when exploring the API with curl. Suppose I was a new user to the API, and I see very specific messages like |
I meant that if the user is using something like Yomichan, under the hood it is constructing a very proper JSON and there's no way they would see the cryptic error message. And if the user is constructing JSON themselves, they probably won't get confused by the exact error. Still, you have a point. There's still a case where a user might use some poor software that constructs JSON in a manual way. And, something like
This would also solve the issue of Anki raising errors without text, so the user gets |
when JSON is malformed