Re: tgext.routes changes

[Thomas De Schampheleire]

On Thu, Jul 7, 2016 at 12:41 PM, Søren Løvborg  wrote:
> Sorry for the delay, I've been deep in the Alembic stuff (supporting
> migrations across three database engines is hard). :-)
>
> There are a number of ways to proceed:
>
> 1) "The status quo hack"
>
> Hack the TG port to emulate Routes (that is: use _method for routing, but
> don't actually change HTTP request method), or hack Kallithea to reset
> request method to POST after the routing step. Example:
>
> https://bitbucket.org/Unity-Technologies/kallithea/commits/714021984b5894fa74a88bfaa228fef4b0f545b7
>
> As can be seen, this the quick and dirty solution, and might be viable as a
> transitional hack.

This indeed solves most of the remaining test failures, and I think I
will commit it to the kallithea-tg repo, at least temporarily.

Thanks!
Thomas
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general

Re: tgext.routes changes

[Mads Kiilerich]

On 07/07/2016 12:41 PM, Søren Løvborg wrote:

2) "Kill _method and change the URLs"

Remove _method overrides completely (and good riddance) by changing 
PUT/DELETE routes to POST and adjusting URLs as needed to avoid ambiguity.


https://bitbucket.org/Unity-Technologies/kallithea/commits/0a71c69001b2b54e24a9bc1bc89b2fe0f1ea9898

As can be seen, there's a lot of work down this road; the above 
changeset fixes 2 out of 40 PUT/DELETE routes.


The work is however mainly about deleting code. The old code has some 
redundancy in having to specify put/delete both in the referenced 
routing entry and when using the URL.


The main risk is probably that the redundancy do that we in some places 
use a POST url for DELETE - we will have to check that carefully.


I suggest this option - as a separate refactoring that can land soon. It 
will be easier to review if each of the 40 changes you mention are in 
separate changesets ... but that might be a bit overkill ...


This also changes a number of URLs, it might be worthwhile to consider 
where we want to go with Kallithea's URLs before embarking on this, 
since there are numerous other problems in this area; the Kallithea 
URLs are often ambiguous in regards to repository paths, and we really 
need to fix that too. (Never name a repository "changelog", or "1", 
for that matter...)


This will probably not change any user visible URLs so I think we freely 
can change these both now and later.


(One related thing to consider is that we have some places where we make 
a POST that returns a html page. In such cases it is nice if there is a 
corresponding GET handler for returning the same page. But again: The 
refactoring discussed here will not change any existing POST handlers - 
only add new ones.)


/Mads
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general

Re: tgext.routes changes

[Søren Løvborg]

Sorry for the delay, I've been deep in the Alembic stuff (supporting
migrations across three database engines is hard). :-)

There are a number of ways to proceed:

1) "The status quo hack"

Hack the TG port to emulate Routes (that is: use _method for routing, but
don't actually change HTTP request method), or hack Kallithea to reset
request method to POST after the routing step. Example:

https://bitbucket.org/Unity-Technologies/kallithea/commits/714021984b5894fa74a88bfaa228fef4b0f545b7

As can be seen, this the quick and dirty solution, and might be viable as a
transitional hack.

2) "Kill _method and change the URLs"

Remove _method overrides completely (and good riddance) by changing
PUT/DELETE routes to POST and adjusting URLs as needed to avoid ambiguity.

https://bitbucket.org/Unity-Technologies/kallithea/commits/0a71c69001b2b54e24a9bc1bc89b2fe0f1ea9898

As can be seen, there's a lot of work down this road; the above changeset
fixes 2 out of 40 PUT/DELETE routes.

This also changes a number of URLs, it might be worthwhile to consider
where we want to go with Kallithea's URLs before embarking on this, since
there are numerous other problems in this area; the Kallithea URLs are
often ambiguous in regards to repository paths, and we really need to fix
that too. (Never name a repository "changelog", or "1", for that matter...)

3) "s/_method/action/"

As an alternative to 2, the URLs could be kept the same, and the PUT/DELETE
controllers could be merged into a unified (possibly existing) POST
controller. The controller would branch based on form data, like an
"action" field (just like "_method", but without the HTTP baggage). This
might be even more work, though.

Best,
Søren


On Wed, Jun 29, 2016 at 9:35 PM, Thomas De Schampheleire <
patrickdeping...@gmail.com> wrote:

> Hi Søren,
>
> On Tue, May 3, 2016 at 8:25 PM, Mads Kiilerich  wrote:
> > On 05/03/2016 03:13 PM, Søren Løvborg wrote:
> >>
> >> Considering that method overrides are designed specifically to
> >> accommodate HTML forms, we could pull the CSRF token out of the POST
> >> request body and stuff it into a header as part of the override
> >> process. But at that point, it just feels like we're digging ourselves
> >> in even deeper. A saner approach would be to phase out method
> >> overrides altogether, and just let POST requests be POST requests.
> >> (Add an "action" argument or similar as needed, but leave that to the
> >> controller, and keep it out of routing and security checks.)
> >
> >
> > It seems like that would be a general refactoring and code improvement
> that
> > could be done on the default branch and pave the way for the TG
> migration?
>
> I was wondering whether you would be up to tackling this in the near
> future? (or perhaps you already started with this?)
>
> Of the current tests that are failing, the majority is failing due to
> the DELETE method not being accepted. I am meanwhile fixing the other
> failures, but it would be great if in a parallel path someone could
> look at the DELETE thing.
>
> You can see the current state of the Turbogears2 migration code to
> https://bitbucket.org/_amol_/kallithea-tg/
> See also:
> https://bitbucket.org/conservancy/kallithea/wiki/Turbogears2Migration
>
> Do let me know your thoughts...
>
> Best regards,
> Thomas
>
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general

Re: tgext.routes changes

[Thomas De Schampheleire]

Hi Søren,

On Tue, May 3, 2016 at 8:25 PM, Mads Kiilerich  wrote:
> On 05/03/2016 03:13 PM, Søren Løvborg wrote:
>>
>> Considering that method overrides are designed specifically to
>> accommodate HTML forms, we could pull the CSRF token out of the POST
>> request body and stuff it into a header as part of the override
>> process. But at that point, it just feels like we're digging ourselves
>> in even deeper. A saner approach would be to phase out method
>> overrides altogether, and just let POST requests be POST requests.
>> (Add an "action" argument or similar as needed, but leave that to the
>> controller, and keep it out of routing and security checks.)
>
>
> It seems like that would be a general refactoring and code improvement that
> could be done on the default branch and pave the way for the TG migration?

I was wondering whether you would be up to tackling this in the near
future? (or perhaps you already started with this?)

Of the current tests that are failing, the majority is failing due to
the DELETE method not being accepted. I am meanwhile fixing the other
failures, but it would be great if in a parallel path someone could
look at the DELETE thing.

You can see the current state of the Turbogears2 migration code to
https://bitbucket.org/_amol_/kallithea-tg/
See also: https://bitbucket.org/conservancy/kallithea/wiki/Turbogears2Migration

Do let me know your thoughts...

Best regards,
Thomas
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general

Re: tgext.routes changes

[Mads Kiilerich]

On 05/03/2016 03:13 PM, Søren Løvborg wrote:

Considering that method overrides are designed specifically to
accommodate HTML forms, we could pull the CSRF token out of the POST
request body and stuff it into a header as part of the override
process. But at that point, it just feels like we're digging ourselves
in even deeper. A saner approach would be to phase out method
overrides altogether, and just let POST requests be POST requests.
(Add an "action" argument or similar as needed, but leave that to the
controller, and keep it out of routing and security checks.)


It seems like that would be a general refactoring and code improvement 
that could be done on the default branch and pave the way for the TG 
migration?


I think it would be nice to have more of those ... or to use the 
modularity of TG to migrate to TG module by module - for example 
perhapsly by switching from paster to to gearbox or by changing the 
routing mechanism or adding a new State of the TG Art RESTish web 
services API.


/Mads
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general

