Repository: google/go-safeweb (go)

Issue:
fetchmetadata: switch to configs for exceptions
The fetchmetadata plugin dates back to when we didn't have configs.

This means that exceptions for the protection must currently be passed on construction instead of handler registration.

We should switch to the new mechanism.

Pull request description:
fetchmetadata: Update to new API.
Fixes #300

Patch:
diff --git a/examples/echo/security/web/web.go b/examples/echo/security/web/web.go
index 4c8f125..2c34a80 100644
--- a/examples/echo/security/web/web.go
+++ b/examples/echo/security/web/web.go
@@ -49,7 +49,7 @@ func NewMuxConfig() *safehttp.ServeMuxConfig {
 
 	c.Intercept(coop.Default(""))
 	c.Intercept(csp.Default(""))
-	c.Intercept(fetchmetadata.NewInterceptor(""))
+	c.Intercept(&fetchmetadata.Interceptor{})
 	c.Intercept(staticheaders.Interceptor{})
 	return c
 }
diff --git a/examples/sample-application/secure/mux.go b/examples/sample-application/secure/mux.go
index dc42fd4..ca3cffe 100644
--- a/examples/sample-application/secure/mux.go
+++ b/examples/sample-application/secure/mux.go
@@ -35,7 +35,7 @@ func NewMuxConfig(db *storage.DB, addr string) *safehttp.ServeMuxConfig {
 	c := safehttp.NewServeMuxConfig(dispatcher{})
 	c.Intercept(coop.Default(""))
 	c.Intercept(csp.Default(""))
-	c.Intercept(fetchmetadata.NewInterceptor())
+	c.Intercept(&fetchmetadata.Interceptor{})
 	c.Intercept(hostcheck.New(addr))
 	c.Intercept(hsts.Default())
 	c.Intercept(staticheaders.Interceptor{})
diff --git a/safehttp/defaults/defaults.go b/safehttp/defaults/defaults.go
index c2f67b2..146d982 100644
--- a/safehttp/defaults/defaults.go
+++ b/safehttp/defaults/defaults.go
@@ -47,7 +47,7 @@ func ServeMuxConfig(hosts []string, xsrfKey string) (*safehttp.ServeMuxConfig, e
 	// TODO(clap): add a report-uri once we support reporting.
 	// TODO(clap): find a way to make the FramingPolicy here work together with the Framing plugin once we have it.
 	c.Intercept(csp.Default(""))
-	c.Intercept(fetchmetadata.NewInterceptor())
+	c.Intercept(&fetchmetadata.Interceptor{})
 	c.Intercept(hostcheck.New(hosts...))
 	c.Intercept(hsts.Default())
 	c.Intercept(staticheaders.Interceptor{})
diff --git a/safehttp/plugins/fetchmetadata/fetchmetadata.go b/safehttp/plugins/fetchmetadata/fetchmetadata.go
index a27ca14..71977a7 100644
--- a/safehttp/plugins/fetchmetadata/fetchmetadata.go
+++ b/safehttp/plugins/fetchmetadata/fetchmetadata.go
@@ -23,7 +23,7 @@ import (
 
 // RequestLogger is used for logging Fetch Metadata policy violations;
 type RequestLogger interface {
-	Log(ir *safehttp.IncomingRequest)
+	Log(ir *safehttp.IncomingRequest, navigationIsolation bool)
 }
 
 var (
@@ -41,11 +41,12 @@ var (
 	}
 )
 
-// Plugin implements Fetch Metadata functionality.
+// Interceptor implements Fetch Metadata functionality.
+// A pointer to the zero value for Interceptor is safe and ready to use.
 //
 // See https://www.w3.org/TR/fetch-metadata/ and
 // https://web.dev/fetch-metadata/  for more details.
-type Plugin struct {
+type Interceptor struct {
 	// NavIsolation indicates whether the Navigation Isolation Policy should
 	// be applied to the request before the Resource Isolation Policy as an
 	// additional layer of checking. This provides a way to mitigate
@@ -57,27 +58,14 @@ type Plugin struct {
 	NavIsolation bool
 	// RedirectURL can optionally indicate an URL to which the user can be
 	// redirected in case the Navigation Isolation policy rejects the request.
-	RedirectURL   *safehttp.URL
-	Logger        RequestLogger
-	reportOnly    bool
-	corsProtected map[string]bool
+	RedirectURL *safehttp.URL
+	Logger      RequestLogger
+	reportOnly  bool
 }
 
-var _ safehttp.Interceptor = &Plugin{}
+var _ safehttp.Interceptor = &Interceptor{}
 
-// NewInterceptor creates a new Fetch Metadata plugin in enforce mode that will apply
-// the Resource Isolation Policy by default. The user can provide a set of
-// endpoints that will not be protected. Any request targeted to those endpoints
-// will be allowed by default without the policies being applied.
-func NewInterceptor(endpoints ...string) *Plugin {
-	m := map[string]bool{}
-	for _, e := range endpoints {
-		m[e] = true
-	}
-	return &Plugin{corsProtected: m}
-}
-
-func (p *Plugin) checkResourceIsolationPolicy(r *safehttp.IncomingRequest) bool {
+func (p *Interceptor) checkResourceIsolationPolicy(r *safehttp.IncomingRequest) bool {
 	h := r.Header
 	if h.Get("Sec-Fetch-Site") != "cross-site" {
 		// The request is allowed to pass because one of the following applies:
@@ -107,7 +95,7 @@ func (p *Plugin) checkResourceIsolationPolicy(r *safehttp.IncomingRequest) bool
 	return false
 }
 
-func (p *Plugin) checkNavigationIsolationPolicy(r *safehttp.IncomingRequest) bool {
+func (p *Interceptor) checkNavigationIsolationPolicy(r *safehttp.IncomingRequest) bool {
 	if h := r.Header; p.NavIsolation && h.Get("Sec-Fetch-Site") == "cross-site" && navigationalModes[h.Get("Sec-Fetch-Mode")] {
 		return false
 	}
@@ -117,7 +105,7 @@ func (p *Plugin) checkNavigationIsolationPolicy(r *safehttp.IncomingRequest) boo
 // SetReportOnly sets the Fetch Metadata policy mode to "report". This will
 // allow requests that violate the policy to pass, but will report the violation
 // using the Logger. The method will panic if Logger is nil.
-func (p *Plugin) SetReportOnly() {
+func (p *Interceptor) SetReportOnly() {
 	if p.Logger == nil {
 		panic("logging service required for Fetch Metadata report mode")
 	}
@@ -126,7 +114,7 @@ func (p *Plugin) SetReportOnly() {
 
 // SetEnforce sets the Fetch Metadata policy mode to "enforce". This will reject
 // any requests that violates the policy provided by the plugin.
-func (p *Plugin) SetEnforce() {
+func (p *Interceptor) SetEnforce() {
 	p.reportOnly = false
 }
 
@@ -138,37 +126,55 @@ func (p *Plugin) SetEnforce() {
 // the  violation is reported. If a redirectURL was provided and the Navigation
 // Isolation Policy is enabled and fails, the IncomingRequest will be
 // redirected to redirectURL.
-func (p *Plugin) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequest, _ safehttp.InterceptorConfig) safehttp.Result {
-	// TODO(mihalimara22): Remove and disable using configurations when those
-	// have been implemented
-	if p.corsProtected[r.URL.Path()] {
-		// The request is targeted to an endpoint on which Fetch Metadata
-		// policies are disabled because it is CORS-protected so we don't apply
-		// the policies.
-		return safehttp.NotWritten()
-	}
-
+func (p *Interceptor) Before(w safehttp.ResponseWriter, r *safehttp.IncomingRequest, cfg safehttp.InterceptorConfig) safehttp.Result {
 	rejected := false
+	nav := false
+
 	if !p.checkNavigationIsolationPolicy(r) {
 		rejected = true
-		if p.RedirectURL != nil {
-			return safehttp.Redirect(w, r, p.RedirectURL.String(), safehttp.StatusMovedPermanently)
-		}
+		nav = true
 	}
 
-	if rejected || !p.checkResourceIsolationPolicy(r) {
-		if p.Logger != nil {
-			p.Logger.Log(r)
-		}
-		if p.reportOnly {
-			return safehttp.NotWritten()
+	if !p.checkResourceIsolationPolicy(r) {
+		rejected = true
+	}
+
+	if !rejected {
+		// Both Navigation Isolation and Resource Isolation are happy.
+		return safehttp.NotWritten()
+	}
+
+	// Overridden to be disabled.
+	if d, ok := cfg.(Disable); ok {
+		if !d.SkipReporting && p.Logger != nil {
+			p.Logger.Log(r, nav)
 		}
-		return w.WriteError(safehttp.StatusForbidden)
+		return safehttp.NotWritten()
 	}
 
-	return safehttp.NotWritten()
+	if p.Logger != nil {
+		p.Logger.Log(r, nav)
+	}
+	if p.reportOnly {
+		return safehttp.NotWritten()
+	}
+	if nav && p.RedirectURL != nil {
+		return safehttp.Redirect(w, r, p.RedirectURL.String(), safehttp.StatusSeeOther)
+	}
+	return w.WriteError(safehttp.StatusForbidden)
 }
 
 // Commit is a no-op, required to satisfy the safehttp.Interceptor interface.
-func (p *Plugin) Commit(w safehttp.ResponseHeadersWriter, r *safehttp.IncomingRequest, resp safehttp.Response, _ safehttp.InterceptorConfig) {
+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
+}
+
+func (Disable) Match(i safehttp.Interceptor) bool {
+	_, ok := i.(*Interceptor)
+	return ok
 }


Test patch:
diff --git a/safehttp/plugins/fetchmetadata/fetchmetadata_test.go b/safehttp/plugins/fetchmetadata/fetchmetadata_test.go
index 793ce5c..dbe179f 100644
--- a/safehttp/plugins/fetchmetadata/fetchmetadata_test.go
+++ b/safehttp/plugins/fetchmetadata/fetchmetadata_test.go
@@ -168,7 +168,7 @@ func TestAllowedResourceIsolationEnforceMode(t *testing.T) {
 			req.Header.Add("Sec-Fetch-Dest", test.dest)
 			fakeRW, rr := safehttptest.NewFakeResponseWriter()
 
-			p := fetchmetadata.NewInterceptor()
+			p := &fetchmetadata.Interceptor{}
 			p.Before(fakeRW, req, nil)
 
 			if want, got := int(safehttp.StatusOK), rr.Code; got != want {
@@ -194,7 +194,7 @@ func TestRejectedResourceIsolationEnforceMode(t *testing.T) {
 			req.Header.Add("Sec-Fetch-Dest", test.dest)
 			fakeRW, rr := safehttptest.NewFakeResponseWriter()
 
-			p := fetchmetadata.NewInterceptor()
+			p := &fetchmetadata.Interceptor{}
 			p.Before(fakeRW, req, nil)
 
 			if want, got := safehttp.StatusForbidden, safehttp.StatusCode(rr.Code); want != got {
@@ -210,10 +210,12 @@ type reportTests struct {
 
 type methodLogger struct {
 	report string
+	nav    bool
 }
 
-func (l *methodLogger) Log(r *safehttp.IncomingRequest) {
+func (l *methodLogger) Log(r *safehttp.IncomingRequest, nav bool) {
 	l.report = r.Method()
+	l.nav = nav
 }
 
 func TestRejectedResourceIsolationEnforceModeWithLogger(t *testing.T) {
@@ -236,9 +238,10 @@ func TestRejectedResourceIsolationEnforceModeWithLogger(t *testing.T) {
 			req.Header.Add("Sec-Fetch-Dest", test.dest)
 			fakeRW, rr := safehttptest.NewFakeResponseWriter()
 
-			p := fetchmetadata.NewInterceptor()
 			logger := &methodLogger{}
-			p.Logger = logger
+			p := &fetchmetadata.Interceptor{
+				Logger: logger,
+			}
 			p.Before(fakeRW, req, nil)
 
 			if want, got := safehttp.StatusForbidden, safehttp.StatusCode(rr.Code); want != got {
@@ -247,6 +250,9 @@ func TestRejectedResourceIsolationEnforceModeWithLogger(t *testing.T) {
 			if test.report != logger.report {
 				t.Errorf("logger.report: got %s, want %s", logger.report, test.report)
 			}
+			if logger.nav != false {
+				t.Errorf("logger.nav: got %v, want %v", logger.nav, false)
+			}
 		})
 	}
 }
@@ -281,7 +287,7 @@ func TestResourceIsolationReportMode(t *testing.T) {
 			req.Header.Add("Sec-Fetch-Dest", test.dest)
 			fakeRW, rr := safehttptest.NewFakeResponseWriter()
 
-			p := fetchmetadata.NewInterceptor()
+			p := &fetchmetadata.Interceptor{}
 			logger := &methodLogger{}
 			p.Logger = logger
 			p.SetReportOnly()
@@ -299,12 +305,15 @@ func TestResourceIsolationReportMode(t *testing.T) {
 			if logger.report != test.report {
 				t.Errorf("logger.report: got %s, want %s", logger.report, test.report)
 			}
+			if logger.nav != false {
+				t.Errorf("logger.nav: got %v, want %v", logger.nav, false)
+			}
 		})
 	}
 }
 
 func TestReportModeMissingLogger(t *testing.T) {
-	p := fetchmetadata.NewInterceptor()
+	p := &fetchmetadata.Interceptor{}
 	defer func() {
 		if r := recover(); r != nil {
 			return
@@ -324,8 +333,9 @@ func TestNavIsolationEnforceMode(t *testing.T) {
 			req.Header.Add("Sec-Fetch-Dest", test.dest)
 			fakeRW, rr := safehttptest.NewFakeResponseWriter()
 
-			p := fetchmetadata.NewInterceptor()
-			p.NavIsolation = true
+			p := &fetchmetadata.Interceptor{
+				NavIsolation: true,
+			}
 			p.Before(fakeRW, req, nil)
 
 			if want, got := safehttp.StatusForbidden, safehttp.StatusCode(rr.Code); want != got {
@@ -368,10 +378,11 @@ func TestNavIsolationReportMode(t *testing.T) {
 			req.Header.Add("Sec-Fetch-Dest", test.dest)
 			fakeRW, rr := safehttptest.NewFakeResponseWriter()
 
-			p := fetchmetadata.NewInterceptor()
 			logger := &methodLogger{}
-			p.Logger = logger
-			p.NavIsolation = true
+			p := &fetchmetadata.Interceptor{
+				Logger:       logger,
+				NavIsolation: true,
+			}
 			p.SetReportOnly()
 			p.Before(fakeRW, req, nil)
 
@@ -387,12 +398,38 @@ func TestNavIsolationReportMode(t *testing.T) {
 			if logger.report != test.report {
 				t.Errorf("logger.report: got %s, want %s", logger.report, test.report)
 			}
+			if logger.nav != true {
+				t.Errorf("logger.nav: got %v, want %v", logger.nav, true)
+			}
 		})
 	}
 }
 
-func TestCORSEndpoint(t *testing.T) {
-	tests := append(disallowedRIPHeaders, disallowedRIPNavHeaders...)
+func TestDisable(t *testing.T) {
+	type reportTests struct {
+		name, method, site, mode, dest, report string
+	}
+	var tests []reportTests
+	for _, t := range allowedRIPNavHeaders {
+		tests = append(tests, reportTests{
+			name:   t.name,
+			method: t.method,
+			site:   t.site,
+			mode:   t.mode,
+			dest:   t.dest,
+			report: t.method,
+		})
+	}
+	for _, t := range disallowedRIPNavHeaders {
+		tests = append(tests, reportTests{
+			name:   t.name,
+			method: t.method,
+			site:   t.site,
+			mode:   t.mode,
+			dest:   t.dest,
+			report: t.method,
+		})
+	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			req := safehttptest.NewRequest(test.method, "https://spaghetti.com/carbonara", nil)
@@ -401,10 +438,14 @@ func TestCORSEndpoint(t *testing.T) {
 			req.Header.Add("Sec-Fetch-Dest", test.dest)
 			fakeRW, rr := safehttptest.NewFakeResponseWriter()
 
-			p := fetchmetadata.NewInterceptor("/carbonara")
-			p.Before(fakeRW, req, nil)
+			logger := &methodLogger{}
+			p := &fetchmetadata.Interceptor{
+				Logger:       logger,
+				NavIsolation: true,
+			}
+			p.Before(fakeRW, req, fetchmetadata.Disable{})
 
-			if want, got := safehttp.StatusOK, safehttp.StatusCode(rr.Code); got != want {
+			if want, got := safehttp.StatusOK, safehttp.StatusCode(rr.Code); want != got {
 				t.Errorf("rr.Code got: %v want: %v", got, want)
 			}
 			if diff := cmp.Diff(map[string][]string{}, map[string][]string(rr.Header())); diff != "" {
@@ -413,61 +454,68 @@ func TestCORSEndpoint(t *testing.T) {
 			if want, got := "", rr.Body.String(); got != want {
 				t.Errorf("rr.Body.String() got: %q want: %q", got, want)
 			}
+			if logger.report != test.report {
+				t.Errorf("logger.report: got %s, want %s", logger.report, test.report)
+			}
+			if logger.nav != true {
+				t.Errorf("logger.nav: got %v, want %v", logger.nav, true)
+			}
 		})
 	}
 }
 
-func TestCORSAfterRedirect(t *testing.T) {
-	tests := []struct {
-		name, method, site, mode, dest string
-	}{
-		{
-			name:   "cross origin POST",
-			method: safehttp.MethodPost,
-			site:   "cross-site",
-			mode:   "navigate",
-			dest:   "document",
-		},
-		{
-			name:   "cross origin GET from object",
-			method: safehttp.MethodGet,
-			site:   "cross-site",
-			mode:   "navigate",
-			dest:   "object",
-		},
-		{
-			name:   "cross origin HEAD from embed",
-			method: safehttp.MethodHead,
-			site:   "cross-site",
-			mode:   "navigate",
-			dest:   "embed",
-		},
+func TestDisableSkipLogger(t *testing.T) {
+	type reportTests struct {
+		name, method, site, mode, dest, report string
+	}
+	var tests []reportTests
+	for _, t := range allowedRIPNavHeaders {
+		tests = append(tests, reportTests{
+			name:   t.name,
+			method: t.method,
+			site:   t.site,
+			mode:   t.mode,
+			dest:   t.dest,
+			report: t.method,
+		})
+	}
+	for _, t := range disallowedRIPNavHeaders {
+		tests = append(tests, reportTests{
+			name:   t.name,
+			method: t.method,
+			site:   t.site,
+			mode:   t.mode,
+			dest:   t.dest,
+			report: t.method,
+		})
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			req := safehttptest.NewRequest(test.method, "https://spaghetti.com/bolognese", nil)
+			req := safehttptest.NewRequest(test.method, "https://spaghetti.com/carbonara", nil)
 			req.Header.Add("Sec-Fetch-Site", test.site)
 			req.Header.Add("Sec-Fetch-Mode", test.mode)
 			req.Header.Add("Sec-Fetch-Dest", test.dest)
 			fakeRW, rr := safehttptest.NewFakeResponseWriter()
 
-			p := fetchmetadata.NewInterceptor("/carbonara")
-			p.NavIsolation = true
-			p.RedirectURL, _ = safehttp.ParseURL("https://spaghetti.com/carbonara")
-			p.Before(fakeRW, req, nil)
+			logger := &methodLogger{}
+			p := &fetchmetadata.Interceptor{
+				Logger:       logger,
+				NavIsolation: true,
+			}
+			p.Before(fakeRW, req, fetchmetadata.Disable{SkipReporting: true})
 
-			if want, got := safehttp.StatusMovedPermanently, safehttp.StatusCode(rr.Code); got != want {
+			if want, got := safehttp.StatusOK, safehttp.StatusCode(rr.Code); want != got {
 				t.Errorf("rr.Code got: %v want: %v", got, want)
 			}
-			resp := fakeRW.Dispatcher.Written
-			redir, ok := resp.(safehttp.RedirectResponse)
-			if !ok {
-				t.Fatalf("got %T, wanted a RedirectResponse", resp)
+			if diff := cmp.Diff(map[string][]string{}, map[string][]string(rr.Header())); diff != "" {
+				t.Errorf("rr.Header() mismatch (-want +got):\n%s", diff)
+			}
+			if want, got := "", rr.Body.String(); got != want {
+				t.Errorf("rr.Body.String() got: %q want: %q", got, want)
 			}
-			if got, want := redir.Location, "https://spaghetti.com/carbonara"; got != want {
-				t.Errorf("RedirectResponse.Location got %q, want %q", got, want)
+			if logger.report != "" {
+				t.Error("logger was called but it shouldn't have been")
 			}
 		})
 	}
-
 }