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

Added same-site policy for session options #1381

Merged
merged 4 commits into from
Dec 16, 2022

Conversation

AH-dark
Copy link
Contributor

@AH-dark AH-dark commented Jul 2, 2022

Store.Options(sessions.Options{
    HttpOnly: true,
    MaxAge:   7 * 86400,
    Path:     "/",
    SameSite: http.SameSiteNoneMode,
    Secure:   true,
})

@AH-dark AH-dark changed the title Added same-site policy for session options Added same-site policy for session options Jul 2, 2022
@HFO4
Copy link
Member

HFO4 commented Jul 7, 2022

Why do we need this? Smaesite=None requires HTTPS to work, this will break some existing user's application.

@AH-dark
Copy link
Contributor Author

AH-dark commented Jul 7, 2022

SameSite=None can save us from deploying the front-end and back-end under a domain name.

You are right, there is such a problem. Maybe we can add a configuration to the conf file for this?

@HFO4
Copy link
Member

HFO4 commented Jul 7, 2022

SameSite=None can save us from deploying the front-end and back-end under a domain name.

You are right, there is such a problem. Maybe we can add a configuration to the conf file for this?

Yes, sounds good to me.

@AH-dark
Copy link
Contributor Author

AH-dark commented Jul 7, 2022

So should I put it in the CORS section or open a new section? How do you think it should be?

@HFO4
Copy link
Member

HFO4 commented Jul 12, 2022

So should I put it in the CORS section or open a new section? How do you think it should be?

I think under CORS section is good.

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #1381 (ca0d81c) into master (a188067) will increase coverage by 0.08%.
The diff coverage is 66.66%.

❗ Current head ca0d81c differs from pull request most recent head 6a60081. Consider uploading reports for the commit 6a60081 to get more accurate results

@@            Coverage Diff             @@
##           master    #1381      +/-   ##
==========================================
+ Coverage   89.27%   89.35%   +0.08%     
==========================================
  Files          96       96              
  Lines        8306     8323      +17     
==========================================
+ Hits         7415     7437      +22     
+ Misses        727      723       -4     
+ Partials      164      163       -1     
Impacted Files Coverage Δ
pkg/conf/conf.go 72.72% <ø> (ø)
middleware/session.go 84.09% <66.66%> (-12.21%) ⬇️
pkg/cluster/pool.go 94.82% <0.00%> (+9.48%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@AH-dark
Copy link
Contributor Author

AH-dark commented Jul 20, 2022

I have finished adding the relevant configuration.

By the way, I just learned that the Set-Cookie header also has a directive called Secure, which should correspond to HTTPS. That is, if it is a cross-site request and the request side is an HTTPS site, it should be enable.

See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie

@AH-dark
Copy link
Contributor Author

AH-dark commented Aug 6, 2022

@HFO4 Sir, you haven't given me an answer yet!

go.mod Show resolved Hide resolved
@HFO4
Copy link
Member

HFO4 commented Sep 29, 2022

There seems to be conflict in go.mod.

@AH-dark
Copy link
Contributor Author

AH-dark commented Sep 29, 2022

There seems to be conflict in go.mod.

Solved.

@HFO4 HFO4 merged commit 74e1bd6 into cloudreve:master Dec 16, 2022
@HFO4
Copy link
Member

HFO4 commented Dec 16, 2022

Thanks!

@AH-dark AH-dark deleted the patch-samesite branch December 16, 2022 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants