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

Users posting links with destination "URL" instead of intended one #809

Closed
ninedraft opened this issue Nov 5, 2020 · 13 comments
Closed
Assignees
Milestone

Comments

@ninedraft
Copy link

Not sure if bug or UI misuse, but sometimes links are replaced with $REMARK_DOMAIN/web/url URL.

For example checkout this page: https://radio-t.com/p/2020/11/03/prep-727/#comments

Examples of generated HTML:

  • <a href="url" rel="nofollow">https://winxp.now.sh/</a>
  • <a href="url" rel="nofollow">https://samy.pl/slipstream/</a>

If somebody can reproduce, please answer - I can spent some time to fix this bug (if presented).

@umputun
Copy link
Owner

umputun commented Nov 5, 2020

I can confirm - this is not an imaginary issue but the real one. I have seen it a few times on the demo site, but can't reproduce it.

this is what I have tried

- http://google.com
- https://google.com
- [link](http://google.com)
- <a href="https://google.com" blah="xyz">link</a>

All the above make the expected URL. The only thing remotely similar to the reported issue is an attempt to publish a relative link, for example:

  • [link](google.com) -> https://demo.remark42.com/web/google.com
  • <a href="google.com" blah="xyz">link</a> -> https://demo.remark42.com/web/google.com
  • [link](/google.com) -> https://demo.remark42.com/google.com

None of those really surprising, and such links make no sense in general.

@umputun
Copy link
Owner

umputun commented Nov 5, 2020

ok, from what I see the incorrect one was submitted as this {"text":"[https://samy.pl/slipstream/](url)\n\n**NAT** **Slipstreaming ** - новый метод обхода NAT и файервола. \n\nПозволяет получить удаленный доступ к любой службе TCP/UDP путем посещения жертвой веб-сайта. "}

I think this is the user's error. Highly unlikely something like this may happen if the user typing URL in MD manually, so I guess the user clicked on the button (toolbar), which populates [](url) by default and typed the link to the wrong place (inside []). If this is the case, it is probably a usability issue but not a bug. We can address it by prepopulating both parts by default, I.e., something like [title](URL) or, even better - on a click, open a dialog box with a pair of text inputs and asking for both fields.

cc @akellbl4

note: just in case if a reader wonders how I got access to the log - this is not because remark42 reports to the mothership, but because I'm the admin for that particular site (radio-t.com)

@ninedraft
Copy link
Author

ninedraft commented Nov 6, 2020

Maybe a linter-like functionality for markdown renderer can be a good idea.

It can detect suspicious link usage, such as:

  • [](INVALID_URL)
  • [](RELATIVE_URL)
  • [](DEFAULT_URL_VALUE)

@akellbl4
Copy link
Collaborator

akellbl4 commented Nov 8, 2020

@umputun I've checked if we can put the cursor to another place. It could be done only if we fork lib that we use for MD buttons. Because they don't want to expose base class for these buttons which implement snippet inserting and put caret position.
If we would go with linter-like validation I think better to have it on server-side.

P.S. We use the same buttons as GitHub has in the comments and they have the same behavior.

@paskal paskal changed the title Broken link embedding Users posting links with destination "URL" instead of intended one Jan 8, 2023
@paskal
Copy link
Collaborator

paskal commented Jan 8, 2023

I think matching ](url) and showing a validation error should be done better on the frontend.

@akellbl4
Copy link
Collaborator

akellbl4 commented Jan 9, 2023

@paskal we don't do any parsing for markdown on frontend, so it could involve adding those packages and I don't think it a good way to check links. The ideal situation when user is trying to post comment it's validated on BE and if error is there it returns the error as we do with all of the possible problems in comment text.

@umputun
Copy link
Owner

umputun commented Jan 9, 2023

Do we really need a parser for this? The particular case is about a substring match, i.e., if the text area has ](url) in we my want to reject submit

@akellbl4
Copy link
Collaborator

akellbl4 commented Jan 9, 2023

I mean we do validate comment text on server so I wouldn't spread those validations between FE and BE. Especially in order to keep FE simple BE is always the better place to do those things.

Maybe we even should add http/https automatically.

@akellbl4
Copy link
Collaborator

akellbl4 commented Jan 9, 2023

Btw this is how it's handled here
google.com

@umputun
Copy link
Owner

umputun commented Jan 9, 2023

Well, we can validate on the server side for sure; this is easy to do. However, it may return you a very basic "rejection", like a bad request or expectation failed status. In the json body, we can add extra things like "invalid URL" or something like this; however, I don't think this will help you highlight the text's troublesome part. If you do it on your side, I guess it is much easier to indicate what part of the text is invalid.

Generally, I have nothing against server-side verification here; the only issue I'm trying to address here is the better user experience.

@paskal paskal self-assigned this Jan 9, 2023
@akellbl4
Copy link
Collaborator

akellbl4 commented Jan 9, 2023

I don't think this will help you highlight the text's troublesome part

Hm, I didn't plan to highlight specific text in the text area. To be honest I don't know any way of doing that. I can imagine how we can do that but adding such highlighting will be way over engineered for such problem.

@paskal
Copy link
Collaborator

paskal commented Jan 9, 2023

@akellbl4 I've prepared the backend part of the validation. Could you please alter the frontend will work properly with it?

@paskal
Copy link
Collaborator

paskal commented Sep 16, 2023

I've created the issue #1673 about what is left to be done about this issue. I am resolving this one in favour of 1673.

@paskal paskal closed this as completed Sep 16, 2023
@paskal paskal added this to the v1.12.0 milestone Sep 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants