From b7c10a3e2f5c6557c9f6a3061f16ebce1fd2bbcc Mon Sep 17 00:00:00 2001
From: Takumi Sueda <puhitaku@gmail.com>
Date: Wed, 22 Mar 2023 02:02:30 +0900
Subject: [PATCH] Better logging / rename vars

---
 server/path.go   | 67 ++++++++++++++++++++++++++++++++++++++++++------
 server/server.go | 27 +++++++++++++------
 2 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/server/path.go b/server/path.go
index 15d149f..a944bf3 100644
--- a/server/path.go
+++ b/server/path.go
@@ -1,19 +1,59 @@
 package server
 
 import (
-	"errors"
-	"fmt"
 	"path/filepath"
 	"strings"
+
+	"github.com/rs/zerolog"
+)
+
+type InvalidPathError struct {
+	PathRequest string
+	PathAbs     string
+	PathEval    string
+	Reason      reason
+
+	BaseErr error
+}
+
+type reason string
+
+const (
+	AbsFail   reason = "failed to get the absolute path"
+	EvalFail  reason = "failed to evaluate symlinks"
+	OutOfRoot reason = "out of the root directory"
 )
 
-var invalidPathError = errors.New("invalid path, or not found")
+func (i InvalidPathError) Error() string {
+	// Prevent unexpected exposure of the actual reason, for security
+	return "invalid path, or not found"
+}
+
+func (i InvalidPathError) Unwrap() error {
+	return i.BaseErr
+}
+
+func (i InvalidPathError) MarshalZerologObject(e *zerolog.Event) {
+	e.Str("pathRequest", i.PathRequest)
+	if i.PathAbs != "" {
+		e.Str("pathAbs", i.PathAbs)
+	}
+	if i.PathEval != "" {
+		e.Str("pathEval", i.PathEval)
+	}
+	e.Str("reason", string(i.Reason))
+	e.AnErr("baseErr", i.BaseErr)
+}
 
-func evaluatePath(untrustedPath, trustedRoot string, evalSymlink bool) (string, error) {
-	joined := filepath.Join(trustedRoot, untrustedPath) // Join() also cleans the path
+func evaluatePath(requestPath, trustedRoot string, evalSymlink bool) (string, error) {
+	joined := filepath.Join(trustedRoot, requestPath) // Join() also cleans the path
 	abs, err := filepath.Abs(joined)
 	if err != nil {
-		return "", fmt.Errorf("failed to get the absolute path: %w", err)
+		return "", InvalidPathError{
+			PathRequest: requestPath,
+			Reason:      AbsFail,
+			BaseErr:     err,
+		}
 	}
 
 	evaluated := abs
@@ -21,13 +61,24 @@ func evaluatePath(untrustedPath, trustedRoot string, evalSymlink bool) (string,
 	if evalSymlink {
 		evaluated, err = filepath.EvalSymlinks(abs)
 		if err != nil {
-			return "", invalidPathError
+			return "", InvalidPathError{
+				PathRequest: requestPath,
+				PathAbs:     abs,
+				Reason:      EvalFail,
+				BaseErr:     err,
+			}
 		}
 	}
 
 	trusted := strings.HasPrefix(evaluated, trustedRoot)
 	if !trusted {
-		return "", invalidPathError
+		return "", InvalidPathError{
+			PathRequest: requestPath,
+			PathAbs:     abs,
+			PathEval:    evaluated,
+			Reason:      OutOfRoot,
+			BaseErr:     err,
+		}
 	}
 
 	return evaluated, nil
diff --git a/server/server.go b/server/server.go
index c2e6cff..59448d7 100644
--- a/server/server.go
+++ b/server/server.go
@@ -1,6 +1,7 @@
 package server
 
 import (
+	"errors"
 	"io"
 	"os"
 
@@ -15,13 +16,18 @@ func SetRoot(p string) {
 }
 
 // ReadHandler is called when client starts file download from server
-func ReadHandler(filename string, rf io.ReaderFrom) error {
+func ReadHandler(requestedPath string, rf io.ReaderFrom) error {
 	reqID := ulid.Make().String()
-	log.Info().Str("requestId", reqID).Msgf("read request: %s", filename)
+	log.Info().Str("requestId", reqID).Msgf("read request: %s", requestedPath)
 
-	evalPath, err := evaluatePath(filename, root, true)
+	evalPath, err := evaluatePath(requestedPath, root, true)
 	if err != nil {
-		log.Error().Str("requestId", reqID).Msgf("invalid path specified: %s", err)
+		perr := InvalidPathError{}
+		if errors.As(err, &perr) {
+			log.Error().Str("requestId", reqID).EmbedObject(perr).Msgf("failed to evaluate path")
+		} else {
+			log.Error().Str("requestId", reqID).Msgf("failed to evaluate path: %s", err)
+		}
 		return err
 	}
 
@@ -44,13 +50,18 @@ func ReadHandler(filename string, rf io.ReaderFrom) error {
 }
 
 // WriteHandler is called when client starts file upload to server
-func WriteHandler(filename string, wt io.WriterTo) error {
+func WriteHandler(requestedPath string, wt io.WriterTo) error {
 	reqID := ulid.Make().String()
-	log.Info().Str("requestId", reqID).Msgf("write request: %s", filename)
+	log.Info().Str("requestId", reqID).Msgf("write request: %s", requestedPath)
 
-	evalPath, err := evaluatePath(filename, root, false)
+	evalPath, err := evaluatePath(requestedPath, root, false)
 	if err != nil {
-		log.Error().Str("requestId", reqID).Msgf("invalid path specified: %s", err)
+		perr := InvalidPathError{}
+		if errors.As(err, &perr) {
+			log.Error().Str("requestId", reqID).EmbedObject(perr).Msgf("failed to evaluate path")
+		} else {
+			log.Error().Str("requestId", reqID).Msgf("failed to evaluate path: %s", err)
+		}
 		return err
 	}