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

User descriptions support common URI schemes #2496

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Pullze
Copy link

@Pullze Pullze commented Nov 3, 2024

Hi, this is my first time contributing to liberapay. If there's anything needed, please feel free to point out!

What's fixed:
As #2492, some URI schemes are not supported yet. By inspecting, the reason is that the regex in markdown.py only matches http, https, and xmpp. Common URI schemes are added to this regex.

Test string:

[NNTP](nntp://comp.unix.bsd.freebsd.misc), [IRC](irc://network/#channel), [HTTPS](https://test.com/#), [HTTP](http://testhttp.com/#)

Render result before fix:
image

Render result after fix:
image

For the common URI schemes, I referred to this list. I only added the most common ones for now since handling other uri such as files or app deeplinks may not be the desired behavior.

@Changaco
Copy link
Member

Changaco commented Nov 4, 2024

Welcome! It's refreshing to see a new contributor.

I don't think I've ever seen an imap: URI. We should probably copy Wordpress' default list of allowed URI schemes instead of continuing to create our own.

That said, ideally we should rewrite any link that doesn't match https://([\w\-]+\.)?liberapay\.(com|net|org)/ to point to an intermediate warning page, perhaps except for known-dangerous URIs which would be blocked completely, although a better solution to the XSS risk might be for the intermediate page to be on a separate domain (or in a sandboxed <iframe>).

@Pullze
Copy link
Author

Pullze commented Nov 4, 2024

Welcome! It's refreshing to see a new contributor.

I don't think I've ever seen an imap: URI. We should probably copy Wordpress' default list of allowed URI schemes instead of continuing to create our own.

That said, ideally we should rewrite any link that doesn't match https://([\w\-]+\.)?liberapay\.(com|net|org)/ to point to an intermediate warning page, perhaps except for known-dangerous URIs which would be blocked completely, although a better solution to the XSS risk might be for the intermediate page to be on a separate domain (or in a sandboxed <iframe>).

Hi! Thanks for your comments on this. Can I continue working on this issue? I could begin implementing this approach. Is the rendered markdown text the only entry point to external URLs? Or we should use a more general approach that handles all components for example?

@Changaco
Copy link
Member

Changaco commented Nov 4, 2024

Can I continue working on this issue?

Of course.

Is the rendered markdown text the only entry point to external URLs?

It is.

@Pullze
Copy link
Author

Pullze commented Nov 15, 2024

I have done a primitive solution. Any URIs not on WordPress's list will not be rendered as an <a/> tag. And external ones will be re-written as [hostname]/redirect?url=[URL].

I overrode the markdown render so the <a/> tag now comes with target="_blank".

Test markdown string:

[internal] (https://liberapay.com/about), [allowed uri 1](nntp://comp.unix.bsd.freebsd.misc), [allowed uri 2](irc://network/#channel), [HTTPS external](https://test.com/#), [HTTP external](http://testhttp.com/#)

[dangerous](javascript:something)

Autolink external:
https://google.com

Autolink internal:
https://liberapay.com/about

Rendered Result:

image

Linking to (for example) google.com will generate a URL like /redirect?url=https%3A%26%2347%3B%26%2347%3Bgoogle.com and the page will look like:
image
Clicking on "No, cancel" will jump to "/", while clicking "Yes, proceed" will jump to the destination. These button's text are borrowed from other pages, so I think only the warning message requires new translations.

I have not yet put this page in a sandbox iframe, since that may require the page to come from somewhere else (like the page is from some path like /export/redirect?url=[URL] and it would be the source of the iframe, and in the /export page we display that iframe?

@Pullze
Copy link
Author

Pullze commented Dec 9, 2024

Just added some test cases for the link rewrite. I also modified old tests to check the rewrite.

@Changaco
Copy link
Member

I overrode the markdown render so the <a/> tag now comes with target="_blank".

Why? There should be a good reason to “force” the opening of a new tab. It's typically done when the current page potentially contains volatile data that might be unexpectedly lost by clicking on the link, but that isn't true here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants