roxen.lists.roxen.general

Subject Author Date
Re: Lots of patches that can be integrated into Roxen 5.0 in a heartbeat Martin Stjernholm <mast[at]roxen[dot]com> 10-01-2009
"Stephen R. van den Berg" <<srb[at]cuci.nl>> wrote:

>>> commit b4b503c88cf69d991e23acf4598e14686d635f39
>>>     Prevent cimg from asking for username/password at times
>
>>Don't understand what the bug is that this would solve.
>
> If I recall correctly, the problem was/is that:
> - A user accesses a cimg from inside a password protected area of the tree.
> - cimg sees the image is not present in the database, so it converts the
>   source image and stores a copy *alongside* with the user/password
>   requirements of the original image.
> - The cimg-URL produced is unguessable (or should be), but outside the
>   password protected subtree of the original image.
> - Now the browser uses this cimg-URL and is surprised by a not-authorised
>   message from the server.  The browser doesn't have a username/password
>   to supply because it's from outside the previous tree where it had already
>   cached that information and presents a box to the user.
> - That was one username/password check too many.  The cimg-URL should
>   be username/password check free, because the URL should only be
>   available to someone already able to get at the original image.

I actually thought the reverse was the problem, i.e. that it is
possible to access protected data through the _internal tree if one
knows the url. So maybe someone has fixed that, then.

What you advocate is a weaker form of security: Sure, an adversary
_shouldn't_ know those urls, but if (s)he somehow does, it allows
retrieval of more information that should be protected.

We usually solve those repeated login requests by redirecting to /,
prompt for the login there, and then redirecting back to the original
page (which is stored as a variable during the login procedure). That
takes care of all the issues with too-specific authorized trees. Even
though a really correct solution would be to introduce a system where
more local _internal trees can be used, our experience shows that this
simple redirect approach works well enough in practice.

>>> commit cd3cd82a2af0d19ddb060a561d296e8ca6f308cb
>>>     Take the local tree out of the repository proper
>
>>I don't think we want this. Those files are there to provide a
>>skeleton local dir hierarchy and some help.
>
> How about storing that local tree on a different directory in the source
> management tree, and in the install script, simply copy it into the proper
> spot?  The problem is that there are ../local paths in quite a lot of places,
> which makes a live sourcetree a liability, because the local tree should
> not be managed by git in this case.

Sounds like the remedy is worse than the pain. Can't you just use more
specific .gitignore's instead? The old .cvsignore files has worked
fairly ok, afterall.

>>Is there something that destructs the sql connection object on error?
>>Otherwise con will never be zero there.
>
> There are some places, yes, though I might be responsible for those, I'll
> reevaluate why I did that, and maybe it can be fixed at that end instead.

Probably better. I wouldn't want to get my connection objects
destructed on me if an error happens. Those objects could even contain
the error code.

>>> commit 26ca63251a023c65a6468c8d9f1c52077caddc48
>>>     Alternatives don't always contain an array
>
>>Have you gotten a real error there? The plan is that the mapping
>>contains only arrays when a timeout is set, and otherwise it shouldn't
>>get into that loop.
>
> Yes, got real errors there. /.../

It could perhaps happen if the timeout comes and goes between
invocations to the same cache tag. That's not advisable - the code
probably assumes in more than one place that the timeout is constant.

> Not quite.
> Picture this:
> <eval>
> <maketag name=emit type=container>
> <attrib name=host>&var.db;</attrib>
> <attrib name=source>sql</attrib>
> <attrib name=query>
> SELECT abc
> FROM def
> WHERE xyz='&form.&var._fieldid;:mysql-rxml;'
> </attrib>
> ...
> </maketag>
> </eval>

Uh-oh, double parsing and entities inside entities.. What about this
instead?

  <set variable="var.findval" from="form.&var._fieldid;"/>
  <emit host="&var.db;" source="sql"
        query="SELECT abc FROM def WHERE xyz='&var.findval:mysql;'">
    ...
  </emit>

That'd also be safe from sql injection via var._fieldid (in case that
one can be controlled by the client).