From 222781abcaa2134c2db2857ed0cd1db0e894ad06 Mon Sep 17 00:00:00 2001 From: Austin Date: Mon, 12 Oct 2015 19:59:11 -0700 Subject: [PATCH 01/11] websocket refactored to use gorilla --- config/setup/websocket.go | 16 +- config/setup/websocket_test.go | 13 +- middleware/websocket/websocket.go | 220 ++++++++++++++++++++++++++++ middleware/websockets/websocket.go | 89 ----------- middleware/websockets/websockets.go | 60 -------- 5 files changed, 235 insertions(+), 163 deletions(-) create mode 100644 middleware/websocket/websocket.go delete mode 100644 middleware/websockets/websocket.go delete mode 100644 middleware/websockets/websockets.go diff --git a/config/setup/websocket.go b/config/setup/websocket.go index 9178bd13..33df76d7 100644 --- a/config/setup/websocket.go +++ b/config/setup/websocket.go @@ -2,26 +2,26 @@ package setup import ( "github.com/mholt/caddy/middleware" - "github.com/mholt/caddy/middleware/websockets" + "github.com/mholt/caddy/middleware/websocket" ) -// WebSocket configures a new WebSockets middleware instance. +// WebSocket configures a new WebSocket middleware instance. func WebSocket(c *Controller) (middleware.Middleware, error) { websocks, err := webSocketParse(c) if err != nil { return nil, err } - websockets.GatewayInterface = c.AppName + "-CGI/1.1" - websockets.ServerSoftware = c.AppName + "/" + c.AppVersion + websocket.GatewayInterface = c.AppName + "-CGI/1.1" + websocket.ServerSoftware = c.AppName + "/" + c.AppVersion return func(next middleware.Handler) middleware.Handler { - return websockets.WebSockets{Next: next, Sockets: websocks} + return websocket.WebSocket{Next: next, Sockets: websocks} }, nil } -func webSocketParse(c *Controller) ([]websockets.Config, error) { - var websocks []websockets.Config +func webSocketParse(c *Controller) ([]websocket.Config, error) { + var websocks []websocket.Config var respawn bool optionalBlock := func() (hadBlock bool, err error) { @@ -74,7 +74,7 @@ func webSocketParse(c *Controller) ([]websockets.Config, error) { return nil, err } - websocks = append(websocks, websockets.Config{ + websocks = append(websocks, websocket.Config{ Path: path, Command: cmd, Arguments: args, diff --git a/config/setup/websocket_test.go b/config/setup/websocket_test.go index 86af253d..750f2a1d 100644 --- a/config/setup/websocket_test.go +++ b/config/setup/websocket_test.go @@ -1,8 +1,9 @@ package setup import ( - "github.com/mholt/caddy/middleware/websockets" "testing" + + "github.com/mholt/caddy/middleware/websocket" ) func TestWebSocket(t *testing.T) { @@ -20,10 +21,10 @@ func TestWebSocket(t *testing.T) { } handler := mid(EmptyNext) - myHandler, ok := handler.(websockets.WebSockets) + myHandler, ok := handler.(websocket.WebSocket) if !ok { - t.Fatalf("Expected handler to be type WebSockets, got: %#v", handler) + t.Fatalf("Expected handler to be type WebSocket, got: %#v", handler) } if myHandler.Sockets[0].Path != "/" { @@ -38,15 +39,15 @@ func TestWebSocketParse(t *testing.T) { tests := []struct { inputWebSocketConfig string shouldErr bool - expectedWebSocketConfig []websockets.Config + expectedWebSocketConfig []websocket.Config }{ - {`websocket /api1 cat`, false, []websockets.Config{{ + {`websocket /api1 cat`, false, []websocket.Config{{ Path: "/api1", Command: "cat", }}}, {`websocket /api3 cat - websocket /api4 cat `, false, []websockets.Config{{ + websocket /api4 cat `, false, []websocket.Config{{ Path: "/api3", Command: "cat", }, { diff --git a/middleware/websocket/websocket.go b/middleware/websocket/websocket.go new file mode 100644 index 00000000..9f3ffe14 --- /dev/null +++ b/middleware/websocket/websocket.go @@ -0,0 +1,220 @@ +// Package websocket implements a WebSocket server by executing +// a command and piping its input and output through the WebSocket +// connection. +package websocket + +import ( + "io" + "net" + "net/http" + "os/exec" + "strings" + "time" + + "github.com/gorilla/websocket" + "github.com/mholt/caddy/middleware" +) + +const ( + // Time allowed to write a message to the peer. + writeWait = 10 * time.Second + + // Time allowed to read the next pong message from the peer. + pongWait = 60 * time.Second + + // Send pings to peer with this period. Must be less than pongWait. + pingPeriod = (pongWait * 9) / 10 + + // Maximum message size allowed from peer. + maxMessageSize = 1024 * 1024 * 10 // 10 MB default. +) + +var ( + // GatewayInterface is the dialect of CGI being used by the server + // to communicate with the script. See CGI spec, 4.1.4 + GatewayInterface string + + // ServerSoftware is the name and version of the information server + // software making the CGI request. See CGI spec, 4.1.17 + ServerSoftware string +) + +type ( + // WebSocket is a type that holds configuration for the + // websocket middleware generally, like a list of all the + // websocket endpoints. + WebSocket struct { + // Next is the next HTTP handler in the chain for when the path doesn't match + Next middleware.Handler + + // Sockets holds all the web socket endpoint configurations + Sockets []Config + } + + // Config holds the configuration for a single websocket + // endpoint which may serve multiple websocket connections. + Config struct { + Path string + Command string + Arguments []string + Respawn bool // TODO: Not used, but parser supports it until we decide on it + } +) + +// ServeHTTP converts the HTTP request to a WebSocket connection and serves it up. +func (ws WebSocket) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { + for _, sockconfig := range ws.Sockets { + if middleware.Path(r.URL.Path).Matches(sockconfig.Path) { + return serveWS(w, r, &sockconfig) + } + } + + // Didn't match a websocket path, so pass-thru + return ws.Next.ServeHTTP(w, r) +} + +// serveWS is used for setting and upgrading the HTTP connection to a websocket connection. +// It also spawns the child process that is associated with matched HTTP path/url. +func serveWS(w http.ResponseWriter, r *http.Request, config *Config) (int, error) { + upgrader := websocket.Upgrader{ + ReadBufferSize: 1024, + WriteBufferSize: 1024, + CheckOrigin: func(r *http.Request) bool { return true }, + } + conn, err := upgrader.Upgrade(w, r, nil) + if err != nil { + return 0, err + } + defer conn.Close() + + cmd := exec.Command(config.Command, config.Arguments...) + stdout, err := cmd.StdoutPipe() + if err != nil { + panic(err) // TODO + } + + stdin, err := cmd.StdinPipe() + if err != nil { + panic(err) // TODO + } + + metavars, err := buildEnv(cmd.Path, r) + if err != nil { + panic(err) // TODO + } + + cmd.Env = metavars + + if err := cmd.Start(); err != nil { + panic(err) + } + + reader(conn, stdout, stdin) + + return 0, nil // we shouldn't get here. +} + +// buildEnv creates the meta-variables for the child process according +// to the CGI 1.1 specification: http://tools.ietf.org/html/rfc3875#section-4.1 +// cmdPath should be the path of the command being run. +// The returned string slice can be set to the command's Env property. +func buildEnv(cmdPath string, r *http.Request) (metavars []string, err error) { + remoteHost, remotePort, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + return + } + + serverHost, serverPort, err := net.SplitHostPort(r.Host) + if err != nil { + return + } + + metavars = []string{ + `AUTH_TYPE=`, // Not used + `CONTENT_LENGTH=`, // Not used + `CONTENT_TYPE=`, // Not used + `GATEWAY_INTERFACE=` + GatewayInterface, + `PATH_INFO=`, // TODO + `PATH_TRANSLATED=`, // TODO + `QUERY_STRING=` + r.URL.RawQuery, + `REMOTE_ADDR=` + remoteHost, + `REMOTE_HOST=` + remoteHost, // Host lookups are slow - don't do them + `REMOTE_IDENT=`, // Not used + `REMOTE_PORT=` + remotePort, + `REMOTE_USER=`, // Not used, + `REQUEST_METHOD=` + r.Method, + `REQUEST_URI=` + r.RequestURI, + `SCRIPT_NAME=` + cmdPath, // path of the program being executed + `SERVER_NAME=` + serverHost, + `SERVER_PORT=` + serverPort, + `SERVER_PROTOCOL=` + r.Proto, + `SERVER_SOFTWARE=` + ServerSoftware, + } + + // Add each HTTP header to the environment as well + for header, values := range r.Header { + value := strings.Join(values, ", ") + header = strings.ToUpper(header) + header = strings.Replace(header, "-", "_", -1) + value = strings.Replace(value, "\n", " ", -1) + metavars = append(metavars, "HTTP_"+header+"="+value) + } + + return +} + +// reader is the guts of this package. It takes the stdin and stdout pipes +// of the cmd we created in ServeWS and pipes them between the client and server +// over websockets. +func reader(conn *websocket.Conn, stdout io.ReadCloser, stdin io.WriteCloser) { + // Setup our connection's websocket ping/pong handlers from our const values. + conn.SetReadLimit(maxMessageSize) + conn.SetReadDeadline(time.Now().Add(pongWait)) + conn.SetPongHandler(func(string) error { conn.SetReadDeadline(time.Now().Add(pongWait)); return nil }) + go ticker(conn) + + for { + msgType, r, err := conn.NextReader() + if err != nil { + if msgType == -1 { + return // we are done, as we got a close method. + } + panic(err) // TODO do something else here. + } + + w, err := conn.NextWriter(msgType) + if err != nil { + panic(err) // TODO do something else here. + } + + if _, err := io.Copy(stdin, r); err != nil { + panic(err) // TODO do something else here. + } + + go func() { + if _, err := io.Copy(w, stdout); err != nil { + panic(err) // TODO do something else here. + } + if err := w.Close(); err != nil { + panic(err) // TODO do something else here. + } + }() + } +} + +// ticker is start by the reader. Basically it is the method that simulates the websocket +// between the server and client to keep it alive with ping messages. +func ticker(conn *websocket.Conn) { + ticker := time.NewTicker(pingPeriod) + defer func() { + ticker.Stop() + conn.WriteMessage(websocket.CloseMessage, nil) + }() + + for { // blocking loop with select to wait for stimulation. + select { + case <-ticker.C: + conn.WriteMessage(websocket.PingMessage, nil) + } + } +} diff --git a/middleware/websockets/websocket.go b/middleware/websockets/websocket.go deleted file mode 100644 index 4c843f05..00000000 --- a/middleware/websockets/websocket.go +++ /dev/null @@ -1,89 +0,0 @@ -package websockets - -import ( - "net" - "net/http" - "os/exec" - "strings" - - "golang.org/x/net/websocket" -) - -// WebSocket represents a web socket server instance. A WebSocket -// is instantiated for each new websocket request/connection. -type WebSocket struct { - Config - *http.Request -} - -// Handle handles a WebSocket connection. It launches the -// specified command and streams input and output through -// the command's stdin and stdout. -func (ws WebSocket) Handle(conn *websocket.Conn) { - cmd := exec.Command(ws.Command, ws.Arguments...) - - cmd.Stdin = conn - cmd.Stdout = conn - cmd.Stderr = conn // TODO: Make this configurable from the Caddyfile - - metavars, err := ws.buildEnv(cmd.Path) - if err != nil { - panic(err) // TODO - } - - cmd.Env = metavars - - err = cmd.Run() - if err != nil { - panic(err) - } -} - -// buildEnv creates the meta-variables for the child process according -// to the CGI 1.1 specification: http://tools.ietf.org/html/rfc3875#section-4.1 -// cmdPath should be the path of the command being run. -// The returned string slice can be set to the command's Env property. -func (ws WebSocket) buildEnv(cmdPath string) (metavars []string, err error) { - remoteHost, remotePort, err := net.SplitHostPort(ws.RemoteAddr) - if err != nil { - return - } - - serverHost, serverPort, err := net.SplitHostPort(ws.Host) - if err != nil { - return - } - - metavars = []string{ - `AUTH_TYPE=`, // Not used - `CONTENT_LENGTH=`, // Not used - `CONTENT_TYPE=`, // Not used - `GATEWAY_INTERFACE=` + GatewayInterface, - `PATH_INFO=`, // TODO - `PATH_TRANSLATED=`, // TODO - `QUERY_STRING=` + ws.URL.RawQuery, - `REMOTE_ADDR=` + remoteHost, - `REMOTE_HOST=` + remoteHost, // Host lookups are slow - don't do them - `REMOTE_IDENT=`, // Not used - `REMOTE_PORT=` + remotePort, - `REMOTE_USER=`, // Not used, - `REQUEST_METHOD=` + ws.Method, - `REQUEST_URI=` + ws.RequestURI, - `SCRIPT_NAME=` + cmdPath, // path of the program being executed - `SERVER_NAME=` + serverHost, - `SERVER_PORT=` + serverPort, - `SERVER_PROTOCOL=` + ws.Proto, - `SERVER_SOFTWARE=` + ServerSoftware, - } - - // Add each HTTP header to the environment as well - for header, values := range ws.Header { - value := strings.Join(values, ", ") - header = strings.ToUpper(header) - header = strings.Replace(header, "-", "_", -1) - value = strings.Replace(value, "\n", " ", -1) - metavars = append(metavars, "HTTP_"+header+"="+value) - } - - return -} diff --git a/middleware/websockets/websockets.go b/middleware/websockets/websockets.go deleted file mode 100644 index 81e40510..00000000 --- a/middleware/websockets/websockets.go +++ /dev/null @@ -1,60 +0,0 @@ -// Package websockets implements a WebSocket server by executing -// a command and piping its input and output through the WebSocket -// connection. -package websockets - -import ( - "net/http" - - "github.com/mholt/caddy/middleware" - "golang.org/x/net/websocket" -) - -type ( - // WebSockets is a type that holds configuration for the - // websocket middleware generally, like a list of all the - // websocket endpoints. - WebSockets struct { - // Next is the next HTTP handler in the chain for when the path doesn't match - Next middleware.Handler - - // Sockets holds all the web socket endpoint configurations - Sockets []Config - } - - // Config holds the configuration for a single websocket - // endpoint which may serve multiple websocket connections. - Config struct { - Path string - Command string - Arguments []string - Respawn bool // TODO: Not used, but parser supports it until we decide on it - } -) - -// ServeHTTP converts the HTTP request to a WebSocket connection and serves it up. -func (ws WebSockets) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { - for _, sockconfig := range ws.Sockets { - if middleware.Path(r.URL.Path).Matches(sockconfig.Path) { - socket := WebSocket{ - Config: sockconfig, - Request: r, - } - websocket.Handler(socket.Handle).ServeHTTP(w, r) - return 0, nil - } - } - - // Didn't match a websocket path, so pass-thru - return ws.Next.ServeHTTP(w, r) -} - -var ( - // GatewayInterface is the dialect of CGI being used by the server - // to communicate with the script. See CGI spec, 4.1.4 - GatewayInterface string - - // ServerSoftware is the name and version of the information server - // software making the CGI request. See CGI spec, 4.1.17 - ServerSoftware string -) From 4544dabd56483382dd973457e746adc872694481 Mon Sep 17 00:00:00 2001 From: makpoc Date: Tue, 13 Oct 2015 14:39:18 +0300 Subject: [PATCH 02/11] Add tests for command splitting --- middleware/commands_test.go | 138 ++++++++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) create mode 100644 middleware/commands_test.go diff --git a/middleware/commands_test.go b/middleware/commands_test.go new file mode 100644 index 00000000..3a5b3334 --- /dev/null +++ b/middleware/commands_test.go @@ -0,0 +1,138 @@ +package middleware + +import ( + "fmt" + "strings" + "testing" +) + +func TestSplitCommandAndArgs(t *testing.T) { + var parseErrorContent = "error parsing command:" + var noCommandErrContent = "no command contained in" + + tests := []struct { + input string + expectedCommand string + expectedArgs []string + expectedErrContent string + }{ + // Test case 0 - emtpy command + { + input: ``, + expectedCommand: ``, + expectedArgs: nil, + expectedErrContent: noCommandErrContent, + }, + // Test case 1 - command without arguments + { + input: `command`, + expectedCommand: `command`, + expectedArgs: nil, + expectedErrContent: ``, + }, + // Test case 2 - command with single argument + { + input: `command arg1`, + expectedCommand: `command`, + expectedArgs: []string{`arg1`}, + expectedErrContent: ``, + }, + // Test case 3 - command with multiple arguments + { + input: `command arg1 arg2`, + expectedCommand: `command`, + expectedArgs: []string{`arg1`, `arg2`}, + expectedErrContent: ``, + }, + // Test case 4 - command with single argument with space character - in quotes + { + input: `command "arg1 arg1"`, + expectedCommand: `command`, + expectedArgs: []string{`arg1 arg1`}, + expectedErrContent: ``, + }, + // Test case 4 - command with single argument with space character - escaped + { + input: `command arg1\ arg1`, + expectedCommand: `command`, + expectedArgs: []string{`arg1 arg1`}, + expectedErrContent: ``, + }, + // Test case 6 - command with escaped quote character + { + input: `command "arg1 \" arg1"`, + expectedCommand: `command`, + expectedArgs: []string{`arg1 " arg1`}, + expectedErrContent: ``, + }, + // Test case 7 - command with escaped backslash + { + input: `command '\arg1'`, + expectedCommand: `command`, + expectedArgs: []string{`\arg1`}, + expectedErrContent: ``, + }, + // Test case 8 - command with comments + { + input: `command arg1 #comment1 comment2`, + expectedCommand: `command`, + expectedArgs: []string{`arg1`}, + expectedErrContent: "", + }, + // Test case 9 - command with multiple spaces and tab character + { + input: "command arg1 arg2\targ3", + expectedCommand: `command`, + expectedArgs: []string{`arg1`, `arg2`, "arg3"}, + expectedErrContent: "", + }, + // Test case 10 - command with unclosed quotes + { + input: `command "arg1 arg2`, + expectedCommand: "", + expectedArgs: nil, + expectedErrContent: parseErrorContent, + }, + // Test case 11 - command with unclosed quotes + { + input: `command 'arg1 arg2"`, + expectedCommand: "", + expectedArgs: nil, + expectedErrContent: parseErrorContent, + }, + } + + for i, test := range tests { + errorPrefix := fmt.Sprintf("Test [%d]: ", i) + errorSuffix := fmt.Sprintf(" Command to parse: [%s]", test.input) + actualCommand, actualArgs, actualErr := SplitCommandAndArgs(test.input) + + // test if error matches expectation + if test.expectedErrContent != "" { + if actualErr == nil { + t.Errorf(errorPrefix+"Expected error with content [%s], found no error."+errorSuffix, test.expectedErrContent) + } else if !strings.Contains(actualErr.Error(), test.expectedErrContent) { + t.Errorf(errorPrefix+"Expected error with content [%s], found [%v]."+errorSuffix, test.expectedErrContent, actualErr) + } + } else if actualErr != nil { + t.Errorf(errorPrefix+"Expected no error, found [%v]."+errorSuffix, actualErr) + } + + // test if command matches + if test.expectedCommand != actualCommand { + t.Errorf("Expected command: [%s], actual: [%s]."+errorSuffix, test.expectedCommand, actualCommand) + } + + // test if arguments match + if len(test.expectedArgs) != len(actualArgs) { + t.Errorf("Wrong number of arguments! Expected [%v], actual [%v]."+errorSuffix, test.expectedArgs, actualArgs) + } + + for j, actualArg := range actualArgs { + expectedArg := test.expectedArgs[j] + if actualArg != expectedArg { + t.Errorf(errorPrefix+"Argument at position [%d] differ! Expected [%s], actual [%s]"+errorSuffix, j, expectedArg, actualArg) + } + } + } +} From 6717edcb87c632d82695a77e9046501f21416d2d Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Tue, 13 Oct 2015 09:52:47 -0600 Subject: [PATCH 03/11] Add AppVeyor badge Distinguish between windows and linux builds --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 44fd023b..917e094c 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ [![Caddy](https://caddyserver.com/resources/images/caddy-boxed.png)](https://caddyserver.com) -[![Documentation](https://img.shields.io/badge/godoc-reference-blue.svg?style=flat-square)](https://godoc.org/github.com/mholt/caddy) [![Build Status](https://img.shields.io/travis/mholt/caddy.svg?style=flat-square)](https://travis-ci.org/mholt/caddy) +[![Documentation](https://img.shields.io/badge/godoc-reference-blue.svg?style=flat-square)](https://godoc.org/github.com/mholt/caddy) [![Linux Build Status](https://img.shields.io/travis/mholt/caddy.svg?style=flat-square&label=linux+build)](https://travis-ci.org/mholt/caddy) [![Windows Build Status](https://img.shields.io/appveyor/ci/mholt/caddy.svg?style=flat-square&label=windows+build)](https://ci.appveyor.com/project/mholt/caddy) Caddy is a lightweight, general-purpose web server for Windows, Mac, Linux, BSD, and [Android](https://github.com/mholt/caddy/wiki/Running-Caddy-on-Android). It is a capable alternative to other popular and easy to use web servers. From f122b3bbdf6159ec6c87a435cac3bce381f30c3e Mon Sep 17 00:00:00 2001 From: Makpoc Date: Tue, 13 Oct 2015 23:35:24 +0300 Subject: [PATCH 04/11] Fix failing test (windows) - simulate an error by executing stat on a filename with zero-byte in it. Fix cleanup of created files after the tests. --- config/setup/root_test.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/config/setup/root_test.go b/config/setup/root_test.go index f34e05d2..ff3dad3f 100644 --- a/config/setup/root_test.go +++ b/config/setup/root_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strings" "testing" + "runtime" ) func TestRoot(t *testing.T) { @@ -26,10 +27,13 @@ func TestRoot(t *testing.T) { if err != nil { t.Fatalf("BeforeTest: Failed to create temp file for testing! Error was: %v", err) } - defer os.Remove(existingFile.Name()) - - unaccessiblePath := filepath.Join(existingFile.Name(), "some_name") + defer func () { + existingFile.Close() + os.Remove(existingFile.Name()) + }() + unaccessiblePath := getInaccessibleOsDependentPath(existingFile.Name()) + tests := []struct { input string shouldErr bool @@ -60,8 +64,9 @@ func TestRoot(t *testing.T) { for i, test := range tests { c := NewTestController(test.input) mid, err := Root(c) + if test.shouldErr && err == nil { - t.Errorf("Test %d: Expected error but found nil for input %s", i, test.input) + t.Errorf("Test %d: Expected error but found %s for input %s", i, err, test.input) } if err != nil { @@ -97,3 +102,12 @@ func getTempDirPath() (string, error) { return tempDir, nil } + +func getInaccessibleOsDependentPath(file string) string{ + if runtime.GOOS == "windows"{ + return filepath.Join("C:", "file\x00name")// 0 byte breaks the lstat syscall + } else { + // TODO - check if the zero-byte filename works for linux only. If it does - use it instead + return filepath.Join(file, "some_name") + } +} \ No newline at end of file From e158cda0570ae1565fe18e9ef92639ea27f3bd58 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Tue, 13 Oct 2015 19:49:53 -0400 Subject: [PATCH 05/11] Fix test failures on Windows. Most of the Windows test failures are due to the path separator not being "/". The general approach I took here was to keep paths in "URL form" (ie using "/" separators) as much as possible, and only convert to native paths when we attempt to open a file. This will allow the most consistency between different host OS. For example, data structures that store paths still store them with "/" delimiters. Functions that accepted paths as input and return them as outputs still use "/". There are still a few test failures that need to be sorted out. - config/setup/TestRoot (I hear this has already been fixed by someone else) - middleware/basicauth/TestBrowseTemplate and middleware/templates/Test (a line endings issue that I'm still working through) --- config/setup/controller.go | 4 +++- config/setup/errors.go | 4 ++-- config/setup/markdown.go | 4 ++-- config/setup/markdown_test.go | 10 ++++++---- middleware/basicauth/basicauth.go | 1 + middleware/basicauth/basicauth_test.go | 8 ++++++-- middleware/markdown/generator.go | 3 ++- middleware/markdown/page.go | 2 +- middleware/markdown/process.go | 7 +++++-- middleware/middleware.go | 11 +++++++++-- middleware/middleware_test.go | 9 +++++++-- 11 files changed, 44 insertions(+), 19 deletions(-) diff --git a/config/setup/controller.go b/config/setup/controller.go index 0399ab81..c917a148 100644 --- a/config/setup/controller.go +++ b/config/setup/controller.go @@ -24,7 +24,9 @@ type Controller struct { // add-ons can use this as a convenience. func NewTestController(input string) *Controller { return &Controller{ - Config: &server.Config{}, + Config: &server.Config{ + Root: ".", + }, Dispenser: parse.NewDispenser("Testfile", strings.NewReader(input)), } } diff --git a/config/setup/errors.go b/config/setup/errors.go index bc131976..5aa09b9f 100644 --- a/config/setup/errors.go +++ b/config/setup/errors.go @@ -5,7 +5,7 @@ import ( "io" "log" "os" - "path" + "path/filepath" "strconv" "github.com/hashicorp/go-syslog" @@ -105,7 +105,7 @@ func errorsParse(c *Controller) (*errors.ErrorHandler, error) { } } else { // Error page; ensure it exists - where = path.Join(c.Root, where) + where = filepath.Join(c.Root, where) f, err := os.Open(where) if err != nil { fmt.Println("Warning: Unable to open error page '" + where + "': " + err.Error()) diff --git a/config/setup/markdown.go b/config/setup/markdown.go index eb24a595..65344a74 100644 --- a/config/setup/markdown.go +++ b/config/setup/markdown.go @@ -114,11 +114,11 @@ func loadParams(c *Controller, mdc *markdown.Config) error { if _, ok := mdc.Templates[markdown.DefaultTemplate]; ok { return c.Err("only one default template is allowed, use alias.") } - fpath := filepath.Clean(c.Root + string(filepath.Separator) + tArgs[0]) + fpath := filepath.ToSlash(filepath.Clean(c.Root + string(filepath.Separator) + tArgs[0])) mdc.Templates[markdown.DefaultTemplate] = fpath return nil case 2: - fpath := filepath.Clean(c.Root + string(filepath.Separator) + tArgs[1]) + fpath := filepath.ToSlash(filepath.Clean(c.Root + string(filepath.Separator) + tArgs[1])) mdc.Templates[tArgs[0]] = fpath return nil default: diff --git a/config/setup/markdown_test.go b/config/setup/markdown_test.go index 1ada8a1f..61c5d0c1 100644 --- a/config/setup/markdown_test.go +++ b/config/setup/markdown_test.go @@ -1,6 +1,7 @@ package setup import ( + "bytes" "fmt" "io/ioutil" "net/http" @@ -92,7 +93,7 @@ func TestMarkdownStaticGen(t *testing.T) { t.Fatalf("An error occured when getting the file content: %v", err) } - expectedBody := ` + expectedBody := []byte(` first_post @@ -104,9 +105,10 @@ func TestMarkdownStaticGen(t *testing.T) { -` - if string(html) != expectedBody { - t.Fatalf("Expected file content: %v got: %v", expectedBody, html) +`) + // TODO: html contains Windows line endings, expectedBody doesn't... + if !bytes.Equal(html, expectedBody) { + //t.Fatalf("Expected file content: %s got: %s", string(expectedBody), string(html)) } fp := filepath.Join(c.Root, markdown.DefaultStaticDir) diff --git a/middleware/basicauth/basicauth.go b/middleware/basicauth/basicauth.go index 14e7d210..8cf8c4aa 100644 --- a/middleware/basicauth/basicauth.go +++ b/middleware/basicauth/basicauth.go @@ -87,6 +87,7 @@ var ( ) func GetHtpasswdMatcher(filename, username, siteRoot string) (PasswordMatcher, error) { + fmt.Printf("ZBZB joining '%s' and '%s' and trying to open\n", siteRoot, filename) filename = filepath.Join(siteRoot, filename) htpasswordsMu.Lock() if htpasswords == nil { diff --git a/middleware/basicauth/basicauth_test.go b/middleware/basicauth/basicauth_test.go index aa1fc244..aad5ed39 100644 --- a/middleware/basicauth/basicauth_test.go +++ b/middleware/basicauth/basicauth_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "os" + "path/filepath" "testing" "github.com/mholt/caddy/middleware" @@ -124,15 +125,18 @@ md5:$apr1$l42y8rex$pOA2VJ0x/0TwaFeAF9nX61` t.Skipf("Error creating temp file (%v), will skip htpassword test") return } + defer os.Remove(htfh.Name()) if _, err = htfh.Write([]byte(htpasswdFile)); err != nil { t.Fatalf("write htpasswd file %q: %v", htfh.Name(), err) } htfh.Close() - defer os.Remove(htfh.Name()) for i, username := range []string{"sha1", "md5"} { rule := Rule{Username: username, Resources: []string{"/testing"}} - if rule.Password, err = GetHtpasswdMatcher(htfh.Name(), rule.Username, "/"); err != nil { + + siteRoot := filepath.Dir(htfh.Name()) + filename := filepath.Base(htfh.Name()) + if rule.Password, err = GetHtpasswdMatcher(filename, rule.Username, siteRoot); err != nil { t.Fatalf("GetHtpasswdMatcher(%q, %q): %v", htfh.Name(), rule.Username, err) } t.Logf("%d. username=%q password=%v", i, rule.Username, rule.Password) diff --git a/middleware/markdown/generator.go b/middleware/markdown/generator.go index c1dfd67a..a02cb3b0 100644 --- a/middleware/markdown/generator.go +++ b/middleware/markdown/generator.go @@ -70,7 +70,7 @@ func generateLinks(md Markdown, cfg *Config) (bool, error) { return generated, g.lastErr } -// generateStaticFiles generates static html files from markdowns. +// generateStaticHTML generates static HTML files from markdowns. func generateStaticHTML(md Markdown, cfg *Config) error { // If generated site already exists, clear it out _, err := os.Stat(cfg.StaticDir) @@ -98,6 +98,7 @@ func generateStaticHTML(md Markdown, cfg *Config) error { if err != nil { return err } + reqPath = filepath.ToSlash(reqPath) reqPath = "/" + reqPath // Generate the static file diff --git a/middleware/markdown/page.go b/middleware/markdown/page.go index 2b7816ba..3185d8ec 100644 --- a/middleware/markdown/page.go +++ b/middleware/markdown/page.go @@ -116,7 +116,7 @@ func (l *linkGen) generateLinks(md Markdown, cfg *Config) bool { if err != nil { return err } - reqPath = "/" + reqPath + reqPath = "/" + filepath.ToSlash(reqPath) parser := findParser(body) if parser == nil { diff --git a/middleware/markdown/process.go b/middleware/markdown/process.go index 93fe5147..0fb48dba 100644 --- a/middleware/markdown/process.go +++ b/middleware/markdown/process.go @@ -134,7 +134,10 @@ func (md Markdown) generatePage(c *Config, requestPath string, content []byte) e } } - filePath := filepath.Join(c.StaticDir, requestPath) + // the URL will always use "/" as a path separator, + // convert that to a native path to support OS that + // use different path separators + filePath := filepath.Join(c.StaticDir, filepath.FromSlash(requestPath)) // If it is index file, use the directory instead if md.IsIndexFile(filepath.Base(requestPath)) { @@ -154,7 +157,7 @@ func (md Markdown) generatePage(c *Config, requestPath string, content []byte) e } c.Lock() - c.StaticFiles[requestPath] = filePath + c.StaticFiles[requestPath] = filepath.ToSlash(filePath) c.Unlock() } diff --git a/middleware/middleware.go b/middleware/middleware.go index 0310ba2f..ba7699ce 100644 --- a/middleware/middleware.go +++ b/middleware/middleware.go @@ -3,7 +3,7 @@ package middleware import ( "net/http" - "path/filepath" + "path" ) type ( @@ -57,12 +57,19 @@ func (f HandlerFunc) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, err // and false is returned. fpath must end in a forward slash '/' // otherwise no index files will be tried (directory paths must end // in a forward slash according to HTTP). +// +// All paths passed into and returned from this function use '/' as the +// path separator, just like URLs. IndexFle handles path manipulation +// internally for systems that use different path separators. func IndexFile(root http.FileSystem, fpath string, indexFiles []string) (string, bool) { if fpath[len(fpath)-1] != '/' || root == nil { return "", false } for _, indexFile := range indexFiles { - fp := filepath.Join(fpath, indexFile) + // func (http.FileSystem).Open wants all paths separated by "/", + // regardless of operating system convention, so use + // path.Join instead of filepath.Join + fp := path.Join(fpath, indexFile) f, err := root.Open(fp) if err == nil { f.Close() diff --git a/middleware/middleware_test.go b/middleware/middleware_test.go index e5b238e6..2bc211c0 100644 --- a/middleware/middleware_test.go +++ b/middleware/middleware_test.go @@ -1,6 +1,7 @@ package middleware import ( + "fmt" "net/http" "testing" ) @@ -15,13 +16,17 @@ func TestIndexfile(t *testing.T) { expectedBoolValue bool //return value }{ { - http.Dir("./templates/testdata"), "/images/", []string{"img.htm"}, + http.Dir("./templates/testdata"), + "/images/", + []string{"img.htm"}, false, - "/images/img.htm", true, + "/images/img.htm", + true, }, } for i, test := range tests { actualFilePath, actualBoolValue := IndexFile(test.rootDir, test.fpath, test.indexFiles) + fmt.Println("ZBZB IndexFile returned", actualFilePath, ",", actualBoolValue) if actualBoolValue == true && test.shouldErr { t.Errorf("Test %d didn't error, but it should have", i) } else if actualBoolValue != true && !test.shouldErr { From 16bd63fc26fd4d1db980ea5de26d19086407a37b Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Tue, 13 Oct 2015 20:04:34 -0400 Subject: [PATCH 06/11] Removed my debug prints --- middleware/basicauth/basicauth.go | 1 - middleware/middleware_test.go | 2 -- 2 files changed, 3 deletions(-) diff --git a/middleware/basicauth/basicauth.go b/middleware/basicauth/basicauth.go index 8cf8c4aa..14e7d210 100644 --- a/middleware/basicauth/basicauth.go +++ b/middleware/basicauth/basicauth.go @@ -87,7 +87,6 @@ var ( ) func GetHtpasswdMatcher(filename, username, siteRoot string) (PasswordMatcher, error) { - fmt.Printf("ZBZB joining '%s' and '%s' and trying to open\n", siteRoot, filename) filename = filepath.Join(siteRoot, filename) htpasswordsMu.Lock() if htpasswords == nil { diff --git a/middleware/middleware_test.go b/middleware/middleware_test.go index 2bc211c0..700beed8 100644 --- a/middleware/middleware_test.go +++ b/middleware/middleware_test.go @@ -1,7 +1,6 @@ package middleware import ( - "fmt" "net/http" "testing" ) @@ -26,7 +25,6 @@ func TestIndexfile(t *testing.T) { } for i, test := range tests { actualFilePath, actualBoolValue := IndexFile(test.rootDir, test.fpath, test.indexFiles) - fmt.Println("ZBZB IndexFile returned", actualFilePath, ",", actualBoolValue) if actualBoolValue == true && test.shouldErr { t.Errorf("Test %d didn't error, but it should have", i) } else if actualBoolValue != true && !test.shouldErr { From f7fcd7447a01a218b12d8ec1a8a41de8df97d143 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Tue, 13 Oct 2015 20:16:43 -0400 Subject: [PATCH 07/11] Fix test failure on non-Windows OS. NewTestController now sets the site root to '.' to accomodate Windows. This introduced a failure on Linux because we join "." and an absolute path in /tmp/ and end up looking for the temp file in the wrong place. This change puts the temp file under the current working directory, which should resolve the issue. --- config/setup/basicauth_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/setup/basicauth_test.go b/config/setup/basicauth_test.go index 7588f0f0..a94d6e69 100644 --- a/config/setup/basicauth_test.go +++ b/config/setup/basicauth_test.go @@ -38,7 +38,7 @@ func TestBasicAuthParse(t *testing.T) { md5:$apr1$l42y8rex$pOA2VJ0x/0TwaFeAF9nX61` var skipHtpassword bool - htfh, err := ioutil.TempFile("", "basicauth-") + htfh, err := ioutil.TempFile(".", "basicauth-") if err != nil { t.Logf("Error creating temp file (%v), will skip htpassword test", err) skipHtpassword = true From 26cbea9e1266a23e3fc4ba8b6f5ba34b78390fca Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Tue, 13 Oct 2015 20:23:05 -0400 Subject: [PATCH 08/11] Re-enable test I had commented out this check just to make sure the rest of the test cases were succeeding and forgot to add it back in. --- config/setup/markdown_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/setup/markdown_test.go b/config/setup/markdown_test.go index 61c5d0c1..5bf012b0 100644 --- a/config/setup/markdown_test.go +++ b/config/setup/markdown_test.go @@ -106,9 +106,9 @@ func TestMarkdownStaticGen(t *testing.T) { `) - // TODO: html contains Windows line endings, expectedBody doesn't... + if !bytes.Equal(html, expectedBody) { - //t.Fatalf("Expected file content: %s got: %s", string(expectedBody), string(html)) + t.Fatalf("Expected file content: %s got: %s", string(expectedBody), string(html)) } fp := filepath.Join(c.Root, markdown.DefaultStaticDir) From 24893bf740d2d80386c107c14f7580b6fffab3ec Mon Sep 17 00:00:00 2001 From: Austin Date: Tue, 13 Oct 2015 19:07:54 -0700 Subject: [PATCH 09/11] removed panics, cleaned up leaking ticker routine --- middleware/websocket/websocket.go | 39 +++++++++++++++++++------------ 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/middleware/websocket/websocket.go b/middleware/websocket/websocket.go index 9f3ffe14..f344fe51 100644 --- a/middleware/websocket/websocket.go +++ b/middleware/websocket/websocket.go @@ -83,35 +83,35 @@ func serveWS(w http.ResponseWriter, r *http.Request, config *Config) (int, error } conn, err := upgrader.Upgrade(w, r, nil) if err != nil { - return 0, err + return http.StatusBadRequest, err } defer conn.Close() cmd := exec.Command(config.Command, config.Arguments...) stdout, err := cmd.StdoutPipe() if err != nil { - panic(err) // TODO + return http.StatusBadGateway, err } stdin, err := cmd.StdinPipe() if err != nil { - panic(err) // TODO + return http.StatusBadGateway, err } metavars, err := buildEnv(cmd.Path, r) if err != nil { - panic(err) // TODO + return http.StatusBadGateway, err } cmd.Env = metavars if err := cmd.Start(); err != nil { - panic(err) + return http.StatusBadGateway, err } reader(conn, stdout, stdin) - return 0, nil // we shouldn't get here. + return 0, nil } // buildEnv creates the meta-variables for the child process according @@ -171,32 +171,39 @@ func reader(conn *websocket.Conn, stdout io.ReadCloser, stdin io.WriteCloser) { conn.SetReadLimit(maxMessageSize) conn.SetReadDeadline(time.Now().Add(pongWait)) conn.SetPongHandler(func(string) error { conn.SetReadDeadline(time.Now().Add(pongWait)); return nil }) - go ticker(conn) + tickerChan := make(chan bool) + defer func() { tickerChan <- true }() // make sure to close the ticker when we are done. + go ticker(conn, tickerChan) for { msgType, r, err := conn.NextReader() if err != nil { if msgType == -1 { - return // we are done, as we got a close method. + return // we got a disconnect from the client. We are good to close. } - panic(err) // TODO do something else here. + conn.WriteControl(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseGoingAway, ""), time.Time{}) + return } w, err := conn.NextWriter(msgType) if err != nil { - panic(err) // TODO do something else here. + conn.WriteControl(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseGoingAway, ""), time.Time{}) + return } if _, err := io.Copy(stdin, r); err != nil { - panic(err) // TODO do something else here. + conn.WriteControl(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseGoingAway, ""), time.Time{}) + return } go func() { if _, err := io.Copy(w, stdout); err != nil { - panic(err) // TODO do something else here. + conn.WriteControl(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseGoingAway, ""), time.Time{}) + return } if err := w.Close(); err != nil { - panic(err) // TODO do something else here. + conn.WriteControl(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseGoingAway, ""), time.Time{}) + return } }() } @@ -204,17 +211,19 @@ func reader(conn *websocket.Conn, stdout io.ReadCloser, stdin io.WriteCloser) { // ticker is start by the reader. Basically it is the method that simulates the websocket // between the server and client to keep it alive with ping messages. -func ticker(conn *websocket.Conn) { +func ticker(conn *websocket.Conn, c chan bool) { ticker := time.NewTicker(pingPeriod) defer func() { ticker.Stop() - conn.WriteMessage(websocket.CloseMessage, nil) + close(c) }() for { // blocking loop with select to wait for stimulation. select { case <-ticker.C: conn.WriteMessage(websocket.PingMessage, nil) + case <-c: + return // clean up this routine. } } } From 6af26e23066f646181af4c73511477137b27e868 Mon Sep 17 00:00:00 2001 From: makpoc Date: Wed, 14 Oct 2015 09:35:50 +0300 Subject: [PATCH 10/11] Use null byte in filename to simulate 'unable to access' on both windows and linux --- config/setup/root_test.go | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/config/setup/root_test.go b/config/setup/root_test.go index ff3dad3f..8b38e6d0 100644 --- a/config/setup/root_test.go +++ b/config/setup/root_test.go @@ -7,7 +7,6 @@ import ( "path/filepath" "strings" "testing" - "runtime" ) func TestRoot(t *testing.T) { @@ -27,13 +26,13 @@ func TestRoot(t *testing.T) { if err != nil { t.Fatalf("BeforeTest: Failed to create temp file for testing! Error was: %v", err) } - defer func () { + defer func() { existingFile.Close() os.Remove(existingFile.Name()) }() - unaccessiblePath := getInaccessibleOsDependentPath(existingFile.Name()) - + inaccessiblePath := getInaccessiblePath(existingFile.Name()) + tests := []struct { input string shouldErr bool @@ -52,7 +51,7 @@ func TestRoot(t *testing.T) { `root `, true, "", parseErrContent, }, { - fmt.Sprintf(`root %s`, unaccessiblePath), true, "", unableToAccessErrContent, + fmt.Sprintf(`root %s`, inaccessiblePath), true, "", unableToAccessErrContent, }, { fmt.Sprintf(`root { @@ -103,11 +102,7 @@ func getTempDirPath() (string, error) { return tempDir, nil } -func getInaccessibleOsDependentPath(file string) string{ - if runtime.GOOS == "windows"{ - return filepath.Join("C:", "file\x00name")// 0 byte breaks the lstat syscall - } else { - // TODO - check if the zero-byte filename works for linux only. If it does - use it instead - return filepath.Join(file, "some_name") - } -} \ No newline at end of file +func getInaccessiblePath(file string) string { + // null byte in filename is not allowed on Windows AND unix + return filepath.Join("C:", "file\x00name") +} From b713a7796e19d55cf3703d4b7819ea7d0ba83fdb Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Mon, 12 Oct 2015 19:17:50 -0600 Subject: [PATCH 11/11] Change c:\go to c:\gopath to avoid conflicts --- appveyor.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/appveyor.yml b/appveyor.yml index 1866debe..a436177c 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -2,10 +2,10 @@ version: "{build}" os: Windows Server 2012 R2 -clone_folder: c:\go\src\github.com\mholt\caddy +clone_folder: c:\gopath\src\github.com\mholt\caddy environment: - GOPATH: c:\go + GOPATH: c:\gopath install: - go get golang.org/x/tools/cmd/vet