Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #6277 Detect unknown unicode in sign message requests and some improvement for misleading message #6300

Merged
merged 4 commits into from
Nov 2, 2022

Conversation

nuo-xu
Copy link
Contributor

@nuo-xu nuo-xu commented Nov 1, 2022

Summary of Changes

We will detect unknown unicode in sign request message. (any unicode that is no inside [0, 127] would be considered as unknown)
When unknown unicode is detected, a corresponding warning will show up with a cta button that users can switch between original message and message in ascii-encoding.
We will also detect consecutive newline characters (2 or more consecutive newline characters).
If those consecutive newline characters will push textview content size overflowing the textview frame size, another warning will show up and the message consecutive newline characters will be replaced with pilcrow sign with a number right after indicates how many newline characters are detected.
When message contains both unknown characters and consecutive newline characters that will make message overflow the textbox, two warnings will show up.
Note: the CTA button that to show message unknown unicodes will escape the pilcrow sign.

This pull request fixes #6277

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()
  • New or updated UI has been tested across:
    • Light & dark mode
    • Different size classes (iPhone, landscape, iPad)
    • Different dynamic type sizes

Test Plan:

  1. Test message has unknown unicode (check warning message and CTA)
  2. Test message has multiple consecutive newline characters that will overflow the textbox (check warning message and the pilcrow signs)
  3. Test message has multiple consecutive newline characters that will NOT overflow the textbox (check there should not be a warning or pilcrow formatted)
  4. Test message has both unknown unicode and consecutive newline characters that will overflow the textbox (check two warning messages; check the pilcrow sign; check warning CTA display correct message)
  5. Test message has unknown unicode and consecutive newline characters that will NOT overflow the textbox (check one warning message and CTA)
  6. Test a regular message (check no warning message)

Screenshots:

Simulator.Screen.Recording.-.iPhone.SE.2nd.generation.-.2022-11-01.at.11.21.55.mp4

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue and pull request is assigned to a milestone (should happen at merge time).

…s that will overflow the textbox, or has unknown uncode. Add the ability for user to display message in ascii-encoding. and pilcrow sign for consecutive newline characters.
@nuo-xu nuo-xu added this to the 1.45 milestone Nov 1, 2022
@nuo-xu nuo-xu self-assigned this Nov 1, 2022
@StephenHeaps
Copy link
Contributor

Small UI bug when showing 2+ different requests. One with the ASCII warning and one with the pilcrow warning view:
https://user-images.githubusercontent.com/5314553/199275325-4c064226-d186-48c2-b67b-9f6522372c1b.mov

I believe the ASCII should only show on one request, and the pilcrow on the other. Seems like the pilcrow warning message isn't being removed

@thypon
Copy link
Member

thypon commented Nov 1, 2022

Should we have a Show Original Message if we mangle the spaces or the ASCII characters here?
Cc @stoletheminerals

@stoletheminerals
Copy link
Contributor

Should we have a Show Original Message if we mangle the spaces or the ASCII characters here? Cc @stoletheminerals

We have this for non-ascii, probably a good idea to have a similar toggle for new lines

@diracdeltas
Copy link
Member

Should we have a Show Original Message if we mangle the spaces or the ASCII characters here? Cc @stoletheminerals

We have this for non-ascii, probably a good idea to have a similar toggle for new lines

need to be careful here so that if the original message actually contains pilcrows, they don't get replaced with newlines

@stoletheminerals
Copy link
Contributor

Should we have a Show Original Message if we mangle the spaces or the ASCII characters here? Cc @stoletheminerals

We have this for non-ascii, probably a good idea to have a similar toggle for new lines

need to be careful here so that if the original message actually contains pilcrows, they don't get replaced with newlines

Yep. We don't need to replace pilcrows back to /n, we can just show the original message

Copy link
Contributor

@stoletheminerals stoletheminerals left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for the quick fix @nuo-xu

@nuo-xu nuo-xu merged commit a2991d7 into development Nov 2, 2022
@nuo-xu nuo-xu deleted the wallet-misleading-signing-request-message branch November 2, 2022 16:03
@StephenHeaps StephenHeaps mentioned this pull request Oct 26, 2023
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misleading Signing Request Message (BRA-Q322-7)
5 participants