No longer show the version number in the public by DeepDiver1975 · Pull Request #27473 · owncloud/core

DeepDiver1975 · 2017-03-23T15:41:55Z

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.

PVince81 · 2017-03-23T18:37:56Z

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 · 2017-03-23T18:38:50Z

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 · 2017-03-24T07:37:55Z

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 · 2017-03-24T07:42:40Z

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 · 2017-03-27T10:06:39Z

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

DeepDiver1975 · 2017-03-27T13:34:59Z

Make it a config option.

done @felixboehm @PVince81 @guruz @davivel @nasli

davivel · 2017-03-29T08:20:53Z

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...

PVince81 · 2017-04-03T09:20:12Z

config/config.sample.php

 'version' => '',

+/**
+ * While hardening an ownCloud instance hiding the version information in status.php


"legitimate"

"condult" => "consult"

PVince81 · 2017-04-03T09:22:55Z

core/js/config.php

 *
 */

+if (OC_User::getUser() === null) {


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.

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 ?

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

michaelstingl · 2017-04-12T08:26:10Z

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

DeepDiver1975 · 2017-04-12T12:05:52Z

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 · 2017-04-12T14:02:36Z

@guruz addressed - please test - thx

guruz

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

PVince81 · 2017-04-12T15:46:44Z

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 · 2017-04-13T07:38:26Z

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

PVince81 · 2017-04-13T07:41:35Z

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

DeepDiver1975 · 2017-04-13T07:41:41Z

👍

PVince81 · 2017-04-13T07:45:21Z

imprecise response.

(T)icket or (P)r today ?

DeepDiver1975 · 2017-04-13T08:51:44Z

P

PVince81 · 2017-04-19T06:58:07Z

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

lock · 2019-08-03T13:02:38Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

DeepDiver1975 added the 3 - To Review label Mar 23, 2017

DeepDiver1975 added this to the 10.0 milestone Mar 23, 2017

DeepDiver1975 force-pushed the kill-status.php branch from 29977b7 to 0b02b10 Compare March 27, 2017 13:33

This was referenced Mar 29, 2017

Adapt app for hidden status.php owncloud/android#1931

Closed

Adapt app for hidden status.php owncloud/ios-legacy#867

Closed

PVince81 reviewed Apr 3, 2017

View reviewed changes

guruz mentioned this pull request Apr 10, 2017

status.php: Be lenient when version parsing, also parse version from server capabilities owncloud/client#5691

Closed

jesmrec mentioned this pull request Apr 12, 2017

Support hidden version in status.php taking version from capabilities… owncloud/android#1936

Merged

No longer show the version number in the public

5c93377

DeepDiver1975 force-pushed the kill-status.php branch from 3711790 to 5c93377 Compare April 12, 2017 11:18

guruz suggested changes Apr 12, 2017

View reviewed changes

fix print out of version in capabilities

4abd357

guruz approved these changes Apr 12, 2017

View reviewed changes

PVince81 merged commit 54150e7 into master Apr 13, 2017

PVince81 deleted the kill-status.php branch April 13, 2017 07:41

DeepDiver1975 mentioned this pull request Apr 13, 2017

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

Merged

9 tasks

SamuAlfageme mentioned this pull request Jul 31, 2017

Federation: add trusted servers rely on status.php's exposed version #28536

Closed

butonic mentioned this pull request Mar 23, 2018

status.php security: information disclosure #19515

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019

Conversation

DeepDiver1975 commented Mar 23, 2017

Description

Types of changes

Checklist:

Uh oh!

PVince81 commented Mar 23, 2017

Uh oh!

PVince81 commented Mar 23, 2017

Uh oh!

davivel commented Mar 24, 2017

Uh oh!

nasli commented Mar 24, 2017

Uh oh!

felixboehm commented Mar 27, 2017

Uh oh!

DeepDiver1975 commented Mar 27, 2017

Uh oh!

davivel commented Mar 29, 2017

Uh oh!

PVince81 Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 Apr 12, 2017

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 Apr 12, 2017

Choose a reason for hiding this comment

Uh oh!

michaelstingl commented Apr 12, 2017

Uh oh!

DeepDiver1975 commented Apr 12, 2017

Uh oh!

guruz left a comment

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 commented Apr 12, 2017

Uh oh!

guruz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Apr 12, 2017

Uh oh!

DeepDiver1975 commented Apr 13, 2017

Uh oh!

PVince81 commented Apr 13, 2017

Uh oh!

DeepDiver1975 commented Apr 13, 2017

Uh oh!

PVince81 commented Apr 13, 2017

Uh oh!

DeepDiver1975 commented Apr 13, 2017

Uh oh!

PVince81 commented Apr 19, 2017

Uh oh!

lock bot commented Aug 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

guruz left a comment •

edited

Loading