mirror of
https://github.com/yusing/godoxy.git
synced 2026-04-10 18:56:55 +02:00
fix(api): confine file edits to rooted config paths and restrict unauthenticated local API binds
Finish the file API traversal fix by rooting both GET and SET operations at the
actual file-type directory instead of the process working directory. This blocks
`..` escapes from `config/` and `config/middlewares/` while preserving valid
in-root reads and writes.
Also harden the optional unauthenticated local API listener so it only starts on
loopback addresses (`localhost`, `127.0.0.1`, `::1`). This preserves same-host
automation while preventing accidental exposure on wildcard, LAN, bridge, or
public interfaces.
Add regression tests for blocked traversal on GET and SET, valid in-root writes,
and loopback-only local API address validation. Fix an unrelated config test
cleanup panic so the touched package verification can run cleanly.
Constraint: `GODOXY_LOCAL_API_ADDR` is documented for local automation and must remain usable without adding a new auth flow
Constraint: File API behavior must keep valid config/provider/middleware edits working while blocking path escapes
Rejected: Mirror the previous GET `OpenInRoot(".", ...)` approach in SET | still allows escapes from `config/` to sibling paths under the working directory
Rejected: Keep unauthenticated non-loopback local API binds and document the risk | preserves a high-severity pre-auth network exposure
Confidence: high
Scope-risk: moderate
Reversibility: clean
Directive: Treat `LOCAL_API_ADDR` as same-host only; if non-loopback unauthenticated access is ever needed, gate it behind a separately named explicit insecure opt-in
Tested: `go test -count=1 -ldflags='-checklinkname=0' ./internal/api/v1/file -run 'Test(Get|Set)_PathTraversalBlocked' -v`
Tested: `go test -count=1 -ldflags='-checklinkname=0' ./internal/config -run '^TestValidateLocalAPIAddr$|^TestRouteValidateInboundMTLSProfile$' -v`
Tested: `go test -count=1 -ldflags='-checklinkname=0' ./internal/api/... ./internal/config/...`
Not-tested: End-to-end runtime verification of fsnotify reload behavior after a valid in-root provider edit
This commit is contained in:
@@ -115,7 +115,7 @@ No dedicated metrics exposed by handlers. Request metrics collected by middlewar
|
||||
|
||||
- All endpoints (except `/api/v1/version`) require authentication
|
||||
- Input validation using Gin binding tags
|
||||
- Path traversal prevention in file operations
|
||||
- File read/write handlers are rooted per file type (`config/` or `config/middlewares/`) to prevent traversal into sibling paths
|
||||
- WebSocket connections use same auth middleware as HTTP
|
||||
|
||||
## Failure Modes and Recovery
|
||||
@@ -195,5 +195,3 @@ func listContainers() ([]Container, error) {
|
||||
```bash
|
||||
curl http://localhost:8888/health
|
||||
```
|
||||
|
||||
)
|
||||
|
||||
@@ -45,7 +45,7 @@ func Get(c *gin.Context) {
|
||||
return
|
||||
}
|
||||
|
||||
f, err := os.OpenInRoot(".", request.FileType.GetPath(request.Filename))
|
||||
f, err := request.FileType.OpenFile(request.Filename, os.O_RDONLY, 0)
|
||||
if err != nil {
|
||||
c.Error(apitypes.InternalServerError(err, "failed to open root"))
|
||||
return
|
||||
@@ -73,9 +73,18 @@ func GetFileType(file string) FileType {
|
||||
return FileTypeProvider
|
||||
}
|
||||
|
||||
func (t FileType) GetPath(filename string) string {
|
||||
func (t FileType) RootPath() string {
|
||||
if t == FileTypeMiddleware {
|
||||
return path.Join(common.MiddlewareComposeBasePath, filename)
|
||||
return common.MiddlewareComposeBasePath
|
||||
}
|
||||
return path.Join(common.ConfigBasePath, filename)
|
||||
return common.ConfigBasePath
|
||||
}
|
||||
|
||||
func (t FileType) OpenFile(filename string, flag int, perm os.FileMode) (*os.File, error) {
|
||||
root, err := os.OpenRoot(t.RootPath())
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
defer root.Close()
|
||||
return root.OpenFile(filename, flag, perm)
|
||||
}
|
||||
|
||||
@@ -5,6 +5,7 @@ import (
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
|
||||
"github.com/gin-gonic/gin"
|
||||
@@ -12,25 +13,57 @@ import (
|
||||
"github.com/stretchr/testify/require"
|
||||
api "github.com/yusing/godoxy/internal/api"
|
||||
fileapi "github.com/yusing/godoxy/internal/api/v1/file"
|
||||
"github.com/yusing/goutils/fs"
|
||||
)
|
||||
|
||||
func TestGet_PathTraversalBlocked(t *testing.T) {
|
||||
func setupFileAPITestRoot(t *testing.T) string {
|
||||
t.Helper()
|
||||
|
||||
oldWD, err := os.Getwd()
|
||||
require.NoError(t, err)
|
||||
|
||||
root := t.TempDir()
|
||||
require.NoError(t, os.MkdirAll(filepath.Join(root, "config", "middlewares"), 0o755))
|
||||
require.NoError(t, os.Chdir(root))
|
||||
|
||||
t.Cleanup(func() {
|
||||
require.NoError(t, os.Chdir(oldWD))
|
||||
})
|
||||
|
||||
return root
|
||||
}
|
||||
|
||||
func newFileContentRouter() *gin.Engine {
|
||||
gin.SetMode(gin.TestMode)
|
||||
|
||||
files, err := fs.ListFiles("..", 1, false)
|
||||
require.NoError(t, err)
|
||||
|
||||
require.Greater(t, len(files), 0, "no files found")
|
||||
|
||||
relativePath := files[0]
|
||||
|
||||
fileContent, err := os.ReadFile(relativePath)
|
||||
require.NoError(t, err)
|
||||
|
||||
r := gin.New()
|
||||
r.Use(api.ErrorHandler())
|
||||
r.GET("/api/v1/file/content", fileapi.Get)
|
||||
r.PUT("/api/v1/file/content", fileapi.Set)
|
||||
return r
|
||||
}
|
||||
|
||||
func TestGet_PathTraversalBlocked(t *testing.T) {
|
||||
root := setupFileAPITestRoot(t)
|
||||
|
||||
const (
|
||||
insideFilename = "providers.yml"
|
||||
insideContent = "app: inside\n"
|
||||
outsideContent = "app: outside\n"
|
||||
)
|
||||
|
||||
require.NoError(t, os.WriteFile(filepath.Join(root, "config", insideFilename), []byte(insideContent), 0o644))
|
||||
require.NoError(t, os.WriteFile(filepath.Join(root, "secret.yml"), []byte(outsideContent), 0o644))
|
||||
|
||||
r := newFileContentRouter()
|
||||
|
||||
t.Run("read_in_root_file", func(t *testing.T) {
|
||||
req := httptest.NewRequest(http.MethodGet, "/api/v1/file/content?type=config&filename="+insideFilename, nil)
|
||||
w := httptest.NewRecorder()
|
||||
r.ServeHTTP(w, req)
|
||||
|
||||
assert.Equal(t, http.StatusOK, w.Code)
|
||||
assert.Equal(t, insideContent, w.Body.String())
|
||||
})
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
@@ -38,12 +71,12 @@ func TestGet_PathTraversalBlocked(t *testing.T) {
|
||||
queryEscaped bool
|
||||
}{
|
||||
{
|
||||
name: "dotdot_traversal",
|
||||
filename: relativePath,
|
||||
name: "dotdot_traversal_to_sibling_file",
|
||||
filename: "../secret.yml",
|
||||
},
|
||||
{
|
||||
name: "url_encoded_dotdot_traversal",
|
||||
filename: relativePath,
|
||||
name: "url_encoded_dotdot_traversal_to_sibling_file",
|
||||
filename: "../secret.yml",
|
||||
queryEscaped: true,
|
||||
},
|
||||
}
|
||||
@@ -62,7 +95,7 @@ func TestGet_PathTraversalBlocked(t *testing.T) {
|
||||
|
||||
// "Blocked" means we should never successfully read the outside file.
|
||||
assert.NotEqual(t, http.StatusOK, w.Code)
|
||||
assert.NotEqual(t, fileContent, w.Body.String())
|
||||
assert.NotEqual(t, outsideContent, w.Body.String())
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -43,7 +43,14 @@ func Set(c *gin.Context) {
|
||||
return
|
||||
}
|
||||
|
||||
err = os.WriteFile(request.FileType.GetPath(request.Filename), content, 0o644)
|
||||
f, err := request.FileType.OpenFile(request.Filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644)
|
||||
if err != nil {
|
||||
c.Error(apitypes.InternalServerError(err, "failed to open file"))
|
||||
return
|
||||
}
|
||||
defer f.Close()
|
||||
|
||||
_, err = f.Write(content)
|
||||
if err != nil {
|
||||
c.Error(apitypes.InternalServerError(err, "failed to write file"))
|
||||
return
|
||||
|
||||
87
internal/api/v1/file/set_test.go
Normal file
87
internal/api/v1/file/set_test.go
Normal file
@@ -0,0 +1,87 @@
|
||||
package fileapi_test
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
const validProviderYAML = `app:
|
||||
host: attacker.com
|
||||
port: 443
|
||||
scheme: https
|
||||
`
|
||||
|
||||
func TestSet_PathTraversalBlocked(t *testing.T) {
|
||||
root := setupFileAPITestRoot(t)
|
||||
r := newFileContentRouter()
|
||||
|
||||
t.Run("write_in_root_file", func(t *testing.T) {
|
||||
req := httptest.NewRequest(
|
||||
http.MethodPut,
|
||||
"/api/v1/file/content?type=provider&filename=providers.yml",
|
||||
strings.NewReader(validProviderYAML),
|
||||
)
|
||||
req.Header.Set("Content-Type", "text/plain")
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
r.ServeHTTP(w, req)
|
||||
|
||||
assert.Equal(t, http.StatusOK, w.Code)
|
||||
|
||||
content, err := os.ReadFile(filepath.Join(root, "config", "providers.yml"))
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, validProviderYAML, string(content))
|
||||
})
|
||||
|
||||
const originalContent = "do not overwrite\n"
|
||||
require.NoError(t, os.WriteFile(filepath.Join(root, "secret.yml"), []byte(originalContent), 0o644))
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
filename string
|
||||
queryEscaped bool
|
||||
}{
|
||||
{
|
||||
name: "dotdot_traversal_to_sibling_file",
|
||||
filename: "../secret.yml",
|
||||
},
|
||||
{
|
||||
name: "url_encoded_dotdot_traversal_to_sibling_file",
|
||||
filename: "../secret.yml",
|
||||
queryEscaped: true,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
filename := tt.filename
|
||||
if tt.queryEscaped {
|
||||
filename = url.QueryEscape(filename)
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(
|
||||
http.MethodPut,
|
||||
"/api/v1/file/content?type=provider&filename="+filename,
|
||||
strings.NewReader(validProviderYAML),
|
||||
)
|
||||
req.Header.Set("Content-Type", "text/plain")
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
r.ServeHTTP(w, req)
|
||||
|
||||
assert.NotEqual(t, http.StatusOK, w.Code)
|
||||
|
||||
content, err := os.ReadFile(filepath.Join(root, "secret.yml"))
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, originalContent, string(content))
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user