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

Add CSRF protection to ReverseProxy authentication on API #22221

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Dec 22, 2022

Add CSRF protection to the API when using ReverseProxy authentication.

This is a recreation of #22077 with the copyright headers left unchanged, and adding HEAD to the allowed methods.

Replace #22077
Close #22219
Close #22077

@zeripath zeripath assigned zeripath and unassigned zeripath Dec 22, 2022
@zeripath zeripath added type/bug outdated/backport/v1.18 This PR should be backported to Gitea 1.18 labels Dec 22, 2022
@zeripath zeripath added this to the 1.19.0 milestone Dec 22, 2022
@zeripath
Copy link
Contributor Author

zeripath commented Dec 22, 2022

Now one of the things against this PR as it stands is that it adds another CORS library - I don't fully understand why this is the case.

Realistically for this to be mergable IMO we need to use the same library in both the API and the main code

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 22, 2022
@silverwind
Copy link
Member

I wonder if all this CSFR stuff is actually needed when we set SameSite cookie attribute.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@lunny lunny closed this in #22219 Dec 27, 2022
lunny pushed a commit that referenced this pull request Dec 27, 2022
Since we changed the /api/v1/ routes to disallow session authentication
we also removed their reliance on CSRF. However, we left the
ReverseProxy authentication here - but this means that POSTs to the API
are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication,
and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication
from the API and therefore users of the API must explicitly use tokens
or basic authentication.

Replace #22077
Close #22221 
Close #22077 

Signed-off-by: Andrew Thornton <art27@cantab.net>
lunny pushed a commit to lunny/gitea that referenced this pull request Dec 27, 2022
Since we changed the /api/v1/ routes to disallow session authentication
we also removed their reliance on CSRF. However, we left the
ReverseProxy authentication here - but this means that POSTs to the API
are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication,
and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication
from the API and therefore users of the API must explicitly use tokens
or basic authentication.

Replace go-gitea#22077
Close go-gitea#22221 
Close go-gitea#22077 

Signed-off-by: Andrew Thornton <art27@cantab.net>
lunny pushed a commit to lunny/gitea that referenced this pull request Dec 27, 2022
Since we changed the /api/v1/ routes to disallow session authentication
we also removed their reliance on CSRF. However, we left the
ReverseProxy authentication here - but this means that POSTs to the API
are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication,
and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication
from the API and therefore users of the API must explicitly use tokens
or basic authentication.

Replace go-gitea#22077
Close go-gitea#22221 
Close go-gitea#22077 

Signed-off-by: Andrew Thornton <art27@cantab.net>
KN4CK3R pushed a commit that referenced this pull request Dec 27, 2022
backport from #22219

Since we changed the /api/v1/ routes to disallow session authentication
we also removed their reliance on CSRF. However, we left the
ReverseProxy authentication here - but this means that POSTs to the API
are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication,
and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication
from the API and therefore users of the API must explicitly use tokens
or basic authentication.

Replace #22077
Close #22221 
Close #22077 

Signed-off-by: Andrew Thornton <art27@cantab.net>

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: zeripath <art27@cantab.net>
@zeripath zeripath deleted the replace-22077-fix-copyright-header-and-make-updatable branch December 29, 2022 17:59
lafriks pushed a commit that referenced this pull request Dec 30, 2022
backport #22219

Since we changed the /api/v1/ routes to disallow session authentication
we also removed their reliance on CSRF. However, we left the
ReverseProxy authentication here - but this means that POSTs to the API
are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication,
and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication
from the API and therefore users of the API must explicitly use tokens
or basic authentication.

Replace #22077
Close #22221 
Close #22077 

Signed-off-by: Andrew Thornton <art27@cantab.net>

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: zeripath <art27@cantab.net>
@zeripath zeripath removed the outdated/backport/v1.18 This PR should be backported to Gitea 1.18 label Jan 13, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants