-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CORS middleware #1349
Add CORS middleware #1349
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,8 @@ type AuthenticationService struct { | |
sessionTokenExpiry time.Duration | ||
// sessionCookieMaxAge holds the max age for session cookies in seconds. | ||
sessionCookieMaxAge int | ||
// secureCookies decides whether to set the secure flag on cookies. | ||
secureCookies bool | ||
// jwtSessionKey holds the secret key used for signing/verifying JWT tokens. | ||
// According to https://datatracker.ietf.org/doc/html/rfc7518 minimum key lengths are | ||
// HSXXX e.g. HS256 - 256 bits, RSA - at least 2048 bits. | ||
|
@@ -119,6 +121,9 @@ type AuthenticationServiceParams struct { | |
// SessionCookieMaxAge holds the max age for session cookies in seconds. | ||
SessionCookieMaxAge int | ||
|
||
// SecureCookies decides whether to set the secure flag on cookies. | ||
SecureCookies bool | ||
|
||
// JWTSessionKey holds the secret key used for signing/verifying JWT tokens. | ||
// See AuthenticationService.JWTSessionKey for more details. | ||
JWTSessionKey string | ||
|
@@ -163,6 +168,7 @@ func NewAuthenticationService(ctx context.Context, params AuthenticationServiceP | |
db: params.Store, | ||
sessionStore: params.SessionStore, | ||
sessionCookieMaxAge: params.SessionCookieMaxAge, | ||
secureCookies: params.SecureCookies, | ||
}, nil | ||
} | ||
|
||
|
@@ -420,13 +426,23 @@ func (as *AuthenticationService) VerifyClientCredentials(ctx context.Context, cl | |
return nil | ||
} | ||
|
||
// sessionCrossOriginSafe sets parameters on the session that allow its use in cross-origin requests. | ||
// Options are not saved to the database so this must be called whenever a session cookie will be returned to a client. | ||
// | ||
// Note browsers require cookies with the same-site policy as 'none' to additionally have the secure flag set. | ||
func sessionCrossOriginSafe(session *sessions.Session, secure bool) *sessions.Session { | ||
session.Options.Secure = secure // Ensures only sent with HTTPS | ||
session.Options.HttpOnly = false // Allow Javascript to read it | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we allowing javascript to read it? Do they manipulate the cookies in some way? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No they call whoami to get details There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point @SimoneDutto I went and tried my test again, setting HttpOnly = true and the browser still sends the cookie. Had a google and I see from here
So I'll change this to true. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, this is more secure! thank you for this |
||
session.Options.SameSite = http.SameSiteNoneMode // Allow cross-origin requests via Javascript | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it's possible to set a domain for cookies, like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The domain automatically gets set to the domain that set the cookie. Doc source |
||
return session | ||
} | ||
|
||
// CreateBrowserSession creates a session and updates the cookie for a browser | ||
// login callback. | ||
func (as *AuthenticationService) CreateBrowserSession( | ||
ctx context.Context, | ||
w http.ResponseWriter, | ||
r *http.Request, | ||
secureCookies bool, | ||
email string, | ||
) error { | ||
const op = errors.Op("auth.AuthenticationService.CreateBrowserSession") | ||
|
@@ -438,8 +454,7 @@ func (as *AuthenticationService) CreateBrowserSession( | |
|
||
session.IsNew = true // Sets cookie to a fresh new cookie | ||
session.Options.MaxAge = as.sessionCookieMaxAge // Expiry in seconds | ||
session.Options.Secure = secureCookies // Ensures only sent with HTTPS | ||
session.Options.HttpOnly = false // Allow Javascript to read it | ||
session = sessionCrossOriginSafe(session, as.secureCookies) | ||
|
||
session.Values[SessionIdentityKey] = email | ||
if err = session.Save(r, w); err != nil { | ||
|
@@ -465,6 +480,7 @@ func (as *AuthenticationService) AuthenticateBrowserSession(ctx context.Context, | |
if err != nil { | ||
return ctx, errors.E(op, err, "failed to retrieve session") | ||
} | ||
session = sessionCrossOriginSafe(session, as.secureCookies) | ||
|
||
identityId, ok := session.Values[SessionIdentityKey] | ||
if !ok { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,7 @@ func setupTestAuthSvc(ctx context.Context, c *qt.C, expiry time.Duration) (*auth | |
SessionStore: sessionStore, | ||
SessionCookieMaxAge: 60, | ||
JWTSessionKey: "secret-key", | ||
SecureCookies: false, | ||
}) | ||
c.Assert(err, qt.IsNil) | ||
cleanup := func() { | ||
|
@@ -295,7 +296,7 @@ func TestCreateBrowserSession(t *testing.T) { | |
req, err := http.NewRequest("GET", "", nil) | ||
c.Assert(err, qt.IsNil) | ||
|
||
err = authSvc.CreateBrowserSession(ctx, rec, req, false, "[email protected]") | ||
err = authSvc.CreateBrowserSession(ctx, rec, req, "[email protected]") | ||
c.Assert(err, qt.IsNil) | ||
|
||
cookies := rec.Header().Get("Set-Cookie") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you can add some requests forged to trigger cors policies in service_test.go