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.

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

Use SecureQLabel for message previews #720

Merged
merged 1 commit into from Jan 22, 2020

Conversation

emkll
Copy link
Contributor

@emkll emkll commented Jan 21, 2020

Description

Fixes #706 , ensures the message previews are properly escaped. Reply previews are also escaped.

before:

before

after:

after

Test Plan

  • Build a deb on this branch
  • install into sd-app or sd-app-buster-template
  • submit a message with rich text tags on source interface
  • observe tags are escaped properly
  • the tests provided in this pr provide sufficient coverage

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

If these changes add or remove files other than client code, packaging logic (e.g., the AppArmor profile) may need to be updated. Please check as applicable:

  • I have submitted a separate PR to the packaging repo
  • No update to the packaging logic (e.g., AppArmor profile) is required for these changes
  • I don't know and would appreciate guidance

@eloquence
Copy link
Member

eloquence commented Jan 21, 2020

Screenshot from 2020-01-21 15-04-55

Regression: "Compose a reply to" formatting in reply placeholder is now escaped

@emkll emkll moved this from Ready for Review to In Development in SecureDrop Team Board Jan 21, 2020
@emkll emkll force-pushed the 706-plaintext-message-previews branch from 4a62479 to da4a714 Compare January 21, 2020 23:15
@emkll
Copy link
Contributor Author

emkll commented Jan 21, 2020

Dropped the second commit, as the ReplyWidget inherits from SpeechBubble https://github.com/freedomofpress/securedrop-client/blob/master/securedrop_client/gui/widgets.py#L1638 which uses SecureQLabel https://github.com/freedomofpress/securedrop-client/blob/master/securedrop_client/gui/widgets.py#L1589 so that change was not required

@emkll emkll moved this from In Development to Ready for Review in SecureDrop Team Board Jan 21, 2020
@eloquence
Copy link
Member

That did the trick, regression is fixed. Tested outside Qubes for now, works as expected -- preview snippets & messages/replies look identical, and are displayed safely.

@sssoleileraaa sssoleileraaa moved this from Ready for Review to Under Review in SecureDrop Team Board Jan 22, 2020
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

tested in qubes and lgtm!

@sssoleileraaa sssoleileraaa merged commit 2f8a0a9 into master Jan 22, 2020
SecureDrop Team Board automation moved this from Under Review to Done Jan 22, 2020
@sssoleileraaa sssoleileraaa deleted the 706-plaintext-message-previews branch January 22, 2020 14:48
@rmol rmol mentioned this pull request Jan 28, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Snippets are not correctly escaped
3 participants