Repository: oauth2-proxy/oauth2-proxy
Pull request #453 (2020-03-18 20:22:48)

Prevent browser caching during auth flow
<!--- Provide a general summary of your changes in the Title above -->

## Description

Add some headers to prevent browser caching during authentication flow.

<!--- Describe your changes in detail -->

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->

<!--- If it fixes an open issue, please link to the issue here. -->

Browser might cache `/oauth2/start` request etc. under certain conditions.

In my environment, Chrome will **not**  send `GET /oauth2/start` to the oauth2_proxy,  and the Chrome re-uses previous `GET /authorize?approval_prompt=force&client_id=xxx&redirect_uri=yyyy.......` request and send it to the provider.

Below are my steps for browser to reproduce.
1. Login as usual our site using oauth2_proxy. ( and open a blank page on the next tab for step 4 )
2. Clear all cookies (for our site and provider)
3. Close our site tab
4. Reopen tab (Right click on the top of the empty tab)
5. Login with Provider
6. oauth2_proxy returns 403

I think oauth2_proxy gets bad OIDC state param.
If my explanation is not enough, I would supplement more further.

The implementation using some headers to prevent caching referred from go-chi/chi.
https://github.com/go-chi/chi/blob/master/middleware/nocache.go

## How Has This Been Tested?

Added unit tests.

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.


johejo: Hi, thank you for your input.
I've fixed the header addition logic a bit and add tests, referring to Google's documentation and such.
And I've updated changelog.
johejo: I added test. 
https://github.com/oauth2-proxy/oauth2-proxy/pull/453/commits/2dca7fbc2cc4ee9a46c9ed58dbdca02214bfb9c0

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 86b894a3..7ed07159 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,8 @@
 
 ## Important Notes
 
+- [#453](https://github.com/oauth2-proxy/oauth2-proxy/pull/453) Responses to endpoints with a proxy prefix will now return headers for preventing browser caching.
+
 ## Breaking Changes
 
 - Migration from Pusher to independent org may have introduced breaking changes for your environment.
@@ -12,6 +14,7 @@
 
 ## Changes since v5.1.0
 
+- [#453](https://github.com/oauth2-proxy/oauth2-proxy/pull/453) Prevent browser caching during auth flow (@johejo)
 - [#481](https://github.com/oauth2-proxy/oauth2-proxy/pull/481) Update Okta docs (@trevorbox)
 - [#474](https://github.com/oauth2-proxy/oauth2-proxy/pull/474) Always log hasMember request error object (@jbielick)
 - [#468](https://github.com/oauth2-proxy/oauth2-proxy/pull/468) Implement graceful shutdown and propagate request context (@johejo)
diff --git a/oauthproxy.go b/oauthproxy.go
index 1b3e3fbe..c5470544 100644
--- a/oauthproxy.go
+++ b/oauthproxy.go
@@ -453,6 +453,7 @@ func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, code int, title string, m
 
 // SignInPage writes the sing in template to the response
 func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code int) {
+	prepareNoCache(rw)
 	p.ClearSessionCookie(rw, req)
 	rw.WriteHeader(code)
 
@@ -633,7 +634,26 @@ func getRemoteAddr(req *http.Request) (s string) {
 	return
 }
 
+// See https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/http-caching?hl=en
+var noCacheHeaders = map[string]string{
+	"Expires":         time.Unix(0, 0).Format(time.RFC1123),
+	"Cache-Control":   "no-cache, no-store, must-revalidate, max-age=0",
+	"X-Accel-Expires": "0", // https://www.nginx.com/resources/wiki/start/topics/examples/x-accel/
+}
+
+// prepareNoCache prepares headers for preventing browser caching.
+func prepareNoCache(w http.ResponseWriter) {
+	// Set NoCache headers
+	for k, v := range noCacheHeaders {
+		w.Header().Set(k, v)
+	}
+}
+
 func (p *OAuthProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
+	if strings.HasPrefix(req.URL.Path, p.ProxyPrefix) {
+		prepareNoCache(rw)
+	}
+
 	switch path := req.URL.Path; {
 	case path == p.RobotsPath:
 		p.RobotsTxt(rw)
@@ -715,6 +735,7 @@ func (p *OAuthProxy) SignOut(rw http.ResponseWriter, req *http.Request) {
 
 // OAuthStart starts the OAuth2 authentication flow
 func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request) {
+	prepareNoCache(rw)
 	nonce, err := encryption.Nonce()
 	if err != nil {
 		logger.Printf("Error obtaining nonce: %s", err.Error())


Test patch:
diff --git a/oauthproxy_test.go b/oauthproxy_test.go
index eebad11e..ec46bdd8 100644
--- a/oauthproxy_test.go
+++ b/oauthproxy_test.go
@@ -1524,3 +1524,46 @@ func TestFindJwtBearerToken(t *testing.T) {
 
 	fmt.Printf("%s", token)
 }
+
+func Test_prepareNoCache(t *testing.T) {
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		prepareNoCache(w)
+	})
+	mux := http.NewServeMux()
+	mux.Handle("/", handler)
+
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	mux.ServeHTTP(rec, req)
+
+	for k, v := range noCacheHeaders {
+		assert.Equal(t, rec.Header().Get(k), v)
+	}
+}
+
+func Test_noCacheHeadersDoesNotExistsInResponseHeadersFromUpstream(t *testing.T) {
+	upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte("upstream"))
+	}))
+	t.Cleanup(upstream.Close)
+
+	opts := NewOptions()
+	opts.Upstreams = []string{upstream.URL}
+	opts.SkipAuthRegex = []string{".*"}
+	_ = opts.Validate()
+	proxy := NewOAuthProxy(opts, func(email string) bool {
+		return true
+	})
+
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/upstream", nil)
+	proxy.ServeHTTP(rec, req)
+
+	assert.Equal(t, http.StatusOK, rec.Code)
+	assert.Equal(t, "upstream", rec.Body.String())
+
+	// checking noCacheHeaders does not exists in response headers from upstream
+	for k := range noCacheHeaders {
+		assert.Equal(t, "", rec.Header().Get(k))
+	}
+}