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

Enable authorization header as CSRF protection #201

Merged
merged 1 commit into from
Aug 29, 2022

Conversation

feedmeapples
Copy link
Contributor

@feedmeapples feedmeapples commented Aug 5, 2022

What was changed

  • Allows HTTP requests with set authorization header to pass CSRF check

Why?

In case when API is hosted under different origin from the UI the existing CSRF rules block API POST/PUT/.. insecure HTTPs methods which result in Terminate/.. buttons to not work. The change allows the requests with Authorization header set to pass the CSRF check as it still protects against CSRF

Checklist

  1. Closes

  2. How was this tested:

new unit tests

  1. Any docs updates needed?

@feedmeapples feedmeapples requested a review from mcbryde August 18, 2022 16:41
)

func SkipOnAuthorizationHeader(c echo.Context) bool {
return c.Request().Header.Get(echo.HeaderAuthorization) != ""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it'll skip csrf protection if the authorization header is set to any value. If that's the case, then it allows a full bypass of CSRF protection with an invalid authorization header.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since CSRF attacks use the implicit sending of cookies shouldn't this be okay because a user wouldn't be able to add this header to bypass the CSRF check AND successfully become authenticated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, if authorization header is set (though since this is an explicit auth flow attackers shouldn't be able to do that with the CSRF attack) then it's up to the authorization plugin on Temporal side to deny the request

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed directly adding unit tests to make sure authorization header is always prioritized over an auth cookie here

rToken := c.Request().Header.Get(echo.HeaderAuthorization)

@feedmeapples feedmeapples requested a review from mcbryde August 29, 2022 17:09
@feedmeapples feedmeapples merged commit 90548ae into main Aug 29, 2022
@feedmeapples feedmeapples deleted the csrf-with-auth-header branch August 29, 2022 17:09
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.

4 participants