Repository: google/go-safeweb
Pull request #312 (2021-05-31 16:07:27)

Make interceptors match configs instead of configs match interceptors.
This makes it possible to have configs live in separate packages from their interceptors, thus empowering us to use simple restrictions rules.

Moreover we had inconsistent disabling mechanisms that didn't force user to attach a reason to them, this PR addresses that too.


Patch:
diff --git a/examples/sample-application/secure/auth/auth.go b/examples/sample-application/secure/auth/auth.go
index 4015067..1c7efef 100644
--- a/examples/sample-application/secure/auth/auth.go
+++ b/examples/sample-application/secure/auth/auth.go
@@ -85,6 +85,11 @@ func (ip Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.Incom
 	}
 }
 
+func (Interceptor) Match(cfg safehttp.InterceptorConfig) bool {
+	_, ok := cfg.(Skip)
+	return ok
+}
+
 // User retrieves the user.
 func User(r *safehttp.IncomingRequest) string {
 	return ctxUser(r.Context())
@@ -125,9 +130,3 @@ func CreateSession(r *safehttp.IncomingRequest, user string) {
 // https://github.com/google/go-safeweb/blob/master/cmd/bancheck tool to enforce
 // this.
 type Skip struct{}
-
-func (Skip) Match(i safehttp.Interceptor) bool {
-	// This configuration only applies to the auth plugin.
-	_, ok := i.(Interceptor)
-	return ok
-}
diff --git a/safehttp/interceptor.go b/safehttp/interceptor.go
index 2116853..fd814a8 100644
--- a/safehttp/interceptor.go
+++ b/safehttp/interceptor.go
@@ -30,25 +30,13 @@ type Interceptor interface {
 	// is written to the ResponseWriter, then the Commit phases from the
 	// remaining interceptors won't execute.
 	Commit(w ResponseHeadersWriter, r *IncomingRequest, resp Response, cfg InterceptorConfig)
-}
 
-// WrappedInterceptor is a wrapped interceptor.
-// This can be used to decorate an already existing interceptor and still allow
-// its native configurations to match with it.
-type WrappedInterceptor interface {
-	Interceptor
-	// Unwrap returns the wrapped interceptor.
-	Unwrap() Interceptor
+	// Match checks whether the given config is meant to be applied to the Interceptor.
+	Match(InterceptorConfig) bool
 }
 
-// InterceptorConfig is a configuration of an interceptor.
-type InterceptorConfig interface {
-	// Match checks whether this InterceptorConfig is meant to be applied to the
-	// given Interceptor.
-	// If the Interceptor implements WrappedInterceptor the framework will take care of calling
-	// Match for all the nested interceptors.
-	Match(Interceptor) bool
-}
+// InterceptorConfig is a configuration for an interceptor.
+type InterceptorConfig interface{}
 
 // configuredInterceptor holds an interceptor together with its configuration.
 type configuredInterceptor struct {
diff --git a/safehttp/mux.go b/safehttp/mux.go
index a5a31ad..b5ad0fa 100644
--- a/safehttp/mux.go
+++ b/safehttp/mux.go
@@ -223,17 +223,9 @@ func configureInterceptors(interceptors []Interceptor, cfgs []InterceptorConfig)
 	for _, it := range interceptors {
 		var matches []InterceptorConfig
 		for _, c := range cfgs {
-			if c.Match(it) {
+			if it.Match(c) {
 				matches = append(matches, c)
 			}
-			wrapped, ok := it.(WrappedInterceptor)
-			for ok {
-				inner := wrapped.Unwrap()
-				if c.Match(inner) {
-					matches = append(matches, c)
-				}
-				wrapped, ok = inner.(WrappedInterceptor)
-			}
 		}
 
 		if len(matches) > 1 {
diff --git a/safehttp/plugins/coop/coop.go b/safehttp/plugins/coop/coop.go
index 1a179e3..e6ce50d 100644
--- a/safehttp/plugins/coop/coop.go
+++ b/safehttp/plugins/coop/coop.go
@@ -19,6 +19,8 @@ import (
 	"github.com/google/go-safeweb/safehttp"
 )
 
+var _ safehttp.Interceptor = Interceptor{}
+
 // Mode represents a COOP mode.
 type Mode string
 
@@ -94,16 +96,16 @@ func (it Interceptor) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequ
 func (it Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, _ safehttp.InterceptorConfig) {
 }
 
+// Match recognizes Overriders as COOP configurations.
+func (it Interceptor) Match(cfg safehttp.InterceptorConfig) bool {
+	_, ok := cfg.(Overrider)
+	return ok
+}
+
 // Overrider is a safehttp.InterceptorConfig that allows to override COOP for a specific handler.
 type Overrider serializedPolicies
 
 // Override creates an Overrider with the given policies.
-func Override(policies ...Policy) Overrider {
+func Override(reason string, policies ...Policy) Overrider {
 	return Overrider(serializePolicies(policies...))
 }
-
-// Match recognizes just this package Interceptor.
-func (p Overrider) Match(i safehttp.Interceptor) bool {
-	_, ok := i.(Interceptor)
-	return ok
-}
diff --git a/safehttp/plugins/cors/cors.go b/safehttp/plugins/cors/cors.go
index fe6dd26..edf4c03 100644
--- a/safehttp/plugins/cors/cors.go
+++ b/safehttp/plugins/cors/cors.go
@@ -158,6 +158,11 @@ func (it *Interceptor) Before(w safehttp.ResponseWriter, r *safehttp.IncomingReq
 func (it *Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, cfg safehttp.InterceptorConfig) {
 }
 
+// Match returns false since there are no supported configurations.
+func (*Interceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
+
 func appendToVary(w safehttp.ResponseWriter, val string) {
 	h := w.Header()
 	if curr := h.Get("Vary"); curr != "" {
diff --git a/safehttp/plugins/csp/csp.go b/safehttp/plugins/csp/csp.go
index e1ac56f..1b3840e 100644
--- a/safehttp/plugins/csp/csp.go
+++ b/safehttp/plugins/csp/csp.go
@@ -290,3 +290,8 @@ func (it Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.Incom
 	}
 	tmplResp.FuncMap[htmlinject.CSPNoncesDefaultFuncName] = func() string { return nonce }
 }
+
+// Match returns false since there are no supported configurations.
+func (Interceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
diff --git a/safehttp/plugins/fetchmetadata/fetchmetadata.go b/safehttp/plugins/fetchmetadata/fetchmetadata.go
index 803040b..d8006f5 100644
--- a/safehttp/plugins/fetchmetadata/fetchmetadata.go
+++ b/safehttp/plugins/fetchmetadata/fetchmetadata.go
@@ -145,8 +145,8 @@ func (p *Interceptor) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequ
 	}
 
 	// Overridden to be disabled.
-	if d, ok := cfg.(Disable); ok {
-		if !d.SkipReporting && p.Logger != nil {
+	if d, ok := cfg.(disable); ok {
+		if !d.skipReporting && p.Logger != nil {
 			p.Logger.Log(r, nav)
 		}
 		return safehttp.NotWritten()
@@ -168,14 +168,18 @@ func (p *Interceptor) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequ
 func (p *Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, _ safehttp.InterceptorConfig) {
 }
 
-// Disable disables Fetch Metadata protection and switches it to report-only.
-type Disable struct {
-	// SkipReporting also disables reporting.
-	SkipReporting bool
+// Match recongnizes configs to disable fetch metadata protection.
+func (*Interceptor) Match(cfg safehttp.InterceptorConfig) bool {
+	_, ok := cfg.(disable)
+	return ok
 }
 
-// Match matches this config to *Interceptor.
-func (Disable) Match(i safehttp.Interceptor) bool {
-	_, ok := i.(*Interceptor)
-	return ok
+type disable struct {
+	skipReporting bool
+}
+
+// Disable returns a configuration that disables Fetch Metadata protection and switches it to report-only.
+// If skipReporting is true, it will also disable reporting.
+func Disable(reason string, skipReporting bool) safehttp.InterceptorConfig {
+	return disable{skipReporting: skipReporting}
 }
diff --git a/safehttp/plugins/hostcheck/hostcheck.go b/safehttp/plugins/hostcheck/hostcheck.go
index 3d86548..818a85d 100644
--- a/safehttp/plugins/hostcheck/hostcheck.go
+++ b/safehttp/plugins/hostcheck/hostcheck.go
@@ -53,3 +53,8 @@ func (it Interceptor) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequ
 // Commit is a no-op, required to satisfy the safehttp.Interceptor interface.
 func (Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, _ safehttp.InterceptorConfig) {
 }
+
+// Match returns false since there are no supported configurations.
+func (Interceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
diff --git a/safehttp/plugins/hsts/hsts.go b/safehttp/plugins/hsts/hsts.go
index 7e9f77f..ba3c2a0 100644
--- a/safehttp/plugins/hsts/hsts.go
+++ b/safehttp/plugins/hsts/hsts.go
@@ -118,5 +118,10 @@ func (it Interceptor) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequ
 }
 
 // Commit is a no-op, required to satisfy the safehttp.Interceptor interface.
-func (it Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, _ safehttp.InterceptorConfig) {
+func (Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, _ safehttp.InterceptorConfig) {
+}
+
+// Match returns false since there are no supported configurations.
+func (Interceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
 }
diff --git a/safehttp/plugins/reportingapi/reporting.go b/safehttp/plugins/reportingapi/reporting.go
index 983d85c..cea1003 100644
--- a/safehttp/plugins/reportingapi/reporting.go
+++ b/safehttp/plugins/reportingapi/reporting.go
@@ -25,6 +25,8 @@ import (
 	"github.com/google/go-safeweb/safehttp"
 )
 
+var _ safehttp.Interceptor = Interceptor{}
+
 // DefaultMaxAge is used as default cache duration for report groups and will make them last 7 days.
 const DefaultMaxAge = 7 * 24 * 60 * 60
 
@@ -101,5 +103,10 @@ func (i Interceptor) Before(w safehttp.ResponseWriter, r *safehttp.IncomingReque
 }
 
 // Commit is a no-op, required to satisfy the safehttp.Interceptor interface.
-func (i Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, _ safehttp.InterceptorConfig) {
+func (Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, _ safehttp.InterceptorConfig) {
+}
+
+// Match returns false since there are no supported configurations.
+func (Interceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
 }
diff --git a/safehttp/plugins/staticheaders/staticheaders.go b/safehttp/plugins/staticheaders/staticheaders.go
index 0c2733a..548b6e7 100644
--- a/safehttp/plugins/staticheaders/staticheaders.go
+++ b/safehttp/plugins/staticheaders/staticheaders.go
@@ -54,3 +54,8 @@ func (Interceptor) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequest
 // Commit is a no-op, required to satisfy the safehttp.Interceptor interface.
 func (Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, _ safehttp.InterceptorConfig) {
 }
+
+// Match returns false since there are no supported configurations.
+func (Interceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
diff --git a/safehttp/plugins/xsrf/xsrfangular/xsrf.go b/safehttp/plugins/xsrf/xsrfangular/xsrf.go
index d287034..c886d62 100644
--- a/safehttp/plugins/xsrf/xsrfangular/xsrf.go
+++ b/safehttp/plugins/xsrf/xsrfangular/xsrf.go
@@ -112,3 +112,8 @@ func (it *Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.Inco
 		panic("cannot add token cookie")
 	}
 }
+
+// Match returns false since there are no supported configurations.
+func (*Interceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
diff --git a/safehttp/plugins/xsrf/xsrfhtml/xsrf.go b/safehttp/plugins/xsrf/xsrfhtml/xsrf.go
index d7b4ee9..d94f503 100644
--- a/safehttp/plugins/xsrf/xsrfhtml/xsrf.go
+++ b/safehttp/plugins/xsrf/xsrfhtml/xsrf.go
@@ -131,3 +131,8 @@ func (it *Interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.Inco
 	}
 	tmplResp.FuncMap[htmlinject.XSRFTokensDefaultFuncName] = func() string { return tok }
 }
+
+// Match returns false since there are no supported configurations.
+func (*Interceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
+}


Test patch:
diff --git a/safehttp/flight_test.go b/safehttp/flight_test.go
index 8bcc6d8..fb8af3f 100644
--- a/safehttp/flight_test.go
+++ b/safehttp/flight_test.go
@@ -40,6 +40,10 @@ func (p panickingInterceptor) Commit(w safehttp.ResponseHeadersWriter, r *safeht
 	}
 }
 
+func (panickingInterceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
+
 func TestFlightInterceptorPanic(t *testing.T) {
 	tests := []struct {
 		desc        string
diff --git a/safehttp/mux_test.go b/safehttp/mux_test.go
index db725da..2401d76 100644
--- a/safehttp/mux_test.go
+++ b/safehttp/mux_test.go
@@ -213,6 +213,10 @@ func (p setHeaderInterceptor) Before(w safehttp.ResponseWriter, _ *safehttp.Inco
 func (p setHeaderInterceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, cfg safehttp.InterceptorConfig) {
 }
 
+func (setHeaderInterceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
+
 type internalErrorInterceptor struct{}
 
 func (internalErrorInterceptor) Before(w safehttp.ResponseWriter, _ *safehttp.IncomingRequest, cfg safehttp.InterceptorConfig) safehttp.Result {
@@ -222,6 +226,10 @@ func (internalErrorInterceptor) Before(w safehttp.ResponseWriter, _ *safehttp.In
 func (internalErrorInterceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, cfg safehttp.InterceptorConfig) {
 }
 
+func (internalErrorInterceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
+
 type claimHeaderInterceptor struct {
 	headerToClaim string
 }
@@ -237,6 +245,10 @@ func (p *claimHeaderInterceptor) Before(w safehttp.ResponseWriter, r *safehttp.I
 func (p *claimHeaderInterceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, cfg safehttp.InterceptorConfig) {
 }
 
+func (claimHeaderInterceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
+
 func claimInterceptorSetHeader(w safehttp.ResponseWriter, r *safehttp.IncomingRequest, value string) {
 	f := r.Context().Value(claimCtxKey{}).(func([]string))
 	f([]string{value})
@@ -252,6 +264,10 @@ func (committerInterceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp
 	w.Header().Set("foo", "bar")
 }
 
+func (committerInterceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
+
 type setHeaderErroringInterceptor struct{}
 
 func (setHeaderErroringInterceptor) Before(w safehttp.ResponseWriter, _ *safehttp.IncomingRequest, cfg safehttp.InterceptorConfig) safehttp.Result {
@@ -383,11 +399,6 @@ type setHeaderConfig struct {
 	value string
 }
 
-func (setHeaderConfig) Match(i safehttp.Interceptor) bool {
-	_, ok := i.(setHeaderConfigInterceptor)
-	return ok
-}
-
 type setHeaderConfigInterceptor struct{}
 
 func (p setHeaderConfigInterceptor) Before(w safehttp.ResponseWriter, _ *safehttp.IncomingRequest, cfg safehttp.InterceptorConfig) safehttp.Result {
@@ -411,6 +422,11 @@ func (p setHeaderConfigInterceptor) Commit(w safehttp.ResponseHeadersWriter, r *
 	w.Header().Set(name, value)
 }
 
+func (setHeaderConfigInterceptor) Match(cfg safehttp.InterceptorConfig) bool {
+	_, ok := cfg.(setHeaderConfig)
+	return ok
+}
+
 type noInterceptorConfig struct{}
 
 type wrappedInterceptor struct {
@@ -425,8 +441,8 @@ func (wi wrappedInterceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehtt
 	wi.w.Commit(w, r, resp, cfg)
 }
 
-func (wi wrappedInterceptor) Unwrap() safehttp.Interceptor {
-	return wi.w
+func (wi wrappedInterceptor) Match(cfg safehttp.InterceptorConfig) bool {
+	return wi.w.Match(cfg)
 }
 
 func (noInterceptorConfig) Match(i safehttp.Interceptor) bool {
@@ -526,6 +542,10 @@ func (interceptorOne) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.Incom
 	w.Header().Set("Commit1", "a")
 }
 
+func (interceptorOne) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
+
 type interceptorTwo struct{}
 
 func (interceptorTwo) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequest, cfg safehttp.InterceptorConfig) safehttp.Result {
@@ -543,6 +563,10 @@ func (interceptorTwo) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.Incom
 	w.Header().Set("Commit2", "b")
 }
 
+func (interceptorTwo) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
+
 type interceptorThree struct{}
 
 func (interceptorThree) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequest, cfg safehttp.InterceptorConfig) safehttp.Result {
@@ -560,6 +584,10 @@ func (interceptorThree) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.Inc
 	w.Header().Set("Commit3", "c")
 }
 
+func (interceptorThree) Match(safehttp.InterceptorConfig) bool {
+	return false
+}
+
 func TestMuxDeterministicInterceptorOrder(t *testing.T) {
 	mb := safehttp.NewServeMuxConfig(nil)
 	mb.Intercept(interceptorOne{})
@@ -689,13 +717,13 @@ func (ip *methodNotAllowedInterceptor) Commit(w safehttp.ResponseHeadersWriter,
 	w.Header().Set("Commit-Interceptor", cfg.commit)
 }
 
-type methodNotAllowedInterceptorConfig struct {
-	before, commit string
+func (*methodNotAllowedInterceptor) Match(cfg safehttp.InterceptorConfig) bool {
+	_, ok := cfg.(methodNotAllowedInterceptorConfig)
+	return ok
 }
 
-func (methodNotAllowedInterceptorConfig) Match(ip safehttp.Interceptor) bool {
-	_, ok := ip.(*methodNotAllowedInterceptor)
-	return ok
+type methodNotAllowedInterceptorConfig struct {
+	before, commit string
 }
 
 func TestMuxMethodNotAllowedCustom(t *testing.T) {
diff --git a/safehttp/plugins/coop/coop_test.go b/safehttp/plugins/coop/coop_test.go
index d32d9d2..9e32173 100644
--- a/safehttp/plugins/coop/coop_test.go
+++ b/safehttp/plugins/coop/coop_test.go
@@ -36,7 +36,7 @@ func TestBefore(t *testing.T) {
 		{
 			name:           "No policies, override on header",
 			interceptor:    NewInterceptor(),
-			overrider:      Override(Policy{Mode: SameOrigin}),
+			overrider:      Override("testing", Policy{Mode: SameOrigin}),
 			wantOverridden: want{enf: []string{"same-origin"}},
 		},
 		{
@@ -55,7 +55,7 @@ func TestBefore(t *testing.T) {
 				ReportOnly:     true,
 			},
 			),
-			overrider: Override(Policy{
+			overrider: Override("testing", Policy{
 				Mode:           SameOrigin,
 				ReportingGroup: "coop-so",
 				ReportOnly:     true,
diff --git a/safehttp/plugins/fetchmetadata/fetchmetadata_test.go b/safehttp/plugins/fetchmetadata/fetchmetadata_test.go
index dbe179f..4ade959 100644
--- a/safehttp/plugins/fetchmetadata/fetchmetadata_test.go
+++ b/safehttp/plugins/fetchmetadata/fetchmetadata_test.go
@@ -443,7 +443,7 @@ func TestDisable(t *testing.T) {
 				Logger:       logger,
 				NavIsolation: true,
 			}
-			p.Before(fakeRW, req, fetchmetadata.Disable{})
+			p.Before(fakeRW, req, fetchmetadata.Disable("testing", false))
 
 			if want, got := safehttp.StatusOK, safehttp.StatusCode(rr.Code); want != got {
 				t.Errorf("rr.Code got: %v want: %v", got, want)
@@ -502,7 +502,7 @@ func TestDisableSkipLogger(t *testing.T) {
 				Logger:       logger,
 				NavIsolation: true,
 			}
-			p.Before(fakeRW, req, fetchmetadata.Disable{SkipReporting: true})
+			p.Before(fakeRW, req, fetchmetadata.Disable("testing", true))
 
 			if want, got := safehttp.StatusOK, safehttp.StatusCode(rr.Code); want != got {
 				t.Errorf("rr.Code got: %v want: %v", got, want)
diff --git a/tests/integration/errors/errors_test.go b/tests/integration/errors/errors_test.go
index 43e1dfa..cc38f04 100644
--- a/tests/integration/errors/errors_test.go
+++ b/tests/integration/errors/errors_test.go
@@ -119,7 +119,11 @@ func (it interceptor) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequ
 	return safehttp.NotWritten()
 }
 
-func (it interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, _ safehttp.InterceptorConfig) {
+func (interceptor) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, _ safehttp.InterceptorConfig) {
+}
+
+func (interceptor) Match(safehttp.InterceptorConfig) bool {
+	return false
 }
 
 func TestCustomErrorsInBefore(t *testing.T) {