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

Show parts of new URL instead incoming values #4619

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nicodh
Copy link
Member

@nicodh nicodh commented Feb 10, 2025

resolves partly #4559

This PR fixes two issues:

  • punycode is shown to the user
  • detect capital letters looking similar to other lower case letters like capital i looking like lower case L in some font-types (l != I) and show a warning

It does not fix the issue that message parser is too greedy and adds some characters to domain names. (deltachat/message-parser#91)

Before: opening a markup link with trailing )

image

After:

image

Clicking a link like https://deIta.chat just opened https://deita.chat

Now:

image

#skip-changelog

- warn user if URL.hostname is not equal to target.hostname
Copy link
Member

@Simon-Laux Simon-Laux left a comment

Choose a reason for hiding this comment

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

showing the punycode to the user is intentional - it is a security feature to show the real url instead of what it appears to be: https://en.wikipedia.org/wiki/IDN_homograph_attack

the bug is in the message parser.

@Simon-Laux
Copy link
Member

Simon-Laux commented Feb 11, 2025

Since there is also deltachat/message-parser#92 (which breaks labeled links with unicode) we could ignore puny code completely for this dialog for now. but we should read it once it is fixed in the message parser -> so if we go that route then please add a comment on the issue I just linked to that we don't forget to re-add it

@nicodh
Copy link
Member Author

nicodh commented Feb 12, 2025

showing the punycode to the user is intentional - it is a security feature to show the real url instead of what it appears to be

If any Destination coming from message parser which has a punycode property != undefined is an invalid or suspicious Link, then we should add a condition not go to LabeledLinkConfirmationDialog but always show a warning if it is present.

I saw in the source code that punycode is named PunycodeWarning in message parser, so maybe that would be a better name to understand "from outside" what's behind it

the bug is in the message parser.

yes that's what I wrote in the description

@nicodh nicodh marked this pull request as draft February 15, 2025 17:44
@nicodh
Copy link
Member Author

nicodh commented Feb 15, 2025

Waiting for another solution Simon said he is planning to implement

@Simon-Laux Simon-Laux self-assigned this Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants