Fix GH-15395: avoid copying empty auth password for PHP_AUTH_PW. #15416
Conversation
sapi/fpm/tests/gh15395.phpt
Outdated
| address: '{{ADDR:UDS}}', | ||
| scriptFilename: "/", | ||
| scriptName: "/", | ||
| httpAuthorization: "Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==", |
There was a problem hiding this comment.
Please use headers param for this. No need to add extra parameter. There's nothing special about HTTP_AUTHORIZATION.
sapi/fpm/tests/gh15395.phpt
Outdated
| $tester | ||
| ->request( | ||
| uri: $scriptName, | ||
| address: '{{ADDR:UDS}}', | ||
| scriptFilename: "/", | ||
| scriptName: "/", | ||
| httpAuthorization: "Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==", | ||
| ); | ||
|
|
||
| $tester | ||
| ->request( | ||
| uri: $scriptName, | ||
| address: '{{ADDR:UDS}}', | ||
| scriptFilename: "/", | ||
| scriptName: "/", | ||
| ); |
There was a problem hiding this comment.
Each request should have some expectations to verify it went through fine.
sapi/fpm/tests/gh15395.phpt
Outdated
| address: '{{ADDR:UDS}}', | ||
| scriptFilename: "/", | ||
| scriptName: "/", | ||
| headers: ["HTTP_AUTHORIZATION" => "Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ=="], |
There was a problem hiding this comment.
Note that tests should use spaces, not tab...
|
everything is alright now @bukka ? |
sapi/fpm/tests/gh15395.phpt
Outdated
| address: '{{ADDR:UDS}}', | ||
| scriptFilename: "/", | ||
| scriptName: "/", | ||
| ); |
There was a problem hiding this comment.
This still need some expectations on the response or at least the status.
But we still consider the authentication handling Basic part successful.
There was a problem hiding this comment.
It looked to me that the test might not be actually failing without the fix so I just run the test without any changes applied and it is a pass. Have you actually seen this test ever failing? Can you either let me know when it should fail or make sure that it really fails without the code changes applied?
main/SAPI.c
Outdated
| if (SG(request_info).content_type_dup) { | ||
| efree(SG(request_info).content_type_dup); | ||
| SG(request_info).content_type_dup = NULL; | ||
| } | ||
| if (SG(request_info).current_user) { | ||
| efree(SG(request_info).current_user); | ||
| SG(request_info).current_user = NULL; | ||
| } |
There was a problem hiding this comment.
What's the reason for resetting those two? They are set to NULL explicitly in sapi_activate and sapi_read_post_data for additional setting of content_type_dup.
That s the thing I have a hard time to reproduce with your framework, do you have any suggestions ? |
|
I will try to look into this properly next week and see if I can figure some test out. |
|
Create a new PR with a test that reproduces the issue: #16227 . |
But we still consider the authentication handling Basic part successful.