Skip to content
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

Verification of the setup for the static backend is broken #92

Closed
arkascha opened this issue Jan 10, 2015 · 14 comments
Closed

Verification of the setup for the static backend is broken #92

arkascha opened this issue Jan 10, 2015 · 14 comments
Labels
Milestone

Comments

@arkascha
Copy link
Contributor

Apparently since porting to OC-7 the verification feature for the setup of the static backend base in the settings dialog is broken. It is one of many thing broken by changes in OC-core.

The problem here is that those validation attempts often are requests to third party servers, since that base url often is located on another server (shorted domain name). This violates the same origin policy.
An obvious solution would be to verify the setup on the server side, but that would tell little: the backend has to be used by clients, not by the server.

@arkascha arkascha added the bug label Jan 10, 2015
@arkascha
Copy link
Contributor Author

I reemplemented the verification completely, since the old strategy was blocked by security changes in the newer OC core.
The result is actually more pleasing: no more need to click to validate and a much more relaxed UI. YeeHaw!

This really was a tough task! To work through all those layers, options and security hassles was quite a challenge. But as always it also allowed to learn a lot :-)

@fredl99 you want to give that one a try before I merge it? Testing this is pretty hard for me, since there are so many different setups. So I am more than happy for all feed.
The reimplementation is contained in branch "stable7-backend-verification".

@arkascha
Copy link
Contributor Author

Hm, looks like there is still some issue with cross domain validation.
Thought I tackled this last night, but no, today I again get refused execution errors. No idea why, actually, since according to the development console the Content-Security-Policy header is sent correctly. But apparently it is not considered, cause I still get the unfamous error violates the following Content Security Policy directive: "script-src 'self' 'unsafe-eval'".

😢 :

@fredl99
Copy link

fredl99 commented Jan 12, 2015

Well, it says, "Setup valid and usable" if i don't change my settings.
If I do that it says, "Setup invalid and not usable".

Looks very fine so far, but also a bit mysterious because there's no explanation as to why it's valid or not. Has a touch of black magic 😈

Also the "test-link" is shown although it can't be selected (clicked).

@arkascha
Copy link
Contributor Author

Valid and usable is what you want. Not more, not less.
Invalid means you cannot use that setting. Shorty refuses to accept that setting. Actually it will NOT be stored that way. Sounds right for me.

And yes, the url example is shown and updated according to the input. Indeed it is not clickable any more, since clicking it is not required any more compared to before where you had to click to verify the setting. Maybe I should remove the text underline? OK...

@fredl99
Copy link

fredl99 commented Jan 12, 2015

Valid and usable is what you want. Not more, not less.

Yeah, right :)

Shorty refuses to accept that setting.

Good to know, but it should say so. Although I don't know exactly how this verification is done, I suppose it takes a few steps. How about displaying them, along with a green checkmark / red cross? This way one could guess where to look at, if it doesn't work.

Maybe I should remove the text underline?

Yes, otherwise it would look as if something doesn't work. There's a second one in the dialog-box of the table.

BTW: There's a "hidden" link to the tracking extension a few lines below within the admin's dialog:

Erweiterungen:
Shorty Tracking:

That doesn't do anything, too. It only turns the cursor into a pointer.

@fredl99
Copy link

fredl99 commented Jan 12, 2015

BTW:

Shorty refuses to accept that setting.

Wouldn't this solve issue #89 too?

@arkascha
Copy link
Contributor Author

Fixed the visualization of backend url examples in the settings and the preferences dialogs. No more underlining, some other cleanups as well.

@arkascha
Copy link
Contributor Author

Sorry, but I fail to see the ""hidden" link to the tracking extension" in the admin dialog...
There is a mentioning of that plugin in both, the settings (admin) and the preferences dialog. But no link. That was the case in older versions when I linked to either the app store entry or the apps dialog. But that was removed...

@arkascha
Copy link
Contributor Author

This reimplementation does not resolve issue #89 too.
That issue asks for setups to be denied that are syntactically correct, but carry risks. For example risks of collisions like what you had: using the owncloud installation folder as "virtual folder" for Shortys source addresses.

@fredl99
Copy link

fredl99 commented Jan 12, 2015

It's getting better and better. I'm impressed :)

  • The dialog in table view (is this what you call "preferences"?) contains remainders of the explanation about how to use the sample link, which are obsolete now. ("Test the sample-link by clicking ...")

  • The "hidden" link to the tracking extension in the admin's dialog looks like this:
    image
    and when the mouse cursor hovers over this area it turns into a pointer (exactly as it does here on github). It's not really a link, but looks like one. I think I spotted the cause somewhere in styles.css, might be here:

    media="screen"
    a, a *, input, input *, select, .button span, label {
    cursor: pointer;
    }

  • You're right, issue Configuration of static backend should deny setups known to create issues #89 doesn't describe the same thing. Funny enough, my virtual folder is still the OC-install folder, but I already explained the twist I'm using. I still dont't know how the validation works in technical terms, but wouldn't that be a good location to implement a check for "risky" setups?

  • Didn't you mention that short URLs should be updated when the global base URL is changed? I still have some shortys in my list from the earlier tests, which are now invalid.

@arkascha
Copy link
Contributor Author

Merged into arkascha/owncloud-shorty//stable7 and into owncloud/shorty//stable7

@arkascha arkascha added this to the shorty-0.5 milestone Jan 18, 2015
@arkascha
Copy link
Contributor Author

sigh there still is an issue with cross domain verification requests.
Such is required when the domain for the static backend differs from the one owncloud is running from.
I fail to understand what the problem is, so this will take some more time.

@arkascha
Copy link
Contributor Author

The verification implementation got a streamlining overhaul, it appears to be working reliably across domain borders now. This is required when using a different domain as a static backend base than the domain owncloud uses.

@arkascha
Copy link
Contributor Author

Will be released as part of Shorty version 0.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants