From 32259f9d20bc929d4b34778ef8383d2bb791c784 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:38:31 +0200 Subject: [PATCH 01/36] fix(security): prevent path traversal in script execution Validate the wildcard request path resolves within ApiDir before executing, rejecting escapes like /api/../../../tmp/evil that could run arbitrary .sh files anywhere on the filesystem. --- handlers/exec.go | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/handlers/exec.go b/handlers/exec.go index 4a109a8..5b84b99 100644 --- a/handlers/exec.go +++ b/handlers/exec.go @@ -7,6 +7,8 @@ import ( "log" "os" "os/exec" + "path/filepath" + "strings" "github.com/gin-gonic/gin" ) @@ -15,20 +17,43 @@ type ExecHandler struct { ApiDir *string } +// scriptPath resolves the wildcard request path to an absolute `.sh` path and +// guarantees it stays within ApiDir, preventing path-traversal escapes such as +// `/api/../../../tmp/evil`. It returns false when the path escapes ApiDir. +func (h *ExecHandler) scriptPath(reqPath string) (string, bool) { + base, err := filepath.Abs(*h.ApiDir) + if err != nil { + return "", false + } + // filepath.Join cleans the result, collapsing any `..` segments. + script := filepath.Join(base, reqPath+".sh") + if script != base && !strings.HasPrefix(script, base+string(os.PathSeparator)) { + return "", false + } + return script, true +} + func (h *ExecHandler) ExecScript(c *gin.Context) { var stdout []byte var err error - // Build script name + // Build and validate script name path := c.Param("path") - script := fmt.Sprintf("%s%s%s", *h.ApiDir, path, ".sh") + script, ok := h.scriptPath(path) + if !ok { + c.JSON(404, gin.H{ + "error": "Resource not found", + }) + fmt.Printf("[error] invalid resource path: %s\n", path) + return + } // Check script exists if _, err := os.Stat(script); os.IsNotExist(err) { c.JSON(404, gin.H{ "error": "Resource not found", }) - fmt.Printf("[error] resource not found: %s", script) + fmt.Printf("[error] resource not found: %s\n", script) return } @@ -70,16 +95,23 @@ func (h *ExecHandler) PostExecScript(c *gin.Context) { var stdout []byte var err error - // Build script name + // Build and validate script name path := c.Param("path") - script := fmt.Sprintf("%s%s%s", *h.ApiDir, path, ".sh") + script, ok := h.scriptPath(path) + if !ok { + c.JSON(404, gin.H{ + "error": "Resource not found", + }) + fmt.Printf("[error] invalid resource path: %s\n", path) + return + } // Check script exists if _, err := os.Stat(script); os.IsNotExist(err) { c.JSON(404, gin.H{ "error": "Resource not found", }) - fmt.Printf("[error] resource not found: %s", script) + fmt.Printf("[error] resource not found: %s\n", script) return } From 5d4ad6f02a09d2f3b3219e29d91d6b1cb8a490de Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:39:35 +0200 Subject: [PATCH 02/36] fix(security): gate /docker behind -enableDocker flag The /docker endpoint runs arbitrary 'docker run' commands, which is effectively root on the host. Make it opt-in (off by default) and replace strings.Split with a quote-aware tokenizer so quoted args with spaces are preserved. --- handlers/docker.go | 40 +++++++++++++++++++++++++++++++++++++++- main.go | 9 +++++---- router.go | 6 +++++- 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/handlers/docker.go b/handlers/docker.go index 11cd584..fe02948 100644 --- a/handlers/docker.go +++ b/handlers/docker.go @@ -28,7 +28,7 @@ func DockerRun(c *gin.Context) { } // Exec docker run - args := append([]string{"run"}, strings.Split(form.Cmd, " ")...) + args := append([]string{"run"}, splitArgs(form.Cmd)...) output, err := exec.Command("docker", args...).CombinedOutput() if err != nil { message := err.Error() + ": " + strings.Replace(string(output), "\n", " ", -1) @@ -46,3 +46,41 @@ func DockerRun(c *gin.Context) { c.String(200, string(output)) } + +// splitArgs splits a command string into arguments, honoring single and double +// quotes so that quoted arguments containing spaces stay intact and consecutive +// spaces don't produce empty arguments. +func splitArgs(s string) []string { + args := make([]string, 0) + var cur strings.Builder + var quote rune + inWord := false + + for _, r := range s { + switch { + case quote != 0: + if r == quote { + quote = 0 + } else { + cur.WriteRune(r) + } + inWord = true + case r == '\'' || r == '"': + quote = r + inWord = true + case r == ' ' || r == '\t': + if inWord { + args = append(args, cur.String()) + cur.Reset() + inWord = false + } + default: + cur.WriteRune(r) + inWord = true + } + } + if inWord { + args = append(args, cur.String()) + } + return args +} diff --git a/main.go b/main.go index 37e8c04..3b6315b 100644 --- a/main.go +++ b/main.go @@ -15,10 +15,11 @@ var ( gitCommit = "undefined" buildDate = "undefined" - port = flag.Int("port", 4242, "HTTP port to listen") - password = flag.String("password", "", "Admin password for basic auth") - apiKey = flag.String("apiKey", "42", "API key for header auth") - apiDir = flag.String("apiDir", "./api", "API directory (sh scripts and html pages)") + port = flag.Int("port", 4242, "HTTP port to listen") + password = flag.String("password", "", "Admin password for basic auth") + apiKey = flag.String("apiKey", "42", "API key for header auth") + apiDir = flag.String("apiDir", "./api", "API directory (sh scripts and html pages)") + enableDocker = flag.Bool("enableDocker", false, "Enable the /docker endpoint (grants full host access via docker run)") ) func ConfigRuntime() { diff --git a/router.go b/router.go index 8969b86..e6cd06c 100644 --- a/router.go +++ b/router.go @@ -47,7 +47,11 @@ func Router() *gin.Engine { authorized.GET("/api/*path", execHandler.ExecScript) authorized.POST("/api/*path", execHandler.PostExecScript) - authorized.POST("/docker", h.DockerRun) + // Arbitrary `docker run` execution — off by default as it grants full + // host access. Enable explicitly with -enableDocker. + if *enableDocker { + authorized.POST("/docker", h.DockerRun) + } // Static files authorized.Static("/s/", *apiDir+"/_static") From cd3a9493b1376dc5420e8a85f0c32d201ad19be9 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:41:56 +0200 Subject: [PATCH 03/36] fix(security): drop invalid wildcard+credentials CORS combo Access-Control-Allow-Origin: * together with Allow-Credentials: true is rejected by browsers. Remove the credentials header and add X-Auth to the allowed request headers so header auth works cross-origin. --- middlewares/cors.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/middlewares/cors.go b/middlewares/cors.go index 95abe34..ef34997 100644 --- a/middlewares/cors.go +++ b/middlewares/cors.go @@ -4,13 +4,15 @@ import "github.com/gin-gonic/gin" func CORSMiddleware() gin.HandlerFunc { return func(c *gin.Context) { + // A wildcard origin must not be combined with credentials: browsers + // reject that pairing. This is an open API authenticated via the + // X-Auth header (allowed below), so credentials are not advertised. domain := "*" c.Writer.Header().Set("Access-Control-Allow-Origin", domain) c.Writer.Header().Set("Access-Control-Max-Age", "86400") c.Writer.Header().Set("Access-Control-Allow-Methods", "POST, GET, OPTIONS, PUT, DELETE, UPDATE") - c.Writer.Header().Set("Access-Control-Allow-Headers", "Origin, Content-Type, Content-Length, Accept-Encoding, X-CSRF-Token, Authorization") + c.Writer.Header().Set("Access-Control-Allow-Headers", "Origin, Content-Type, Content-Length, Accept-Encoding, X-CSRF-Token, X-Auth, Authorization") c.Writer.Header().Set("Access-Control-Expose-Headers", "Content-Length") - c.Writer.Header().Set("Access-Control-Allow-Credentials", "true") if c.Request.Method == "OPTIONS" { c.AbortWithStatus(200) From d8e4b379aba885ceca229f087bff06a0cce2542c Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:42:26 +0200 Subject: [PATCH 04/36] fix(security): compare API key in constant time Use crypto/subtle.ConstantTimeCompare for the X-Auth header check to avoid timing side channels. --- middlewares/auth.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/middlewares/auth.go b/middlewares/auth.go index 4611da2..7affdf5 100644 --- a/middlewares/auth.go +++ b/middlewares/auth.go @@ -1,6 +1,10 @@ package middlewares -import "github.com/gin-gonic/gin" +import ( + "crypto/subtle" + + "github.com/gin-gonic/gin" +) var AuthHeaderKey = "X-Auth" @@ -8,12 +12,13 @@ func AuthMiddleware(apiKey string, accounts gin.Accounts) gin.HandlerFunc { basicAuth := gin.BasicAuthForRealm(accounts, "") return func(c *gin.Context) { - // Try header auth - if c.Request.Header.Get(AuthHeaderKey) == apiKey { + // Try header auth, comparing in constant time to avoid leaking the + // key through response-timing differences. + got := c.Request.Header.Get(AuthHeaderKey) + if subtle.ConstantTimeCompare([]byte(got), []byte(apiKey)) == 1 { return - } else { - // Try basic auth - basicAuth(c) } + // Fall back to basic auth + basicAuth(c) } } From ca898bf36113292d5e869552a150025c05000013 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:43:09 +0200 Subject: [PATCH 05/36] fix(security): warn when running without authentication When -password is unset the server is fully open; emit a loud startup warning so this isn't silently the default. --- router.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/router.go b/router.go index e6cd06c..7600c2b 100644 --- a/router.go +++ b/router.go @@ -1,6 +1,7 @@ package main import ( + "log" "net/http" "os" @@ -30,6 +31,8 @@ func Router() *gin.Engine { basicAuthUser: *password, }, )) + } else { + log.Println("[warn] no -password set: authentication is DISABLED and all endpoints are publicly accessible") } // Version (commit and date) From c064f08b7f0f906ba492ad19495e5f8f7db1fef5 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:44:27 +0200 Subject: [PATCH 06/36] docs(example): build JSON safely with jq in example scripts The canonical examples interpolated values straight into the JSON output; $1 in param.sh could break the JSON (or worse in a script that evaluates it). Use 'jq -n --arg/--argjson' so values are always correctly typed and escaped. --- example/api/test/param.sh | 6 +++--- example/api/time/date.sh | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/example/api/test/param.sh b/example/api/test/param.sh index 94fbc40..6ea02d3 100755 --- a/example/api/test/param.sh +++ b/example/api/test/param.sh @@ -1,6 +1,6 @@ #!/bin/sh set -eu -echo '{ - "param": "'$1'" -}' +# Build JSON with jq so the parameter is always safely quoted/escaped, +# rather than interpolating it straight into the output. +jq -n --arg param "${1:-}" '{param: $param}' diff --git a/example/api/time/date.sh b/example/api/time/date.sh index 8c55ba0..921bab5 100755 --- a/example/api/time/date.sh +++ b/example/api/time/date.sh @@ -1,8 +1,8 @@ #!/bin/sh set -eu -echo '{ - "date": '$(date +%s)', - "human_date": "'$(date)'" -}' - +# Build JSON with jq so values are always correctly typed and escaped. +jq -n \ + --argjson date "$(date +%s)" \ + --arg human_date "$(date)" \ + '{date: $date, human_date: $human_date}' From 56245fc5776cfb1a625d4a26909f82d5009051a1 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:44:50 +0200 Subject: [PATCH 07/36] fix: handle script execution errors in POST handler PostExecScript discarded the Start()/Wait() errors with '_ =' and then checked a never-assigned err, so failing scripts fell through to a confusing 'Invalid JSON' 400. Use cmd.Run(), check its error, and capture stderr for the error log. --- handlers/exec.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/handlers/exec.go b/handlers/exec.go index 5b84b99..b872297 100644 --- a/handlers/exec.go +++ b/handlers/exec.go @@ -115,24 +115,21 @@ func (h *ExecHandler) PostExecScript(c *gin.Context) { return } - // Exec script with or without body + // Exec script, piping the request body to its stdin + cmd := exec.Command(script) + cmd.Stdin = c.Request.Body - c1 := exec.Command(script) - - body := c.Request.Body var buf bytes.Buffer + var stderr bytes.Buffer + cmd.Stdout = &buf + cmd.Stderr = &stderr - c1.Stdin = body - c1.Stdout = &buf - _ = c1.Start() - _ = c1.Wait() - - if err != nil { + if err = cmd.Run(); err != nil { serr := err.Error() c.JSON(500, gin.H{ "error": serr, }) - log.Printf("[error] executing `%s`: %s", path, serr) + log.Printf("[error] executing `%s`: %s: %s", path, serr, stderr.String()) return } From 4f03acd489505c71549769eacee4e21b886ef622 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:45:24 +0200 Subject: [PATCH 08/36] fix: return 204 No Content for favicon c.JSON(200, nil) sent the literal 'null' as a favicon. Return 204 instead, and update the test expectation. --- main_test.go | 2 +- router.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/main_test.go b/main_test.go index b6f329a..5ce0492 100644 --- a/main_test.go +++ b/main_test.go @@ -33,7 +33,7 @@ func TestBase(t *testing.T) { assert.Equal(t, 200, status, "should get a 200") status, _ = test.Get(t, "/favicon.ico", nil) - assert.Equal(t, 200, status, "should get a 200") + assert.Equal(t, 204, status, "should get a 204") status, _ = test.Get(t, "/blablabla", nil) assert.Equal(t, 404, status, "should get a 404") diff --git a/router.go b/router.go index 7600c2b..fd236f4 100644 --- a/router.go +++ b/router.go @@ -86,7 +86,7 @@ func index(c *gin.Context) { } func favicon(c *gin.Context) { - c.JSON(200, nil) + c.Status(http.StatusNoContent) } func version(c *gin.Context) { From 467766d9c019a6fd912e1d1c46b7f73225f6bb20 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:46:11 +0200 Subject: [PATCH 09/36] fix: treat any stat error as missing index in indexExists Previously a non-NotExist error (e.g. permissions) returned true and triggered a redirect to a file that can't be read. Only treat a nil error as present. --- router.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/router.go b/router.go index fd236f4..692fa5f 100644 --- a/router.go +++ b/router.go @@ -65,12 +65,10 @@ func Router() *gin.Engine { /** Base routes */ func indexExists() bool { - if _, err := os.Stat(*apiDir + "/_static/index.html"); err != nil { - if os.IsNotExist(err) { - return false - } - } - return true + // Any stat error (not found, permission, ...) means we can't serve it, + // so treat it as absent rather than redirecting to an unreadable file. + _, err := os.Stat(*apiDir + "/_static/index.html") + return err == nil } func index(c *gin.Context) { From 8a95b79975fbc23066417a1a25c4045870b46dff Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:46:44 +0200 Subject: [PATCH 10/36] fix: log and exit on server error instead of busy-looping The bare 'for { s.ListenAndServe() }' silently restarted the server on error (busy-looping when the port is unavailable) and never logged why. Log the error and exit. Also clarify the startup log measures setup time. --- main.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/main.go b/main.go index 3b6315b..7858bf7 100644 --- a/main.go +++ b/main.go @@ -42,10 +42,12 @@ func StartGin() { MaxHeaderBytes: 1 << 20, } - log.Printf("[info] API started in %v on %s\n", time.Since(start), sport) + log.Printf("[info] API ready in %v, listening on %s\n", time.Since(start), sport) - for { - s.ListenAndServe() + // ListenAndServe only returns on error; log it and exit instead of + // silently busy-looping a restart. + if err := s.ListenAndServe(); err != nil { + log.Fatalf("[error] server stopped: %v", err) } } From 81f8cb7a5cf8773dfc30dafb2af70b6823a6bc29 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:47:07 +0200 Subject: [PATCH 11/36] refactor: expose /version without authentication Move /version out of the auth group so health/version checks work even when -password is set. --- router.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/router.go b/router.go index 692fa5f..cbb08c7 100644 --- a/router.go +++ b/router.go @@ -17,9 +17,10 @@ func Router() *gin.Engine { router.Use(m.CORSMiddleware()) - // Default routes + // Default routes (no auth: useful for health checks) router.GET("/", index) router.GET("/favicon.ico", favicon) + router.GET("/version", version) // Authentication authorized := router.Group("/") @@ -35,9 +36,6 @@ func Router() *gin.Engine { log.Println("[warn] no -password set: authentication is DISABLED and all endpoints are publicly accessible") } - // Version (commit and date) - authorized.GET("/version", version) - lsHandler := &h.LsHandler{ApiDir: apiDir} execHandler := &h.ExecHandler{ApiDir: apiDir} From b833b7d69a71254c925ee4948e82d371c2b79b15 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:48:14 +0200 Subject: [PATCH 12/36] refactor: deduplicate GET/POST script execution handlers Extract shared scriptPath validation, existence check (resolve) and JSON-emitting execution (run). ExecScript and PostExecScript now differ only in how the command is built (query arg vs stdin body). GET also gains stderr capture for consistency. --- handlers/exec.go | 132 +++++++++++++++-------------------------------- 1 file changed, 41 insertions(+), 91 deletions(-) diff --git a/handlers/exec.go b/handlers/exec.go index b872297..73089a5 100644 --- a/handlers/exec.go +++ b/handlers/exec.go @@ -3,7 +3,6 @@ package handlers import ( "bytes" "encoding/json" - "fmt" "log" "os" "os/exec" @@ -33,120 +32,71 @@ func (h *ExecHandler) scriptPath(reqPath string) (string, bool) { return script, true } -func (h *ExecHandler) ExecScript(c *gin.Context) { - var stdout []byte - var err error - - // Build and validate script name +// resolve validates the request path and ensures the target script exists, +// writing a 404 and returning ok=false when it cannot be served. +func (h *ExecHandler) resolve(c *gin.Context) (string, bool) { path := c.Param("path") script, ok := h.scriptPath(path) if !ok { - c.JSON(404, gin.H{ - "error": "Resource not found", - }) - fmt.Printf("[error] invalid resource path: %s\n", path) - return + c.JSON(404, gin.H{"error": "Resource not found"}) + log.Printf("[error] invalid resource path: %s", path) + return "", false } - - // Check script exists if _, err := os.Stat(script); os.IsNotExist(err) { - c.JSON(404, gin.H{ - "error": "Resource not found", - }) - fmt.Printf("[error] resource not found: %s\n", script) - return + c.JSON(404, gin.H{"error": "Resource not found"}) + log.Printf("[error] resource not found: %s", script) + return "", false } + return script, true +} - // Exec script with or without param - q := c.Request.URL.Query() - param, isParam := q["q"] - if isParam { - stdout, err = exec.Command(script, param[0]).Output() - } else { - stdout, err = exec.Command(script).Output() - } +// run executes cmd, which is expected to print JSON to stdout, and writes the +// parsed JSON (or an appropriate error) to the response. +func (h *ExecHandler) run(c *gin.Context, script string, cmd *exec.Cmd) { + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr - if err != nil { - serr := err.Error() - c.JSON(500, gin.H{ - "error": serr, - }) - log.Printf("[error] executing `%s`: %s", path, serr) + if err := cmd.Run(); err != nil { + c.JSON(500, gin.H{"error": err.Error()}) + log.Printf("[error] executing `%s`: %s: %s", script, err, stderr.String()) return } - // Try to unmarshal JSON - var someJson interface{} - err = json.Unmarshal(stdout, &someJson) - - if err != nil { - c.JSON(400, gin.H{ - "error": "Invalid JSON", - }) - log.Printf("[error] invalid JSON for `%s`: %s", script, stdout) + var payload any + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + c.JSON(400, gin.H{"error": "Invalid JSON"}) + log.Printf("[error] invalid JSON for `%s`: %s", script, stdout.Bytes()) return } - c.JSON(200, someJson) - //log.Printf("[info] executing `%s`: %s", script, stdout) + c.JSON(200, payload) } -func (h *ExecHandler) PostExecScript(c *gin.Context) { - var stdout []byte - var err error - - // Build and validate script name - path := c.Param("path") - script, ok := h.scriptPath(path) +// ExecScript runs the script for a GET request, optionally passing the `q` +// query parameter as the script's first argument. +func (h *ExecHandler) ExecScript(c *gin.Context) { + script, ok := h.resolve(c) if !ok { - c.JSON(404, gin.H{ - "error": "Resource not found", - }) - fmt.Printf("[error] invalid resource path: %s\n", path) - return - } - - // Check script exists - if _, err := os.Stat(script); os.IsNotExist(err) { - c.JSON(404, gin.H{ - "error": "Resource not found", - }) - fmt.Printf("[error] resource not found: %s\n", script) return } - // Exec script, piping the request body to its stdin cmd := exec.Command(script) - cmd.Stdin = c.Request.Body - - var buf bytes.Buffer - var stderr bytes.Buffer - cmd.Stdout = &buf - cmd.Stderr = &stderr - - if err = cmd.Run(); err != nil { - serr := err.Error() - c.JSON(500, gin.H{ - "error": serr, - }) - log.Printf("[error] executing `%s`: %s: %s", path, serr, stderr.String()) - return + if q, isParam := c.Request.URL.Query()["q"]; isParam { + cmd = exec.Command(script, q[0]) } + h.run(c, script, cmd) +} - stdout = buf.Bytes() - - // Try to unmarshal JSON - var someJson interface{} - err = json.Unmarshal(stdout, &someJson) - - if err != nil { - c.JSON(400, gin.H{ - "error": "Invalid JSON", - }) - log.Printf("[error] invalid JSON for `%s`: %s", script, stdout) +// PostExecScript runs the script for a POST request, piping the request body +// to the script's stdin. +func (h *ExecHandler) PostExecScript(c *gin.Context) { + script, ok := h.resolve(c) + if !ok { return } - c.JSON(200, someJson) - //log.Printf("[info] executing `%s`: %s", script, stdout) + cmd := exec.Command(script) + cmd.Stdin = c.Request.Body + h.run(c, script, cmd) } From ae9e4e9c7cff1bc5c51243f6b25502a0bb996565 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:49:48 +0200 Subject: [PATCH 13/36] refactor: unify logging on logrus Replace the standard 'log' package and fmt.Printf logging in main.go, router.go and handlers with logrus (already used by docker.go), and drop the redundant [info]/[error]/[warn] prefixes now that levels convey them. --- handlers/exec.go | 10 +++++----- main.go | 8 ++++---- router.go | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/handlers/exec.go b/handlers/exec.go index 73089a5..8edea50 100644 --- a/handlers/exec.go +++ b/handlers/exec.go @@ -3,13 +3,13 @@ package handlers import ( "bytes" "encoding/json" - "log" "os" "os/exec" "path/filepath" "strings" "github.com/gin-gonic/gin" + "github.com/sirupsen/logrus" ) type ExecHandler struct { @@ -39,12 +39,12 @@ func (h *ExecHandler) resolve(c *gin.Context) (string, bool) { script, ok := h.scriptPath(path) if !ok { c.JSON(404, gin.H{"error": "Resource not found"}) - log.Printf("[error] invalid resource path: %s", path) + logrus.Errorf("invalid resource path: %s", path) return "", false } if _, err := os.Stat(script); os.IsNotExist(err) { c.JSON(404, gin.H{"error": "Resource not found"}) - log.Printf("[error] resource not found: %s", script) + logrus.Errorf("resource not found: %s", script) return "", false } return script, true @@ -59,14 +59,14 @@ func (h *ExecHandler) run(c *gin.Context, script string, cmd *exec.Cmd) { if err := cmd.Run(); err != nil { c.JSON(500, gin.H{"error": err.Error()}) - log.Printf("[error] executing `%s`: %s: %s", script, err, stderr.String()) + logrus.Errorf("executing `%s`: %s: %s", script, err, stderr.String()) return } var payload any if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { c.JSON(400, gin.H{"error": "Invalid JSON"}) - log.Printf("[error] invalid JSON for `%s`: %s", script, stdout.Bytes()) + logrus.Errorf("invalid JSON for `%s`: %s", script, stdout.Bytes()) return } diff --git a/main.go b/main.go index 7858bf7..ff97a1d 100644 --- a/main.go +++ b/main.go @@ -3,12 +3,12 @@ package main import ( "flag" "fmt" - "log" "net/http" "runtime" "time" "github.com/gin-gonic/gin" + "github.com/sirupsen/logrus" ) var ( @@ -25,7 +25,7 @@ var ( func ConfigRuntime() { nuCPU := runtime.NumCPU() runtime.GOMAXPROCS(nuCPU) - fmt.Printf("[info] Running with %d CPUs\n", nuCPU) + logrus.Infof("Running with %d CPUs", nuCPU) } func StartGin() { @@ -42,12 +42,12 @@ func StartGin() { MaxHeaderBytes: 1 << 20, } - log.Printf("[info] API ready in %v, listening on %s\n", time.Since(start), sport) + logrus.Infof("API ready in %v, listening on %s", time.Since(start), sport) // ListenAndServe only returns on error; log it and exit instead of // silently busy-looping a restart. if err := s.ListenAndServe(); err != nil { - log.Fatalf("[error] server stopped: %v", err) + logrus.Fatalf("server stopped: %v", err) } } diff --git a/router.go b/router.go index cbb08c7..0572a26 100644 --- a/router.go +++ b/router.go @@ -1,11 +1,11 @@ package main import ( - "log" "net/http" "os" "github.com/gin-gonic/gin" + "github.com/sirupsen/logrus" h "github.com/thbkrkr/go-apish/handlers" m "github.com/thbkrkr/go-apish/middlewares" ) @@ -33,7 +33,7 @@ func Router() *gin.Engine { }, )) } else { - log.Println("[warn] no -password set: authentication is DISABLED and all endpoints are publicly accessible") + logrus.Warn("no -password set: authentication is DISABLED and all endpoints are publicly accessible") } lsHandler := &h.LsHandler{ApiDir: apiDir} From 67f1701780a0f220fce964cea346b801f9990ae7 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:53:37 +0200 Subject: [PATCH 14/36] refactor: pass apiDir to handlers by value Store ApiDir as string instead of *string in ExecHandler/LsHandler. The flag value is resolved once at Router() construction, removing handler dependence on a mutable global pointer. --- handlers/exec.go | 4 ++-- handlers/ls.go | 12 ++++++------ router.go | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/handlers/exec.go b/handlers/exec.go index 8edea50..6b5cd7c 100644 --- a/handlers/exec.go +++ b/handlers/exec.go @@ -13,14 +13,14 @@ import ( ) type ExecHandler struct { - ApiDir *string + ApiDir string } // scriptPath resolves the wildcard request path to an absolute `.sh` path and // guarantees it stays within ApiDir, preventing path-traversal escapes such as // `/api/../../../tmp/evil`. It returns false when the path escapes ApiDir. func (h *ExecHandler) scriptPath(reqPath string) (string, bool) { - base, err := filepath.Abs(*h.ApiDir) + base, err := filepath.Abs(h.ApiDir) if err != nil { return "", false } diff --git a/handlers/ls.go b/handlers/ls.go index c3c2ba1..f9cc7ee 100644 --- a/handlers/ls.go +++ b/handlers/ls.go @@ -10,7 +10,7 @@ import ( ) type LsHandler struct { - ApiDir *string + ApiDir string } type resources struct { @@ -27,9 +27,9 @@ func (h *LsHandler) ListResources(c *gin.Context) { hostname := strings.Replace(c.Request.Host, "/", "", -1) // List scripts - err := filepath.Walk(*h.ApiDir, func(path string, f os.FileInfo, err error) error { + err := filepath.Walk(h.ApiDir, func(path string, f os.FileInfo, err error) error { if strings.HasSuffix(path, "sh") && !strings.Contains(path, "_static") { - url := fileToUrl(hostname, "api", path, *h.ApiDir) + url := fileToUrl(hostname, "api", path, h.ApiDir) scripts = append(scripts, url) } return nil @@ -42,12 +42,12 @@ func (h *LsHandler) ListResources(c *gin.Context) { } staticDir := "_static" - htmlDir := fmt.Sprintf("%s/%s", *h.ApiDir, staticDir) + htmlDir := fmt.Sprintf("%s/%s", h.ApiDir, staticDir) // List html files err = filepath.Walk(htmlDir, func(path string, f os.FileInfo, err error) error { if strings.HasSuffix(path, "html") { - url := fileToUrl(hostname, "s", path, *h.ApiDir+"/_static") + url := fileToUrl(hostname, "s", path, h.ApiDir+"/_static") pages = append(pages, url) } return nil @@ -62,7 +62,7 @@ func (h *LsHandler) ListResources(c *gin.Context) { // List static files err = filepath.Walk(htmlDir, func(path string, f os.FileInfo, err error) error { if f != nil && !f.IsDir() && !strings.HasSuffix(path, "html") { - url := fileToUrl(hostname, "s", path, *h.ApiDir+"/_static") + url := fileToUrl(hostname, "s", path, h.ApiDir+"/_static") static = append(static, url) } return nil diff --git a/router.go b/router.go index 0572a26..cbb481f 100644 --- a/router.go +++ b/router.go @@ -36,8 +36,8 @@ func Router() *gin.Engine { logrus.Warn("no -password set: authentication is DISABLED and all endpoints are publicly accessible") } - lsHandler := &h.LsHandler{ApiDir: apiDir} - execHandler := &h.ExecHandler{ApiDir: apiDir} + lsHandler := &h.LsHandler{ApiDir: *apiDir} + execHandler := &h.ExecHandler{ApiDir: *apiDir} // List resources authorized.GET("/ls", func(c *gin.Context) { From 334173cccf40a183dcdd3d6dec6328c917eb31bd Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:57:18 +0200 Subject: [PATCH 15/36] refactor: single static walk and real error propagation in ListResources Collapse the two _static walks into one pass that splits HTML pages from other files, and propagate walk errors (the walk funcs previously always returned nil, making the error checks dead code). The optional _static directory is skipped when absent. --- handlers/ls.go | 60 +++++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/handlers/ls.go b/handlers/ls.go index f9cc7ee..e9691a6 100644 --- a/handlers/ls.go +++ b/handlers/ls.go @@ -25,12 +25,15 @@ func (h *LsHandler) ListResources(c *gin.Context) { static := make([]string, 0) hostname := strings.Replace(c.Request.Host, "/", "", -1) + staticDir := h.ApiDir + "/_static" - // List scripts + // List API scripts (every .sh outside _static), propagating walk errors. err := filepath.Walk(h.ApiDir, func(path string, f os.FileInfo, err error) error { + if err != nil { + return err + } if strings.HasSuffix(path, "sh") && !strings.Contains(path, "_static") { - url := fileToUrl(hostname, "api", path, h.ApiDir) - scripts = append(scripts, url) + scripts = append(scripts, fileToUrl(hostname, "api", path, h.ApiDir)) } return nil }) @@ -41,37 +44,30 @@ func (h *LsHandler) ListResources(c *gin.Context) { return } - staticDir := "_static" - htmlDir := fmt.Sprintf("%s/%s", h.ApiDir, staticDir) - - // List html files - err = filepath.Walk(htmlDir, func(path string, f os.FileInfo, err error) error { - if strings.HasSuffix(path, "html") { - url := fileToUrl(hostname, "s", path, h.ApiDir+"/_static") - pages = append(pages, url) - } - return nil - }) - if err != nil { - c.JSON(500, gin.H{ - "error": err.Error(), + // List static resources in a single pass, splitting HTML pages from other + // files. The _static directory is optional, so skip it when absent. + if _, statErr := os.Stat(staticDir); statErr == nil { + err = filepath.Walk(staticDir, func(path string, f os.FileInfo, err error) error { + if err != nil { + return err + } + if f.IsDir() { + return nil + } + url := fileToUrl(hostname, "s", path, staticDir) + if strings.HasSuffix(path, "html") { + pages = append(pages, url) + } else { + static = append(static, url) + } + return nil }) - return - } - - // List static files - err = filepath.Walk(htmlDir, func(path string, f os.FileInfo, err error) error { - if f != nil && !f.IsDir() && !strings.HasSuffix(path, "html") { - url := fileToUrl(hostname, "s", path, h.ApiDir+"/_static") - static = append(static, url) + if err != nil { + c.JSON(500, gin.H{ + "error": err.Error(), + }) + return } - return nil - }) - if err != nil { - c.JSON(500, gin.H{ - "error": err.Error(), - }) - return } c.JSON(200, resources{ From ad69a4b90ed6ec61aed64edd8d3bf08bfffd2e63 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 17:58:13 +0200 Subject: [PATCH 16/36] fix: derive listed URL scheme from the request fileToUrl hardcoded http://, producing wrong links behind TLS/a proxy. Build the base URL from the request scheme (TLS or X-Forwarded-Proto) and pass it in. Also switch strings.Replace(-1) to strings.ReplaceAll. --- handlers/ls.go | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/handlers/ls.go b/handlers/ls.go index e9691a6..67efc0a 100644 --- a/handlers/ls.go +++ b/handlers/ls.go @@ -24,7 +24,8 @@ func (h *LsHandler) ListResources(c *gin.Context) { pages := make([]string, 0) static := make([]string, 0) - hostname := strings.Replace(c.Request.Host, "/", "", -1) + hostname := strings.ReplaceAll(c.Request.Host, "/", "") + baseURL := requestScheme(c) + "://" + hostname staticDir := h.ApiDir + "/_static" // List API scripts (every .sh outside _static), propagating walk errors. @@ -33,7 +34,7 @@ func (h *LsHandler) ListResources(c *gin.Context) { return err } if strings.HasSuffix(path, "sh") && !strings.Contains(path, "_static") { - scripts = append(scripts, fileToUrl(hostname, "api", path, h.ApiDir)) + scripts = append(scripts, fileToUrl(baseURL, "api", path, h.ApiDir)) } return nil }) @@ -54,7 +55,7 @@ func (h *LsHandler) ListResources(c *gin.Context) { if f.IsDir() { return nil } - url := fileToUrl(hostname, "s", path, staticDir) + url := fileToUrl(baseURL, "s", path, staticDir) if strings.HasSuffix(path, "html") { pages = append(pages, url) } else { @@ -77,17 +78,23 @@ func (h *LsHandler) ListResources(c *gin.Context) { }) } -func fileToUrl(hostname string, prefix string, path string, apiDir string) string { +// requestScheme reports whether the request arrived over https, honoring a +// reverse proxy's X-Forwarded-Proto header. +func requestScheme(c *gin.Context) string { + if c.Request.TLS != nil || c.Request.Header.Get("X-Forwarded-Proto") == "https" { + return "https" + } + return "http" +} + +func fileToUrl(baseURL string, prefix string, path string, apiDir string) string { // Remove ./ from apiDir - apiDir = strings.Replace(apiDir, "./", "", -1) + apiDir = strings.ReplaceAll(apiDir, "./", "") // Replace $apiDir by prefix - filePath := strings.Replace(path, apiDir, prefix, -1) - baseUrl := fmt.Sprintf("http://%v", hostname) + filePath := strings.ReplaceAll(path, apiDir, prefix) if strings.Contains(path, "_static") { - return fmt.Sprintf("%v/%v", baseUrl, filePath) - } else { - return fmt.Sprintf("%v/%v", baseUrl, strings.Replace(filePath, ".sh", "", -1)) + return fmt.Sprintf("%v/%v", baseURL, filePath) } - + return fmt.Sprintf("%v/%v", baseURL, strings.ReplaceAll(filePath, ".sh", "")) } From a77beddbbee4a1870f3ae25b84a5ff10952c1a30 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 18:04:55 +0200 Subject: [PATCH 17/36] fix(security): configurable basic-auth user, no default API key Add a -user flag for the basic-auth username (was a hardcoded global) and default -apiKey to empty. An empty key now disables X-Auth header auth entirely so it can't accidentally match a missing header. --- main.go | 3 ++- main_test.go | 1 + middlewares/auth.go | 13 ++++++++----- router.go | 4 +--- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/main.go b/main.go index ff97a1d..8d0e37e 100644 --- a/main.go +++ b/main.go @@ -16,8 +16,9 @@ var ( buildDate = "undefined" port = flag.Int("port", 4242, "HTTP port to listen") + user = flag.String("user", "zuperadmin", "Username for basic auth") password = flag.String("password", "", "Admin password for basic auth") - apiKey = flag.String("apiKey", "42", "API key for header auth") + apiKey = flag.String("apiKey", "", "API key for X-Auth header auth (empty disables header auth)") apiDir = flag.String("apiDir", "./api", "API directory (sh scripts and html pages)") enableDocker = flag.Bool("enableDocker", false, "Enable the /docker endpoint (grants full host access via docker run)") ) diff --git a/main_test.go b/main_test.go index 5ce0492..73a722e 100644 --- a/main_test.go +++ b/main_test.go @@ -22,6 +22,7 @@ func init() { gin.SetMode(gin.TestMode) *apiDir = "example/api" *password = "42" + *apiKey = "42" server = httptest.NewServer(Router()) test.ServerURL = server.URL diff --git a/middlewares/auth.go b/middlewares/auth.go index 7affdf5..3b8627c 100644 --- a/middlewares/auth.go +++ b/middlewares/auth.go @@ -12,11 +12,14 @@ func AuthMiddleware(apiKey string, accounts gin.Accounts) gin.HandlerFunc { basicAuth := gin.BasicAuthForRealm(accounts, "") return func(c *gin.Context) { - // Try header auth, comparing in constant time to avoid leaking the - // key through response-timing differences. - got := c.Request.Header.Get(AuthHeaderKey) - if subtle.ConstantTimeCompare([]byte(got), []byte(apiKey)) == 1 { - return + // Try header auth when a key is configured, comparing in constant time + // to avoid leaking the key through response-timing differences. An + // empty key disables header auth (so it can't match a missing header). + if apiKey != "" { + got := c.Request.Header.Get(AuthHeaderKey) + if subtle.ConstantTimeCompare([]byte(got), []byte(apiKey)) == 1 { + return + } } // Fall back to basic auth basicAuth(c) diff --git a/router.go b/router.go index cbb481f..eb74009 100644 --- a/router.go +++ b/router.go @@ -10,8 +10,6 @@ import ( m "github.com/thbkrkr/go-apish/middlewares" ) -var basicAuthUser = "zuperadmin" - func Router() *gin.Engine { router := gin.Default() @@ -29,7 +27,7 @@ func Router() *gin.Engine { authorized = router.Group("/", m.AuthMiddleware( *apiKey, gin.Accounts{ - basicAuthUser: *password, + *user: *password, }, )) } else { From ecb72f2d9c12774cce8c10cbf93d14526f5dd143 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 18:06:01 +0200 Subject: [PATCH 18/36] build: modernize Docker build with multi-stage Go 1.25 Replace the golang:1.6.2 + alpine:3.7 build (which can't build a go 1.25 module) with a multi-stage Dockerfile on golang:1.25-alpine + alpine:3.20. The image now builds itself; 'make binary' builds locally and 'make build' builds the image, both injecting git commit/date via ldflags. --- Dockerfile | 23 ++++++++++++++++++++--- Makefile | 21 ++++++++++----------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/Dockerfile b/Dockerfile index 644b1dd..3540e6e 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,5 +1,22 @@ -FROM alpine:3.7 +# Build stage +FROM golang:1.25-alpine AS build + +WORKDIR /src +COPY go.mod go.sum ./ +RUN go mod download +COPY . . + +ARG GIT_COMMIT=undefined +ARG BUILD_DATE=undefined +RUN CGO_ENABLED=0 go build \ + -ldflags "-X main.gitCommit=${GIT_COMMIT} -X main.buildDate=${BUILD_DATE}" \ + -o /go-apish . + +# Runtime stage +FROM alpine:3.20 RUN apk --no-cache add bash jq curl -COPY go-apish /go-apish -CMD ["/go-apish"] +COPY --from=build /go-apish /go-apish + +EXPOSE 4242 +ENTRYPOINT ["/go-apish"] diff --git a/Makefile b/Makefile index 84c888b..22f76ef 100644 --- a/Makefile +++ b/Makefile @@ -1,19 +1,18 @@ GIT_COMMIT = $(shell git rev-parse --short HEAD) BUILD_DATE = $(shell date '+%Y%m%d-%H%M%S') +LDFLAGS = -X main.gitCommit=$(GIT_COMMIT) -X main.buildDate=$(BUILD_DATE) -build: build-binary build-image +build: + docker build --rm \ + --build-arg GIT_COMMIT=$(GIT_COMMIT) \ + --build-arg BUILD_DATE=$(BUILD_DATE) \ + -t krkr/apish . -build-binary: - docker run --rm \ - -w /go/src/github.com/thbkrkr/go-apish \ - -v $(shell pwd):/go/src/github.com/thbkrkr/go-apish \ - -e CGO_ENABLED=0 -e GOOS=linux \ - -ti golang:1.6.2 \ - go build -a -installsuffix cgo \ - -ldflags "-X=main.gitCommit=$(GIT_COMMIT) -X=main.buildDate=$(BUILD_DATE)" +binary: + CGO_ENABLED=0 go build -ldflags "$(LDFLAGS)" -o go-apish . -build-image: - @docker build --rm -t krkr/apish . +test: + go test ./... release: ./release.sh $(GIT_COMMIT) From 49bd455cc7019c604f087c9a95130c378dc0ba24 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 18:09:10 +0200 Subject: [PATCH 19/36] build: drop release target referencing missing release.sh The 'make release' target called ./release.sh, which is not in the repo. Remove it; image publishing is covered by 'make push'. --- Makefile | 3 --- 1 file changed, 3 deletions(-) diff --git a/Makefile b/Makefile index 22f76ef..a379ade 100644 --- a/Makefile +++ b/Makefile @@ -14,9 +14,6 @@ binary: test: go test ./... -release: - ./release.sh $(GIT_COMMIT) - push: docker push krkr/apish From b1b0db2ab62b16776d5d33fb7d5080efd0f9da92 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 18:15:34 +0200 Subject: [PATCH 20/36] docs: document flags, endpoints, auth and security in README Add a flags table, endpoint reference, the JSON output contract, the authentication model, and a security section covering -password and the -enableDocker host-access risk. --- README.md | 70 +++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 63 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 8bf1aee..eae548c 100644 --- a/README.md +++ b/README.md @@ -1,13 +1,69 @@ -# apish - Rest API for shell scripts +# apish — REST API for shell scripts -Write shell scripts that return JSON ([example](example/api/time/date.sh)). +`apish` turns a directory of shell scripts into a JSON REST API and serves +static files alongside them. Each script must print **valid JSON** to stdout +([example](example/api/time/date.sh)); the server parses it and returns it to +the caller. -Serve static files from [api/_static](example/api/_static) directory. +## Build & run -Build +```sh +make binary # build the ./go-apish binary locally +make build # build the krkr/apish Docker image +make run # run the image, mounting ./example as /api on port 80 +``` - make build +Run the binary directly against the example API: -Run +```sh +./go-apish -apiDir=example/api -password=secret +``` - make run +## Flags + +| Flag | Default | Description | +| --------------- | ----------- | -------------------------------------------------------- | +| `-port` | `4242` | HTTP port to listen on | +| `-apiDir` | `./api` | Directory of `.sh` scripts and `_static` files | +| `-user` | `zuperadmin`| Basic-auth username | +| `-password` | *(empty)* | Basic-auth password. **Empty disables all auth.** | +| `-apiKey` | *(empty)* | Key for `X-Auth` header auth. Empty disables header auth.| +| `-enableDocker` | `false` | Enable `POST /docker` (see Security) | + +## Endpoints + +| Method | Path | Description | +| ------ | ------------ | ---------------------------------------------------------------- | +| GET | `/` | JSON status, or redirect to `/s` if `_static/index.html` exists | +| GET | `/version` | Build commit and date (no auth) | +| GET | `/ls` | List script, HTML and static resource URLs | +| GET | `/api/*path` | Run `/.sh`; `?q=value` is passed as `$1` | +| POST | `/api/*path` | Run `/.sh` with the request body piped to stdin | +| POST | `/docker` | Run `docker run ` (only when `-enableDocker` is set) | +| GET | `/s/*` | Serve static files from `/_static` | + +Scripts must emit valid JSON; otherwise the caller receives `400 Invalid JSON`. +A script that exits non-zero yields `500` with its error (stderr is logged). + +## Authentication + +When `-password` is set, all endpoints except `/`, `/favicon.ico` and +`/version` require either: + +- HTTP basic auth (`-user` / `-password`), or +- an `X-Auth: ` header (when `-apiKey` is set). + +If `-password` is empty, **the server is fully open** and logs a warning at +startup. + +## Security + +`apish` executes shell scripts and, optionally, arbitrary containers — treat it +as a privileged service: + +- Always set `-password` (and ideally an `-apiKey`) in any non-local deployment. +- `-enableDocker` lets callers run **any** `docker run` command, which is + effectively root on the host. Leave it off unless you fully trust callers. +- Scripts receive request input (`$1` / stdin). Build their JSON output with a + tool like `jq` so values are safely escaped — see + [param.sh](example/api/test/param.sh). From f1c7222b93b755fe4e983df191d1b39720b04dcb Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 21:58:46 +0200 Subject: [PATCH 21/36] test: cover path traversal, POST, invalid JSON and /ls Add a handler unit test asserting scriptPath rejects escapes from ApiDir, plus HTTP tests for POST execution, invalid-JSON 400, /ls listing, and a path-traversal request. Fix TestBase for the /version no-auth route and the authenticated / -> /s redirect. --- handlers/exec_test.go | 21 +++++++++++++++++++++ main_test.go | 32 +++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 handlers/exec_test.go diff --git a/handlers/exec_test.go b/handlers/exec_test.go new file mode 100644 index 0000000..a7f49b7 --- /dev/null +++ b/handlers/exec_test.go @@ -0,0 +1,21 @@ +package handlers + +import "testing" + +func TestScriptPathStaysWithinApiDir(t *testing.T) { + h := &ExecHandler{ApiDir: "example/api"} + + cases := map[string]bool{ + "/time/date": true, // normal script + "/test/param": true, // nested script + "/../secret": false, // climbs out of ApiDir + "/../../../etc/passwd": false, // deep traversal + "/time/../../../tmp/x": false, // traversal after a valid segment + } + + for reqPath, wantOK := range cases { + if _, ok := h.scriptPath(reqPath); ok != wantOK { + t.Errorf("scriptPath(%q) ok = %v, want %v", reqPath, ok, wantOK) + } + } +} diff --git a/main_test.go b/main_test.go index 73a722e..c9f37b5 100644 --- a/main_test.go +++ b/main_test.go @@ -30,7 +30,14 @@ func init() { func TestBase(t *testing.T) { test.PrefixURL = "" - status, _ := test.Get(t, "/", nil) + + // / redirects to /s (index.html exists in the example), which is behind + // auth, so the request must carry credentials to follow through to 200. + status, _ := test.Get(t, "/", auth) + assert.Equal(t, 200, status, "should get a 200") + + // /version is exposed without auth. + status, _ = test.Get(t, "/version", nil) assert.Equal(t, 200, status, "should get a 200") status, _ = test.Get(t, "/favicon.ico", nil) @@ -84,3 +91,26 @@ func TestPages(t *testing.T) { status, _ = test.Get(t, "/s/js/script.js", auth) assert.Equal(t, 200, status, "should get a 200") } + +func TestPost(t *testing.T) { + status, body := test.Post(t, "/api/test/post", `{"o": 42}`, auth) + assert.Equal(t, 200, status, "should get a 200") + assert.Contains(t, body, "jackpot") +} + +func TestInvalidJSON(t *testing.T) { + status, _ := test.Get(t, "/api/test/invalid-json", auth) + assert.Equal(t, 400, status, "invalid JSON output should yield a 400") +} + +func TestListResources(t *testing.T) { + status, body := test.Get(t, "/ls", auth) + assert.Equal(t, 200, status, "should get a 200") + assert.Contains(t, body, "/api/time/date") +} + +func TestPathTraversal(t *testing.T) { + // Attempting to escape apiDir must not execute an arbitrary script. + status, _ := test.Get(t, "/api/../../../../etc/hostname", auth) + assert.NotEqual(t, 200, status, "path traversal must be rejected") +} From 06a9052391efe5313db79f39128224a71338bf12 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 22:00:33 +0200 Subject: [PATCH 22/36] test: use keyed struct literals and drop dead reader var Key the test.BasicAuth literals (go vet warned on unkeyed fields) and remove the unused 'reader' variable and its 'io' import. --- main_test.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/main_test.go b/main_test.go index c9f37b5..b7218d4 100644 --- a/main_test.go +++ b/main_test.go @@ -1,7 +1,6 @@ package main import ( - "io" "net/http/httptest" "testing" @@ -11,12 +10,9 @@ import ( test "github.com/thbkrkr/go-apish/test" ) -var ( - server *httptest.Server - reader io.Reader //Ignore this for now -) +var server *httptest.Server -var auth = &test.BasicAuth{"zuperadmin", "42"} +var auth = &test.BasicAuth{Username: "zuperadmin", Password: "42"} func init() { gin.SetMode(gin.TestMode) @@ -51,7 +47,7 @@ func TestAuthentication(t *testing.T) { status, _ := test.Get(t, "/api/time/date", nil) assert.Equal(t, 401, status, "should get a 401") - auth := &test.BasicAuth{"zuperadmin", "42"} + auth := &test.BasicAuth{Username: "zuperadmin", Password: "42"} status, _ = test.Get(t, "/api/time/date", auth) assert.Equal(t, 200, status, "should get a 200") From 074ec6e35e573bcda841ba329eebf5e3e3b9703f Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 22:03:28 +0200 Subject: [PATCH 23/36] feat: remove the /docker endpoint The docker run endpoint is no longer used. Remove the handler, the -enableDocker flag, its route, the example docker script/page, and the related README docs. --- README.md | 7 +- code-review.md | 193 +++++++++++++++++++++++++++++ example/api/_static/docker/ps.html | 39 ------ example/api/docker/ps.sh | 8 -- handlers/docker.go | 86 ------------- main.go | 11 +- router.go | 6 - 7 files changed, 199 insertions(+), 151 deletions(-) create mode 100644 code-review.md delete mode 100644 example/api/_static/docker/ps.html delete mode 100755 example/api/docker/ps.sh delete mode 100644 handlers/docker.go diff --git a/README.md b/README.md index eae548c..28de631 100644 --- a/README.md +++ b/README.md @@ -28,7 +28,6 @@ Run the binary directly against the example API: | `-user` | `zuperadmin`| Basic-auth username | | `-password` | *(empty)* | Basic-auth password. **Empty disables all auth.** | | `-apiKey` | *(empty)* | Key for `X-Auth` header auth. Empty disables header auth.| -| `-enableDocker` | `false` | Enable `POST /docker` (see Security) | ## Endpoints @@ -39,7 +38,6 @@ Run the binary directly against the example API: | GET | `/ls` | List script, HTML and static resource URLs | | GET | `/api/*path` | Run `/.sh`; `?q=value` is passed as `$1` | | POST | `/api/*path` | Run `/.sh` with the request body piped to stdin | -| POST | `/docker` | Run `docker run ` (only when `-enableDocker` is set) | | GET | `/s/*` | Serve static files from `/_static` | Scripts must emit valid JSON; otherwise the caller receives `400 Invalid JSON`. @@ -58,12 +56,9 @@ startup. ## Security -`apish` executes shell scripts and, optionally, arbitrary containers — treat it -as a privileged service: +`apish` executes shell scripts — treat it as a privileged service: - Always set `-password` (and ideally an `-apiKey`) in any non-local deployment. -- `-enableDocker` lets callers run **any** `docker run` command, which is - effectively root on the host. Leave it off unless you fully trust callers. - Scripts receive request input (`$1` / stdin). Build their JSON output with a tool like `jq` so values are safely escaped — see [param.sh](example/api/test/param.sh). diff --git a/code-review.md b/code-review.md new file mode 100644 index 0000000..a9bca53 --- /dev/null +++ b/code-review.md @@ -0,0 +1,193 @@ +# Code Review — go-apish + +Review date: 2026-06-02 + +`go-apish` is a small Gin-based HTTP server that exposes shell scripts (and static +files) as a REST API. The idea is neat and the codebase is small and readable. The +notes below are grouped by severity. The most important section is **Security** — by +its very nature this project runs arbitrary shell commands, so the attack surface +deserves the most care. + +--- + +## 🔴 Security (high priority) + +### 1. Path traversal → arbitrary script execution — `handlers/exec.go:23-24` +```go +path := c.Param("path") +script := fmt.Sprintf("%s%s%s", *h.ApiDir, path, ".sh") +``` +The wildcard `*path` is concatenated straight onto `ApiDir` with no validation. A +request such as `GET /api/../../../../tmp/evil` resolves to `./api/../../../../tmp/evil.sh`, +letting a caller execute any `.sh` file on the filesystem outside `apiDir`. Mitigate by: +- `filepath.Clean` the joined path and verify it still has `apiDir` as a prefix + (use `filepath.Abs` on both and check `strings.HasPrefix`), or +- reject any path containing `..`. + +This applies to both `ExecScript` and `PostExecScript` (duplicated logic). + +### 2. `/docker` runs arbitrary `docker run` — `handlers/docker.go:31-32` +```go +args := append([]string{"run"}, strings.Split(form.Cmd, " ")...) +output, err := exec.Command("docker", args...).CombinedOutput() +``` +Any authenticated caller can run any container with any flags +(`-v /:/host`, `--privileged`, `--pid=host`, …), which is effectively root on the +host. This may be intentional for the tool's purpose, but it should be: +- gated behind an explicit opt-in flag (off by default), and +- clearly documented as "grants full host access." + +Also note `strings.Split(cmd, " ")` breaks on quoted arguments and multiple spaces; +a real shell-style tokenizer (e.g. `shellwords`) would be more correct if this stays. + +### 3. CORS allows any origin *with* credentials — `middlewares/cors.go:8,13` +```go +c.Writer.Header().Set("Access-Control-Allow-Origin", "*") +c.Writer.Header().Set("Access-Control-Allow-Credentials", "true") +``` +`Allow-Origin: *` together with `Allow-Credentials: true` is rejected by browsers and +is a misconfiguration. If credentials are needed, reflect a vetted origin instead of +`*`. If not, drop the credentials header. For an unauthenticated, open API the wildcard +is fine — but then the basic-auth story below conflicts with it. + +### 4. Auth middleware comparisons are not constant-time — `middlewares/auth.go:12` +```go +if c.Request.Header.Get(AuthHeaderKey) == apiKey { +``` +The API-key check uses `==`, which is vulnerable to timing attacks. Use +`crypto/subtle.ConstantTimeCompare`. Minor for a hobby tool, but trivial to fix. + +### 5. Auth is opt-in — `router.go:26` +Authentication is only enabled when `-password` is set. The default is a fully open +server that can execute scripts. Consider failing closed (require a password, or print +a loud warning at startup when running unauthenticated). + +### 6. Example scripts demonstrate shell injection — `example/api/test/param.sh` +```sh +echo '{ "param": "'$1'" }' +``` +Because Go's `exec.Command` does not invoke a shell, the Go side is safe from command +injection — but `$1` unquoted/unescaped inside the script means a value like `"} ...` +breaks the JSON, and any script that does `eval`/backticks on `$1` would be exploitable. +Since these are the canonical examples users copy, they should model safe quoting +(e.g. build JSON with `jq -n --arg p "$1" '{param:$p}'`). + +--- + +## 🟠 Correctness / bugs + +### 7. `PostExecScript` ignores all execution errors — `handlers/exec.go:88-105` +```go +c1 := exec.Command(script) +... +_ = c1.Start() +_ = c1.Wait() +if err != nil { // err is still nil here — declared at top, never assigned +``` +The `err` checked on line 98 is the zero-value from line 71; the real errors from +`Start()`/`Wait()` are discarded with `_ =`. A script that fails will fall through and +likely produce an "Invalid JSON" 400 instead of a 500 with the real error. Capture and +check those errors. Also `c1.Stderr` is never set, so stderr is lost. + +### 8. `favicon` returns JSON `null` with 200 — `router.go:81-83` +`c.JSON(200, nil)` sends `null` as a favicon. Harmless but odd; returning `204 No Content` +would be cleaner. + +### 9. `indexExists` swallows non-NotExist errors — `router.go:60-67` +If `os.Stat` fails for a reason other than "not exist" (e.g. permissions), the function +returns `true` and a redirect is issued for a file that can't be read. Treat any error as +"not present." + +### 10. Server restart loop hides crashes — `main.go:46-48` +```go +for { + s.ListenAndServe() +} +``` +`ListenAndServe` only returns on error; the bare `for` loop silently restarts it (busy-loop +if the port is unavailable) and the returned error is never logged. Log the error and exit, +or restart with backoff. Also, the "API started" log on line 44 prints *before* +`ListenAndServe` is called, so it's measuring `Router()` build time, not real startup. + +### 11. `version` route is inside the authorized group — `router.go:36` +Minor: a version/health endpoint is usually fine to expose unauthenticated; currently it's +behind auth when a password is set, which complicates health checks. + +--- + +## 🟡 Code quality / maintainability + +### 12. `ExecScript` and `PostExecScript` are ~90% duplicated — `handlers/exec.go` +The path-building, existence check, JSON-unmarshal, and error responses are copy-pasted. +Extract a helper like `runScript(c, script, stdin io.Reader)` and a shared +`resolveScript(path) (string, bool)`. This also means the path-traversal fix (item 1) only +has to be made once. + +### 13. Inconsistent logging — `fmt.Printf` vs `log.Printf` vs `logrus` +The project uses three logging styles: `fmt.Printf` (`exec.go:31,82`), `log.Printf` +(`exec.go:49`), and `logrus` (`docker.go`). Pick one (logrus is already a dependency) and +use structured, leveled logging consistently. The `fmt.Printf` calls also omit trailing +`\n`. + +### 14. Resolving `*string` flags via pointers passed into handlers — `router.go:38-39` +`LsHandler{ApiDir: apiDir}` stores a `*string` to a global flag. It works, but passing the +resolved `string` value (after `flag.Parse`) is simpler and avoids handlers depending on +global mutable state — which is exactly what makes the test on `main_test.go:23` have to +poke `*apiDir` directly. + +### 15. `ListResources` walks `_static` twice — `handlers/ls.go:48-69` +Two `filepath.Walk` passes over the same `htmlDir`, one for `.html` and one for everything +else. A single walk with a branch would halve the I/O and the code. The `err` from the +first scripts walk (line 30) can never be non-nil because the walk func always returns +`nil` — so the error checks are dead code unless the walk func propagates `err`. + +### 16. `fileToUrl` hardcodes `http://` — `handlers/ls.go:89` +Generated URLs are always `http://`, so behind TLS/a proxy the listed links are wrong. +Derive the scheme from the request (`X-Forwarded-Proto` / `c.Request.TLS`). + +### 17. Magic values / globals — `router.go:12` +`basicAuthUser = "zuperadmin"` is a hardcoded global; the API key default `"42"` +(`main.go:20`) is a weak, shipped default. Make the username configurable and avoid a +guessable default key (or require it to be set). + +--- + +## 🟢 Build / tooling / docs + +### 18. `go.mod` says `go 1.25.1` but Makefile builds with `golang:1.6.2` — `Makefile:11` +These are wildly out of sync. `golang:1.6.2` predates modules entirely and cannot build a +`go 1.25` module. The Dockerfile (`alpine:3.7`) is also from 2018 and has known CVEs. Update +to a current Go builder image and a maintained base (e.g. multi-stage build on `golang:1.25` ++ `alpine:3.20` or distroless). + +### 19. `release.sh` referenced but missing — `Makefile:19` +`make release` calls `./release.sh` which isn't in the repo. Either add it or drop the target. + +### 20. README is thin +No mention of the auth model, the `-apiKey`/`-password`/`-port`/`-apiDir` flags, the +`/docker` endpoint's risks, or the JSON contract (scripts must emit valid JSON or callers +get a 400). A short "Endpoints" and "Security" section would help a lot. + +### 21. Tests don't cover the risky paths +`main_test.go` is a good start but there are no tests for: POST script execution, the +`/docker` endpoint, invalid-JSON handling (a 400 case), or — most importantly — path +traversal. Add a test asserting `/api/../../something` is rejected; it will fail today and +guard the fix for item 1. + +### 22. Struct literal without field names — `main_test.go:19` +```go +var auth = &test.BasicAuth{"zuperadmin", "42"} +``` +`go vet` flags unkeyed struct literals. Use `&test.BasicAuth{Username: ..., Password: ...}`. + +--- + +## Summary + +The architecture is clean and the intent is clear. Priorities: + +1. **Fix path traversal** (item 1) — this is the one outright vulnerability that isn't + "by design." +2. **Gate / document `/docker`** (item 2) and **fix the swallowed POST errors** (item 7). +3. **Modernize the build** (item 18) so the project actually builds reproducibly. +4. Then the deduplication (item 12) and logging cleanup (item 13) for maintainability. diff --git a/example/api/_static/docker/ps.html b/example/api/_static/docker/ps.html deleted file mode 100644 index a09bcaf..0000000 --- a/example/api/_static/docker/ps.html +++ /dev/null @@ -1,39 +0,0 @@ - - - - - - apish - - - - - - -

