Skip to content
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

Support setting cookie domain #6288

Open
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@tamalsaha
Copy link

tamalsaha commented Mar 9, 2019

We are building a website where the session cookie is set to example.com domain. Later a SPA hosted on a subdomain.example.com will make api calls to example.com/api/xyz. In this scenario we want to set the csrf cookie domain to .example.com so that SPA can pass it along the ajax api calls.

Thanks.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 9, 2019

Codecov Report

Merging #6288 into master will decrease coverage by <.01%.
The diff coverage is 73.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6288      +/-   ##
==========================================
- Coverage   38.89%   38.88%   -0.01%     
==========================================
  Files         359      359              
  Lines       50993    50996       +3     
==========================================
- Hits        19833    19830       -3     
- Misses      28290    28297       +7     
+ Partials     2870     2869       -1
Impacted Files Coverage Δ
routers/user/setting/profile.go 40.41% <100%> (ø) ⬆️
routers/routes/routes.go 83.1% <100%> (+0.04%) ⬆️
modules/setting/session.go 100% <100%> (ø) ⬆️
routers/user/auth.go 13.07% <50%> (ø) ⬆️
models/unit.go 0% <0%> (-14.29%) ⬇️
routers/repo/view.go 41.08% <0%> (-1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8211e01...7aad9de. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 label Mar 9, 2019

Support setting cookie domain
Signed-off-by: Tamal Saha <tamal@appscode.com>

@tamalsaha tamalsaha force-pushed the tamalsaha:cookie-domain-pr branch from 96433e9 to 7aad9de Mar 9, 2019

@techknowlogick
Copy link
Member

techknowlogick left a comment

The changes are concerning from a security perspective. Perhaps now that we have OAuth2 you could use that to generate a token that could be used for the API in your SPA instead.

@@ -145,8 +146,9 @@ func NewMacaron() *macaron.Macaron {
Cookie: setting.CSRFCookieName,
SetCookie: true,
Secure: setting.SessionConfig.Secure,
CookieHttpOnly: true,

This comment has been minimized.

@techknowlogick

techknowlogick Mar 9, 2019

Member

This is an important security measure and shouldn't be set to false. By setting this to false an attacker could gain access to the cookie.

This comment has been minimized.

@tamalsaha

tamalsaha Mar 9, 2019

Author

I am following this article: http://www.redotheweb.com/2015/11/09/api-security.html

You are right in that session cookie must be HTTPOnly to protect against XSS attack. But it is ok to make CSRF cookie readable by JS and usually passed as header or request body in a CORS ajax call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.