Skip to content

Conversation

@shahar1
Copy link
Contributor

@shahar1 shahar1 commented Jun 8, 2024

closes: #33030

cc: @potiuk, @eladkal


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues provider:fab labels Jun 8, 2024
@shahar1 shahar1 force-pushed the add-csrf-protection-to-logout branch 2 times, most recently from 2d7834b to 3b44edf Compare June 8, 2024 21:10
@shahar1
Copy link
Contributor Author

shahar1 commented Jun 8, 2024

General question - Should Non-DB tests for FAB provider be skipped? (currently they fail)

@potiuk
Copy link
Member

potiuk commented Jun 9, 2024

General question - Should Non-DB tests for FAB provider be skipped? (currently they fail)

I think by modifying logout and adding decorator, some of the tests simply started requiring database - have not looked in detail, but that's essentially what is happening here. I guess it can be fixed by changing the test code to not trigger DB operations in those tests (preferrable) or marking them as db_tests.

@shahar1
Copy link
Contributor Author

shahar1 commented Jun 9, 2024

General question - Should Non-DB tests for FAB provider be skipped? (currently they fail)

I think by modifying logout and adding decorator, some of the tests simply started requiring database - have not looked in detail, but that's essentially what is happening here. I guess it can be fixed by changing the test code to not trigger DB operations in those tests (preferrable) or marking them as db_tests.

It makes sense, I'll check it out

@shahar1 shahar1 force-pushed the add-csrf-protection-to-logout branch from 3b44edf to 3bd6d6e Compare June 11, 2024 14:18
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

I think we also need a newsfragment for that one. While it can be treated as 'security fix" it also potentially breaks somoene workflow if they have some non-CSRF logout integrated somewhere in their setup.

@shahar1
Copy link
Contributor Author

shahar1 commented Jun 11, 2024

I think we also need a newsfragment for that one. While it can be treated as 'security fix" it also potentially breaks somoene workflow if they have some non-CSRF logout integrated somewhere in their setup.

I will add one.
I also checked locally - I managed to reproduce the errors, with or without the change, when running the following command:
breeze testing non-db-tests --parallel-test-types "Always Providers[fab] WWW"

So I see no other good choice, but marking test_role_and_permission_endpoint.py as a db test.

@potiuk
Copy link
Member

potiuk commented Jun 11, 2024

So I see no other good choice, but marking test_role_and_permission_endpoint.py as a db test.

Yep. Saw the marker. Indeed that's simpler.

@shahar1 shahar1 force-pushed the add-csrf-protection-to-logout branch 4 times, most recently from 8806141 to 3c9847a Compare June 12, 2024 12:14
@shahar1 shahar1 force-pushed the add-csrf-protection-to-logout branch from 3c9847a to 4c19e19 Compare June 12, 2024 12:20
@shahar1 shahar1 requested a review from potiuk June 12, 2024 12:20
@eladkal eladkal added this to the Airflow 2.10.0 milestone Jun 12, 2024
@eladkal eladkal added the type:improvement Changelog: Improvements label Jun 12, 2024
@potiuk potiuk merged commit 14deaa2 into apache:main Jun 12, 2024
@shahar1 shahar1 deleted the add-csrf-protection-to-logout branch June 12, 2024 16:29
shahar1 added a commit to shahar1/airflow that referenced this pull request Jun 12, 2024
potiuk pushed a commit that referenced this pull request Jun 12, 2024
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
@shahar1 shahar1 mentioned this pull request Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues provider:fab type:improvement Changelog: Improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CSRF protection to "/logout"

4 participants