Preserve original $_SERVER headers in CsrfCounterMeasureTest - #407
Closed
jrauh01 wants to merge 1 commit into
Closed
Preserve original $_SERVER headers in CsrfCounterMeasureTest#407jrauh01 wants to merge 1 commit into
$_SERVER headers in CsrfCounterMeasureTest#407jrauh01 wants to merge 1 commit into
Conversation
The test unconditionally unset `HTTP_SEC_FETCH_SITE` and `REQUEST_METHOD` in tearDown, discarding whatever values the environment held before the test ran. Capture the originals in setUp and restore them afterwards so the suite no longer leaks a cleared global state into later tests.
Member
|
I'm genuinely interested how this was noticed. To me, the previous setUp/tearDown logic looks fine as in a unittest environment these headers are not set or relevant. Other tests have to set up their own prerequisites and should not depend on global state. |
Contributor
Author
To be honest I just tried to follow Icinga/icingaweb2@6d9619d. There was no specific problem observed happening. |
Member
|
Then I'd question the same over there. Regardless, this isn't a best practice either so I'd rather not see it applied without reason. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CsrfCounterMeasureTestmanipulated the global$_SERVERsuperglobal to exercise the CSRF request-safety logic (HTTP_SEC_FETCH_SITE,REQUEST_METHOD). ItstearDown()unconditionallyunset()these keys, discarding whatever values were present before the test ran. Any later test that relies on those values would observe a cleared global state it never set.Change
setUp()now captures the original values ofHTTP_SEC_FETCH_SITEandREQUEST_METHODbefore clearing them, andtearDown()restores them: unsetting only the keys that were absent originally, and reinstating the rest. The test suite no longer leaks mutations of these globals into other tests.