Derive the source detail BaseURL scheme from reqtls (closes #272)
All checks were successful
check / check (push) Successful in 3m10s
All checks were successful
check / check (push) Successful in 3m10s
The source detail page assigned the raw X-Forwarded-Proto value straight into the URL scheme, so `HTTPS` rendered `HTTPS://host`, a chained proxy's `https, http` rendered `https, http://host`, and any token at all landed there verbatim. That URL is what the operator pastes into GitHub or Stripe, so a malformed scheme means the webhook never arrives. A present header also overwrote what a direct TLS connection had already proved. The scheme now comes from reqtls.IsTLS, the shared predicate, so it is only ever http or https and all three TLS-decision sites agree. The host stays as the request sent it: nothing in the configuration names a canonical hostname to validate against, and constraining it would break the deployments behind a proxy on a non-default port or an IPv6 literal. A comment marks why the unvalidated value is inert where it is rendered.
This commit is contained in:
283
internal/handlers/source_detail_baseurl_test.go
Normal file
283
internal/handlers/source_detail_baseurl_test.go
Normal file
@@ -0,0 +1,283 @@
|
||||
package handlers_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"crypto/tls"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"regexp"
|
||||
"testing"
|
||||
|
||||
"github.com/go-chi/chi"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"sneak.berlin/go/webhooker/internal/database"
|
||||
"sneak.berlin/go/webhooker/internal/handlers"
|
||||
"sneak.berlin/go/webhooker/internal/session"
|
||||
)
|
||||
|
||||
// The only two schemes a rendered entrypoint URL may carry,
|
||||
// whatever the request claimed.
|
||||
const (
|
||||
schemeHTTPS = "https"
|
||||
schemeHTTP = "http"
|
||||
)
|
||||
|
||||
// entrypointURLPattern captures the entrypoint URL the source
|
||||
// detail page renders, which is the operator-visible product of
|
||||
// BaseURL. Asserting on the extracted string rather than on a
|
||||
// substring of the page proves the raw header value cannot reach
|
||||
// the scheme by any route.
|
||||
var entrypointURLPattern = regexp.MustCompile(
|
||||
`<code id="entrypoint-url-[^"]*"[^>]*>([^<]*)</code>`,
|
||||
)
|
||||
|
||||
// baseURLFixture is one started app plus the webhook whose
|
||||
// entrypoint URL the BaseURL cases read.
|
||||
type baseURLFixture struct {
|
||||
handlers *handlers.Handlers
|
||||
session *session.Session
|
||||
webhook string
|
||||
path string
|
||||
}
|
||||
|
||||
// newBaseURLFixture starts the app and seeds a webhook with one
|
||||
// entrypoint.
|
||||
func newBaseURLFixture(t *testing.T) *baseURLFixture {
|
||||
t.Helper()
|
||||
|
||||
var (
|
||||
h *handlers.Handlers
|
||||
sess *session.Session
|
||||
db *database.Database
|
||||
)
|
||||
|
||||
app := newTestApp(t, &h, &sess, &db)
|
||||
app.RequireStart()
|
||||
|
||||
t.Cleanup(app.RequireStop)
|
||||
|
||||
wh := seedWebhook(t, db)
|
||||
seedEntrypoint(t, db, wh.ID)
|
||||
|
||||
return &baseURLFixture{
|
||||
handlers: h,
|
||||
session: sess,
|
||||
webhook: wh.ID,
|
||||
path: "ep-" + wh.ID,
|
||||
}
|
||||
}
|
||||
|
||||
// entrypointURL renders the source detail page for the fixture's
|
||||
// webhook over a request the caller shapes, and returns the
|
||||
// entrypoint URL as an operator would copy it.
|
||||
func (f *baseURLFixture) entrypointURL(
|
||||
t *testing.T,
|
||||
host string,
|
||||
shape func(*http.Request),
|
||||
) string {
|
||||
t.Helper()
|
||||
|
||||
req := httptest.NewRequestWithContext(
|
||||
context.Background(),
|
||||
http.MethodGet,
|
||||
"/source/"+f.webhook,
|
||||
nil,
|
||||
)
|
||||
req.Host = host
|
||||
|
||||
shape(req)
|
||||
|
||||
for _, c := range authenticatedCookies(
|
||||
t, f.session, deleteTestUserID, deleteTestUsername,
|
||||
) {
|
||||
req.AddCookie(c)
|
||||
}
|
||||
|
||||
rctx := chi.NewRouteContext()
|
||||
rctx.URLParams.Add(paramSourceID, f.webhook)
|
||||
req = req.WithContext(
|
||||
context.WithValue(
|
||||
req.Context(), chi.RouteCtxKey, rctx,
|
||||
),
|
||||
)
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
f.handlers.HandleSourceDetail().ServeHTTP(w, req)
|
||||
|
||||
require.Equal(t, http.StatusOK, w.Code)
|
||||
|
||||
match := entrypointURLPattern.FindStringSubmatch(w.Body.String())
|
||||
require.Len(
|
||||
t, match, 2,
|
||||
"the page must render exactly one entrypoint URL",
|
||||
)
|
||||
|
||||
return match[1]
|
||||
}
|
||||
|
||||
// forwardedProto returns a request shaper setting
|
||||
// X-Forwarded-Proto, or leaving the request alone for "".
|
||||
func forwardedProto(value string) func(*http.Request) {
|
||||
return func(r *http.Request) {
|
||||
if value == "" {
|
||||
return
|
||||
}
|
||||
|
||||
r.Header.Set("X-Forwarded-Proto", value)
|
||||
}
|
||||
}
|
||||
|
||||
// baseURLCase is one X-Forwarded-Proto spelling and the scheme
|
||||
// the rendered entrypoint URL owes it.
|
||||
type baseURLCase struct {
|
||||
name string
|
||||
header string
|
||||
scheme string
|
||||
why string
|
||||
}
|
||||
|
||||
// baseURLCases enumerate the spellings a proxy really emits. The
|
||||
// scheme is only ever http or https: the header value itself is
|
||||
// never a scheme, however it is spelled.
|
||||
func baseURLCases() []baseURLCase {
|
||||
return []baseURLCase{
|
||||
{
|
||||
name: "lowercase",
|
||||
header: schemeHTTPS,
|
||||
scheme: schemeHTTPS,
|
||||
why: "the ordinary spelling",
|
||||
},
|
||||
{
|
||||
name: "uppercase",
|
||||
header: "HTTPS",
|
||||
scheme: schemeHTTPS,
|
||||
why: "the token is case-insensitive; the scheme " +
|
||||
"in a copyable URL is not",
|
||||
},
|
||||
{
|
||||
name: "chain with plaintext inner hop",
|
||||
header: "https, http",
|
||||
scheme: schemeHTTPS,
|
||||
why: "a chained proxy appends its hop; the " +
|
||||
"leftmost element faces the client",
|
||||
},
|
||||
{
|
||||
name: "chain of two TLS hops",
|
||||
header: "https,https",
|
||||
scheme: schemeHTTPS,
|
||||
why: "appended chain with no space after the comma",
|
||||
},
|
||||
{
|
||||
name: "trailing space",
|
||||
header: "https ",
|
||||
scheme: schemeHTTPS,
|
||||
why: "whitespace is not part of the token",
|
||||
},
|
||||
{
|
||||
name: "plaintext",
|
||||
header: schemeHTTP,
|
||||
scheme: schemeHTTP,
|
||||
why: "the negative control: the proxy reports plaintext",
|
||||
},
|
||||
{
|
||||
name: "no header",
|
||||
header: "",
|
||||
scheme: schemeHTTP,
|
||||
why: "a plaintext request asserting nothing is http",
|
||||
},
|
||||
{
|
||||
name: "garbage token",
|
||||
header: "javascript:alert(1)//",
|
||||
scheme: schemeHTTP,
|
||||
why: "anything that is not https is not TLS, and " +
|
||||
"the token never becomes the scheme",
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
// TestSourceDetailBaseURL_ForwardedProtoSpellings is the
|
||||
// regression test for the entrypoint URL an operator pastes into
|
||||
// the sending system: a header spelling that used to land in the
|
||||
// scheme verbatim produced a URL no sender could deliver to.
|
||||
func TestSourceDetailBaseURL_ForwardedProtoSpellings(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const host = "hooks.example.com"
|
||||
|
||||
fixture := newBaseURLFixture(t)
|
||||
|
||||
for _, tc := range baseURLCases() {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
assert.Equal(
|
||||
t,
|
||||
tc.scheme+"://"+host+"/webhook/"+fixture.path,
|
||||
fixture.entrypointURL(
|
||||
t, host, forwardedProto(tc.header),
|
||||
),
|
||||
"X-Forwarded-Proto %q: %s", tc.header, tc.why,
|
||||
)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestSourceDetailBaseURL_DirectTLSBeatsPlaintextHeader pins the
|
||||
// precedence the old code had backwards: it let any present
|
||||
// header overwrite what the connection itself proved, so a
|
||||
// direct-TLS request behind a proxy reporting http rendered an
|
||||
// http URL.
|
||||
func TestSourceDetailBaseURL_DirectTLSBeatsPlaintextHeader(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
|
||||
const host = "hooks.example.com"
|
||||
|
||||
fixture := newBaseURLFixture(t)
|
||||
|
||||
got := fixture.entrypointURL(t, host, func(r *http.Request) {
|
||||
r.TLS = &tls.ConnectionState{}
|
||||
r.Header.Set("X-Forwarded-Proto", "http")
|
||||
})
|
||||
|
||||
assert.Equal(
|
||||
t,
|
||||
"https://"+host+"/webhook/"+fixture.path,
|
||||
got,
|
||||
"a connection this process terminated with TLS "+
|
||||
"outranks a header claiming plaintext",
|
||||
)
|
||||
}
|
||||
|
||||
// TestSourceDetailBaseURL_KeepsHostAuthority pins the host half
|
||||
// of the URL: it is taken from the request unchanged, so the
|
||||
// deployments that do not sit on port 443 still get a URL that
|
||||
// works. Constraining the host would break exactly these.
|
||||
func TestSourceDetailBaseURL_KeepsHostAuthority(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
fixture := newBaseURLFixture(t)
|
||||
|
||||
hosts := []string{
|
||||
"hooks.example.com:8443",
|
||||
"[2001:db8::1]:8443",
|
||||
"internal-host",
|
||||
}
|
||||
|
||||
for _, host := range hosts {
|
||||
t.Run(host, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
assert.Equal(
|
||||
t,
|
||||
"https://"+host+"/webhook/"+fixture.path,
|
||||
fixture.entrypointURL(
|
||||
t, host, forwardedProto("HTTPS"),
|
||||
),
|
||||
"the authority must survive verbatim, port and all",
|
||||
)
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user