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

Added httpsOn function how is checking if the connection fixes #4473 #4474

Merged
merged 4 commits into from Jan 8, 2018

Conversation

Projects
None yet
4 participants
@albertlast
Collaborator

albertlast commented Jan 6, 2018

to the webserver or load balancer is done by https.

Since the check got more meat to check and it used on many placed i decide to create a function how got
the complet logic.
I'm not fully able to test this change because i got no ssl env.

Could also fix the issue #4473

Added httpsOn function how is checking if the connection
to the webserver is done by https or to the load balancer.
Signed-off-by: albertlast albertlast@hotmail.de

@albertlast albertlast referenced this pull request Jan 6, 2018

Closed

Error with Forcing SSL #4473

albertlast added some commits Jan 6, 2018

add the function to the installer
Signed-off-by: albertlast albertlast@hotmail.de
Remove the function in the installer and place the code directly
Signed-off-by: albertlast albertlast@hotmail.de
@Sefank

Sefank approved these changes Jan 6, 2018

I've tested and the bug was fixed.

@albertlast albertlast changed the title from Added httpsOn function how is checking if the connection to Added httpsOn function how is checking if the connection fixes #4473 Jan 6, 2018

less space
Signed-off-by: albertlast albertlast@hotmail.de
@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Jan 6, 2018

Member

Great catch on that. Load balancers would have indeed failed without that forward check.

LGTM

Member

jdarwood007 commented Jan 6, 2018

Great catch on that. Load balancers would have indeed failed without that forward check.

LGTM

if (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on')
$secure = true;
elseif (!empty($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https' || !empty($_SERVER['HTTP_X_FORWARDED_SSL']) && $_SERVER['HTTP_X_FORWARDED_SSL'] == 'on')

This comment has been minimized.

@Sesquipedalian

Sesquipedalian Jan 7, 2018

Member

Rather than elseif, why not just use || in the if's condition?

@Sesquipedalian

Sesquipedalian Jan 7, 2018

Member

Rather than elseif, why not just use || in the if's condition?

This comment has been minimized.

@albertlast

albertlast Jan 7, 2018

Collaborator

first because it's suck to read,
secondly forward stuff is because of load balancer.(to make clear where the https is)
third performance wise shouldn't be a point -> read able > performance

@albertlast

albertlast Jan 7, 2018

Collaborator

first because it's suck to read,
secondly forward stuff is because of load balancer.(to make clear where the https is)
third performance wise shouldn't be a point -> read able > performance

This comment has been minimized.

@Sesquipedalian

Sesquipedalian Jan 8, 2018

Member

Fair enough.

@Sesquipedalian

Sesquipedalian Jan 8, 2018

Member

Fair enough.

@Sesquipedalian Sesquipedalian merged commit cef4341 into SimpleMachines:release-2.1 Jan 8, 2018

2 checks passed

Scrutinizer 7 new issues, 1 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@albertlast albertlast deleted the albertlast:SSLCheck branch Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment