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

[InstallDB] Modifying the check to see whether http or https protocol is being used. #3658

Merged
merged 2 commits into from
May 23, 2018

Conversation

um4r12
Copy link
Contributor

@um4r12 um4r12 commented May 10, 2018

This pull request attempts to determine whether the http or https protocol is being used. It modifies the getBaseURL() function to check if $_SERVER['HTTP_X_FORWARDED_PROTO'] is set to https rather than $_SERVER['HTTPS'].

@um4r12 um4r12 added the Release: Add to release notes PR whose changes should be highlighted in the release notes label May 10, 2018
@um4r12 um4r12 requested a review from driusan May 10, 2018 16:27
@um4r12 um4r12 removed the Release: Add to release notes PR whose changes should be highlighted in the release notes label May 10, 2018
@@ -273,7 +273,8 @@ class Installer
// This is apparently the most reliable way to figure out if
// the requery is over http or https.. $_SERVER[REQUEST_SCHEME]
// is not reliable.
$RequestScheme = isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] === 'on'
$RequestScheme = isset($_SERVER['HTTP_X_FORWARDED_PROTO'])
Copy link
Contributor

@johnsaigle johnsaigle May 10, 2018

Choose a reason for hiding this comment

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

Perhaps it would be better to check for both? Check first for $_SERVER['HTTPS'] and go to $_SERVER['HTTP_X_FORWARDED_PROTO'] if it fails

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should definitely check for both. HTTP_X_FORWARDED_PROTO is only set if there's a proxy in between the server and PHP (and that proxy is setting the field).. it's not a reliable way to detect the protocol for servers that are directly served by a web server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay cool, will add that back again. If I were to also add an additional check for server_port (443), is that too much?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if PHP provides a way to get the server port, but even if it did it wouldn't be reliable since the server could be configured to run on any port for either http or https.

I think

$RequestScheme = 'http';
if (old way of checking || x-forwarded-proto) {
 $RequestSchema = 'https';
}

should handle most configurations.

@ZainVirani
Copy link
Contributor

ZainVirani commented May 14, 2018

Script failed on my side, error log sent on slack

I think it failed for a completely unrelated reason. Someone else should test this.

@driusan
Copy link
Collaborator

driusan commented May 16, 2018

It looks good to me, but any word on @ZainVirani's problem? Has it been (better?) tested?

@driusan driusan merged commit 5c50f90 into aces:bugfix May 23, 2018
@ridz1208 ridz1208 added this to the 19.3 milestone May 28, 2018
@ridz1208 ridz1208 modified the milestones: 19.3, 20.0 Jun 20, 2018
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.

5 participants