Skip to content

Conversation

@juliusknorr
Copy link
Member

Make sure that the link we create is still valid in markdown, even if a user wrongly enters spaces into the input when adding a link through the UI.

To reproduce.

  • Insert a link into a text file with "mailto: [email protected]" as the url
  • Close the document
  • Make sure to reset the text state for the document so that the document is refetched from the markdown source occ text:reset FILEID -f
  • Reopen

@juliusknorr juliusknorr added bug Something isn't working 3. to review labels Jul 26, 2021
Copy link
Contributor

@azul azul 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.

I checked that a mailto link with %20 still works. Also confirmed that rfc3986 does not allow for spaces to occure anywhere in urls.

I tried to use other characters that usually do not appear in urls but these days are encoded and still displayed by the browser. In particular i tried: https://はじめよう.みんな . The browser opened the link just fine. So limiting the encoding to space charactes seems like a good approach as not everything needs to be encoded that is not a strictly valid url char.

@juliusknorr
Copy link
Member Author

Yep, markdown-it which we use to transform the markdown to the document seems to explicitly stop parsing links with ascii control characters including whitespace while allowing anything else: https://github.com/markdown-it/markdown-it/blob/e5986bb7cca20ac95dc81e4741c08949bf01bb77/lib/helpers/parse_link_destination.js#L50

@juliusknorr juliusknorr merged commit 95159cb into master Jul 26, 2021
@juliusknorr juliusknorr deleted the bugfix/noid/link-with-space branch July 26, 2021 19:07
@juliusknorr
Copy link
Member Author

/backport to stable22

@juliusknorr
Copy link
Member Author

/backport to stable21

@juliusknorr
Copy link
Member Author

/backport to stable20

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable21 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable20 failed. Please do this backport manually.

@azul azul added the backported successfully backported label Jan 11, 2022
@max-nextcloud max-nextcloud removed backport-request backported successfully backported labels Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants