fix: Set 'Secure' attribute to true for sensitive cookies #3407
Conversation
app/api/handler/account/cookie.go
Outdated
| @@ -33,6 +33,9 @@ func includeTokenCookie( | |||
| cookie.Expires = time.UnixMilli(*tokenResponse.Token.ExpiresAt) | |||
| } | |||
|
|
|||
| // Set the Secure attribute to true | |||
| cookie.Secure = true | |||
There was a problem hiding this comment.
A cookie with the Secure attribute is only sent to the server with an encrypted request over the HTTPS protocol. MDN
why we enforce true here?, bcoz in newEmptyTokenCookie there is already isSecure value.
There was a problem hiding this comment.
Agree - this will actually break gitness running on http (localhost might work as some browsers ignore secure flag there and still send the cookie). I've set a custom domain in my local /etc/hosts file, and with the change here it'll break that
app/api/handler/account/cookie.go
Outdated
| @@ -48,16 +51,21 @@ func deleteTokenCookieIfPresent(r *http.Request, w http.ResponseWriter, cookieNa | |||
| cookie.Value = "" | |||
| cookie.Expires = time.UnixMilli(0) // this effectively tells the browser to delete the cookie | |||
|
|
|||
| // Set the Secure attribute to true | |||
| cookie.Secure = true | |||
app/api/handler/account/cookie.go
Outdated
| http.SetCookie(w, cookie) | ||
| } | ||
|
|
||
| func newEmptyTokenCookie(r *http.Request, cookieName string) *http.Cookie { | ||
| isSecure := r.TLS != nil // Check if the request is using HTTPS |
|
Thank you for your feedback and the reference to the MDN documentation on cookies.As you mentioned, a cookie with the ' By setting the secure attribute to true it serves as a clear indication that these cookies should only be transmitted over HTTPS, aligning with the best practices outlined in the MDN documentation.And also explicitly setting ' If you believe it's unnecessary for our codebase or if there are better ways to handle this, please let me know, or we can close this pull request.Thanks :) |
app/api/handler/account/cookie.go
Outdated
| http.SetCookie(w, cookie) | ||
| } | ||
|
|
||
| func newEmptyTokenCookie(r *http.Request, cookieName string) *http.Cookie { | ||
| isSecure := r.TLS != nil // Check if the request is using HTTPS |
There was a problem hiding this comment.
Not sure how r.TLS is fixing anything specific, could you elaborate?
One scenario I'm thinking of improving is gitness behind a reverse proxy with TLS offloading - in that case r.TLS would be nil and r.URL.Proto would be http, which would lead to cookie.Secure = false eventhough the caller is using https.
So an improvement would be to check for the availability of a X-Forwarded-Proto header (otherwise fall back to r.URL.Proto or r.TLS) to determine whether cookie.Secure should be set?
There was a problem hiding this comment.
I agree @johannesHarness
|
|
|
ATBBt26NFnTCjw8nhPR8HVcqYZxeBF349DC8 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
DharunKumar04
force-pushed
the
fix-secure-cookie-issue
branch
from
749b1a5
to
c6ed151
Compare
This pull request addresses the security issue raised by Snyk regarding sensitive cookies in the account package not having the '
Secure' attribute set. The 'Secure' attribute is essential for ensuring that cookies are transmitted securely over HTTPS, preventing potential man-in-the-middle attacks.Changes Made:
includeTokenCookie' and 'deleteTokenCookieIfPresent' functions.Secure' attribute is set to true when the request is made over HTTPS, ensuring that sensitive cookies are only sent over encrypted connections.