Logout link should be protected

[vlastimil.zima@…]

Logout link is not protected against CSRF. It is very easy to include link to a fake image, even in Trac itself:

[[Image(http://trac.edgewall.org/logout)]]

It is a kind of security issue, because it is possible to block user from performing any action in private parts of any Trac system.

Used here for demonstration of the same problem: ​https://code.djangoproject.com/ticket/15619#comment:25

[cboos (or was, before being logged out when using the trick presented here...)]

Could make for nice jokes ;-)

Right, we probably don't want that.

[Ryan J Ollos]

Proposed changes can be found in log:rjollos.git:t11060. I would really appreciate some review on this one, since this is a change that I'm not too confident with. I patterned the change after similar changes for the ​th:VotePlugin that I worked with Steffen (​th:#7744, which led to ​[th 13086]). I'll look into adding a functional test before committing final changes.

[Peter Suter]

I'm no expert in this area. The ​common advice seems to suggest using POST requests. (GET requests might open up new attack vectors like logging, referrers, etc. Possibly ​these are rather harmless or can be mitigated using HTTPS, per-request-tokens etc. But simply using POST requests just seems less fragile.)

Is there a reason we can't use POST requests for logging out? (E.g. by sending the user to a confirmation page.)

In the ​discussion linked by the reporter a similar approach is proposed.

Trac already has a policy to use this approach for deleting wiki pages etc.

---

(OT: By the way, your git author name rjollos <rqTkzckbm0CUTEJqsQVX7kLmCHsur67zbkxfywbzVT7Rsbx0yHcqcMeGagsJoAE1> looks a bit weird recently. Is that intentional? It triggers a funny layout on the Trac timeline if the browser window is small and the prefs form gets in the way.)

[Ryan J Ollos]

Replying to psuter:

(OT: By the way, your git author name rjollos <rqTkzckbm0CUTEJqsQVX7kLmCHsur67zbkxfywbzVT7Rsbx0yHcqcMeGagsJoAE1> looks a bit weird recently. Is that intentional? It triggers a funny layout on the Trac timeline if the browser window is small and the prefs form gets in the way.)

Those were changes that I pushed from a VM that I usually don't develop on any longer and I guess there must be something strange with my Git configuration on that system. I'll see if I can get that fixed before pushing any more changes from it.

Thanks for the other hints, I'll take a look tomorrow.

[Jun Omae]

Proposed changes in log:jomae.git:t11060.1 which creates an inline form to log out using POST method as metanav item.

[Ryan J Ollos]

It looks good to me. Should we target it to 1.0.2?

Two questions:

• What is the reason for wrapping the Logout text in a span?
• Do you know the reason why the FunctionalTester.go_to_front() calls fix errors such as the following?

  ======================================================================
  FAIL: runTest (__main__.RegressionTestTicket6318)
  Regression test for non-ascii usernames (#6318)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/home/user/Workspace/t11060/teo-rjollos.git/trac/tests/functional/testcases.py", line 236, in runTest
      self._tester.logout()
    File "trac/tests/functional/tester.py", line 66, in logout
      tc.follow(r"\bLogout\b")
    File "/home/user/Workspace/twill-0.9/twill/commands.py", line 202, in follow
      raise TwillAssertionError("no links match to '%s'" % (what,))
  TwillAssertionError: no links match to '\bLogout\b'
  
  ----------------------------------------------------------------------
  Ran 11 tests in 12.474s
  
  FAILED (failures=1)
  

[Jun Omae]

I've verified log:jomae.git:t11060.1 with Firefox 26, Google Chrome 32 beta and IE 8 on Windows.

It looks good to me. Should we target it to 1.0.2?

Agreed. Changed to 1.0.2.

• What is the reason for wrapping the Logout text in a span?

I couldn't fix invisible border-bottom in Google Chrome or Firefox yesterday…. Ah, I just fixed it. Removed the span in [f6a240c5/jomae.git].

• Do you know the reason why the FunctionalTester.go_to_front() calls fix errors such as the following?

Once twill submits logout form immediately after login, it is redirected to login. This means it can't log out.

[Jun Omae]

Committed in [12379-12380] and merged in [12381].

After the changes, when delete user in ​th:AccountManagerPlugin, the session probably doesn't log out. See ​http://trac-hacks.org/browser/accountmanagerplugin/trunk/acct_mgr/web_ui.py#L246.