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

gitea panics on a markdown syntax error (nested links) #4946

Closed
2 of 7 tasks
earwig opened this issue Sep 16, 2018 · 6 comments
Closed
2 of 7 tasks

gitea panics on a markdown syntax error (nested links) #4946

earwig opened this issue Sep 16, 2018 · 6 comments
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug

Comments

@earwig
Copy link

earwig commented Sep 16, 2018

Description

The syntax is this: [\[\[foobar\]\]](https://example.org)

Expected result: [[foobar]]

I noticed it when trying to mirror one of my repos.

@jonasfranz jonasfranz added this to the 1.6.0 milestone Sep 17, 2018
@SagePtr
Copy link
Contributor

SagePtr commented Sep 19, 2018

I don't get why "visitLinksForShortLinks" logic in modules/markup/html.go is used, where it's used at all. Why nested links should be processed, any working example where they are needed. I cannot remember any example where <a> tag is included into another <a> tag in production, and how the broswers should handle this mess.
Probably over-engeneering?

@earwig
Copy link
Author

earwig commented Sep 19, 2018 via email

@jonasfranz jonasfranz modified the milestones: 1.6.0, 1.6.1 Oct 9, 2018
@techknowlogick techknowlogick modified the milestones: 1.6.1, 1.6.2 Dec 9, 2018
@techknowlogick techknowlogick modified the milestones: 1.6.2, 1.6.3 Dec 19, 2018
@lafriks lafriks removed this from the 1.6.3 milestone Jan 4, 2019
@stale
Copy link

stale bot commented Mar 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.

@stale stale bot added the issue/stale label Mar 6, 2019
@earwig
Copy link
Author

earwig commented Mar 6, 2019

This bug still causes an uncaught internal server error in production, as you can see from the reproduction I linked in the original post. This issue should not be closed.

@stale stale bot removed the issue/stale label Mar 6, 2019
@zeripath zeripath added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Mar 6, 2019
@mrsdizzie
Copy link
Member

mrsdizzie commented Mar 6, 2019

I was looking at all of the link generating code recently and can see whats causing this here. This happens because of a few things:

First, the markdown processor does correctly render this and outputs

<p><a href="https://example.org">[[foobar]]</a></p>

However, then gitea runs its own processors and this happens to match the following regex:

https://github.com/go-gitea/gitea/blob/master/modules/markup/html.go#L53

It does that because it sees anything inside double brackets as a possible MediaWiki/Github style short link like

[[link|example]]
[[example|link]]

Relevant comments/function here:

https://github.com/go-gitea/gitea/blob/master/modules/markup/html.go#L389

That function seems to assume too much about the text match, and fails when it isn't actually what was expected. This also does other bad things like turn this bogus text into a link:

[[]]example

See: https://try.gitea.io/mrsdizzie/test/issues/2

A quick test of changing shortLinkPattern to

shortLinkPattern = regexp.MustCompile(`\[\[(.+?\|.+?)\]\](\w*)`)

Makes the offending example (and my test) in this issue work as expected, but is complicated because this code is also used for the (imo) separate case of matching basic [[Test]] style wiki links as well.

Setting visitLinksForShortLinks: false doesn't seem to affect any of the current tests, and is perhaps an option for fixing this particular issue as well. As mentioned above, I can't imagine the case where it makes sense to try and process an existing link and render another link inside of it (and apparently nobody who wrote the current tests imagined that either).

mrsdizzie added a commit to mrsdizzie/gitea that referenced this issue Mar 6, 2019
The visitLinksForShortLinks feature would look inside of an <a> tag and
run shortLinkProcessorFull on any text, which attempts to create links
out of potential 'short links' like [[test]] [[link|example]] etc...
This makes no sense because you can't have nested links within an <a>
tag. Specifically, the html5 standard says <a> tags can't include
interactive content if they contain the href attribute:

 http://w3c.github.io/html/single-page.html#the-a-element

And also defines an <a> element with a href attribute as interactive:

 http://w3c.github.io/html/single-page.html#interactive-content

Therefore you can't really put a link inside of another link. In
practice none of this works anyways since browsers won't render it, it
would probably be broken if they tried, and it is causing a bug
(go-gitea#4946). No current tests rely on this behavior either.

This removes the feature and also explicitly excludes the
current visitNodeForShortLinks from looking in <a> tags.
@techknowlogick
Copy link
Member

Closed per merged PR.

techknowlogick pushed a commit that referenced this issue Mar 7, 2019
The visitLinksForShortLinks feature would look inside of an <a> tag and
run shortLinkProcessorFull on any text, which attempts to create links
out of potential 'short links' like [[test]] [[link|example]] etc...
This makes no sense because you can't have nested links within an <a>
tag. Specifically, the html5 standard says <a> tags can't include
interactive content if they contain the href attribute:

 http://w3c.github.io/html/single-page.html#the-a-element

And also defines an <a> element with a href attribute as interactive:

 http://w3c.github.io/html/single-page.html#interactive-content

Therefore you can't really put a link inside of another link. In
practice none of this works anyways since browsers won't render it, it
would probably be broken if they tried, and it is causing a bug
(#4946). No current tests rely on this behavior either.

This removes the feature and also explicitly excludes the
current visitNodeForShortLinks from looking in <a> tags.
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug
Projects
None yet
Development

No branches or pull requests

7 participants