Re: tgext.routes changes

[Søren Løvborg]

So, sorry for the delay here, had to go and fix CVE-2016-3691, caused
by 1) Routes having a feature to turn GET requests into POST requests,
and 2) Routes enabling this "feature" by default, causing it to be
enabled in Kallithea, apparently without any Kallithea developers
realizing it. (AFAIK, in normal operation, Kallithea only overrides
POST requests.) Not sure I can possibly add anything else to that
discussion, but a good thing we had it, otherwise I'd never have
noticed this security issue.

To get back to the original topic: When overriding the method (of a
POST request...), it makes perfect sense to replace the REQUEST_METHOD
in the environment, too. Except that if you change the method to
DELETE, WebOb will no longer read out POST data (since the concept of
DELETE data is not entirely well-defined), which then means we can't
read the CSRF token, and you get an error 403.

Now, for a real DELETE request (with no overrides involved), you'd
typically specify CSRF token (or API key or similar auth info) in an
HTTP *header*, not the request body. But since we're sending these
requests via an HTML form, that is not an option. It's also possible
to send the CSRF token in the URL, but that's a no-go due to the
greatly increased risk of leaking the token.

Considering that method overrides are designed specifically to
accommodate HTML forms, we could pull the CSRF token out of the POST
request body and stuff it into a header as part of the override
process. But at that point, it just feels like we're digging ourselves
in even deeper. A saner approach would be to phase out method
overrides altogether, and just let POST requests be POST requests.
(Add an "action" argument or similar as needed, but leave that to the
controller, and keep it out of routing and security checks.)

Best,
Søren


On Thu, Apr 14, 2016 at 10:00 PM, Alessandro Molina
 wrote:
> On Thu, Apr 14, 2016 at 3:17 PM, Søren Løvborg  wrote:
>
>>
>
>> Sorry, this is going to get long. :-)
>>
>> Thomas De Schampheleire wrote:
>> > So this means updating Kallithea. Do you happen to be interested and
>> > available for such change?
>>
>> Yes. I am currently looking into the Kallithea code to see how this
>> would work. There is definitely room for improvement. I'll get back to
>> you (and the list) when I have something more concrete.
>>
>> Next, I wrote:
>> >>> (Aside: I did not look at the tgext.routes code, but I assume the
>> >>> override support is opt-in? Enabling it automatically for all
>> >>> applications
>> >>> could cause security issues for applications that don't have CSRF
>> >>> protection.)
>>
>> Alessandro Molina replied:
>> > Nope, there is no opt-in.
>> > There isn't in routes itself too:
>> >
>> > https://github.com/bbangert/routes/blob/master/routes/middleware.py#L61-L70
>> >
>> > Also even though you would opt-out you can still perform CSRF in any
>> > case by
>> > using an XMLHTTPRequest or a form.
>>
>> Well, in Routes, it's an opt-out, but the option is there (the
>> use_method_override argument). I think it's a mistake to enable by
>> default.
>>
>> Messing around with the HTTP request like this is definitely not
>> something you should do in a library, unless the application
>> explicitly asks for it, and even then only under certain limited
>> circumstances. This is why:
>>
>
> I'm sorry if my reply made you fervent about the topic, I quickly discarded
> the discussion about opt-in/out just because I found it pretty useless in
> this context. As it doesn't guarantee you are safe from cross site attacks
> and kallithea needed that feature on in any case (it was actually added for
> kallithea itself).
>
> I was more interested on the concern of updating the environ key or not,
> which for consinstency I would do, but it's open to interpretation.
>
> I know that by RFC you should theoretically stick to some behaviours, but in
> practice they are not enforced and the standard itself states it might be
> considered a feature being able to override them. I mean while it's wrong
> and will hit you back for many reasons caching included... the world is full
> of apps that change things on GET requests...
>
> I'll gladly add an option to opt in in tgext.routes if that makes you feel
> more comfortable it won't change much for me as there are no other apps
> apart from Kallithea that replace the whole routing stack in tg with
> tgext.routes and it was made for kallitha so no one will complain ;)
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general

Re: tgext.routes changes

[Søren Løvborg]

Sorry, this is going to get long. :-)

Thomas De Schampheleire wrote:
> So this means updating Kallithea. Do you happen to be interested and
> available for such change?

Yes. I am currently looking into the Kallithea code to see how this
would work. There is definitely room for improvement. I'll get back to
you (and the list) when I have something more concrete.

Next, I wrote:
>>> (Aside: I did not look at the tgext.routes code, but I assume the
>>> override support is opt-in? Enabling it automatically for all applications
>>> could cause security issues for applications that don't have CSRF
>>> protection.)

Alessandro Molina replied:
> Nope, there is no opt-in.
> There isn't in routes itself too:
> https://github.com/bbangert/routes/blob/master/routes/middleware.py#L61-L70
>
> Also even though you would opt-out you can still perform CSRF in any case by
> using an XMLHTTPRequest or a form.

Well, in Routes, it's an opt-out, but the option is there (the
use_method_override argument). I think it's a mistake to enable by
default.

Messing around with the HTTP request like this is definitely not
something you should do in a library, unless the application
explicitly asks for it, and even then only under certain limited
circumstances. This is why:

Per the HTTP standard, GET, POST and PUT/DELETE belong to three
different classes of HTTP methods, with different operational
semantics. Changing one for another has implications not only on a web
application level, but throughout the HTTP stack.

First of all, per the HTTP standard, a GET request must be free of
side effects. Using a method override to turn a GET request into e.g.
a POST request at the application layer can cause a host of issues,
since it violates assumptions held by the rest of the stack (that
includes WSGI middleware, HTTP servers, HTTP accelerators, HTTP
proxies and the browser itself). Therefore, you should never allow a
GET method to be overridden, and I frankly consider it a bug that
Routes (and now tgext.routes) does this.

Similarly, PUT and DELETE are required to be idempotent, that is,
sending the request multiple times should have the same effect as
sending it once. Turning e.g. a PUT into a POST could violate this
rule, but is fortunately not supported by Routes nor tgext.routes.

Now, taking a POST request and turning it into a GET, PUT or DELETE
request, is acceptable under the HTTP standard, because there are no
restrictions on side-effects or idempotence for POST requests.
However, it can poke holes (albeit tiny holes) in the fragile security
sandbox of HTML/JavaScript, because the modern browser security model
treats GET/POST differently from PUT/DELETE, in that a HTML form can
only send GET/POST requests, while you need XMLHttpRequest to send PUT
or DELETE requests, and XMLHttpRequest is subject to stricter
same-origin rules than a HTML form. (Which is why CSRF-attacks are
traditionally implemented using an auto-submitting HTML form, not an
XMLHttpRequest.)

These same-origin rules are rather spotty, and even browser vendor and
version dependent, which is why all serious web apps have CSRF
protection. But the fact that the security protections are weak is
still a poor argument for poking more holes in them. Hence method
overrides should be something the application specifically enables,
once it has proper CSRF protection in place. Even then, method
overrides must only be possible for POST requests, not GET/PUT/DELETE
(or any other).

Best,
Søren
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general

Re: tgext.routes changes

[Søren Løvborg]

I would definitely not recommend copying the current _method handling. :-)

The correct thing must be an early pass where the _method override is
applied to the REQUEST_METHOD. All code beyond that point should just see
REQUEST_METHOD, and never know the difference between fake and real HTTP
verbs. Which sounds like exactly what tgext.routes 0.1.2 does.

(Aside: I did not look at the tgext.routes code, but I assume the override
support is opt-in? Enabling it automatically for all applications could
cause security issues for applications that don't have CSRF protection.)

> If I temporarily change that code so that 'DELETE' is also part of the
> accepted request methods, then test_delete fails due to 403 Forbidden
> while it expects 405 Method Not Allowed, and
> test_delete_browser_fakeout fails with 403 too (CSRF protection, I
> guess?)

Yes, and that behavior is arguably the correct one. test_delete should be
changed to expect 403, since the DELETE method is in fact allowed (but the
CSRF check is expected to fail).

Best,
Søren







On Mon, Apr 11, 2016 at 12:42 AM, Alessandro Molina <
alessandro.mol...@gmail.com> wrote:

>
>
> On Sun, Apr 10, 2016 at 9:20 PM, Thomas De Schampheleire <
> patrickdeping...@gmail.com> wrote:
>
>> Previously, using routes, we received a real POST request with
>> _method==PUT or _method==DELETE, while with Alessandro's change in
>> tgext.routes, we receive a real PUT or DELETE request, which is not
>> what the current code expects.
>>
>
> We can easily change tgext.routes to behave like routes which recovers the
> previous REQUEST_METHOD ->
> https://github.com/bbangert/routes/blob/master/routes/middleware.py#L99
>
> but I think it actually makes more sense like this as I find it more
> confusing to have routing that behaves like a DELETE method but if you ask
> request method it gives something else.
>
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general

Re: tgext.routes changes

[Alessandro Molina]

On Sun, Apr 10, 2016 at 9:20 PM, Thomas De Schampheleire <
patrickdeping...@gmail.com> wrote:

> Previously, using routes, we received a real POST request with
> _method==PUT or _method==DELETE, while with Alessandro's change in
> tgext.routes, we receive a real PUT or DELETE request, which is not
> what the current code expects.
>

We can easily change tgext.routes to behave like routes which recovers the
previous REQUEST_METHOD ->
https://github.com/bbangert/routes/blob/master/routes/middleware.py#L99

but I think it actually makes more sense like this as I find it more
confusing to have routing that behaves like a DELETE method but if you ask
request method it gives something else.
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general

Re: tgext.routes changes

[Thomas De Schampheleire]

 Hi Alessandro,

On Wed, Apr 6, 2016 at 1:30 AM, Alessandro Molina
 wrote:
> Hi Thomas,
>
> Had time to give it a quick look.
> Issue was caused by the fact that form was relying on ?_method parameter to
> override request method.
>
> It's now supported in tgext.routes 0.1.2
> Just upgrade tgext.routes and it should work ( please raise dependency to
> kallithea-tg, I cannot commit right now :( )

Thanks, I verified that the mentioned test scenario now works and
pushed the bump to 0.1.2.

When I retry the failing tests from the test suite, I see that the 404
is indeed gone, but one of the two failing tests still fails.
Originally, with tgext.routes 0.1.1, following tests failed with 404:

FAIL 
kallithea/tests/functional/test_admin_defaults.py::TestDefaultsController::()::test_update_browser_fakeout
FAIL 
kallithea/tests/functional/test_admin_defaults.py::TestDefaultsController::()::test_delete_browser_fakeout

With tgext.routes 0.1.2, only the delete test fails, as follows:

――
TestDefaultsController.test_delete_browser_fakeout
――
kallithea/tests/functional/test_admin_defaults.py:70: in
test_delete_browser_fakeout
response = self.app.post(url('default', id=1),
params=dict(_method='delete',
_authentication_token=self.authentication_token()))
../venv/kallithea-tg/lib/python2.7/site-packages/webtest/app.py:371: in post
content_type=content_type)
../venv/kallithea-tg/lib/python2.7/site-packages/webtest/app.py:736:
in _gen_request
expect_errors=expect_errors)
../venv/kallithea-tg/lib/python2.7/site-packages/webtest/app.py:632:
in do_request
self._check_status(status, res)
../venv/kallithea-tg/lib/python2.7/site-packages/webtest/app.py:664:
in _check_status
res)
E   AppError: Bad response: 405 Method Not Allowed (not 200 OK or 3xx
redirect for http://localhost/_admin/defaults/1)
E   405 Method Not Allowed
E
E   The method DELETE is not allowed for this resource.

I looked at your changes in tgext.routes but it does not look a
problem introduced by these changes; maybe it's a latent problem that
now pops up with the 404 fixed.
Is it tg / tgext.routes that generates the 405? How can such a problem
be debugged efficiently?

Thanks,
Thomas
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general