append reappends existing lines (or: exists() not passing use_sudo) #341

Closed
bitprophet opened this Issue Aug 19, 2011 · 8 comments

Projects

None yet

2 participants

@bitprophet
Member

Description

...at least in my environment.

When run this multiple times :

append('/var/lib/pgsql/data/pg_hba.conf', text='local all all trust', use_sudo=True)

I get :
...
# IPv6 local connections:
host all all ::1/128 ident
local all all trust
local all all trust
local all all trust

It's probably just me because it seems too big to be true, but I cannot find what I might be doing wrong.


Originally submitted by **** (dperetti) on 2011-04-22 at 03:08pm EDT

@bitprophet bitprophet was assigned Aug 19, 2011
@bitprophet
Member

Jeff Forcier (bitprophet) posted:


Unless that was you, there was another (unclear) report on IRC that sounds like the same issue. Will try to reproduce.


on 2011-04-22 at 04:04pm EDT

@bitprophet
Member

**** (dperetti) posted:


OK, this is definitely a bug in fabric/contrib/files.py, line 306 :

Instead of :

    if (exists(filename) and line

... we should have :

    if (exists(filename, use_sudo=use_sudo) and line

That was as simple as that !


on 2011-04-22 at 05:47pm EDT

@bitprophet
Member

Michael Chan (mtchan888) posted:


I notice that egrep wont match the text if characters '>' or '<' are escaped by re.escaped . Thus causing reappend as well.
I added following in fabric/contrib/files.py, line 263

text = text.replace('\>','>')
text = text.replace('\<','<')

on 2011-05-26 at 04:12am EDT

@bitprophet
Member

nixon (nixon) posted:


I ran into similar problems with trying to append lines containing double quotes and/or singles quotes and/or dollar signs:


from __future__ import with_statement
from fabric.contrib.files import contains, append
from utils import FabricTest


class TestContribContainsAppend(FabricTest):

    def setup(self):
        from fabric.state import env
        import getpass
        super(TestContribContainsAppend, self).setup()
        env.host_string = "localhost"
        env.user = getpass.getuser()

    def test_contains_handles_double_quotes(self):
        """
        contains() should work OK with strings containing double quotes.
        (assumes a running sshd on localhost)
        """
        text = r'"$VAR"'
        tmpfile = self.path("contains.tmp")
        with open(tmpfile, "w") as f:
            f.write(text+"\n")

        assert text in open(tmpfile).read()
        assert contains(tmpfile, text)

    def test_contains_handles_single_quotes(self):
        """
        contains() should work OK with strings containing single quotes.
        (assumes a running sshd on localhost)
        """
        text = r"'$VAR'"
        tmpfile = self.path("contains.tmp")
        with open(tmpfile, "w") as f:
            f.write(text+"\n")

        assert text in open(tmpfile).read()
        assert contains(tmpfile, text)

    def test_append_handles_double_quotes(self):
        """
        append() should work OK with strings containing double quotes.
        (assumes a running sshd on localhost)
        """
        text = r'"$VAR"'
        tmpfile = self.path("append.tmp")
        with open(tmpfile, "w") as f:
            f.write(text+"\n")

        append(tmpfile, text)
        numlines = len(open(tmpfile).readlines())
        assert numlines == 1, "%d != 1" % numlines

    def test_append_handles_single_quotes(self):
        """
        append() should work OK with strings containing single quotes.
        (assumes a running sshd on localhost)
        """
        text = r"'$VAR'"
        tmpfile = self.path("append.tmp")
        with open(tmpfile, "w") as f:
            f.write(text+"\n")

        append(tmpfile, text)
        numlines = len(open(tmpfile).readlines())
        assert numlines == 1, "%d != 1" % numlines


on 2011-06-14 at 09:48pm EDT

@bitprophet
Member

nixon (nixon) posted:


One solution might be to use "egrep -f pattern_file" where "pattern_file" has the "text" parameter in it. So the "contains" command becomes something like:

egrep -q -f pattern.txt filename 2>/dev/null

It would need to be able to securely create the temporary pattern.txt file on the remote server and remove it afterwards.


on 2011-06-15 at 10:52am EDT

@bitprophet
Member

Jeff Forcier (bitprophet) posted:


nixon nixon wrote:

One solution might be to use "egrep -f pattern_file" where "pattern_file" has the "text" parameter in it.

Thanks, I like this idea, and there is already minor precedent (in put) for temporary remote files. When this ticket's number comes up I'll definitely explore it as a way of working around the escaping woes.

May have to go into its own ticket though if Dominique's note above is accurate (i.e. there would be 2 different use cases where this bug appears and they should be documented separately.)


on 2011-06-15 at 05:16pm EDT

@vilcans vilcans pushed a commit to vilcans/fabric that referenced this issue Aug 30, 2011
Martin Vilcans Do not create duplicate lines when calling append(..., use_sudo=True)
This happened on a file unreadable by an unprivileged user, for
example /root/.ssh/authorized_keys when use_sudo was True.

This fix passes along use_sudo to exists, so it can check if the file
*really* exists before appending to it.

Fixes issue #341.
e082193
@vilcans
vilcans commented Aug 30, 2011

I've created a patch (to the original bug, not the other ones discussed here):

vilcans/fabric@e082193

@bitprophet
Member

Updated title to reflect core bug/problem.

Sometime soon I will try to go through and pick up all the 'quick' fixes, including this one. Sorry for the delay.

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