Using / in Wiki Title break the wiki #1220

@haskaalo commented on GitHub (Nov 11, 2017):

Step to reproduce bug:

Create a page in a wiki on a repo (via browser)
Insert a slash inside title (I named mine GET: /a/page)
Create page.
Going to the page return raw page (in text)

@haskaalo commented on GitHub (Nov 11, 2017): Step to reproduce bug: 1. Create a page in a wiki on a repo (via browser) 2. Insert a slash inside title (I named mine `GET: /a/page`) 3. Create page. 4. Going to the page return raw page (in text)

GiteaMirror commented

@efy commented on GitHub (Nov 13, 2017):

Had a quick look at this and it seems the route is matched on an unescaped path (A feature of the framework?).

So a request to /wiki/a%2Fb becomes /wiki/a/b which ends up matched by the wild card route that renders repo.WikiRaw.

@efy commented on GitHub (Nov 13, 2017): Had a quick look at this and it seems the route is matched on an unescaped path (A feature of the framework?). So a request to `/wiki/a%2Fb` becomes `/wiki/a/b` which ends up matched by the wild card route that renders `repo.WikiRaw`.

GiteaMirror commented

@svarlamov commented on GitHub (Nov 22, 2017):

This is also happening for paths with parenthesis in them, ie., Hello World (note) Is there a place where I can go in to help fix this?

@svarlamov commented on GitHub (Nov 22, 2017): This is also happening for paths with parenthesis in them, ie., `Hello World (note)` Is there a place where I can go in to help fix this?

GiteaMirror commented

@lunny commented on GitHub (Nov 22, 2017):

It should be related with routes.

@lunny commented on GitHub (Nov 22, 2017): It should be related with routes.

GiteaMirror commented

@svarlamov commented on GitHub (Nov 22, 2017):

@lunny I briefly took a look at their docs: https://go-macaron.com/docs/middlewares/routing

