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.

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

Json::encode(): auto-sanitize bad UTF-8 strings #3444

Merged
merged 5 commits into from Jun 22, 2018
Merged

Json::encode(): auto-sanitize bad UTF-8 strings #3444

merged 5 commits into from Jun 22, 2018

Conversation

Al2Klimov
Copy link
Member

fixes #2635

@Al2Klimov Al2Klimov requested a review from lippserd April 27, 2018 15:27
@Thomas-Gelf Thomas-Gelf self-requested a review April 27, 2018 15:35
Thomas-Gelf
Thomas-Gelf reviewed Apr 27, 2018
View reviewed changes
Copy link
Contributor

@Thomas-Gelf Thomas-Gelf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want this to be usable throughout our modules, such dangerous magic should not be the default. I expect Json::encode to be strict and to fail for invalid data. A failsafe variant is of course helpful for situations like the one referenced here. But then there should be a dedicated method call allowing one to explicitly opt for the failsafe variant.

@Al2Klimov
Copy link
Member Author

Why do you think this is dangerous? Icinga 2 does the same thing before IDO inserts and they're fine (no one complained). And: I've not changed any module not in the repo (e.g. Director) neither there is any similar "peer pressure". Anyway IMO one can expect Json::encode() to do somewhat more than just json_encode().

PS: C'mon, aren't you the master of magic? 😉

@Thomas-Gelf
Copy link
Contributor

Being weak on encoding isn't magic, that's acting carelessly, it's dangerous. All sane parts of our stack should accept valid UTF8 only. There are legacy parts in our stack where we have to live with historic facts. But letting the JSON library accept every kind of garbage per default is just the wrong way of dealing with this. This "forgiving" encoding should be opt-in in an explicit way by calling a dedicated function.

Regarding IDO: I gave my suggestions at the time this has been discussed and implemented. My proposal was to not let invalid UTF8 enter the core, in no way. Illegal check command output should be sanitized by the related component when reading the result. I don't know how it has then been implemented.

@nilmerg
Copy link
Member

nilmerg commented Apr 30, 2018

I also expect Json::encode and Json::decode to behave exactly as their functional counterparts. They may behave better in terms of quality/usability (which is their intention) but definitely not in terms of error handling.

Also, since this is a class in the Icinga\Util namespace, it's highly likely it's being used in the wild. Therefore suppressing exceptions a caller previously handled and expected is bad.

So, please make the sanitation optional.

@dnsmichi
Copy link
Contributor

Depending on where these functions are used, you'd need sanitation or not. I wouldn't move this into the Json* functions, but leave them outside. This renders the code more visible and transparent, and you don't need to know the function signature (which would be different from the inner json_encode($value, $options, $depth)).

@Al2Klimov Al2Klimov self-assigned this May 14, 2018
@Al2Klimov Al2Klimov removed the request for review from lippserd May 14, 2018 08:20
@Al2Klimov Al2Klimov removed their assignment May 14, 2018
@Al2Klimov Al2Klimov requested a review from lippserd May 14, 2018 08:20
@Al2Klimov Al2Klimov force-pushed the bugfix/error-responding-non-html-requests-2635 branch from 0bc0ee3 to 201861b Compare May 14, 2018 09:09
lippserd
lippserd requested changes Jun 8, 2018
View reviewed changes
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Alex,

Thanks for the PR. I opt for Json::sanitize() instead of a parameter to encode. Else you always have to specify the default parameters.

Best,
Eric

@Al2Klimov Al2Klimov self-assigned this Jun 8, 2018
@Al2Klimov Al2Klimov force-pushed the bugfix/error-responding-non-html-requests-2635 branch from fd5e805 to 46a93d0 Compare June 20, 2018 16:06
@Al2Klimov Al2Klimov removed their assignment Jun 20, 2018
@Al2Klimov Al2Klimov requested a review from lippserd June 20, 2018 16:06
lippserd
lippserd approved these changes Jun 21, 2018
View reviewed changes
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done. Thanks!

lippserd
lippserd requested changes Jun 21, 2018
View reviewed changes
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, tests are failing because of a fallthrough case in the switch statement. And I added an inline comment for a small change.

library/Icinga/Web/Response/JsonResponse.php Outdated
@@ -190,7 +222,9 @@ public function outputBody()
$body['data'] = $this->getSuccessData();
break;
}
echo json_encode($body, $this->getEncodingOptions());
echo $this->autoSanitize
Copy link
Member

@lippserd lippserd Jun 21, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use getAutoSanitize(). Subclasses may override this method. Though we don't have any at moment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always used $this->whatever – no one complained. Shall I do member reads like you everywhere in the future?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stumbled upon this several times while working on the ipl. I'd say that we did not care about subclass overrides before. Yes, please use the getter for reads in the future.

@Al2Klimov Al2Klimov force-pushed the bugfix/error-responding-non-html-requests-2635 branch from 46a93d0 to 906c166 Compare June 21, 2018 14:02
@Al2Klimov
Copy link
Member Author

I already added a comment. Didn't our tests respect it?

@Al2Klimov Al2Klimov requested a review from lippserd June 21, 2018 14:03
lippserd
lippserd reviewed Jun 21, 2018
View reviewed changes
library/Icinga/Util/Json.php Outdated
if ($autoSanitize) {
return static::encode(static::sanitizeUtf8Recursive($value), $options, $depth);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need a // Fallthrough comment here.

@Al2Klimov Al2Klimov requested a review from lippserd June 22, 2018 07:57
lippserd
lippserd approved these changes Jun 22, 2018
View reviewed changes
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes 👍

@lippserd lippserd merged commit adfcc55 into master Jun 22, 2018
@lippserd lippserd deleted the bugfix/error-responding-non-html-requests-2635 branch June 22, 2018 09:01
@lippserd lippserd added this to the 2.6.0 milestone Jun 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thomas-Gelf Thomas-Gelf Thomas-Gelf left review comments

@lippserd lippserd lippserd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.

[dev.icinga.com #13427] Improve error handling when responding to non-HTML requests
5 participants
@Al2Klimov @Thomas-Gelf @nilmerg @dnsmichi @lippserd