docker ps

-
-
- - - - - - - - - - diff --git a/example/api/docker/ps.sh b/example/api/docker/ps.sh deleted file mode 100755 index 73bf764..0000000 --- a/example/api/docker/ps.sh +++ /dev/null @@ -1,8 +0,0 @@ -#!/bin/bash -set -eu - -docker ps \ - --format '{"id":"{{.ID}}", "name": "{{.Names}}", - "created_at": "{{.CreatedAt}}", "status": "{{.Status}}", - "ports": "{{.Ports}}", "size":"{{.Size}}"}' \ - | jq -s . \ No newline at end of file diff --git a/handlers/docker.go b/handlers/docker.go deleted file mode 100644 index fe02948..0000000 --- a/handlers/docker.go +++ /dev/null @@ -1,86 +0,0 @@ -package handlers - -import ( - "encoding/json" - "os/exec" - "strings" - - "github.com/sirupsen/logrus" - "github.com/gin-gonic/gin" -) - -// DockerRun makes possible the execution of any docker run command -func DockerRun(c *gin.Context) { - // Parse command in json body - var form struct { - Cmd string `json:"run"` - } - if err := c.BindJSON(&form); err != nil { - logrus.Error(err) - c.JSON(400, gin.H{"type": "error", "message": "Invalid docker run command"}) - return - } - - // Invalid empty command - if form.Cmd == "" { - c.JSON(400, gin.H{"type": "error", "message": "Docker run command empty"}) - return - } - - // Exec docker run - args := append([]string{"run"}, splitArgs(form.Cmd)...) - output, err := exec.Command("docker", args...).CombinedOutput() - if err != nil { - message := err.Error() + ": " + strings.Replace(string(output), "\n", " ", -1) - c.JSON(400, gin.H{"type": "error", "message": message}) - return - } - - // Try to unmarshal the output to format json - var obj interface{} - err = json.Unmarshal(output, &obj) - if err == nil { - c.JSON(200, obj) - return - } - - c.String(200, string(output)) -} - -// splitArgs splits a command string into arguments, honoring single and double -// quotes so that quoted arguments containing spaces stay intact and consecutive -// spaces don't produce empty arguments. -func splitArgs(s string) []string { - args := make([]string, 0) - var cur strings.Builder - var quote rune - inWord := false - - for _, r := range s { - switch { - case quote != 0: - if r == quote { - quote = 0 - } else { - cur.WriteRune(r) - } - inWord = true - case r == '\'' || r == '"': - quote = r - inWord = true - case r == ' ' || r == '\t': - if inWord { - args = append(args, cur.String()) - cur.Reset() - inWord = false - } - default: - cur.WriteRune(r) - inWord = true - } - } - if inWord { - args = append(args, cur.String()) - } - return args -} diff --git a/main.go b/main.go index 8d0e37e..548adbc 100644 --- a/main.go +++ b/main.go @@ -15,12 +15,11 @@ var ( gitCommit = "undefined" buildDate = "undefined" - port = flag.Int("port", 4242, "HTTP port to listen") - user = flag.String("user", "zuperadmin", "Username for basic auth") - password = flag.String("password", "", "Admin password for basic auth") - apiKey = flag.String("apiKey", "", "API key for X-Auth header auth (empty disables header auth)") - apiDir = flag.String("apiDir", "./api", "API directory (sh scripts and html pages)") - enableDocker = flag.Bool("enableDocker", false, "Enable the /docker endpoint (grants full host access via docker run)") + port = flag.Int("port", 4242, "HTTP port to listen") + user = flag.String("user", "zuperadmin", "Username for basic auth") + password = flag.String("password", "", "Admin password for basic auth") + apiKey = flag.String("apiKey", "", "API key for X-Auth header auth (empty disables header auth)") + apiDir = flag.String("apiDir", "./api", "API directory (sh scripts and html pages)") ) func ConfigRuntime() { diff --git a/router.go b/router.go index eb74009..e412002 100644 --- a/router.go +++ b/router.go @@ -46,12 +46,6 @@ func Router() *gin.Engine { authorized.GET("/api/*path", execHandler.ExecScript) authorized.POST("/api/*path", execHandler.PostExecScript) - // Arbitrary `docker run` execution — off by default as it grants full - // host access. Enable explicitly with -enableDocker. - if *enableDocker { - authorized.POST("/docker", h.DockerRun) - } - // Static files authorized.Static("/s/", *apiDir+"/_static") From 25b663a250b05d54cecd98f4b09ec41a753fa87b Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 22:05:23 +0200 Subject: [PATCH 24/36] refactor: remove redundant ConfigRuntime GOMAXPROCS already defaults to NumCPU since Go 1.5, so the manual call was a no-op. Drop the function and the runtime import. --- main.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/main.go b/main.go index 548adbc..aad28f6 100644 --- a/main.go +++ b/main.go @@ -4,7 +4,6 @@ import ( "flag" "fmt" "net/http" - "runtime" "time" "github.com/gin-gonic/gin" @@ -22,12 +21,6 @@ var ( apiDir = flag.String("apiDir", "./api", "API directory (sh scripts and html pages)") ) -func ConfigRuntime() { - nuCPU := runtime.NumCPU() - runtime.GOMAXPROCS(nuCPU) - logrus.Infof("Running with %d CPUs", nuCPU) -} - func StartGin() { start := time.Now() gin.SetMode(gin.ReleaseMode) @@ -53,6 +46,5 @@ func StartGin() { func main() { flag.Parse() - ConfigRuntime() StartGin() } From 17bbcbffb877b4e2b82ab7ab3ac1190dcb1af2e4 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 22:06:16 +0200 Subject: [PATCH 25/36] test: simplify HTTP test helpers Rename Get2 to GetWithKey with a plain string key, replace deprecated ioutil.ReadAll with io.ReadAll, make the unexported do() fail fast with t.Fatal, and close the response body. --- main_test.go | 4 +--- test/test.go | 39 ++++++++++++++++++--------------------- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/main_test.go b/main_test.go index b7218d4..ac18944 100644 --- a/main_test.go +++ b/main_test.go @@ -51,9 +51,7 @@ func TestAuthentication(t *testing.T) { status, _ = test.Get(t, "/api/time/date", auth) assert.Equal(t, 200, status, "should get a 200") - apiKey := new(string) - *apiKey = "42" - status, _ = test.Get2(t, "/api/time/date", apiKey) + status, _ = test.GetWithKey(t, "/api/time/date", "42") assert.Equal(t, 200, status, "should get a 200") } diff --git a/test/test.go b/test/test.go index 03a75ef..2c7149d 100644 --- a/test/test.go +++ b/test/test.go @@ -2,7 +2,7 @@ package test import ( "fmt" - "io/ioutil" + "io" "net/http" "strings" "testing" @@ -18,44 +18,41 @@ type BasicAuth struct { Password string } -func MakeHttp(t *testing.T, verb string, path string, json string, auth *BasicAuth, apiKey *string) (int, string) { - reader := strings.NewReader(json) - +func do(t *testing.T, verb, path, body string, auth *BasicAuth, apiKey string) (int, string) { url := fmt.Sprintf("%s%s%s", ServerURL, path, PrefixURL) - req, err := http.NewRequest(verb, url, reader) + req, err := http.NewRequest(verb, url, strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } if auth != nil { req.SetBasicAuth(auth.Username, auth.Password) } - if apiKey != nil { - req.Header.Set("X-Auth", *apiKey) + if apiKey != "" { + req.Header.Set("X-Auth", apiKey) } resp, err := http.DefaultClient.Do(req) if err != nil { - t.Error(err) + t.Fatal(err) } + defer resp.Body.Close() - if resp.Body == nil { - t.Error(err) - } - - body, err := ioutil.ReadAll(resp.Body) + out, err := io.ReadAll(resp.Body) if err != nil { - t.Error(err) + t.Fatal(err) } - - return resp.StatusCode, string(body) + return resp.StatusCode, string(out) } func Get(t *testing.T, path string, auth *BasicAuth) (int, string) { - return MakeHttp(t, "GET", path, "", auth, nil) + return do(t, "GET", path, "", auth, "") } -func Get2(t *testing.T, path string, apiKey *string) (int, string) { - return MakeHttp(t, "GET", path, "", nil, apiKey) +func GetWithKey(t *testing.T, path, apiKey string) (int, string) { + return do(t, "GET", path, "", nil, apiKey) } -func Post(t *testing.T, path string, json string, auth *BasicAuth) (int, string) { - return MakeHttp(t, "POST", path, json, auth, nil) +func Post(t *testing.T, path, body string, auth *BasicAuth) (int, string) { + return do(t, "POST", path, body, auth, "") } From c17a135f1df4e9adaf54d02fc6fc4e93007f1a9a Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 22:07:01 +0200 Subject: [PATCH 26/36] docker --- code-review.md | 193 ------------------------------------------------- 1 file changed, 193 deletions(-) delete mode 100644 code-review.md diff --git a/code-review.md b/code-review.md deleted file mode 100644 index a9bca53..0000000 --- a/code-review.md +++ /dev/null @@ -1,193 +0,0 @@ -# Code Review — go-apish - -Review date: 2026-06-02 - -`go-apish` is a small Gin-based HTTP server that exposes shell scripts (and static -files) as a REST API. The idea is neat and the codebase is small and readable. The -notes below are grouped by severity. The most important section is **Security** — by -its very nature this project runs arbitrary shell commands, so the attack surface -deserves the most care. - ---- - -## 🔴 Security (high priority) - -### 1. Path traversal → arbitrary script execution — `handlers/exec.go:23-24` -```go -path := c.Param("path") -script := fmt.Sprintf("%s%s%s", *h.ApiDir, path, ".sh") -``` -The wildcard `*path` is concatenated straight onto `ApiDir` with no validation. A -request such as `GET /api/../../../../tmp/evil` resolves to `./api/../../../../tmp/evil.sh`, -letting a caller execute any `.sh` file on the filesystem outside `apiDir`. Mitigate by: -- `filepath.Clean` the joined path and verify it still has `apiDir` as a prefix - (use `filepath.Abs` on both and check `strings.HasPrefix`), or -- reject any path containing `..`. - -This applies to both `ExecScript` and `PostExecScript` (duplicated logic). - -### 2. `/docker` runs arbitrary `docker run` — `handlers/docker.go:31-32` -```go -args := append([]string{"run"}, strings.Split(form.Cmd, " ")...) -output, err := exec.Command("docker", args...).CombinedOutput() -``` -Any authenticated caller can run any container with any flags -(`-v /:/host`, `--privileged`, `--pid=host`, …), which is effectively root on the -host. This may be intentional for the tool's purpose, but it should be: -- gated behind an explicit opt-in flag (off by default), and -- clearly documented as "grants full host access." - -Also note `strings.Split(cmd, " ")` breaks on quoted arguments and multiple spaces; -a real shell-style tokenizer (e.g. `shellwords`) would be more correct if this stays. - -### 3. CORS allows any origin *with* credentials — `middlewares/cors.go:8,13` -```go -c.Writer.Header().Set("Access-Control-Allow-Origin", "*") -c.Writer.Header().Set("Access-Control-Allow-Credentials", "true") -``` -`Allow-Origin: *` together with `Allow-Credentials: true` is rejected by browsers and -is a misconfiguration. If credentials are needed, reflect a vetted origin instead of -`*`. If not, drop the credentials header. For an unauthenticated, open API the wildcard -is fine — but then the basic-auth story below conflicts with it. - -### 4. Auth middleware comparisons are not constant-time — `middlewares/auth.go:12` -```go -if c.Request.Header.Get(AuthHeaderKey) == apiKey { -``` -The API-key check uses `==`, which is vulnerable to timing attacks. Use -`crypto/subtle.ConstantTimeCompare`. Minor for a hobby tool, but trivial to fix. - -### 5. Auth is opt-in — `router.go:26` -Authentication is only enabled when `-password` is set. The default is a fully open -server that can execute scripts. Consider failing closed (require a password, or print -a loud warning at startup when running unauthenticated). - -### 6. Example scripts demonstrate shell injection — `example/api/test/param.sh` -```sh -echo '{ "param": "'$1'" }' -``` -Because Go's `exec.Command` does not invoke a shell, the Go side is safe from command -injection — but `$1` unquoted/unescaped inside the script means a value like `"} ...` -breaks the JSON, and any script that does `eval`/backticks on `$1` would be exploitable. -Since these are the canonical examples users copy, they should model safe quoting -(e.g. build JSON with `jq -n --arg p "$1" '{param:$p}'`). - ---- - -## 🟠 Correctness / bugs - -### 7. `PostExecScript` ignores all execution errors — `handlers/exec.go:88-105` -```go -c1 := exec.Command(script) -... -_ = c1.Start() -_ = c1.Wait() -if err != nil { // err is still nil here — declared at top, never assigned -``` -The `err` checked on line 98 is the zero-value from line 71; the real errors from -`Start()`/`Wait()` are discarded with `_ =`. A script that fails will fall through and -likely produce an "Invalid JSON" 400 instead of a 500 with the real error. Capture and -check those errors. Also `c1.Stderr` is never set, so stderr is lost. - -### 8. `favicon` returns JSON `null` with 200 — `router.go:81-83` -`c.JSON(200, nil)` sends `null` as a favicon. Harmless but odd; returning `204 No Content` -would be cleaner. - -### 9. `indexExists` swallows non-NotExist errors — `router.go:60-67` -If `os.Stat` fails for a reason other than "not exist" (e.g. permissions), the function -returns `true` and a redirect is issued for a file that can't be read. Treat any error as -"not present." - -### 10. Server restart loop hides crashes — `main.go:46-48` -```go -for { - s.ListenAndServe() -} -``` -`ListenAndServe` only returns on error; the bare `for` loop silently restarts it (busy-loop -if the port is unavailable) and the returned error is never logged. Log the error and exit, -or restart with backoff. Also, the "API started" log on line 44 prints *before* -`ListenAndServe` is called, so it's measuring `Router()` build time, not real startup. - -### 11. `version` route is inside the authorized group — `router.go:36` -Minor: a version/health endpoint is usually fine to expose unauthenticated; currently it's -behind auth when a password is set, which complicates health checks. - ---- - -## 🟡 Code quality / maintainability - -### 12. `ExecScript` and `PostExecScript` are ~90% duplicated — `handlers/exec.go` -The path-building, existence check, JSON-unmarshal, and error responses are copy-pasted. -Extract a helper like `runScript(c, script, stdin io.Reader)` and a shared -`resolveScript(path) (string, bool)`. This also means the path-traversal fix (item 1) only -has to be made once. - -### 13. Inconsistent logging — `fmt.Printf` vs `log.Printf` vs `logrus` -The project uses three logging styles: `fmt.Printf` (`exec.go:31,82`), `log.Printf` -(`exec.go:49`), and `logrus` (`docker.go`). Pick one (logrus is already a dependency) and -use structured, leveled logging consistently. The `fmt.Printf` calls also omit trailing -`\n`. - -### 14. Resolving `*string` flags via pointers passed into handlers — `router.go:38-39` -`LsHandler{ApiDir: apiDir}` stores a `*string` to a global flag. It works, but passing the -resolved `string` value (after `flag.Parse`) is simpler and avoids handlers depending on -global mutable state — which is exactly what makes the test on `main_test.go:23` have to -poke `*apiDir` directly. - -### 15. `ListResources` walks `_static` twice — `handlers/ls.go:48-69` -Two `filepath.Walk` passes over the same `htmlDir`, one for `.html` and one for everything -else. A single walk with a branch would halve the I/O and the code. The `err` from the -first scripts walk (line 30) can never be non-nil because the walk func always returns -`nil` — so the error checks are dead code unless the walk func propagates `err`. - -### 16. `fileToUrl` hardcodes `http://` — `handlers/ls.go:89` -Generated URLs are always `http://`, so behind TLS/a proxy the listed links are wrong. -Derive the scheme from the request (`X-Forwarded-Proto` / `c.Request.TLS`). - -### 17. Magic values / globals — `router.go:12` -`basicAuthUser = "zuperadmin"` is a hardcoded global; the API key default `"42"` -(`main.go:20`) is a weak, shipped default. Make the username configurable and avoid a -guessable default key (or require it to be set). - ---- - -## 🟢 Build / tooling / docs - -### 18. `go.mod` says `go 1.25.1` but Makefile builds with `golang:1.6.2` — `Makefile:11` -These are wildly out of sync. `golang:1.6.2` predates modules entirely and cannot build a -`go 1.25` module. The Dockerfile (`alpine:3.7`) is also from 2018 and has known CVEs. Update -to a current Go builder image and a maintained base (e.g. multi-stage build on `golang:1.25` -+ `alpine:3.20` or distroless). - -### 19. `release.sh` referenced but missing — `Makefile:19` -`make release` calls `./release.sh` which isn't in the repo. Either add it or drop the target. - -### 20. README is thin -No mention of the auth model, the `-apiKey`/`-password`/`-port`/`-apiDir` flags, the -`/docker` endpoint's risks, or the JSON contract (scripts must emit valid JSON or callers -get a 400). A short "Endpoints" and "Security" section would help a lot. - -### 21. Tests don't cover the risky paths -`main_test.go` is a good start but there are no tests for: POST script execution, the -`/docker` endpoint, invalid-JSON handling (a 400 case), or — most importantly — path -traversal. Add a test asserting `/api/../../something` is rejected; it will fail today and -guard the fix for item 1. - -### 22. Struct literal without field names — `main_test.go:19` -```go -var auth = &test.BasicAuth{"zuperadmin", "42"} -``` -`go vet` flags unkeyed struct literals. Use `&test.BasicAuth{Username: ..., Password: ...}`. - ---- - -## Summary - -The architecture is clean and the intent is clear. Priorities: - -1. **Fix path traversal** (item 1) — this is the one outright vulnerability that isn't - "by design." -2. **Gate / document `/docker`** (item 2) and **fix the swallowed POST errors** (item 7). -3. **Modernize the build** (item 18) so the project actually builds reproducibly. -4. Then the deduplication (item 12) and logging cleanup (item 13) for maintainability. From 8489c7b115c20c472329eedebbd612273a914b25 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 22:07:32 +0200 Subject: [PATCH 27/36] refactor: drop favicon route and slim CORS headers Remove the /favicon.ico 204 convenience route (let it 404 like any other unknown path) and trim the CORS middleware to the origin, methods and headers this API actually uses. --- README.md | 4 ++-- main_test.go | 3 --- middlewares/cors.go | 15 ++++++--------- router.go | 5 ----- 4 files changed, 8 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 28de631..6cb2c9b 100644 --- a/README.md +++ b/README.md @@ -45,8 +45,8 @@ A script that exits non-zero yields `500` with its error (stderr is logged). ## Authentication -When `-password` is set, all endpoints except `/`, `/favicon.ico` and -`/version` require either: +When `-password` is set, all endpoints except `/` and `/version` require +either: - HTTP basic auth (`-user` / `-password`), or - an `X-Auth: ` header (when `-apiKey` is set). diff --git a/main_test.go b/main_test.go index ac18944..c59349a 100644 --- a/main_test.go +++ b/main_test.go @@ -36,9 +36,6 @@ func TestBase(t *testing.T) { status, _ = test.Get(t, "/version", nil) assert.Equal(t, 200, status, "should get a 200") - status, _ = test.Get(t, "/favicon.ico", nil) - assert.Equal(t, 204, status, "should get a 204") - status, _ = test.Get(t, "/blablabla", nil) assert.Equal(t, 404, status, "should get a 404") } diff --git a/middlewares/cors.go b/middlewares/cors.go index ef34997..b28cde3 100644 --- a/middlewares/cors.go +++ b/middlewares/cors.go @@ -4,15 +4,12 @@ import "github.com/gin-gonic/gin" func CORSMiddleware() gin.HandlerFunc { return func(c *gin.Context) { - // A wildcard origin must not be combined with credentials: browsers - // reject that pairing. This is an open API authenticated via the - // X-Auth header (allowed below), so credentials are not advertised. - domain := "*" - c.Writer.Header().Set("Access-Control-Allow-Origin", domain) - c.Writer.Header().Set("Access-Control-Max-Age", "86400") - c.Writer.Header().Set("Access-Control-Allow-Methods", "POST, GET, OPTIONS, PUT, DELETE, UPDATE") - c.Writer.Header().Set("Access-Control-Allow-Headers", "Origin, Content-Type, Content-Length, Accept-Encoding, X-CSRF-Token, X-Auth, Authorization") - c.Writer.Header().Set("Access-Control-Expose-Headers", "Content-Length") + // Open API: wildcard origin (no credentials, which browsers would + // reject alongside "*"). Only the methods and headers this API + // actually uses are advertised. + c.Writer.Header().Set("Access-Control-Allow-Origin", "*") + c.Writer.Header().Set("Access-Control-Allow-Methods", "GET, POST, OPTIONS") + c.Writer.Header().Set("Access-Control-Allow-Headers", "Content-Type, Authorization, X-Auth") if c.Request.Method == "OPTIONS" { c.AbortWithStatus(200) diff --git a/router.go b/router.go index e412002..fb2f8ff 100644 --- a/router.go +++ b/router.go @@ -17,7 +17,6 @@ func Router() *gin.Engine { // Default routes (no auth: useful for health checks) router.GET("/", index) - router.GET("/favicon.ico", favicon) router.GET("/version", version) // Authentication @@ -73,10 +72,6 @@ func index(c *gin.Context) { } } -func favicon(c *gin.Context) { - c.Status(http.StatusNoContent) -} - func version(c *gin.Context) { c.JSON(200, gin.H{ "git_commit": gitCommit, From 4541c2a0f1d0ba1f7c5a453290617f4dd7c00f0f Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 23:22:50 +0200 Subject: [PATCH 28/36] docs(example): remove inception self-calling demo Drop the most complex example script; the simpler date/param/post examples are enough to show the JSON contract. --- example/api/test/inception/get.sh | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100755 example/api/test/inception/get.sh diff --git a/example/api/test/inception/get.sh b/example/api/test/inception/get.sh deleted file mode 100755 index 2d31440..0000000 --- a/example/api/test/inception/get.sh +++ /dev/null @@ -1,18 +0,0 @@ -#!/bin/sh -set -eu - -c1="curl -s -m 1 172.17.42.1" -c2="curl -s -m 1 172.17.42.1/api/time/date" -c3='curl -s -m 1 -u zuperadmin:42 172.17.42.1/api/time/date' -c4='curl -s -m 1 -u zuperadmin:42 172.17.42.1/api/test/param?q=hello' - -w='{"status":"%{http_code}","time":"%{time_total}"}' - -slurp () { jq -s .; } - -echo '{ - "1": '$($c1 -w $w | slurp)', - "2": '$($c2 -w $w | slurp)', - "3": '$($c3 -w $w | slurp)', - "4": '$($c4 -w $w | slurp)' -}' From 9e54589eea95cb874da311b055344cff77e0a1ac Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 23:23:49 +0200 Subject: [PATCH 29/36] test: fold test/ helper package into main_test.go Inline the ~60-line HTTP helper package as unexported helpers in the single test file, dropping the separate package and the ServerURL/ PrefixURL globals (helpers use the test server directly). --- main_test.go | 92 +++++++++++++++++++++++++++++++++++----------------- test/test.go | 58 --------------------------------- 2 files changed, 62 insertions(+), 88 deletions(-) delete mode 100644 test/test.go diff --git a/main_test.go b/main_test.go index c59349a..0400d64 100644 --- a/main_test.go +++ b/main_test.go @@ -1,18 +1,19 @@ package main import ( + "io" + "net/http" "net/http/httptest" + "strings" "testing" "github.com/gin-gonic/gin" "github.com/stretchr/testify/assert" - - test "github.com/thbkrkr/go-apish/test" ) var server *httptest.Server -var auth = &test.BasicAuth{Username: "zuperadmin", Password: "42"} +var auth = &basicAuth{Username: "zuperadmin", Password: "42"} func init() { gin.SetMode(gin.TestMode) @@ -20,88 +21,119 @@ func init() { *password = "42" *apiKey = "42" server = httptest.NewServer(Router()) +} - test.ServerURL = server.URL +type basicAuth struct { + Username string + Password string } -func TestBase(t *testing.T) { - test.PrefixURL = "" +func do(t *testing.T, verb, path, body string, auth *basicAuth, apiKey string) (int, string) { + req, err := http.NewRequest(verb, server.URL+path, strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + if auth != nil { + req.SetBasicAuth(auth.Username, auth.Password) + } + if apiKey != "" { + req.Header.Set("X-Auth", apiKey) + } + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + + out, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatal(err) + } + return resp.StatusCode, string(out) +} + +func get(t *testing.T, path string, auth *basicAuth) (int, string) { + return do(t, "GET", path, "", auth, "") +} + +func getWithKey(t *testing.T, path, apiKey string) (int, string) { + return do(t, "GET", path, "", nil, apiKey) +} +func post(t *testing.T, path, body string, auth *basicAuth) (int, string) { + return do(t, "POST", path, body, auth, "") +} + +func TestBase(t *testing.T) { // / redirects to /s (index.html exists in the example), which is behind // auth, so the request must carry credentials to follow through to 200. - status, _ := test.Get(t, "/", auth) + status, _ := get(t, "/", auth) assert.Equal(t, 200, status, "should get a 200") // /version is exposed without auth. - status, _ = test.Get(t, "/version", nil) + status, _ = get(t, "/version", nil) assert.Equal(t, 200, status, "should get a 200") - status, _ = test.Get(t, "/blablabla", nil) + status, _ = get(t, "/blablabla", nil) assert.Equal(t, 404, status, "should get a 404") } func TestAuthentication(t *testing.T) { - status, _ := test.Get(t, "/api/time/date", nil) + status, _ := get(t, "/api/time/date", nil) assert.Equal(t, 401, status, "should get a 401") - auth := &test.BasicAuth{Username: "zuperadmin", Password: "42"} - status, _ = test.Get(t, "/api/time/date", auth) + status, _ = get(t, "/api/time/date", auth) assert.Equal(t, 200, status, "should get a 200") - status, _ = test.GetWithKey(t, "/api/time/date", "42") + status, _ = getWithKey(t, "/api/time/date", "42") assert.Equal(t, 200, status, "should get a 200") } func TestScripts(t *testing.T) { - status, _ := test.Get(t, "/api/nothing", auth) + status, _ := get(t, "/api/nothing", auth) assert.Equal(t, 404, status, "should get a 404") - status, _ = test.Get(t, "/api/time/date", auth) + status, _ = get(t, "/api/time/date", auth) assert.Equal(t, 200, status, "should get a 200") - status, _ = test.Get(t, "/api/test/param?q=hello", auth) - assert.Equal(t, 200, status, "should get a 200") - - status, _ = test.Get(t, "/api/time/date", auth) + status, _ = get(t, "/api/test/param?q=hello", auth) assert.Equal(t, 200, status, "should get a 200") } func TestPages(t *testing.T) { - status, _ := test.Get(t, "/s/", auth) - assert.Equal(t, 200, status, "should get a 200") - - status, _ = test.Get(t, "/s/date.html", auth) + status, _ := get(t, "/s/", auth) assert.Equal(t, 200, status, "should get a 200") - status, _ = test.Get(t, "/s/css/styles.css", auth) + status, _ = get(t, "/s/date.html", auth) assert.Equal(t, 200, status, "should get a 200") - status, _ = test.Get(t, "/s/css/styles.css", auth) + status, _ = get(t, "/s/css/styles.css", auth) assert.Equal(t, 200, status, "should get a 200") - status, _ = test.Get(t, "/s/js/script.js", auth) + status, _ = get(t, "/s/js/script.js", auth) assert.Equal(t, 200, status, "should get a 200") } func TestPost(t *testing.T) { - status, body := test.Post(t, "/api/test/post", `{"o": 42}`, auth) + status, body := post(t, "/api/test/post", `{"o": 42}`, auth) assert.Equal(t, 200, status, "should get a 200") assert.Contains(t, body, "jackpot") } func TestInvalidJSON(t *testing.T) { - status, _ := test.Get(t, "/api/test/invalid-json", auth) + status, _ := get(t, "/api/test/invalid-json", auth) assert.Equal(t, 400, status, "invalid JSON output should yield a 400") } func TestListResources(t *testing.T) { - status, body := test.Get(t, "/ls", auth) + status, body := get(t, "/ls", auth) assert.Equal(t, 200, status, "should get a 200") assert.Contains(t, body, "/api/time/date") } func TestPathTraversal(t *testing.T) { // Attempting to escape apiDir must not execute an arbitrary script. - status, _ := test.Get(t, "/api/../../../../etc/hostname", auth) + status, _ := get(t, "/api/../../../../etc/hostname", auth) assert.NotEqual(t, 200, status, "path traversal must be rejected") } diff --git a/test/test.go b/test/test.go deleted file mode 100644 index 2c7149d..0000000 --- a/test/test.go +++ /dev/null @@ -1,58 +0,0 @@ -package test - -import ( - "fmt" - "io" - "net/http" - "strings" - "testing" -) - -var ( - ServerURL string - PrefixURL string -) - -type BasicAuth struct { - Username string - Password string -} - -func do(t *testing.T, verb, path, body string, auth *BasicAuth, apiKey string) (int, string) { - url := fmt.Sprintf("%s%s%s", ServerURL, path, PrefixURL) - req, err := http.NewRequest(verb, url, strings.NewReader(body)) - if err != nil { - t.Fatal(err) - } - - if auth != nil { - req.SetBasicAuth(auth.Username, auth.Password) - } - if apiKey != "" { - req.Header.Set("X-Auth", apiKey) - } - - resp, err := http.DefaultClient.Do(req) - if err != nil { - t.Fatal(err) - } - defer resp.Body.Close() - - out, err := io.ReadAll(resp.Body) - if err != nil { - t.Fatal(err) - } - return resp.StatusCode, string(out) -} - -func Get(t *testing.T, path string, auth *BasicAuth) (int, string) { - return do(t, "GET", path, "", auth, "") -} - -func GetWithKey(t *testing.T, path, apiKey string) (int, string) { - return do(t, "GET", path, "", nil, apiKey) -} - -func Post(t *testing.T, path, body string, auth *BasicAuth) (int, string) { - return do(t, "POST", path, body, auth, "") -} From 72349c794a868484cf6abbc497b76933e18e3d18 Mon Sep 17 00:00:00 2001 From: Thibault Richard Date: Tue, 2 Jun 2026 23:24:27 +0200 Subject: [PATCH 30/36] docs(example): fix dead Ractive CDN URL in date.html cdn.ractivejs.org is defunct and the http:// URL was blocked as mixed content on HTTPS pages. Point to a pinned jsDelivr build over HTTPS. --- example/api/_static/date.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/example/api/_static/date.html b/example/api/_static/date.html index 729a8f5..098255d 100644 --- a/example/api/_static/date.html +++ b/example/api/_static/date.html @@ -22,7 +22,7 @@ - +