Should we use a regex instead? Not sure why that URL-encoded string is not being handled the way that we want it (this is just from a few mins of looking at the code and haven't worked with macaron before, but...)

@svarlamov commented on GitHub (Nov 22, 2017): @lunny I briefly took a look at their docs: https://go-macaron.com/docs/middlewares/routing Should we use a regex instead? Not sure why that URL-encoded string is not being handled the way that we want it (this is just from a few mins of looking at the code and haven't worked with macaron before, but...)

GiteaMirror commented

@svarlamov commented on GitHub (Nov 22, 2017):

This is the correct router group for the wiki right? https://github.com/go-gitea/gitea/blob/master/routers/routes/routes.go#L596

@svarlamov commented on GitHub (Nov 22, 2017): This is the correct router group for the wiki right? https://github.com/go-gitea/gitea/blob/master/routers/routes/routes.go#L596

GiteaMirror commented

@svarlamov commented on GitHub (Nov 22, 2017):

I'm also wondering why it is that parentheses are not being handled... It feels like I'm missing something pretty fundamental about how Macaron parses routes...

@svarlamov commented on GitHub (Nov 22, 2017): I'm also wondering why it is that parentheses are not being handled... It feels like I'm missing something pretty fundamental about how Macaron parses routes...

GiteaMirror commented

@svarlamov commented on GitHub (Nov 22, 2017):

Okay, so not saying I've got anything conclusive here, but I decided to test out macaron independently by running with the following little go file:

package main

import (
	"gopkg.in/macaron.v1"
)

func main() {
	m := macaron.Classic()
	m.Get("/foobar/:filename/hello",
		func(ctx *macaron.Context) string {
			return "You got it!"
		},
	)
	m.Run()
}

After starting that server (running on 0.0.0.0:4000 on my laptop), I tried out the following calls:

http://localhost:4000/foobar/file%20name/hello -> 200
http://localhost:4000/foobar/plain/hello -> 200
http://localhost:4000/foobar/url%2Fencoded%2Ftext%2Fwith%2Fslashes/hello -> 404

So it's looking like there's an issue with Macaron here...

@lunny What are your thoughts here? I spent a few mins looking through the macaron code (looks pretty simple), but I noticed you're not a contributor there...

From my work that I've outlined here so far, it seems that we have two options:

Contribute a fix and unit tests to Macaron and wait for that to get merged (who knows how long that will take?)
Refactor the routes such that we serve the page rather than the raw as a default (temporary band-aid while we await number 1 fix)

And, it's probably worth a deeper look if a bug like this has implications elsewhere in Gitea... I'm no expert on the Gitea codebase, but this sort of a routing issue could definitely have pretty far-reaching implications...

@svarlamov commented on GitHub (Nov 22, 2017): Okay, so not saying I've got anything conclusive here, but I decided to test out macaron independently by running with the following little go file: ```go package main import ( "gopkg.in/macaron.v1" ) func main() { m := macaron.Classic() m.Get("/foobar/:filename/hello", func(ctx *macaron.Context) string { return "You got it!" }, ) m.Run() } ``` After starting that server (running on 0.0.0.0:4000 on my laptop), I tried out the following calls: 1. `http://localhost:4000/foobar/file%20name/hello` -> 200 2. `http://localhost:4000/foobar/plain/hello` -> 200 3. `http://localhost:4000/foobar/url%2Fencoded%2Ftext%2Fwith%2Fslashes/hello` -> 404 So it's looking like there's an issue with Macaron here... @lunny What are your thoughts here? I spent a few mins looking through the macaron code (looks pretty simple), but I noticed you're not a contributor there... From my work that I've outlined here so far, it seems that we have two options: 1. Contribute a fix and unit tests to Macaron and wait for that to get merged (who knows how long that will take?) 2. Refactor the routes such that we serve the page rather than the raw as a default (temporary band-aid while we await number 1 fix) And, it's probably worth a deeper look if a bug like this has implications elsewhere in Gitea... I'm no expert on the Gitea codebase, but this sort of a routing issue could definitely have pretty far-reaching implications...

GiteaMirror commented

@lunny commented on GitHub (Nov 22, 2017):

@svarlamov Thanks for your investigate, that's what I guess. Maybe you can send an issue or PR on Macaron and @Unknwon is the owner of Macaron.

@lunny commented on GitHub (Nov 22, 2017): @svarlamov Thanks for your investigate, that's what I guess. Maybe you can send an issue or PR on Macaron and @Unknwon is the owner of Macaron.

GiteaMirror commented

@svarlamov commented on GitHub (Nov 22, 2017):

@lunny Sounds good. I will have a look there and maybe I will be able to create a PR instead of just an issue! 😛

Is the plan to stick with Macaron as the framework? It seems like it's a holdover from the gogs project... I'm a bit nervous to be running that in production -- would be cleaner to have gorilla mux or something like that... I assume you all have considered that?

In the meantime, are you okay to set the default route the Wiki HTML template though? I'm keen to contribute and get involved with this project :)

@svarlamov commented on GitHub (Nov 22, 2017): @lunny Sounds good. I will have a look there and maybe I will be able to create a PR instead of just an issue! 😛 Is the plan to stick with Macaron as the framework? It seems like it's a holdover from the gogs project... I'm a bit nervous to be running that in production -- would be cleaner to have gorilla mux or something like that... I assume you all have considered that? In the meantime, are you okay to set the default route the Wiki HTML template though? I'm keen to contribute and get involved with this project :)

GiteaMirror commented

@lunny commented on GitHub (Nov 22, 2017):

@svarlamov some maintainers suggested Gin instead of Macaron, but it seems that needs many work and maybe cause Gitea unstable. You are welcome to contribute to Gitea, please read CONTRIBUTING at first. And you can discuss about web framework or any plan on #develop channel on our discord server.

@lunny commented on GitHub (Nov 22, 2017): @svarlamov some maintainers suggested Gin instead of Macaron, but it seems that needs many work and maybe cause Gitea unstable. You are welcome to contribute to Gitea, please read [CONTRIBUTING](https://github.com/go-gitea/gitea/blob/master/CONTRIBUTING.md) at first. And you can discuss about web framework or any plan on #develop channel on our discord server.

GiteaMirror commented

@lafriks commented on GitHub (Nov 22, 2017):

I like go-chi router as it uses GoLang context features

@lafriks commented on GitHub (Nov 22, 2017): I like go-chi router as it uses GoLang context features

GiteaMirror commented

@ethantkoenig commented on GitHub (Nov 23, 2017):

@svarlamov I've created a PR to fix the problem in macaron: https://github.com/go-macaron/macaron/pull/149

@ethantkoenig commented on GitHub (Nov 23, 2017): @svarlamov I've created a PR to fix the problem in macaron: https://github.com/go-macaron/macaron/pull/149

GiteaMirror commented