Skip to content

Commit

Permalink
fix: don't copy the decision endpoint request's Content-Length (#422)
Browse files Browse the repository at this point in the history
We currently copy all original request headers send to the decission
endpoint back. This can include the Content-Length header which
describes the request body or response. Including the original
request Content-Length causes issues for the decission endpoint
client if the response body doesn't match the exact size.

This change makes sure the Content-Length doesn't get included in
the response body and adds a test to prevent future regressions.
  • Loading branch information
Marlinc authored Apr 28, 2020
1 parent 81f9c42 commit 0e99045
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 0 deletions.
5 changes: 5 additions & 0 deletions api/decision.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ func (h *DecisionHandler) decisions(w http.ResponseWriter, r *http.Request) {
Info("Access request granted")

for k := range s.Header {
// Avoid copying the original Content-Length header from the client
if k == "content-length" {
continue
}

w.Header().Set(k, s.Header.Get(k))
}

Expand Down
29 changes: 29 additions & 0 deletions api/decision_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@ package api_test
import (
"context"
"fmt"
"io/ioutil"
"net/http"
"net/http/httptest"
"strconv"
"testing"

"github.com/ory/viper"
Expand Down Expand Up @@ -284,6 +286,29 @@ func TestDecisionAPI(t *testing.T) {
}},
code: http.StatusUnauthorized,
},
{
d: "should not pass Content-Length from client",
url: ts.URL + "/decisions" + "/authn-anon/authz-allow/cred-noop/1234",
rulesRegexp: []rule.Rule{{
Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]+>"},
Authenticators: []rule.Handler{{Handler: "anonymous"}},
Authorizer: rule.Handler{Handler: "allow"},
Mutators: []rule.Handler{{Handler: "noop"}},
Upstream: rule.Upstream{URL: ""},
}},
rulesGlob: []rule.Rule{{
Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]*>"},
Authenticators: []rule.Handler{{Handler: "anonymous"}},
Authorizer: rule.Handler{Handler: "allow"},
Mutators: []rule.Handler{{Handler: "noop"}},
Upstream: rule.Upstream{URL: ""},
}},
transform: func(r *http.Request) {
r.Header.Add("Content-Length", "1337")
},
code: http.StatusOK,
authz: "",
},
} {
t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) {
testFunc := func(strategy configuration.MatchingStrategy) {
Expand All @@ -296,10 +321,14 @@ func TestDecisionAPI(t *testing.T) {

res, err := http.DefaultClient.Do(req)
require.NoError(t, err)

entireBody, err := ioutil.ReadAll(res.Body)
require.NoError(t, err)
defer res.Body.Close()

assert.Equal(t, tc.authz, res.Header.Get("Authorization"))
assert.Equal(t, tc.code, res.StatusCode)
assert.Equal(t, strconv.Itoa(len(entireBody)), res.Header.Get("Content-Length"))
}
t.Run("regexp", func(t *testing.T) {
reg.RuleRepository().(*rule.RepositoryMemory).WithRules(tc.rulesRegexp)
Expand Down
1 change: 1 addition & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -899,6 +899,7 @@ github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXf
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk=
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4=
github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
github.com/subosito/gotenv v1.1.1/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw=
github.com/subosito/gotenv v1.2.0 h1:Slr1R9HxAlEKefgq5jn9U+DnETlIUa6HfgEzj0g5d7s=
Expand Down

0 comments on commit 0e99045

Please sign in to comment.