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

Add -base-path flag #762

Merged
merged 18 commits into from
Aug 19, 2024
Merged

Add -base-path flag #762

merged 18 commits into from
Aug 19, 2024

Conversation

mk12
Copy link
Contributor

@mk12 mk12 commented Aug 18, 2024

This allows self-hosting GoatCounter and reverse proxying it into a subdirectory of a larger website instead of hosting it on its own subdomain. (Fixes #707, #750)

I have implemented it as a -base-path flag provided when starting the server. This means when there are multiple sites, they must all use the same base path. An alternative would be to customize it per site, like is done for subdomains. I think this would be more flexible, but we'd have to load the site before ever sending a redirect or emitting a URL in HTML. To implement it that way, I'd need a better idea of what parts of the codebase assume the site is already loaded.

I've done some basic testing, and things seem to work. I'm planning to use this for my own website. It's up to you if you want to merge it. There are a few things that would need to be addressed before merging:

  • Fonts don't work since they're hardcoded to '/fonts/...' in the CSS files.
    • Never mind, they do work thanks to the relative path fallback. I was just seeing errors in the console and assumed they weren't working because of that.
  • It's possible the way I've threaded the Base value through the templates is not ideal/cleanest way. I just did whatever seemed easiest to make it work.
  • When using this feature, cookies might clash with the rest of your domain. An additional feature that would make this more robust would be to implement a `-cookie-prefix flag.

If you're interested in merging, I can work on those things.

This allows self-hosting GoatCounter and reverse proxying it into a
subdirectory instead of hosting it on its own subdomain.

Fixes arp242#707, arp242#750
@arp242
Copy link
Owner

arp242 commented Aug 18, 2024

When using this feature, cookies might clash with the rest of your domain. An additional feature that would make this more robust would be to implement a `-cookie-prefix flag.

Can't we set the Path for cookies? Or am I misunderstanding how that works?

With something like patch for zhttp (didn't test/commit):

diff --git i/auth/auth.go w/auth/auth.go
index 3f87516..9530fcc 100644
--- i/auth/auth.go
+++ w/auth/auth.go
@@ -34,7 +34,7 @@ func SetCookie(w http.ResponseWriter, val, domain string) {
 		Domain:   znet.RemovePort(domain),
 		Name:     cookieKey,
 		Value:    val,
-		Path:     "/",
+		Path:     BasePath,
 		Expires:  time.Now().Add(oneYear),
 		HttpOnly: true,
 		Secure:   zhttp.CookieSecure,
@@ -52,7 +52,7 @@ func ClearCookie(w http.ResponseWriter, domain string) {
 		Domain:  znet.RemovePort(domain),
 		Name:    cookieKey,
 		Value:   "",
-		Path:    "/",
+		Path:    BasePath,
 		Expires: time.Now().Add(-24 * time.Hour),
 	})
 }
diff --git i/flash.go w/flash.go
index fa772ec..c7ae14e 100644
--- i/flash.go
+++ w/flash.go
@@ -76,7 +76,9 @@ func ReadFlash(w http.ResponseWriter, r *http.Request) *FlashMessage {
 		return nil
 	}
 	http.SetCookie(w, &http.Cookie{
-		Name: cookieFlash, Value: "", Path: "/",
+		Name:    cookieFlash,
+		Value:   "",
+		Path:    BasePath,
 		Expires: time.Now().Add(-24 * time.Hour),
 	})
 	return &FlashMessage{string(c.Value[0]), string(b)}
@@ -91,7 +93,7 @@ func flash(w http.ResponseWriter, lvl, msg string, v ...any) {
 	http.SetCookie(w, &http.Cookie{
 		Name:     cookieFlash,
 		Value:    lvl + base64.StdEncoding.EncodeToString([]byte(fmt.Sprintf(msg, v...))),
-		Path:     "/",
+		Path:     BasePath,
 		Expires:  time.Now().Add(1 * time.Minute),
 		HttpOnly: true,
 		Secure:   CookieSecure,
diff --git i/wrap.go w/wrap.go
index c705670..8933cc1 100644
--- i/wrap.go
+++ w/wrap.go
@@ -17,6 +17,9 @@ import (
 	"zgo.at/ztpl"
 )
 
+// BasePath is the base path under which this application runs.
+var BasePath = "/"
+
 // UserError modifies an error for user display.
 //
 //   - Removes any stack traces from zgo.at/errors or github.com/pkg/errors.
@@ -159,7 +162,7 @@ func DefaultErrPage(w http.ResponseWriter, r *http.Request, reported error) {
 		}
 
 		if !ztpl.HasTemplate("error.gohtml") {
-			fmt.Fprintf(w, "<p>Error %d: %s</p>", code, userErr)
+			fmt.Fprintf(w, "<pre>Error %d: %s</pre>", code, userErr)
 			return
 		}
 
@@ -167,7 +170,7 @@ func DefaultErrPage(w http.ResponseWriter, r *http.Request, reported error) {
 			Code  int
 			Error error
 			Path  string
-		}{code, userErr, r.URL.Path})
+		}{code, userErr, BasePath + strings.TrimLeft(r.URL.Path, "/")})
 		if err != nil {
 			zlog.FieldsRequest(r).Error(err)
 		}
@@ -281,6 +284,9 @@ func Template(w http.ResponseWriter, name string, data any) error {
 
 // MovedPermanently redirects to the given URL with a 301.
 func MovedPermanently(w http.ResponseWriter, url string) error {
+	if strings.HasPrefix(path, "/") && BasePath != "/" {
+		url = BasePath + url
+	}
 	w.Header().Set("Location", url)
 	w.WriteHeader(301)
 	return nil
@@ -295,6 +301,9 @@ func MovedPermanently(w http.ResponseWriter, url string) error {
 // on that other resource might result in a representation that is useful to
 // recipients without implying that it represents the original target resource."
 func SeeOther(w http.ResponseWriter, url string) error {
+	if strings.HasPrefix(path, "/") && BasePath != "/" {
+		url = BasePath + url
+	}
 	w.Header().Set("Location", url)
 	w.WriteHeader(303)
 	return nil

And then on startup set that zhttp.BasePath from the CLI. This also fixes the error page, and avoids having to create a custom SeeOther() and MovedPermanently().

-base-path Base path to use in URLs sent back to the client. Default: "/".
You need this if you are reverse proxying GoatCounter into a
subdirectory of your website. This does not affect routing, since
it is assumed the reverse proxy will strip off the base path.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text can probably be a bit easier to understand:

Path under which GoatCounter is available. Usually GoatCounter runs on its own domain or subdomain ("stats.example.com"), but in some cases it's useful to run GoatCounter under a path ("example.com/stats"), in which case you'll beed to set this to "/stats".

And it should probably update the routing as well, unless that's a ton of effort? I think that would be a lot clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense. I don't think it should be too hard, I'll give it a shot. I agree it's clearer, and anyone in a position to configure their reverse proxy should be able to format the path how they like (i.e. not strip off the prefix).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in which case you'll beed to set this to "/stats".

You copied my "beed" typo 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed :)

public/help.js Outdated Show resolved Hide resolved
- Trim slash from start and end of -base-path
- Go back to === in help.js
@mk12
Copy link
Contributor Author

mk12 commented Aug 18, 2024

Can't we set the Path for cookies? Or am I misunderstanding how that works?

With something like patch for zhttp (didn't test/commit): [...]

And then on startup set that zhttp.BasePath from the CLI. This also fixes the error page, and avoids having to create a custom SeeOther() and MovedPermanently().

Oh I think I had a mistaken assumption. I don't know much about cookies. Yes that looks perfect! I can make a PR there too.

Didn't realize these were templates at first
mk12 added a commit to mk12/zhttp that referenced this pull request Aug 18, 2024
- Remove redirect.go. Instead rely on arp242/zhttp#6.
- Use -bath-path in the router so that it's actually served under
  that path rather than needing the reverse proxy to strip it.
@mk12
Copy link
Contributor Author

mk12 commented Aug 19, 2024

Ok, I've now made it use -base-path in the router, and removed redirect.go in favor of arp242/zhttp#6. I've tested manually (using a replace rule in go.mod to use my local zhttp repo) and everything seems to work.

@arp242
Copy link
Owner

arp242 commented Aug 19, 2024

I've tested manually (using a replace rule in go.mod to use my local zhttp repo) and everything seems to work.

How did you test it? If I run it with:

goatcounter serve -dev -listen :9000

I get just "404 page not found" on http://localhost:9000/

Same if I add -base-path=/ or -base-path=/foo (http://localhost:9000/foo doesn't work).

Actually I can't get it to produce anything other than that error 🤔

@mk12
Copy link
Contributor Author

mk12 commented Aug 19, 2024

Ah, it's working for me with -base-path=/foo. But it's not working with the default/empty base path. It's also not working with -base-path=/foo when there is no existing DB. Let me do some more digging.

@arp242
Copy link
Owner

arp242 commented Aug 19, 2024

With the below patch the defaults work, and -base-path=/foo kind-of works, but doesn't render any charts.

diff --git i/cmd/goatcounter/serve.go w/cmd/goatcounter/serve.go
index 12280b2b..4207023d 100644
--- i/cmd/goatcounter/serve.go
+++ w/cmd/goatcounter/serve.go
@@ -183,7 +183,10 @@ func cmdServe(f zli.Flags, ready chan<- struct{}, stop chan struct{}) error {
 			flagTLS = map[bool]string{true: "http", false: "acme,rdr"}[dev]
 		}
 
-		basePath = "/" + strings.Trim(basePath, "/")
+		basePath = strings.Trim(basePath, "/")
+		if basePath != "" {
+			basePath = "/" + basePath
+		}
 		zhttp.BasePath = basePath
 
 		var domainCount, urlStatic string
diff --git i/go.mod w/go.mod
index 79edde75..359df911 100644
--- i/go.mod
+++ w/go.mod
@@ -2,6 +2,9 @@ module zgo.at/goatcounter/v2
 
 go 1.23.0
 
+
+replace zgo.at/zhttp => /home/martin/code/Golib/zhttp
+
 require (
 	code.soquee.net/otp v0.0.4
 	github.com/BurntSushi/toml v1.4.0
diff --git i/handlers/backend.go w/handlers/backend.go
index dd66616d..0b986787 100644
--- i/handlers/backend.go
+++ w/handlers/backend.go
@@ -19,12 +19,16 @@ import (
 	"zgo.at/zstd/zfs"
 )
 
-func NewBackend(db zdb.DB, acmeh http.HandlerFunc, dev, goatcounterCom, websocket bool, domainStatic string, basePath string, dashTimeout, apiMax int) chi.Router {
+func NewBackend(
+	db zdb.DB, acmeh http.HandlerFunc, dev, goatcounterCom, websocket bool,
+	domainStatic, basePath string, dashTimeout, apiMax int,
+) chi.Router {
+
 	root := chi.NewRouter()
 	r := root
 	if basePath != "" {
 		r = chi.NewRouter()
-		root.Mount(basePath+"/", r)
+		root.Mount(basePath, r)
 	}
 
 	backend{dashTimeout, websocket}.Mount(r, db, dev, domainStatic, dashTimeout, apiMax)

mk12 and others added 2 commits August 18, 2024 17:52
Make sure it's "" for the root, not "/".
While the "./fonts/..." fallback ensures the correct font is loaded, it
does give an annoying error in the console which I want to get rid of.
@mk12
Copy link
Contributor Author

mk12 commented Aug 19, 2024

Ok yeah I'm also not seeing the charts. Odd since it is getting the JS files fine and there's no console errors.

@arp242
Copy link
Owner

arp242 commented Aug 19, 2024

Added a commit to make it work. {{.Path}} was with the baseURL, and we match some JS function names to that with:

<body id="page-{{.Path | path_id}}">

and:

    ;[page_dashboard, page_settings_main, page_user_pref, page_user_dashboard, page_bosmang]
        .forEach((f) => document.body.id.match(new RegExp('^' + f.name.replace(/_/g, '-'))) && f.call())

Also used for some other things like displaying the active tab. This should just be the regular path without baseURL prepended.

Previously only http://localhost:9000/foo/ worked, and
http://localhost:9000/foo errored.
@mk12
Copy link
Contributor Author

mk12 commented Aug 19, 2024

Thanks! Confirmed I see the charts now too. Also, not sure when this runs but I added a commit to add BASE_PATH here:

$('#secret-url').val(`${location.protocol}//${location.host}${BASE_PATH}?access-token=${this.value}`)

@arp242 arp242 merged commit f413299 into arp242:master Aug 19, 2024
6 checks passed
@arp242
Copy link
Owner

arp242 commented Aug 19, 2024

Okay, I did a bunch of testing, and I couldn't find any further problems, so hopefully everything should be good now.

Thanks for your PR!

@mk12 mk12 deleted the base-path branch August 19, 2024 02:55
@mk12
Copy link
Contributor Author

mk12 commented Aug 19, 2024

Heh I missed quite a few things, thanks for taking care of that. Excited to start using this on my website :)

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

Successfully merging this pull request may close these issues.

Enable use of subpath rather than domains for sites
2 participants