[server] Fix quotes in system comments #3094
Conversation
| @@ -1613,6 +1613,7 @@ def getComments(self, report_id): | |||
| sys_comment = comment_kind_from_thrift_type( | |||
| ttypes.CommentKind.SYSTEM) | |||
| if comment.kind == sys_comment: | |||
| message = message.replace("'", "\\'") | |||
There was a problem hiding this comment.
I don't think this is enough. What if it contains double quotes, or backticks, or other shell-special characters? Maybe shlex.quote() works good for you? Not sure, since I don't understand what shlex.split() is used for.
There was a problem hiding this comment.
@jimis Good point, you are right. I changed it to use the shlex.quote function and I've also extended the test case with more special characters.
csordasmarton
force-pushed
the
fix_quotes_in_system_comments
branch
from
0368b80 to
2610836
Compare
csordasmarton
force-pushed
the
fix_quotes_in_system_comments
branch
from
2610836 to
50e14c6
Compare
|
I verify that this fixes the issue: quotes are possible to be inserted, and the previously impossible-to-view comments, are now viewable. So +1 from me. On a side discussion however, it might be worth deprecating "status change comments" alltogether. They look ugly (they merge newlines and paragraphs), and when someone writes two-three paragraphs explaining why the issue is a false-positive, it's hard to read. By disabling the addition of new "status change comments", people will add separate comments after changing the status. They preserve all formatting and are easier to read. |
|
@jimis Could you please give us a screenshot from an ugly comment? How it looks like? |
|
Here is almost the same comment written within the status change, and as a separate comment. You can see that in the first case all whitespace is lost, and the comment comes in small letters.
|
csordasmarton
force-pushed
the
fix_quotes_in_system_comments
branch
from
50e14c6 to
4e0c80b
Compare
| new_review_status = \ | ||
| escape_whitespaces(review_status.status.capitalize()) | ||
| old_review_status = shlex.quote(old_status.capitalize()) | ||
| new_review_status = shlex.quote(review_status.status.capitalize()) |
There was a problem hiding this comment.
Since the value of return statuses is determined by review_status_str(), there is no special character to escape. So these shlex.quote() calls don't change the strings.
| # without escaping special characters such as | ||
| # quotes. This is kept only for backward | ||
| # compatibility reason. | ||
| elements = shlex.split(shlex.quote(message)) |
There was a problem hiding this comment.
The result of shlex.split() on the result of shlex.quote() will always be a one-element list. This is not suitable for checking whether the first element is an indicator of system comments. So in this environment this line is equivalent with pass, I mean system comments stored with old CodeChecker version will not be formatted in either case. Another option would be to use simple string split, because it works both for rev_st_changed and rev_st_changed_msg but not for comment_changed if the comment contains spaces.
Commenting on an issue using quotes results to the error "No closing quotation" because of `shlex.split`.
csordasmarton
force-pushed
the
fix_quotes_in_system_comments
branch
from
4e0c80b to
38cd5a1
Compare
Commenting on an issue using quotes results to the error
"No closing quotation" because of
shlex.split.