Accept- or Deny-Link of Notifications Lack Webroot Part #22786

Closed
dragotin opened this Issue Mar 2, 2016 · 25 comments

Projects

None yet

7 participants

@dragotin
Member
dragotin commented Mar 2, 2016

A GET to ocs/v2.php/apps/notifications/api/v1/notifications?format=json returns a json list of notifications if there are any, for example because a remote share was done for you.

On my installation, which lives in http://ip/oc/, the returned accpet- and deny-links look like

"actions": [
                    {
                        "label": "Decline", 
                        "link": "http://172.x.y.z/ocs/v1.php/apps/files_sharing/api/v1/remote_shares/pending/8", 
                        "primary": false, 
                        "type": "DELETE"
                    }, 

As one can easily spot, the link does not contain the oc which is the webroot of the ownCloud. That is the problem.

I drilled that down to function public function getAbsoluteURL($url) in lib/private/urlgenerator.php, it seems that \OC::WEBROOT is empty there for some reason on that installation.

Expected behaviour

The Accept- or Deny-Link of a notification should contain the webroot

Actual behaviour

Tell us what happens instead: it does not.

Server configuration

Where did you install ownCloud from:
git SHA 2773b2b

List of activated apps:

The process control (PCNTL) extensions are required in case you want to interrupt long running commands - see http://php.net/manual/en/book.pcntl.php
Enabled:
  - activity: 2.3.0
  - comments: 0.3.0
  - dav: 0.2.0
  - federatedfilesharing: 0.2.0
  - files: 1.5.0
  - files_pdfviewer: 0.5
  - files_sharing: 0.10.0
  - files_texteditor: 0.4
  - files_trashbin: 0.9.0
  - files_versions: 1.3.0
  - gallery: 0.5.4
  - notifications: 0.3.0
  - provisioning_api: 0.5.0
  - systemtags: 0.3.0
  - updatenotification: 0.2.0
Disabled:
  - announcementcenter
  - encryption
  - federation
  - files_external
  - kraft
  - maps
  - testing
  - user_ldap

The content of config/config.php:

$CONFIG = array (
  'instanceid' => 'oc19elwxrph1',
  'passwordsalt' => 'z4UGk1qGXC3O2NZJ7lyyjqBpTJlGaP',
  'secret' => 'qEx5J5pBOlFlQdyxItX64zjGWBIR+SIK3hwmZVy5erpzWKyh',
  'trusted_domains' =>
  array (
    0 => 'localhost',
    1 => '172.x.y.z',
  ),
  'datadirectory' => '/home/kf/oC/core/data',
  'overwrite.cli.url' => 'http://172.x.y.z/oc',
  'overwritewebroot' => '/oc',
  'dbtype' => 'mysql',
  'version' => '9.1.0.0',
  'dbname' => 'ocm_1',
  'dbhost' => 'localhost',
  'dbtableprefix' => 'oc_',
  'dbuser' => 'oc_admin',
  'dbpassword' => 'xxy',
  'logtimezone' => 'UTC',
  'installed' => true,
  'theme' => '',
  'loglevel' => 0,
  'maintenance' => false,
);

@nickvergessen @LukasReschke

Needless to say that this blocks a client gold ticket, so please handle with high severity.

@PVince81
Collaborator
PVince81 commented Mar 3, 2016

@dragotin mind posting a link to that showstopper ticket, for reference ?

@PVince81 PVince81 added the sev2-high label Mar 3, 2016
@PVince81 PVince81 added this to the 9.0.1-next-maintenance milestone Mar 3, 2016
@PVince81
Collaborator
PVince81 commented Mar 3, 2016

@nickvergessen I suppose that if the links are wrong they also wouldn't work properly in the web UI ?
In our tests they did work, but we didn't try with "overwritewebroot" set.

@dragotin
Member
dragotin commented Mar 3, 2016

the client issue is owncloud/client#3733

Indeed, the webinterface does also not work.

@PVince81
Collaborator
PVince81 commented Mar 3, 2016

Strange. If OC webroot is broken, then many other things should be broken as well.
I hope @LukasReschke can shed some light on this.

@dragotin
Member
dragotin commented Mar 3, 2016

Well, I assume its only in my installation for some reason, otherwise more people would have spotted it I guess...

@PVince81
Collaborator
PVince81 commented Mar 3, 2016

I tried this locally on my dev instance with a config closer to yours:

  'trusted_domains' => 
  array (
    0 => 'localhost',
    1 => 'laptop.vincentpetry.net',
  ),
  'overwrite.cli.url' => 'http://laptop.vincentpetry.net/owncloud',
  'overwritewebroot' => '/owncloud',

Then I accepted a fed share in the web UI and checked the network console.
The URL became http://localhost/owncloud/ocs/v1.php/apps/files_sharing/api/v1/remote_shares/pending/2 which is correct.

@PVince81
Collaborator
PVince81 commented Mar 3, 2016

This was when creating and accepting the share using the first trusted domain "http://localhost/owncloud!.

I then tried again creating the share with "http://laptop.vincentpetry.net/owncloud". After that, if I open the notifications from that domain, the URL points to the other domain: http://localhost/owncloud/ocs/v1.php/apps/files_sharing/api/v1/remote_shares/pending/3

Looks like this could be yet another issue. The webroot part is there but the domain is different.

@dragotin which URL did you use when testing, the one with "localhost" or the IP ?

You might be able to quickfix this by removing "localhost" from the trusted domain list.

@dragotin
Member
dragotin commented Mar 3, 2016

@PVince81 I used the IP type consistently because I have a mitmproxy in between. Now I changed the trusted domain array to contain the IP only, but still the same effect: Accept and deny url are without webroot.

@nickvergessen
Contributor

Btw you need to newly create a remote share all the time, because URLs are generated on sharing, not on reading notifications.

@dragotin
Member
dragotin commented Mar 3, 2016

yes. Retried with a different folder to make sure but same result, read from table oc_notifications.

@nickvergessen
Contributor

Btw you are missing the "Steps" section in your report.
What do you type into the sharing dialog field?

@dragotin
Member
dragotin commented Mar 3, 2016

@nickvergessen: I type kf@ip/oc where ip is the real ip address.

@dragotin
Member
dragotin commented Mar 4, 2016

Tested again with this config file:

...
  'trusted_domains' =>
  array (
      0 => '192.168.178.93',
  ),
  'datadirectory' => '/home/kf/oC/core/data',
  'overwrite.cli.url' => 'http://192.168.178.93/oc',
  'overwritewebroot' => '/oc',
...

and adding a share user kf@localhost/oc. The resulting link in the table oc_notifications results in

http:\/\/localhost\/ocs\/v1.php\/apps\/files_sharing\/api\/v1\/remote_shares\/pending\/10

as you see, the oc webroot is missing.

@dragotin
Member
dragotin commented Mar 4, 2016

and finally tried again with the ip address as sharee like kf@192.168.178.93/oc with the same config.php. It is a valid share in the web UI, but the Decline and Accept link is still lacking the oc webroot.

@PVince81
Collaborator
PVince81 commented Mar 7, 2016

Does it work without mitmproxy ?

How did you configure mitmproxy ?

@dragotin
Member
dragotin commented Mar 7, 2016

Now I set up a similar setup from git on a different machine with core on SHA 19dc02b . The ownCloud is reachable at http://localhost/oc. No mitmproxy involved, I end up with exactly the same Accept- and Decline links as on the other machine, missing the webroot path oc when I successfully share to the user 'kf@localhost/oc'.

@PVince81
Collaborator
PVince81 commented Mar 7, 2016

Did you also try with only using localhost, without any "overwrite*" settings ?

I tried the SHA you mentionned with stable9 branch of notifications app.

The URL is correct in both the web UI network console and CLI:

% curl -X GET 'http://vincent:test@localhost/owncloud/ocs/v2.php/apps/notifications/api/v1/notifications?format=json' | jsonlint -p

{
  "ocs": {
    "meta": {
      "status": "ok",
      "statuscode": 200,
      "message": null
    },
    "data": [
      {
        "notification_id": 1,
        "app": "files_sharing",
        "user": "vincent",
        "datetime": "2016-03-07T14:47:57+00:00",
        "object_type": "remote_share",
        "object_id": "1",
        "subject": "You received \"\\/test\" as a remote share from admin@localhost\\/owncloud",
        "message": "",
        "link": "",
        "actions": [
          {
            "label": "Decline",
            "link": "http:\\/\\/localhost\\/owncloud\\/ocs\\/v1.php\\/apps\\/files_sharing\\/api\\/v1\\/remote_shares\\/pending\\/1",
            "type": "DELETE",
            "primary": false
          },
          {
            "label": "Accept",
            "link": "http:\\/\\/localhost\\/owncloud\\/ocs\\/v1.php\\/apps\\/files_sharing\\/api\\/v1\\/remote_shares\\/pending\\/1",
            "type": "POST",
            "primary": true
          }
        ]
      }
    ]
  }
}

I shared from user "admin" to "vincent@localhost/owncloud".

Do you have any redirects or proxy settings in your Apache config ?

@PVince81
Collaborator
PVince81 commented Mar 7, 2016

@dragotin try this:

  1. apply this patch: https://gist.github.com/PVince81/d3240076e0624bb81c68
  2. run the OCS command from above as you did before
  3. sudo find /tmp -name debug.txt to find the file (systemd puts it in a subdir)
  4. Check the contents

In my case, the output looks like this:

± % sudo cat /tmp/systemd-private-3162c60692c04759bad7eaf5ed955dad-apache2.service-dTCDNE/tmp/debug.txt
SCRIPT_NAME: /owncloud/ocs/v2.php
SCRIPT_FILENAME: /owncloud/ocs/v2.php
OC::$SUBURI before: /ocs/v2.php
OC::$SUBURI after: /ocs/v2.php
OC::$WEBROOT from path 1: /owncloud
@dragotin
Member
dragotin commented Mar 7, 2016

This is my output:

SCRIPT_NAME: /oc/ocs/v2.php
SCRIPT_FILENAME: /oc/ocs/v2.php
OC::$SUBURI before: /ocs/v2.php
OC::$SUBURI after: /ocs/v2.php
OC::$WEBROOT from path 1: /oc
@LukasReschke LukasReschke self-assigned this Mar 8, 2016
@LukasReschke
Member

It really is a bug in core existent since ownCloud 7.0.0.

6292aa5 added a check to \OCP\IURLGenerator::getAbsoluteURL($url) whether OC::$WEBROOT is already prepended to $url. It does this kinda naively and is thus error prone.

So in case ownCloud is installed under /oc, OC::$WEBROOT will be /oc, then the URL generator checks substr($url, 0, strlen(\OC::$WEBROOT)) === \OC::$WEBROOT. Since $url is /ocs/apps/notifications/ it will now compare /oc with /oc and won't append the webroot => 💣

Let me think how we can fix that without breaking other stuff. Best solution would be to remove this "feature". It really does more harm than good.

@nickvergessen
Contributor

check for substr($url, 0, strlen(\OC::$WEBROOT) + 1) === \OC::$WEBROOT . '/'
that should solve @dragotin but yes, if someone installed in ocs/ he still has a problem...

@LukasReschke LukasReschke added a commit that referenced this issue Mar 8, 2016
@LukasReschke LukasReschke Always append the WebRoot
6292aa5 added a check to `\OCP\IURLGenerator::getAbsoluteURL($url)` whether `OC::$WEBROOT` is already prepended to `$url`. It does this kinda naively and is thus error prone.

So in case ownCloud is installed under `/oc`, `OC::$WEBROOT` will be `/oc`, then the URL generator checks `substr($url, 0, strlen(\OC::$WEBROOT)) === \OC::$WEBROOT`. Since `$url` is `/ocs/apps/notifications/` it will now compare `/oc` with `/oc` and won't append the webroot => 💣

Considering that this is rather a very error prone feature which is not used from a search over the core code I'd recommend to remove this logic.

Fixes #22786
100594e
@LukasReschke
Member

I'd recommend to get rid of this in total. Whoever calls getAbsoluteURL with an absolute URL already really misuses this URL.

PR at #22954. Let's discuss there…

@dragotin
Member
dragotin commented Mar 8, 2016

hmm, actually I often have installations in /ocs which means owncloud stable here. That used to work. This problem is newer than 7.0.0.

@LukasReschke
Member

hmm, actually I often have installations in /ocs which means owncloud stable here. That used to work. This problem is newer than 7.0.0.

I'd more say that the actual reason is that \OCP\IURLGenerator::getAbsoluteURL was not called for the OCS API before. Thus no wrong URLs were generated 🙈

@rperezb
Member
rperezb commented Mar 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment