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

No longer show the version number in the public #27473

Merged
merged 2 commits into from Apr 13, 2017

Conversation

Projects
None yet
7 participants
@DeepDiver1975
Member

DeepDiver1975 commented Mar 23, 2017

Description

Don't expose server version

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@DeepDiver1975 DeepDiver1975 added this to the 10.0 milestone Mar 23, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 23, 2017

Member

This will likely break federated sharing because federated sharing current contains a hack that checks if the remote instance is an ownCloud instance by pinging the status.php or to see if the instance is up after getting a 404/403 from a regular endpoint. I'm not sure is such calls are authenticated.

Member

PVince81 commented Mar 23, 2017

This will likely break federated sharing because federated sharing current contains a hack that checks if the remote instance is an ownCloud instance by pinging the status.php or to see if the instance is up after getting a 404/403 from a regular endpoint. I'm not sure is such calls are authenticated.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 23, 2017

Member
apps/files_sharing/lib/Controllers/ExternalSharesController.php|135| $this->testUrl('https://' . $remote . '/status.php', true)
apps/files_sharing/lib/Controllers/ExternalSharesController.php|141| $this->testUrl('http://' . $remote . '/status.php', true)
apps/files_sharing/lib/External/Storage.php|255| || $this->testRemoteUrl($this->remote . '/status.php');
apps/files_sharing/lib/External/Storage.php|296| if(defined('PHPUNIT_RUN') || !$this->testRemoteUrl($this->getRemote() . '/status.php')) {
apps/federation/lib/TrustedServers.php|222| $url . '/status.php',
apps/federation/tests/TrustedServersTest.php|283| $this->httpClient->expects($this->once())->method('get')->with($server . '/status.php')
apps/federation/tests/TrustedServersTest.php|320| $this->httpClient->expects($this->once())->method('get')->with($server . '/status.php')
lib/private/Setup.php|460| $content .= "\n  RewriteCond %{REQUEST_FILENAME} !/status.php";
core/js/maintenance-check.js|1| // Check every 20 seconds via status.php if maintenance is over
core/js/maintenance-check.js|6| request.open("GET", OC.webroot+'/status.php', true);
Member

PVince81 commented Mar 23, 2017

apps/files_sharing/lib/Controllers/ExternalSharesController.php|135| $this->testUrl('https://' . $remote . '/status.php', true)
apps/files_sharing/lib/Controllers/ExternalSharesController.php|141| $this->testUrl('http://' . $remote . '/status.php', true)
apps/files_sharing/lib/External/Storage.php|255| || $this->testRemoteUrl($this->remote . '/status.php');
apps/files_sharing/lib/External/Storage.php|296| if(defined('PHPUNIT_RUN') || !$this->testRemoteUrl($this->getRemote() . '/status.php')) {
apps/federation/lib/TrustedServers.php|222| $url . '/status.php',
apps/federation/tests/TrustedServersTest.php|283| $this->httpClient->expects($this->once())->method('get')->with($server . '/status.php')
apps/federation/tests/TrustedServersTest.php|320| $this->httpClient->expects($this->once())->method('get')->with($server . '/status.php')
lib/private/Setup.php|460| $content .= "\n  RewriteCond %{REQUEST_FILENAME} !/status.php";
core/js/maintenance-check.js|1| // Check every 20 seconds via status.php if maintenance is over
core/js/maintenance-check.js|6| request.open("GET", OC.webroot+'/status.php', true);
@davivel

This comment has been minimized.

Show comment
Hide comment
@davivel

davivel Mar 24, 2017

Member

This breaks log-in with mobile apps and probably desktop client. Requires immediate patch-and-release.

@felixboehm , is this still in discussion or we'll be in OC 10.0 at any cost?

cc @michaelstingl , @nasli , @jesmrec , @davigonz

Member

davivel commented Mar 24, 2017

This breaks log-in with mobile apps and probably desktop client. Requires immediate patch-and-release.

@felixboehm , is this still in discussion or we'll be in OC 10.0 at any cost?

cc @michaelstingl , @nasli , @jesmrec , @davigonz

@nasli

This comment has been minimized.

Show comment
Hide comment
@nasli

nasli Mar 24, 2017

Member

On iOS app to validate the url we only check for "installed" parameter in status.php. You can log in but the server will have the wrong features supported by the server

To update the features supported(capabilities,cookies,forbiddencharacters,FedShares), it is used "version" parameter.

Member

nasli commented Mar 24, 2017

On iOS app to validate the url we only check for "installed" parameter in status.php. You can log in but the server will have the wrong features supported by the server

To update the features supported(capabilities,cookies,forbiddencharacters,FedShares), it is used "version" parameter.

@felixboehm

This comment has been minimized.

Show comment
Hide comment
@felixboehm

felixboehm Mar 27, 2017

Contributor

There is some danger and we need to be fast. Make it a config option.

Contributor

felixboehm commented Mar 27, 2017

There is some danger and we need to be fast. Make it a config option.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Mar 27, 2017

Member

Make it a config option.

done @felixboehm @PVince81 @guruz @davivel @nasli

Member

DeepDiver1975 commented Mar 27, 2017

Make it a config option.

done @felixboehm @PVince81 @guruz @davivel @nasli

@davivel

This comment has been minimized.

Show comment
Hide comment
@davivel

davivel Mar 29, 2017

Member

So, if configuration is "don't hide status.php", nothing changes.

If configuration is "hide it", status.php is accessible but shows no info? Sorry, my server side understanding... you know...

Member

davivel commented Mar 29, 2017

So, if configuration is "don't hide status.php", nothing changes.

If configuration is "hide it", status.php is accessible but shows no info? Sorry, my server side understanding... you know...

Show outdated Hide outdated config/config.sample.php
@@ -80,6 +80,12 @@
'version' => '',
/**
* While hardening an ownCloud instance hiding the version information in status.php
* can be a legitim step. Please condult the documentation before enabling this.

This comment has been minimized.

@PVince81

PVince81 Apr 3, 2017

Member

"legitimate"

"condult" => "consult"

@PVince81

PVince81 Apr 3, 2017

Member

"legitimate"

"condult" => "consult"

Show outdated Hide outdated core/js/config.php
@@ -37,6 +37,11 @@
*
*/
if (OC_User::getUser() === null) {

This comment has been minimized.

@PVince81

PVince81 Apr 3, 2017

Member

This page returns some config settings that might be used by some JS code in the public link page for example (ex: heartbeat interval).
Might need to split this page into two, one for the login-only settings and one for the generic settings.

@PVince81

PVince81 Apr 3, 2017

Member

This page returns some config settings that might be used by some JS code in the public link page for example (ex: heartbeat interval).
Might need to split this page into two, one for the login-only settings and one for the generic settings.

This comment has been minimized.

@PVince81

PVince81 Apr 12, 2017

Member

I wanted to test this... Even when not logged in, config.php (aja oc.js) is still loaded in a public link page.

This if statement doesn't work because getUser() returns false... If I fix it then it breaks the public link page when not logged in. Breaks because the public link page needs access to several JS variables provided by this, like theme information, some app configs, etc.

If we want to move forward with this PR I suggest we discard this change in core/js/config.php.

@DeepDiver1975 agree ?

@PVince81

PVince81 Apr 12, 2017

Member

I wanted to test this... Even when not logged in, config.php (aja oc.js) is still loaded in a public link page.

This if statement doesn't work because getUser() returns false... If I fix it then it breaks the public link page when not logged in. Breaks because the public link page needs access to several JS variables provided by this, like theme information, some app configs, etc.

If we want to move forward with this PR I suggest we discard this change in core/js/config.php.

@DeepDiver1975 agree ?

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Apr 12, 2017

Member

I'll take care .... pushing ....

@DeepDiver1975

DeepDiver1975 Apr 12, 2017

Member

I'll take care .... pushing ....

@michaelstingl

This comment has been minimized.

Show comment
Hide comment
@michaelstingl

michaelstingl Apr 12, 2017

What is the status here? (client and mobile development/qa depend on it)

What is the status here? (client and mobile development/qa depend on it)

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Apr 12, 2017

Member

What is the status here? (client and mobile development/qa depend on it)

ready for the final review and can be merged for oc10 from my pov

Member

DeepDiver1975 commented Apr 12, 2017

What is the status here? (client and mobile development/qa depend on it)

ready for the final review and can be merged for oc10 from my pov

@guruz

👎

04-12 15:24:10:906 1562546 OCC::JsonApiJob::finished: "{\"ocs\":{\"meta\":{\"status\":\"ok\",\"statuscode\":100,\"message\":\"OK\",\"totalitems\":\"\",\"itemsperpage\":\"\"},\"data\":{\"version\":{\"major\":10,\"minor\":0,\"micro\":0,\"string\":\"10.0.0 beta2\",\"edition\":\"Community\"},\"capabilities\":{\"core\":{\"pollinterval\":60,\"webdav-root\":\"remote.php\\/webdav\",\"status\":{\"installed\":\"true\",\"maintenance\":\"false\",\"needsDbUpgrade\":\"false\",\"version\":\"\",\"versionstring\":\"\",\"edition\":\"\",\"productname\":\"\"}},\"dav\":{\"chunking\":\"1.0\"},\"files_sharing\":{\"api_enabled\":true,\"public\":{\"enabled\":true,\"password\":{\"enforced\":false},\"expire_date\":{\"enabled\":false},\"send_mail\":false,\"upload\":true},\"user\":{\"send_mail\":false},\"resharing\":true,\"group_sharing\":true,\"federation\":{\"outgoing\":true,\"incoming\":true}},\"checksums\":{\"supportedTypes\":[\"SHA1\"],\"preferredUploadType\":\"SHA1\"},\"files\":{\"bigfilechunking\":true,\"blacklisted_files\":[\".htaccess\"],\"undelete\":true,\"versioning\":true}}}}}"

Hides version also in capabilities

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Apr 12, 2017

Member

@guruz addressed - please test - thx

Member

DeepDiver1975 commented Apr 12, 2017

@guruz addressed - please test - thx

@guruz

guruz approved these changes Apr 12, 2017 edited

Tested, works.. desktop client fix will be in 2.3.2

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 12, 2017

Member

Now thinking of it, without the "oc.js" fix this PR becomes pointless as someone who really wants to know the version could just ping that...

On the other hand we could merge this and then work on finding a solution to split "oc.js" into config values that are required for the UI to work and others to be kept secret.

The middle ground would be to also check the flag. If the flag to set the version is hidden, then remove the keys "version" and "versionstring" from the oc.js response.

Does that make sense ?

Member

PVince81 commented Apr 12, 2017

Now thinking of it, without the "oc.js" fix this PR becomes pointless as someone who really wants to know the version could just ping that...

On the other hand we could merge this and then work on finding a solution to split "oc.js" into config values that are required for the UI to work and others to be kept secret.

The middle ground would be to also check the flag. If the flag to set the version is hidden, then remove the keys "version" and "versionstring" from the oc.js response.

Does that make sense ?

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Apr 13, 2017

Member

I vote for merge. We can address oc.js in a follow up pr

Member

DeepDiver1975 commented Apr 13, 2017

I vote for merge. We can address oc.js in a follow up pr

@PVince81 PVince81 merged commit 54150e7 into master Apr 13, 2017

4 checks passed

Scrutinizer 1 new issues, 1 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the kill-status.php branch Apr 13, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 13, 2017

Member

Will the follow up PR come today ? If not, we need a ticket to not forget

Member

PVince81 commented Apr 13, 2017

Will the follow up PR come today ? If not, we need a ticket to not forget

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
Member

DeepDiver1975 commented Apr 13, 2017

👍

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 13, 2017

Member

imprecise response.

(T)icket or (P)r today ?

Member

PVince81 commented Apr 13, 2017

imprecise response.

(T)icket or (P)r today ?

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
Member

DeepDiver1975 commented Apr 13, 2017

P

@DeepDiver1975 DeepDiver1975 referenced this pull request Apr 13, 2017

Merged

If not logged in -> 401 and bye-bye #27640

3 of 9 tasks complete
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 19, 2017

Member

#27669 having the fields present with empty values doesn't look that good.

Member

PVince81 commented Apr 19, 2017

#27669 having the fields present with empty values doesn't look that good.

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