caddyhttp/browse: Browse.ServeHTTP has an erraneous os non-existence check

[odeke-em]

Thank you all for the great work on Caddy!

I was just finishing up the OpenCensus https://opencensus.io/ instrumentation of Caddy for distributed tracing and monitoring and while reading through the code I noticed this erraneous os error check.
https://github.com/mholt/caddy/blob/13268db5361dd73d5504a7704c60d76d4755a71f/caddyhttp/browse/browse.go#L353-L363

and in particular
https://github.com/mholt/caddy/blob/13268db5361dd73d5504a7704c60d76d4755a71f/caddyhttp/browse/browse.go#L358-L361

which I believe mistakenly invokes Next or is there something different?

Perhaps I am mistaken, but I don't believe it is possible that on trying to open a file that we'll ever get an "Exists file" error, perhaps it was meant to be os.IsNotExist(err) and the current condition will never be hit, I don't think it is tested for either.
Here is a test to illustrate the problem and perhaps to ensure no future regressions

package browse_test

import (
    "errors"
    "io/ioutil"
    "net/http"
    "net/http/httptest"
    "net/url"
    "os"
    "path/filepath"
    "strings"
    "testing"
    "text/template"

    "github.com/mholt/caddy/caddyhttp/browse"
    "github.com/mholt/caddy/caddyhttp/staticfiles"
)

func TestBrowseConfigMismatch(t *testing.T) {
    tmpdir, err := ioutil.TempDir("", "check_servehttp_test")
    if err != nil {
        t.Fatalf("Failed to create temp directory: %v", err)
    }
    defer os.RemoveAll(tmpdir)

    existentPath := "existent"
    if err := os.MkdirAll(filepath.Join(tmpdir, existentPath), 0755); err != nil {
        t.Fatalf("Failed to create temp file as control: %v", err)
    }

    gameOverHandler := func(w http.ResponseWriter, r *http.Request) (int, error) {
        return 500, errors.New("game over, this next handler got used")
    }
    fs := http.Dir(tmpdir)
    brw := &browse.Browse{
        Next: &handler{next: gameOverHandler},
        Configs: []browse.Config{
            {Fs: staticfiles.FileServer{Root: fs}, Template: template.Must(template.New("test").Parse("{{.Name}}"))},
        },
    }

    tests := []struct {
        path     string
        wantCode int
        wantErr  string
    }{
        {path: "/" + existentPath + "/", wantCode: http.StatusOK},
        {path: "/nonexistent", wantCode: http.StatusNotFound, wantErr: "no such file or directory"},
    }

    for i, tt := range tests {
        rw := httptest.NewRecorder()
        req := &http.Request{URL: &url.URL{Path: tt.path}, Method: "GET"}

        code, err := brw.ServeHTTP(rw, req)
        if err != nil {
            if !strings.Contains(err.Error(), tt.wantErr) {
                t.Errorf("#%d: got\n\t%q\nwant\n\t%s", i, err, tt.wantErr)
            }
        } else if tt.wantErr != "" {
            t.Errorf("#%d: want error to contain %q", i, tt.wantErr)
        }

        if code != tt.wantCode {
            t.Errorf("#%d: code:: got %d want %d", i, code, tt.wantCode)
        }
    }
}

type handler struct {
    next func(http.ResponseWriter, *http.Request) (int, error)
}

func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) {
    return h.next(w, r)
}

which currently prints

$ go test -v -run=Mismatch
=== RUN   TestBrowseConfigMismatch
--- FAIL: TestBrowseConfigMismatch (0.00s)
    check_servehttp_test.go:58: #1: got
            "game over, this next handler got used"
        want
            no such file or directory
    check_servehttp_test.go:65: #1: code:: got 500 want 404
FAIL
exit status 1
FAIL    github.com/mholt/caddy/caddyhttp/browse 0.012s

but it really should return 404 for the non-existent path.

Also the code below it looks suspicious, it uses os.IsExist too; an audit of error handling might suffice. If this isn't a bug, it would help to add some comments in the code as that is an esoteric error way of handling os errors.

Thank you.

[mholt]

Hmm, that might be a bug. Thanks for pointing that out. Would you like to make a pull request? :smile:

[theodesp]

Hey @odeke-em would you able to provide a fix?

[odeke-em]

Sorry for the late reply!

@mholt and @theodesp, for sure I'll roll something out this week :)

[francislavoie]

@odeke-em are you still working on this? I don't see a related PR to that commit.

[odeke-em]

Sorry about that @francislavoie, I actually started a PR on it in https://github.com/orijtech/caddy/commit/943867c44bb1612e43f2c78373af754178253ef5 but I got side tracked with fire-fighting and rolling some code out at the end of the quarter, plus a bunch of work to present.

[mholt]

Fixed in Caddy 2.