gitblit-org / gitblit

pure java git solution
http://gitblit.com
Apache License 2.0
2.28k stars 670 forks source link

Fixes what was broken due to #1358 issue fix. #1392

Closed TomaszSzt closed 2 years ago

TomaszSzt commented 2 years ago

This commit fixes what was broken in commit https://github.com/gitblit/gitblit/commit/b23269acc0f460f583311c679d751925b8402563 due to #1358 issue

No automatic test were included, no regression tests were run.

Manual test scenario

A. Bug presentation

  1. Pull b23269acc0f460f583311c679d751925b8402563, compile and start GO server.
  2. Create repository with readme.md file and x.md file.
  3. In readme.md place link to x.md using relative syntax (x)[x.md] and (xx)[http://google.com] absolute link.
  4. Push repository to server.
  5. Get to repository page, see docs and observe links. The relative link is processed correctly but absolute link is escaped with % syntax and appended to repository path effectively breaking it.

Expected result: 1.Relative link works as in test 2.Absolute link works correctly pointing to specified site.

B. Fix presentation

  1. Pull this branch, compile and start GO server
  2. Do the test again.
  3. Observe correct results.
flaix commented 2 years ago

You are right, this was an oversight. Thanks for finding it and taking the time to fix it! Can I ask why you opted for the URL check instead of using the check that the two methods above use for the same case, i.e. for ExpImageNode and for RefImageNode?

TomaszSzt commented 2 years ago

@flaix "(...)URL check instead of using the check th(...)" First and most important: because I don't understand code which has no comments in it. I don't understand neither what are those methods used for nor what is carried inside parameters, what should they return and what are boundaries of parameters and how they are used between threads. The generic lack of comments in all the code I have found in this project, including libraries used, is really bothering me. Serviceability of code is critical for quality.

Second, the indexof ""://"" check is wrong. URL protocol is separated by ":" not "://", as the authority is optional. Since I have no ways to know from code without comments if ExpLinkNode used any form of checks on url it represents I have to assume that it can be any string, like "bongo\bongo?://string". Which is a correct, I think, relative URL with a strange query. URL is a really badly defined and full of surprising things.

Lets take for an example "file:" schema: the double slash ("file://") is incorrect according to https://datatracker.ietf.org/doc/html/rfc8089, but correct according to RFC1738 while single slash ("file:/") is correct in RFC8089 but incorrect in RFC1738. And guess what? Git is using one standard, Java an another. Great, isn't it?

But back to code.

The only assumption I could make looking at how it ExpLinkNode is used is that node.url is not null. Although it nowhere specified, so I wasn't sure.

Taking all of this in account I decided to let the standard to work for me. I also used "return super.render(..)" instead of " new Rendering(...)" because I don't know what should I return while I know that before this fix it worked correctly.

There is however a hack in net.URL which will make this fix to fail, and I am aware of it, but forgot to comment. The "new java.net.URL("xxxx:\")" will fail with throwing an exception about unsupported schema. So if one will for an example place an absolute reference to, let's say, linux dav file system, like "dav://" then this will barf and will treat it as a relative link.

Now I think it was my wrong, and maybe I should use a more sophisticated check which will allow any schema, like finding first occurrence of ':' and checking if what is inside is like: rfc1738:(...) Scheme names consist of a sequence of characters. The lower case letters "a"--"z", digits, and the characters plus ("+"), period ("."), and hyphen ("-") are allowed. (...) upper and lower case allowed.

Feel free to change it to Your liking, You know the code better. I just needed it to be fixed before next day because it broke all my repositories which cross-linked in "readme.md" and users simply could not just "click and see the manual".

flaix commented 2 years ago

Hi, thank you for sharing your thoughts. Since you don't mind, I'll just adjust the white-space style to the rest of the file and merge it.

TomaszSzt commented 2 years ago

Thanks.