Fix joins not to make strings unsafe #704
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
UweKubosch
approved these changes
Sep 25, 2023
| @@ -81,7 +81,8 @@ def get_error_messages(name) | |||
| end | |||
| end | |||
|
|
|||
| object.errors[name].join(", ") | |||
| safe_join(object.errors[name], ", ") | |||
| # object.errors[name].join(", ") | |||
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The basic
String#joinmakes its output unsafe in all cases. This might have been causing unwanted and problematic escaping in some cases. This PR removes the use ofString#join. It should still escape unsafe strings passed in from the calling code, while not escaping anything that is already safe. However, that's something reviewers should put some thoughts into, and comment on.This PR may cause some text to be escaped that wasn't previously escaped, but those cases would have been potentially security vulnerabilities, so I think it's justifiable in closing that possible loophole. The fix would be for the calling code to pass its string as
string.html_safeif the code is confident the string is safe (i.e. it doesn't come from the user or the database or something external). If the string wasn't safe, then it should get escaped.