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

Respects CSRF_ENABLED environment variable #976

Merged
merged 1 commit into from Jun 27, 2019

Conversation

Projects
None yet
3 participants
@Poldovico
Copy link
Contributor

commented May 7, 2019

The previous statement would evaluate to true for any value of CSRF_ENABLED.
Using the strict comparison operator means if we set the variable to any false-evaluating values other then boolean false (0, '0', '0.0', '' and so on), then CSRF will be disabled.
getenv() evaluates to boolean false if the environment variable is not set, so I know of no simple way to distinguish between the variable being unset, in which case we want to default to enabling CSRF, and it being explicitly set to boolean false.

Respects CSRF_ENABLED environment variable
The previous statement would evaluate to true for any value of CSRF_ENABLED.
Using the strict comparison operator means if we set the variable to any false-evaluating values other then boolean false (0, '0', 'false', '' and so on), then CSRF will be disabled.
getenv() evaluates to boolean false if the environment variable is not set, so I know of no simple way to distinguish between the variable being unset, in which case we want to default to enabling CSRF, and it being explicitly set to boolean false.

@lcharette lcharette self-assigned this May 23, 2019

@lcharette lcharette added this to the 4.2.x milestone May 23, 2019

@codecov

This comment has been minimized.

Copy link

commented Jun 27, 2019

Codecov Report

Merging #976 into hotfix will increase coverage by 13.83%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             hotfix     #976       +/-   ##
=============================================
+ Coverage     49.59%   63.43%   +13.83%     
- Complexity     2134     2283      +149     
=============================================
  Files           159      159               
  Lines          7283     7791      +508     
=============================================
+ Hits           3612     4942     +1330     
+ Misses         3671     2849      -822
Impacted Files Coverage Δ Complexity Δ
app/sprinkles/core/src/Mail/Mailer.php 33.33% <0%> (-3.51%) 29% <0%> (+3%)
...rinkles/account/src/Authenticate/Authenticator.php 92.65% <0%> (-0.21%) 58% <0%> (+19%)
...es/core/src/Database/Migrator/MigrationLocator.php 100% <0%> (ø) 5% <0%> (+1%) ⬆️
...p/sprinkles/account/src/Bakery/CreateAdminUser.php 0% <0%> (ø) 50% <0%> (+3%) ⬆️
.../account/src/ServicesProvider/ServicesProvider.php 83.44% <0%> (+3.31%) 52% <0%> (ø) ⬇️
app/sprinkles/core/src/Throttle/Throttler.php 33.96% <0%> (+7.54%) 21% <0%> (ø) ⬇️
app/sprinkles/core/src/Csrf/SlimCsrfProvider.php 89.18% <0%> (+7.7%) 21% <0%> (+9%) ⬆️
app/sprinkles/account/src/Account/Registration.php 75% <0%> (+11.36%) 25% <0%> (ø) ⬇️
.../sprinkles/admin/src/Sprunje/PermissionSprunje.php 12.5% <0%> (+12.5%) 7% <0%> (ø) ⬇️
...ount/src/Authorize/ParserNodeFunctionEvaluator.php 70.88% <0%> (+12.65%) 27% <0%> (ø) ⬇️
... and 17 more

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 ab2c3fe...4db78fd. Read the comment docs.

@lcharette lcharette merged commit 15c4eab into userfrosting:hotfix Jun 27, 2019

3 checks passed

codecov/patch Coverage not affected when comparing ab2c3fe...4db78fd
Details
codecov/project 63.43% (+13.83%) compared to ab2c3fe
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.