Skip to content

Conversation

@blizzz
Copy link
Contributor

@blizzz blizzz commented Jul 10, 2014

...s with stored site credentials.

Fixes #7038

List of browsers having the bug. Checked ones are tested successfully against.

  • Chromium/Chrome
  • Safari
  • IE (11+)

Please review and test. Also open for better ideas.

< update >
IE 11 has no issues here at all. Only the other two.
< /update >

@quiqueck @ser72 @PVince81 or others please

@karlitschek
Copy link
Contributor

good 👍

@blizzz blizzz changed the title [WIP] Hack to avoid Agent DN + Password being overwritten by some ugly browser... Hack to avoid Agent DN + Password being overwritten by some ugly browser... Jul 10, 2014
@blizzz
Copy link
Contributor Author

blizzz commented Jul 10, 2014

I need someone who could try this with Safari on MacOS ( @georgehrke ?)

Steps:

  1. Log in as admin into your ownCloud, make sure that Safari stores your credentials!
  2. Enable LDAP backend (you need php5-ldap, don't worry about the server)
  3. Go to settings, enter a fake server host and fake port, it does not matter here
  4. Enter something else than your username in the following text field
  5. Enter a significantly longer or short password than your users passwords
  6. Tab/Click away so it is getting stored
  7. Reload this page once, wait a bit
  8. Reload this page again

→ if the entered users and password shows up, very good! (In this case also please ensure that it does not work without this patch)
→ If your admin user and password appears, it did not work.

@georgehrke
Copy link
Contributor

I can test (need to install php5-ldap first)

@georgehrke
Copy link
Contributor

what to enter in base dn?

(update, just entered some fake value :"DC=FAKE,DC=local")

@blizzz
Copy link
Contributor Author

blizzz commented Jul 10, 2014

ty! :) asdf. does not matter here.

@georgehrke
Copy link
Contributor

for me, the entered user/pass shows up (even after reloading it like 10 times) in the latest version of Safari on OS X

even without this patch

sorry, missed to read the description completely, testing with patch now

@georgehrke
Copy link
Contributor

ok, so with and without this patch I see the credentials I entered (not the admin ones) after reloading the page in Safari on OS X

@blizzz
Copy link
Contributor Author

blizzz commented Jul 10, 2014

@georgehrke hm, OK. At least this was how I could reproduce it on Chromium.

@quiqueck @ser72 which Safari versions are affected for you? Those steps would lead to the effects your reported, wouldn't they?

@blizzz
Copy link
Contributor Author

blizzz commented Jul 10, 2014

or maybe this was fixed somewhere between Safari 7.0.3 and 7.0.5?

@ser72
Copy link

ser72 commented Jul 11, 2014

@blizzz

Apparently it is happening with any version, but for reference, I can make it happen on my system on demand, and it has Safari 7.0.4 (9537.76.4)

@blizzz
Copy link
Contributor Author

blizzz commented Jul 11, 2014

@ser72 okay, good. Could you apply the changes and test whether the issue is fixed with this?

@ser72
Copy link

ser72 commented Jul 11, 2014

@blizzz

I was never able to reproduce this in my lab... I have a user who can reproduce at will, but hesitate to have her "try" something.

@blizzz
Copy link
Contributor Author

blizzz commented Jul 11, 2014

So, am I hunting ghosts here?

@ser72
Copy link

ser72 commented Jul 11, 2014

@blizzz I tried a reproduction with the steps you mentioned above (Safari 6.0.5 -- that's all I have access to) and the configured id/pswd remained. Could not reproduce the issue so cannot test the patch.

How "confident" are you that this fixes the issue on Safari? If you have a fairly high confidence level I will ask my user to try it.

@blizzz
Copy link
Contributor Author

blizzz commented Jul 11, 2014

Since no one is able to reproduce it, and reporters did not respond yet, I have no clue. No reason to be confident.

@blizzz
Copy link
Contributor Author

blizzz commented Jul 11, 2014

@lmaestro could you try to reproduce this?

@luciamaestro
Copy link

I can't reproduce it.

@blizzz
Copy link
Contributor Author

blizzz commented Jul 14, 2014

@ser72 we're stuck. could you check back with your user the reproduction steps?

@ser72
Copy link

ser72 commented Jul 14, 2014

@blizzz

Ok good news. I just upgraded our MAC o/s and Safari and have a reproduction.

Will test patch now

@ser72
Copy link

ser72 commented Jul 14, 2014

@blizzz

OK now the bad news. Applied the patch and it is still broken on Safari 7.0.5 and oC 6.0.4 patched

@blizzz
Copy link
Contributor Author

blizzz commented Jul 14, 2014

At least you can verfiy this. Now, without having access to Safari it is hardly possible to find a way around it. I will meet with Markus on Wed evening to get my fingers on one.

@blizzz
Copy link
Contributor Author

blizzz commented Jul 16, 2014

Okay, I think we have it :) Thanks to @guruz for helping out :)

@ser72 please test again

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@ghost
Copy link

ghost commented Jul 16, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6267/

@ser72
Copy link

ser72 commented Jul 17, 2014

👍

Pls let me know if I can send this patch to a user experiencing the issue as well.

@blizzz
Copy link
Contributor Author

blizzz commented Jul 17, 2014

@ser72 yes, please do so.

@ser72
Copy link

ser72 commented Jul 22, 2014

👍 Lets get in 6.0.5

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahaha, what a hack! So that's what it takes!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is awesome.

@PVince81
Copy link
Contributor

Code looks good. I didn't test this myself but @ser72 did 👍

PVince81 pushed a commit that referenced this pull request Jul 22, 2014
Hack to avoid Agent DN + Password being overwritten by some ugly browser...
@PVince81 PVince81 merged commit a66b2f5 into stable6 Jul 22, 2014
@PVince81 PVince81 deleted the fix-7038 branch July 22, 2014 16:25
@jancborchardt
Copy link
Member

So should we backport this to stable7 @karlitschek? (Ref #10137 (comment))

@karlitschek
Copy link
Contributor

@jancborchardt Yes. I guess backporting should be save.

@blizzz
Copy link
Contributor Author

blizzz commented Aug 5, 2014

Port to master 7fd7706
Port to stable7 3ebdb35

@jancborchardt
Copy link
Member

Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants