Skip to content

Commit

Permalink
cmd: Resolve broken wildcard cors (#1159)
Browse files Browse the repository at this point in the history 
Resolves an issue where wildcards would incorrectly be used as literal strings.

Closes #1073

Signed-off-by: aeneasr <aeneas@ory.sh>
  • Loading branch information
@aeneasr
aeneasr authored Nov 5, 2018
1 parent ccc34de commit 330172b
Show file tree
Hide file tree
Showing 6 changed files with 106 additions and 15 deletions.
3 changes: 2 additions & 1 deletion cmd/server/handler_jwk_factory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/ory/hydra/config"
"github.com/ory/hydra/jwk"
"github.com/ory/hydra/oauth2"
"github.com/ory/x/corsx"
"github.com/ory/x/serverx"
)

Expand Down Expand Up @@ -59,7 +60,7 @@ func newJWKHandler(c *config.Config, frontend, backend *httprouter.Router, o fos
wellKnown,
)

corsMiddleware := newCORSMiddleware(viper.GetString("CORS_ENABLED") == "true", c, o.IntrospectToken, clm.GetConcreteClient)
corsMiddleware := newCORSMiddleware(viper.GetString("CORS_ENABLED") == "true", c, corsx.ParseOptions(), o.IntrospectToken, clm.GetConcreteClient)
h.SetRoutes(frontend, backend, corsMiddleware)
return h
}
3 changes: 2 additions & 1 deletion cmd/server/handler_oauth2_factory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import (
"github.com/ory/hydra/oauth2"
"github.com/ory/hydra/tracing"
"github.com/ory/x/cmdx"
"github.com/ory/x/corsx"
"github.com/ory/x/serverx"
)

Expand Down Expand Up @@ -228,7 +229,7 @@ func newOAuth2Handler(c *config.Config, frontend, backend *httprouter.Router, cm
//IDTokenLifespan: c.GetIDTokenLifespan(),
}

corsMiddleware := newCORSMiddleware(viper.GetString("CORS_ENABLED") == "true", c, o.IntrospectToken, clm.GetConcreteClient)
corsMiddleware := newCORSMiddleware(viper.GetString("CORS_ENABLED") == "true", c, corsx.ParseOptions(), o.IntrospectToken, clm.GetConcreteClient)
handler.SetRoutes(frontend, backend, corsMiddleware)
return handler
}
60 changes: 54 additions & 6 deletions cmd/server/helper_cors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,19 @@ package server
import (
"context"
"net/http"
"strings"

"github.com/gobwas/glob"
"github.com/rs/cors"

"github.com/ory/fosite"
"github.com/ory/go-convenience/stringslice"
"github.com/ory/hydra/client"
"github.com/ory/hydra/config"
"github.com/ory/hydra/oauth2"
"github.com/ory/x/corsx"
)

func newCORSMiddleware(
enable bool, c *config.Config,
enable bool, c *config.Config, po cors.Options,
o func(ctx context.Context, token string, tokenType fosite.TokenType, session fosite.Session, scope ...string) (fosite.TokenType, fosite.AccessRequester, error),
clm func(ctx context.Context, id string) (*client.Client, error),
) func(h http.Handler) http.Handler {
Expand All @@ -46,7 +46,27 @@ func newCORSMiddleware(
}

c.GetLogger().Info("Enabled CORS")
po := corsx.ParseOptions()
var patterns []glob.Glob
for _, o := range po.AllowedOrigins {
g, err := glob.Compile(strings.ToLower(o), '.')
if err != nil {
c.GetLogger().WithError(err).Fatalf("Unable to parse cors origin: %s", o)
}
patterns = append(patterns, g)
}

var alwaysAllow bool
for _, o := range po.AllowedOrigins {
if o == "*" {
alwaysAllow = true
break
}
}

if enable && len(po.AllowedOrigins) == 0 {
alwaysAllow = true
}

options := cors.Options{
AllowedOrigins: po.AllowedOrigins,
AllowedMethods: po.AllowedMethods,
Expand All @@ -57,10 +77,17 @@ func newCORSMiddleware(
OptionsPassthrough: po.OptionsPassthrough,
Debug: po.Debug,
AllowOriginRequestFunc: func(r *http.Request, origin string) bool {
if stringslice.Has(po.AllowedOrigins, origin) {
if alwaysAllow {
return true
}

origin = strings.ToLower(origin)
for _, p := range patterns {
if p.Match(origin) {
return true
}
}

username, _, ok := r.BasicAuth()
if !ok || username == "" {
token := fosite.AccessTokenFromRequest(r)
Expand All @@ -82,10 +109,31 @@ func newCORSMiddleware(
return false
}

if stringslice.Has(cl.AllowedCORSOrigins, origin) {
if alwaysAllow {
return true
}

for _, p := range cl.AllowedCORSOrigins {
if p == "*" {
return true
}
}

var clientPatterns []glob.Glob
for _, o := range cl.AllowedCORSOrigins {
g, err := glob.Compile(strings.ToLower(o), '.')
if err != nil {
return false
}
clientPatterns = append(patterns, g)
}

for _, p := range clientPatterns {
if p.Match(origin) {
return true
}
}

return false
},
}
Expand Down
51 changes: 44 additions & 7 deletions cmd/server/helper_cors_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"testing"

"github.com/julienschmidt/httprouter"
"github.com/rs/cors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand All @@ -53,14 +54,14 @@ func TestCORSMiddleware(t *testing.T) {
}{
{
d: "should ignore when disabled",
mw: newCORSMiddleware(false, nil, nil, nil),
mw: newCORSMiddleware(false, nil, cors.Options{AllowedOrigins: []string{"http://not-test-domain.com"}}, nil, nil),
code: 204,
header: http.Header{},
expectHeader: http.Header{},
},
{
d: "should reject when basic auth but client does not exist",
mw: newCORSMiddleware(true, c, nil, func(ctx context.Context, id string) (*client.Client, error) {
mw: newCORSMiddleware(true, c, cors.Options{AllowedOrigins: []string{"http://not-test-domain.com"}}, nil, func(ctx context.Context, id string) (*client.Client, error) {
return nil, errors.New("")
}),
code: 204,
Expand All @@ -69,7 +70,7 @@ func TestCORSMiddleware(t *testing.T) {
},
{
d: "should reject when basic auth client exists but origin not allowed",
mw: newCORSMiddleware(true, c, nil, func(ctx context.Context, id string) (*client.Client, error) {
mw: newCORSMiddleware(true, c, cors.Options{AllowedOrigins: []string{"http://not-test-domain.com"}}, nil, func(ctx context.Context, id string) (*client.Client, error) {
return &client.Client{AllowedCORSOrigins: []string{"http://not-foobar.com"}}, nil
}),
code: 204,
Expand All @@ -78,16 +79,52 @@ func TestCORSMiddleware(t *testing.T) {
},
{
d: "should accept when basic auth client exists and origin allowed",
mw: newCORSMiddleware(true, c, nil, func(ctx context.Context, id string) (*client.Client, error) {
mw: newCORSMiddleware(true, c, cors.Options{AllowedOrigins: []string{"http://not-test-domain.com"}}, nil, func(ctx context.Context, id string) (*client.Client, error) {
return &client.Client{AllowedCORSOrigins: []string{"http://foobar.com"}}, nil
}),
code: 204,
header: http.Header{"Origin": {"http://foobar.com"}, "Authorization": {"Basic Zm9vOmJhcg=="}},
expectHeader: http.Header{"Vary": {"Origin"}, "Access-Control-Allow-Origin": {"http://foobar.com"}},
},
{
d: "should accept when basic auth client exists and origin (with partial wildcard) is allowed per client",
mw: newCORSMiddleware(true, c, cors.Options{AllowedOrigins: []string{"http://not-test-domain.com"}}, nil, func(ctx context.Context, id string) (*client.Client, error) {
return &client.Client{AllowedCORSOrigins: []string{"http://*.foobar.com"}}, nil
}),
code: 204,
header: http.Header{"Origin": {"http://foo.foobar.com"}, "Authorization": {"Basic Zm9vOmJhcg=="}},
expectHeader: http.Header{"Vary": {"Origin"}, "Access-Control-Allow-Origin": {"http://foo.foobar.com"}},
},
{
d: "should accept when basic auth client exists and origin (with full wildcard) is allowed globally",
mw: newCORSMiddleware(true, c, cors.Options{AllowedOrigins: []string{"*"}}, nil, func(ctx context.Context, id string) (*client.Client, error) {
return &client.Client{AllowedCORSOrigins: []string{"http://barbar.com"}}, nil
}),
code: 204,
header: http.Header{"Origin": {"*"}, "Authorization": {"Basic Zm9vOmJhcg=="}},
expectHeader: http.Header{"Vary": {"Origin"}, "Access-Control-Allow-Origin": {"*"}},
},
{
d: "should accept when basic auth client exists and origin (with partial wildcard) is allowed globally",
mw: newCORSMiddleware(true, c, cors.Options{AllowedOrigins: []string{"http://*.foobar.com"}}, nil, func(ctx context.Context, id string) (*client.Client, error) {
return &client.Client{AllowedCORSOrigins: []string{"http://barbar.com"}}, nil
}),
code: 204,
header: http.Header{"Origin": {"http://foo.foobar.com"}, "Authorization": {"Basic Zm9vOmJhcg=="}},
expectHeader: http.Header{"Vary": {"Origin"}, "Access-Control-Allow-Origin": {"http://foo.foobar.com"}},
},
{
d: "should accept when basic auth client exists and origin (with full wildcard) allowed per client",
mw: newCORSMiddleware(true, c, cors.Options{AllowedOrigins: []string{"http://not-test-domain.com"}}, nil, func(ctx context.Context, id string) (*client.Client, error) {
return &client.Client{AllowedCORSOrigins: []string{"*"}}, nil
}),
code: 204,
header: http.Header{"Origin": {"http://foobar.com"}, "Authorization": {"Basic Zm9vOmJhcg=="}},
expectHeader: http.Header{"Vary": {"Origin"}, "Access-Control-Allow-Origin": {"http://foobar.com"}},
},
{
d: "should fail when token introspection fails",
mw: newCORSMiddleware(true, c, func(ctx context.Context, token string, tokenType fosite.TokenType, session fosite.Session, scope ...string) (fosite.TokenType, fosite.AccessRequester, error) {
mw: newCORSMiddleware(true, c, cors.Options{AllowedOrigins: []string{"http://not-test-domain.com"}}, func(ctx context.Context, token string, tokenType fosite.TokenType, session fosite.Session, scope ...string) (fosite.TokenType, fosite.AccessRequester, error) {
return "", nil, errors.New("")
}, func(ctx context.Context, id string) (*client.Client, error) {
return &client.Client{AllowedCORSOrigins: []string{"http://foobar.com"}}, nil
Expand All @@ -98,7 +135,7 @@ func TestCORSMiddleware(t *testing.T) {
},
{
d: "should fail when token introspection fails",
mw: newCORSMiddleware(true, c, func(ctx context.Context, token string, tokenType fosite.TokenType, session fosite.Session, scope ...string) (fosite.TokenType, fosite.AccessRequester, error) {
mw: newCORSMiddleware(true, c, cors.Options{AllowedOrigins: []string{"http://not-test-domain.com"}}, func(ctx context.Context, token string, tokenType fosite.TokenType, session fosite.Session, scope ...string) (fosite.TokenType, fosite.AccessRequester, error) {
if token != "1234" {
return "", nil, errors.New("")
}
Expand All @@ -114,7 +151,7 @@ func TestCORSMiddleware(t *testing.T) {
expectHeader: http.Header{"Vary": {"Origin"}, "Access-Control-Allow-Origin": {"http://foobar.com"}},
},
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) {
req, err := http.NewRequest("GET", "http://foobar.com/", nil)
require.NoError(t, err)
for k := range tc.header {
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ require (
github.com/dgrijalva/jwt-go v3.2.0+incompatible
github.com/go-sql-driver/mysql v1.4.0
github.com/gobuffalo/packd v0.0.0-20181029140631-cf76bd87a5a6 // indirect
github.com/gobwas/glob v0.2.3
github.com/golang/mock v1.1.1
github.com/gorilla/context v1.1.1
github.com/gorilla/securecookie v0.0.0-20160422134519-667fe4e3466a
Expand Down
3 changes: 3 additions & 0 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ github.com/gobuffalo/packd v0.0.0-20181029140631-cf76bd87a5a6 h1:qyhnjK1xyp4xqyP
github.com/gobuffalo/packd v0.0.0-20181029140631-cf76bd87a5a6/go.mod h1:Yf2toFaISlyQrr5TfO3h6DB9pl9mZRmyvBGQb/aQ/pI=
github.com/gobuffalo/packr v1.16.0 h1:s0cqMbFDbio+Z3YxLeDOKRjLW2JKh9QVud0O7+j1fiQ=
github.com/gobuffalo/packr v1.16.0/go.mod h1:Yx/lcR/7mDLXhuJSzsz2MauD/HUwSc+EK6oigMRGGsM=
github.com/gobwas/glob v0.2.3 h1:A4xDbljILXROh+kObIiy5kIaPYD8e96x1tgBhUI5J+Y=
github.com/gobwas/glob v0.2.3/go.mod h1:d3Ez4x06l9bZtSvzIay5+Yzi0fmZzPgnTbPcKjJAkT8=
github.com/golang/gddo v0.0.0-20180828051604-96d2a289f41e h1:8sV50nrSGwclVxkCGHxgWfJhY6cyXS2plGjGvUzrMIw=
github.com/golang/gddo v0.0.0-20180828051604-96d2a289f41e/go.mod h1:xEhNfoBDX1hzLm2Nf80qUvZ2sVwoMZ8d6IE2SrsQfh4=
github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b h1:VKtxabqXZkF25pY9ekfRL6a582T4P37/31XEstQ5p58=
Expand Down Expand Up @@ -144,6 +146,7 @@ github.com/ory/fosite v0.26.2-0.20181031085642-e2441d231a19 h1:8jQrkb3nO4nG5Dzpb
github.com/ory/fosite v0.26.2-0.20181031085642-e2441d231a19/go.mod h1:uttCRNB0lM7+BJFX7CC8Bqo9gAPrcpmA9Ezc80Trwuw=
github.com/ory/fosite v0.27.0 h1:QYHW+asgRRIw5uk8a42/VpiwMQqQMPwZ4TP4xKNIMEA=
github.com/ory/fosite v0.27.0/go.mod h1:uttCRNB0lM7+BJFX7CC8Bqo9gAPrcpmA9Ezc80Trwuw=
github.com/ory/fosite v0.27.1 h1:u6IUOY4FStOZVpxxAe9ZDL/D0uM5VkVFthpx8PrAdB8=
github.com/ory/fosite v0.27.1/go.mod h1:uttCRNB0lM7+BJFX7CC8Bqo9gAPrcpmA9Ezc80Trwuw=
github.com/ory/go-convenience v0.1.0 h1:zouLKfF2GoSGnJwGq+PE/nJAE6dj2Zj5QlTgmMTsTS8=
github.com/ory/go-convenience v0.1.0/go.mod h1:uEY/a60PL5c12nYz4V5cHY03IBmwIAEm8TWB0yn9KNs=
Expand Down

0 comments on commit 330172b

Please sign in to comment.