Skip to content

Overwrite host and webroot when forcessl is enabled#1872

Merged
bartv2 merged 2 commits into
owncloud:masterfrom
herbrechtsmeier:forcessl-with-ssl-proxy
Apr 26, 2013
Merged

Overwrite host and webroot when forcessl is enabled#1872
bartv2 merged 2 commits into
owncloud:masterfrom
herbrechtsmeier:forcessl-with-ssl-proxy

Conversation

@herbrechtsmeier

Copy link
Copy Markdown
Contributor

This patch enables the use of forcessl together with a multiple domains
reverse SSL proxy (#1099) which have different hostname
and webroot for http and https access. The code assumes that the ssl
proxy (https) hostname and webroot is configured via overwritehost and
overwritewebroot.

@karlitschek

Copy link
Copy Markdown
Contributor

looks good 👍

Comment thread lib/request.php

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please dont use <>, use !== instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But this is used all over the code or did you mean line number 17?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Raydiation Should OC_Config::getValue always use !== instead of <> for strings?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should I add a patch to this pull request or via separated pull request?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

to the pull request.

This patch enables the use of forcessl together with a multiple domains
reverse SSL proxy (owncloud#1099) which have different hostname
and webroot for http and https access. The code assumes that the ssl
proxy (https) hostname and webroot is configured via overwritehost and
overwritewebroot.
The not equal comparison (<>) of a variable with an empty string
could lead to false positive results as the compare do not check
the type and thereby could not make sure that the checked variable
is a string. The usage of the not identical comparison operator
(!==) make sure that the variable is a string and not empty.
@herbrechtsmeier

Copy link
Copy Markdown
Contributor Author

@Raydiation Is this pull request okay for you now?

@bartv2

bartv2 commented Apr 24, 2013

Copy link
Copy Markdown
Contributor

looks good, but i would do the forcessl check in serverProtocol to make it clearer when the overwriteprotocol is returned

@herbrechtsmeier

Copy link
Copy Markdown
Contributor Author

@bartv2 You need the forcessl check in all functions except serverProtocol because you need to use the overwrite values for the redirect. This is different for the protocol because it is used to detect http connections. But I can move this to the different functions if this is preferred.

@bartv2

bartv2 commented Apr 24, 2013

Copy link
Copy Markdown
Contributor

@herbrechtsmeier hmm, will take an other look

@bartv2

bartv2 commented Apr 26, 2013

Copy link
Copy Markdown
Contributor

I see when this would be usefull ...

bartv2 added a commit that referenced this pull request Apr 26, 2013
Overwrite host and webroot when forcessl is enabled
@bartv2 bartv2 merged commit 710acde into owncloud:master Apr 26, 2013
@herbrechtsmeier

Copy link
Copy Markdown
Contributor Author

This pull request should be back ported to stable5 to close #3048.

@herbrechtsmeier

Copy link
Copy Markdown
Contributor Author

@bartv2 @karlitschek Is it possible to merged this patches into stable5?

@karlitschek

Copy link
Copy Markdown
Contributor

@bartv2 What do you think?

@bartv2 bartv2 mentioned this pull request Jun 28, 2013
bartv2 added a commit that referenced this pull request Jul 8, 2013
@lock lock Bot locked as resolved and limited conversation to collaborators Aug 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants