owncloud-archive / shorty

15 stars 12 forks source link

States don't seem to work properly anymore #77

Closed arkascha closed 9 years ago

arkascha commented 9 years ago

@fredl99 reported this issue in over there: https://github.com/owncloud/shorty/issues/76 I took the liberty to separated the issue here into a separate entry.

The selected states don't seem to work properly anymore. Any of "closed", "private" or "shared" results in a "forbidden" message when calling the short URL, regardless if or which user is logged in. No login-page appears either. The only working status at the moment is "public".

arkascha commented 9 years ago

Sorry, I can not reproduce the issue you report. For me the behavior is as expected: when accessing a shared Shorty from outside an owncloud session having access I am prompted for authentication.

Indeed the authentication strategy has been reimplemented shortly ago. So far I only had positive results on that modification. So I take your report seriously. @fredl99 : Can you provide any more details / info? Especially which browser you are using? The "Forbidden" message you receive is the result in a failed authentication attempt. So to me this smells like although requested to authenticate your browser does not ask you for authentication...

fredl99 commented 9 years ago

Thanks for the reply, I'm currently trying with different browsers and will report asap.

fredl99 commented 9 years ago

I have tried Google-Chrome, Iceweasel, Qupzilla and the Gnome-Browser (which afair is based on Epiphany). No matter what, it's the same behavior with all of these. You're right, none of them asks for authentication. But after an already successful login to my site this shouldn't be necessary when I request a short URL within the same browser window, right? Don't know where to start looking, could it depend on a .htaccess file? I cannot imagine that, because it only contains a rewrite-rule, and everything else works flawless, also with public Shortys. :grey_question:

When has the authentication strategy changed, may I ask? (commit?)

arkascha commented 9 years ago

This is the commit that reimplemented the authentication strategy:

commit d023c6d58c271ee3b30005e6e9e1b73a4108f909 Author: arkascha github@christian-reiner.info Date: Sun Oct 19 20:38:46 2014 +0200 Shorty: Fix relay access authorization.

The different is that the authentication strategy has been changed when an authentication is required. It uses basic http authentication now. This results in a separate modal authentication dialog displayed by the browser, not on the owncloud login dialog as before. This prevents some routing issues and is logically cleaner in my eyes. In the end such an access has nothing to do with using the shorty UI itself.

I have no idea what the problem is in your case. From your description I understand that you are already logged in, so no authentication is required. I have no idea why the access fails in that case. (Expect if the target denies access, which would be relayed) Please try to access a Shorty using a browser where you are NOT logged in to owncloud. best for this is to use an anonymous session, most browser offer that feature these days. Does that access work as expected at least?

fredl99 commented 9 years ago

It's always the same, regardless which browser or if I'm already authenticated or not. The "Forbidden" message comes immediately, except when the Shorty is "public".

I've reverted to the commit just before the introduction of HTTP auth, and it's the same. So it doesn't look like shorty's fault, but in my server config. My OC is one level below httpd's root-dir an the shortys are created exactly there. Also I'm strictly using https. (That's why I don't like the "unsecure elements" warning in other places, BTW.) Main: https://SERVER_NAME/oc/ Shortys: https://SERVER_NAME/oc/<shorty-id>

Funny that it works with public shortys. Apart from that it would look like trying to get a directory listing, which is of course "Forbidden". That would mean it's using a wrong path, but not with public shortys. Hmm..

arkascha commented 9 years ago

https insecure element warning is tracked separately in issue #87 now.

arkascha commented 9 years ago

This sounds like you are using the static backend and configured it such that it points to the folder your owncloud installation is located in. That indeed will not work. Note that this would also lead to collisions! By chance a Shorty might get an id that is (coincidally) identical to the name of a folder or file inside the owncloud installation! The base URL for your Shortys should be separate of your installation!

arkascha commented 9 years ago

Hm... maybe such configuration should be prevented in an explicit manner. It never occurred to me one could try such a configuration. Classical case of "creator blindness" :-)

fredl99 commented 9 years ago

This sounds like you are using the static backend and configured it such that it points to the folder your owncloud installation is located in.

Correct.

By chance a Shorty might get an id that is (coincidally) identical to the name of a folder or file inside the owncloud installation!

Well, I was heavily relying on the randomness of shorty-ids and the correct behavoiur of OC-files and folders just in case. But basically you're right, I will re-configure with this in mind. Thanks for the hint!

It never occurred to me one could try such a configuration.

"Make it fool-proof and a better fool will come along." :smiley:

arkascha commented 9 years ago

I separated the issue of the backend configuration in to issue #89.

Feel free to reopen this issue if reconfiguring the setup does not solve your problem :-)

arkascha commented 9 years ago

BTW: the Shorty IDs are not random at all! and that is important! They are guaranteed to be unique over time throughout an installation (note: installation, not user). This is important, since ownCloud is a multi user environment. And collisions here would be a privacy issue.

fredl99 commented 9 years ago

Feel free to reopen this issue Seems like I can't do that: arkascha opened this issue 4 days ago...

I'm afraid my problem continues to exist. Tried several variations, but the result is always a "Forbidden" message, when it should work logically.

While Shortys directly within the OC dir might have been a questionable idea, that alone doesn't seem to be the cause. I've tried shortys directly under doc-root as well as with non-existent sub-dirs, always with according rewrite-rules. No luck.

BTW: I took precautions against collisions with existing files/folders. One of them was a sort of "salt" by preceding the <shorty-id> with some characters, like this: https://domain.tld/e3<shorty id> -> there is no file/dir named e3* in doc-root. Same is true with https://domain.tld/oc/e3<shorty id> (existing folder with no e3* files/folders) or https://domain.tld/e3/<shorty id> (subfolder e3/ doesn't exist).

Common facts:

arkascha commented 9 years ago

OK, that brings us forth a step!

Whatever. In both cases best would be to take a look at the log files. Not sure if you have access.

Any entries there during a request?

fredl99 commented 9 years ago

Since "public" Shortys work instantly, the service must be accessible and the request is resolved. The only difference is when the state is any other than public. So I would suspect the obstacle somewhere where a decision is to be made if the caller is authorized to access the shorty. In other words, if an authentication is valid or should be renewed. It looks like this step is missing and the answer is always "no access".

The logs don't show any useful entries. Only errors when resolution fails due to improper rewrite-rules. Interestingly enough there's no logging of succesful access, either. Also no 40x-errors, which would make sense if http-auth takes place.

BTW: Can you catch this page here with the shortlet? The URL field is flashing, until I remove or escape the #... ;-)

arkascha commented 9 years ago

Well, there must be a log entry in the access log file about the request. Otherwise you really have an issue with your system :-)

Most likely there are no 40x errors in the log files because the error is not created by the http server, but by the app.

There most likely is an easy solution for this, but I don't see it right now with the limited information I have. This might very well be a bug, so I take this serious. But I haven't got a clue. Could you grant me access to your system somehow? Temporarily? I know.... that is always a problem. But we have to dig deeper here...

I re-checked the code again. The strategy is this: a 403 is thrown for all states expect public. This matches this issue. The purpose of the 403 is that that the browser retries the request after having asked for authentication credentials. This, because it received according http headers asking for authentication. It would be interesting to see if such headers are sent to your browser. That is why I ask for access. You can also check yourself though: if you have a network sniffer at hand block the specific connection and dump the conversation. but since you are using https you have to decrypt. Not easy. You could use something like 'Charles' for this, but you'd have to install the server certificate inside Charles to be able to decrypt...

Sorry, too tired right now, tomorrow is another day.

Oh: happy new year :-)


BTW: I have no problems at all with the link you mention. I can use it, create a Shorty for it, using the Shortlet or pasting it directly. There is no flashing going on and the target is called when accessing the Shorty.

fredl99 commented 9 years ago

Happy New Year to you and family, too!

there must be a log entry in the access log

I was wondering all the time, why tail -f is so silent. Then I read this. Now it's clear. No useful debugging possible that way. I have requested error-logging now. This should help.

It's not a question of trust, but I guess at the moment you wouldn't see more than I on the server.

I can find a lot of 40x codes in the recent log, but it's hours old. I have to tidy up my list of shortys to regain some overview. My list is full of differently created id's from the last experiments. Will have to start over then.

The owncloud.log is running continous, but there are only creations logged and script errors due to invalid id's.

Thank you for the explanations, I will look at this on the weekend.

arkascha commented 9 years ago

Hm... "script errors due to invalid id's"... might be interesting to take a closer look here. Are those Shorty IDs or something else? The 40x codes might also be of interest, since Shorty creates valid http response codes. Firbidden is 403 for example. Oh, an afterthought: how does that "Forbidden" message look like? Is it a typical browser generated error page (plain black text on a white background), or is it an owncloud view that displays the error?

But in the end more interesting would be to step through the relay code and see what's happening. Either using a debugger or at least using debug output steps.

fredl99 commented 9 years ago

The invalid IDs were caused by different configuration variants. When I changed the path, the formerly created IDs became invalid. Hence the errors.

The "Forbidden" message is in a typical owncloud screen.

Here's a log entry from a "shared" shorty:

[02/Jan/2015:01:25:55 +0100] "GET /oc/public.php?service=shorty_relay&id=Tz2UA02T4h HTTP/1.1" 403 7889 "-" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.95 Safari/537.36"

That's interesting: There's not even one 401-code created by shorty. But from remote.php/carddav, so OC itself can produce 401s. Afaik 401 should go out if auth is required.

BTW: Wikipedia says about 403: "Die Anfrage wurde mangels Berechtigung des Clients nicht durchgeführt. Diese Entscheidung wurde – anders als im Fall des Statuscodes 401 – unabhängig von Authentifizierungsinformationen getroffen, auch etwa wenn eine als HTTPS konfigurierte URL nur mit HTTP aufgerufen wurde." Hmm... Could it be...?

arkascha commented 9 years ago

OK, that makes some sense. If requests to Shortys defined in older configurations are not rewritten correctly any more, then it is not Shortys relay handler that is called with the correct Shorty ID, but something else, typically something undefined, which might well result in a forbiddden error in ownCloud. In taht case the effect is not caused by Shorty. The fact that you do not see any 401 results, nor get an authentication prompt on the browser side hints into the same direction.

So in my eyes this has nothing to do with Shorty, nor with http/https which works fine for me.

What I'd like to know now: what with freshly generated Shortys using a correctly configured static backend. Same behaviour? Then my first guess would be that the configuration is not correct. Two ways to tell:

  1. the configuration of the static backup offers the option to test the current configuration. Does that give a success? 2.) create a bad syntax error in the file /relay.php. Do you get an error (in the logfiles) when accessing a Shorty URL now? If not: the relay is _not_used, so again: should be a configuration issue.
fredl99 commented 9 years ago

Does that mean short URLS should be re-created when the base-url changes? There should't be any shorties with, say e3<id> in the table after changing to e3/<id>? If so, then something went really wrong here.

Neither old nor new shortys work. The old ones have obsolete IDs, so they cannot work. New ones only work when they're "public". All, old and new relay-URLs work, but only if they're public. And that's what confusing me here. Why would these work when your theory is right?

The testing tool does not succeed. I haven't noticed that until now, because it always took a while until it finished. My guess is, all these problems came in with the update from 7.0.3 -> 7.0.4. It definately worked with 7.0.3, but reverting back did not help.

So what would be a death-proof setup to start all over in your opinion?

fredl99 commented 9 years ago

Ok, did some further investigations :walking:

What we have:

To me there are two things obvious now:

fredl99 commented 9 years ago

@arkascha could you please re-open this issue? I don't have that option.

arkascha commented 9 years ago

You are obviously right, that some of the 403s are completely wrong. No idea why I coded that, guess it was a c&p issue whilst cleaning code...

Anyways, I made some changes that look better to me, but unfortunately I currently do not have time to test that as required. Do you want to give it a try? The problem: without testing I do not want to integrate that into the main owncloud/shorty repo. That repo only serves for finalizing and publishing for me. The development is done in a cloned repo in my own account instead, as typical on github. So you'd have to clone that repository instead of the "official" owncloud/shorty repository. That repository in my account is also where the changes mentioned here are to be found. From here I merge tested and validated changes into the main repository: https://github.com/arkascha/owncloud-shorty

This would also have another advantage: you could give it a try and make such changes yourself, if you want to. I'd be happy to grant you write access! :-)

fredl99 commented 9 years ago

Ok, @arkascha I've cloned your personal repo now. I guess it's better to discuss specific issues there? Is it allowed to open tickets in German language?

arkascha commented 9 years ago

To keep things transparent always post and track issues at the "offical" repository. Since owncloud is an international project using English as communication language does make sense. On the other hand a German feedback or ticket is better that none :-) To discuss things in detail you are welcome to contact me directly via email too, the address is enclosed in all files. But keep in mind that transparency is a vital aspect for open source projects.

It is a very typical thing to use private repositories to prepare and evaluate spikes and bug fixes before merging them into the official repository. That allows to keep failed attempts out of the project history.

fredl99 commented 9 years ago

I just thought to discuss things where they happen. But when it's common practice then naturally I will accept the rules.


Back to topic: As I can see, you implemented my suggestion about code 403 vs. 401. As already mentioned, the http-auth is now taking place. But after that there's no way to log out of OC without closing the browser. I consider this a serious issue. While it's only annoying at a personal workstation it's unacceptable at a public computer, where the user may not be able to do so. That leads me to the question: Why was the http-auth implemented some time ago? Please excuse that question, but I'm not so familiar with the internals of OC and Shorty.

Of course I see that somehow it has to be decided if a client's request for a particular shorty is valid. But as a bottom line there are only three options:

  1. the shorty is public -> go ahead.
  2. the shorty is blocked -> send out 403 (by OC or the server, which I personally would prefer for different reasons)
  3. the shorty is shared or private -> check, if the user is already authenticated by OC
    1. no -> call OC's auth routine and proceed next
    2. yes -> get the OC username.
    3. a shared shorty can be delivered now (as long as there's no function to share only with some users)
    4. if it's a private shorty, the OC username must match with the shorty's owner to be delivered. Otherwise send out 403 (this time by OC as it makes sense).

In my opinion the HTTP-auth interferes with OC's auth, or am I missing something?

arkascha commented 9 years ago

First of all apologies for not replying and coding faster - another project requires my attention.

About the authentication strategy: I ran into a number of problems when relying on the OC authentication mechanism. I do not remember the details right now, but there were issues with query parameters not getting handed over during the OC authentication. At least at the time I made the switch... Actually I still think that keeping OC sessions and access to Shortys (so using the relaying service) are two completely separate things. Thus the sessions should be separate.

At the time of testing http authentication to an OC session was not possible. Or so I thought. There actually is an app that implements exactly that, because it is (was) missing in the OC core. But now indeed my short test indeed confirms your observation. This indeed is a problem... When implementing the http based authentication I took care NOT to create a valid OC session. So I did not implement a login using the provided credentials, but merely checked the password in a raw manner. Nevertheless I now see that you are right.

sigh

arkascha commented 9 years ago

I just pushed a reimplementation of the relaying strategy, authorization and forwarding aspects. This appear to be working, but I had to trick the OC core with the authorization. There appears to be a strange regression with session verification in the OC core. I will address the core developers tomorrow, got a few questions anyway.

@fredl99 Wanna give it another try? I followed your advice to switch back to form based authentication... Also I implemented an additional forwarding page informing the user what happens. This is new. I know this means an additional view and an additional click. Still I think it makes sense. I received a few complaints in the past about privacy and security threads caused by url shorteners which are not totally invalid in my eyes. Transparency is important and shorteners typically do not offer that but take you somewhere without giving you a way to stay in control. This additional page offers such control. The layout is not final. I am currently unsure what information should be shown there. Feedback welcome as always.

fredl99 commented 9 years ago

I just tried. When I enter a short URL into the address bar, I get a completely blank page. The log says:

{"app":"PHP","message":"Class 'ChromePhp' not found at \/path\/to\/shorty\/relay.php#181","level":3,"time":"2015-02-23T12:26:18+00:00"}

There is a login screen or not before, depending on the auth state. But the result is the same.

arkascha commented 9 years ago

ooops, sorry for that. Debug output statements that should not have been pushed. As said: work in progress. I fixed that, just pull again.

fredl99 commented 9 years ago

:smile: OK!

tested:

Aside from that: As always, well done! :heavy_plus_sign:

arkascha commented 9 years ago

Fixed the issue about private Shortys. That indeed was a logical twist I implemented :-(

arkascha commented 9 years ago

@fredl99 OK, three issues left here:

fredl99 commented 9 years ago

Regarding the Umlauts, I only mentioned it here because it appeared on the new warning page. Of course it belongs to the other thread.

Code 403 for a private Shorty makes sense, I agree.

A also had the strange encoded address line withe the doubled directory name. First I thought about a wrong rewrite, but this couldn't alter the encoding. As it only happens during the login sequence, I think you're right. Pasting the correct link into the field again does work anyway, when the auth was successful before.

arkascha commented 9 years ago

OK, just talked to Lukas and he confirmed the issue with the scrambled redirection URL. He says it is currently getting fixed in OC-core master. So there is nothing I can do.

@fredl99 I guess this means this one can be closed?

fredl99 commented 9 years ago

Basically the initial issue seems resolved. So this long-term topic could be closed, yes. (I cannot do that, because you opened it.)

I was just thinking if there could be a way out of the one-way street when a user logs in for a Shorty which isn't available to him. Because the "forbidden"-screen without any way out is not so cool. :) A button to take him to his OC-home (since he is already authenticated) or anything else. It would only have to be shown to registered users, not to the unknown.

Of course that'll be a nice extra, but not a requirement.

arkascha commented 9 years ago

Closing for good...