Patchwork [6,of,6] vfs: also audit rename

login
register
mail settings
Submitter Boris Feld
Date Nov. 26, 2018, 6:22 p.m.
Message ID <afdc73b20bd1faeec1b2.1543256568@localhost.localdomain>
Download mbox | patch
Permalink /patch/36780/
State Accepted
Headers show

Comments

Boris Feld - Nov. 26, 2018, 6:22 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1498961267 -7200
#      Sun Jul 02 04:07:47 2017 +0200
# Node ID afdc73b20bd1faeec1b278ef0a2ab8d1dda71eb8
# Parent  cc7d970d99eb242dbe2d8e792a9212aabd06911f
# EXP-Topic vfs.audit-rename
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r afdc73b20bd1
vfs: also audit rename

Renaming through the vfs is not used in many places, and none of them seems to
be at security risk.

However it is still worthwhile to run the auditing on rename file to perform
developer-warning level checks.
Yuya Nishihara - Nov. 28, 2018, 12:04 p.m.
On Mon, 26 Nov 2018 19:22:48 +0100, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1498961267 -7200
> #      Sun Jul 02 04:07:47 2017 +0200
> # Node ID afdc73b20bd1faeec1b278ef0a2ab8d1dda71eb8
> # Parent  cc7d970d99eb242dbe2d8e792a9212aabd06911f
> # EXP-Topic vfs.audit-rename
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r afdc73b20bd1
> vfs: also audit rename

Queued the first 4 patches, thanks.

> diff --git a/mercurial/vfs.py b/mercurial/vfs.py
> --- a/mercurial/vfs.py
> +++ b/mercurial/vfs.py
> @@ -436,6 +436,10 @@ class vfs(abstractvfs):
>  
>          return fp
>  
> +    def rename(self, src, dst, *args, **kwargs):
> +        self._auditpath(dst, 'w')
> +        return super(vfs, self).rename(src, dst, *args, **kwargs)

Can you move this to abstractvfs.rename()?

I don't think it's worth to override rename() just for _auditpath(). Instead,
abstractvfs can implement _auditpath() that raises exception.

Also, 'dst' is likely to be relative in abstractvfs.rename(), so we won't
probably need to convert absolute 'dst' back to relative path.
Boris Feld - Dec. 2, 2018, 3:30 p.m.
On 28/11/2018 13:04, Yuya Nishihara wrote:
> On Mon, 26 Nov 2018 19:22:48 +0100, Boris Feld wrote:
>> # HG changeset patch
>> # User Boris Feld <boris.feld@octobus.net>
>> # Date 1498961267 -7200
>> #      Sun Jul 02 04:07:47 2017 +0200
>> # Node ID afdc73b20bd1faeec1b278ef0a2ab8d1dda71eb8
>> # Parent  cc7d970d99eb242dbe2d8e792a9212aabd06911f
>> # EXP-Topic vfs.audit-rename
>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r afdc73b20bd1
>> vfs: also audit rename
> Queued the first 4 patches, thanks.
>
>> diff --git a/mercurial/vfs.py b/mercurial/vfs.py
>> --- a/mercurial/vfs.py
>> +++ b/mercurial/vfs.py
>> @@ -436,6 +436,10 @@ class vfs(abstractvfs):
>>  
>>          return fp
>>  
>> +    def rename(self, src, dst, *args, **kwargs):
>> +        self._auditpath(dst, 'w')
>> +        return super(vfs, self).rename(src, dst, *args, **kwargs)
> Can you move this to abstractvfs.rename()?
>
> I don't think it's worth to override rename() just for _auditpath(). Instead,
> abstractvfs can implement _auditpath() that raises exception.

I'm not sure what are your concerns and what would be the point of
moving that one layer up. Currently, the _auditpath logic is exclusive
to vfs. It comes from the `vfs.__call__` method that the `abstractvfs`
class do not have. What makes it worthy to move it in `abstractvfs`?

(I'm not opposed to doing so, I'm trying to understand the logic and
responsibility of each class)
>
> Also, 'dst' is likely to be relative in abstractvfs.rename(), so we won't
> probably need to convert absolute 'dst' back to relative path.
What's your reasoning here? We directly call the underlying `rename`
method in the `vfs` class, and we know for sure that they can receive
absolute paths.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Dec. 3, 2018, 11:03 a.m.
On Sun, 2 Dec 2018 16:30:28 +0100, Boris FELD wrote:
> 
> On 28/11/2018 13:04, Yuya Nishihara wrote:
> > On Mon, 26 Nov 2018 19:22:48 +0100, Boris Feld wrote:
> >> # HG changeset patch
> >> # User Boris Feld <boris.feld@octobus.net>
> >> # Date 1498961267 -7200
> >> #      Sun Jul 02 04:07:47 2017 +0200
> >> # Node ID afdc73b20bd1faeec1b278ef0a2ab8d1dda71eb8
> >> # Parent  cc7d970d99eb242dbe2d8e792a9212aabd06911f
> >> # EXP-Topic vfs.audit-rename
> >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r afdc73b20bd1
> >> vfs: also audit rename
> > Queued the first 4 patches, thanks.
> >
> >> diff --git a/mercurial/vfs.py b/mercurial/vfs.py
> >> --- a/mercurial/vfs.py
> >> +++ b/mercurial/vfs.py
> >> @@ -436,6 +436,10 @@ class vfs(abstractvfs):
> >>  
> >>          return fp
> >>  
> >> +    def rename(self, src, dst, *args, **kwargs):
> >> +        self._auditpath(dst, 'w')
> >> +        return super(vfs, self).rename(src, dst, *args, **kwargs)
> > Can you move this to abstractvfs.rename()?
> >
> > I don't think it's worth to override rename() just for _auditpath(). Instead,
> > abstractvfs can implement _auditpath() that raises exception.
> 
> I'm not sure what are your concerns and what would be the point of
> moving that one layer up. Currently, the _auditpath logic is exclusive
> to vfs. It comes from the `vfs.__call__` method that the `abstractvfs`
> class do not have. What makes it worthy to move it in `abstractvfs`?
> 
> (I'm not opposed to doing so, I'm trying to understand the logic and
> responsibility of each class)

Since the abstractvfs provides most of the filesystem functions (except
__call__()), I just thought it would make sense to keep rename() in it.
I have no idea why __call__ isn't implemented by abstractvfs.

> > Also, 'dst' is likely to be relative in abstractvfs.rename(), so we won't
> > probably need to convert absolute 'dst' back to relative path.
> What's your reasoning here? We directly call the underlying `rename`
> method in the `vfs` class, and we know for sure that they can receive
> absolute paths.

Sorry, I misread the inheritance.

FWIW, I think the vfs is designed to receive relative paths, though absolute
path also works. It's probably wrong to access files out of the vfs.base.
Boris Feld - Dec. 21, 2018, 2:08 p.m.
On 03/12/2018 12:03, Yuya Nishihara wrote:
> On Sun, 2 Dec 2018 16:30:28 +0100, Boris FELD wrote:
>> On 28/11/2018 13:04, Yuya Nishihara wrote:
>>> On Mon, 26 Nov 2018 19:22:48 +0100, Boris Feld wrote:
>>>> # HG changeset patch
>>>> # User Boris Feld <boris.feld@octobus.net>
>>>> # Date 1498961267 -7200
>>>> #      Sun Jul 02 04:07:47 2017 +0200
>>>> # Node ID afdc73b20bd1faeec1b278ef0a2ab8d1dda71eb8
>>>> # Parent  cc7d970d99eb242dbe2d8e792a9212aabd06911f
>>>> # EXP-Topic vfs.audit-rename
>>>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>>>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r afdc73b20bd1
>>>> vfs: also audit rename
>>> Queued the first 4 patches, thanks.
>>>
>>>> diff --git a/mercurial/vfs.py b/mercurial/vfs.py
>>>> --- a/mercurial/vfs.py
>>>> +++ b/mercurial/vfs.py
>>>> @@ -436,6 +436,10 @@ class vfs(abstractvfs):
>>>>  
>>>>          return fp
>>>>  
>>>> +    def rename(self, src, dst, *args, **kwargs):
>>>> +        self._auditpath(dst, 'w')
>>>> +        return super(vfs, self).rename(src, dst, *args, **kwargs)
>>> Can you move this to abstractvfs.rename()?
>>>
>>> I don't think it's worth to override rename() just for _auditpath(). Instead,
>>> abstractvfs can implement _auditpath() that raises exception.
>> I'm not sure what are your concerns and what would be the point of
>> moving that one layer up. Currently, the _auditpath logic is exclusive
>> to vfs. It comes from the `vfs.__call__` method that the `abstractvfs`
>> class do not have. What makes it worthy to move it in `abstractvfs`?
>>
>> (I'm not opposed to doing so, I'm trying to understand the logic and
>> responsibility of each class)
> Since the abstractvfs provides most of the filesystem functions (except
> __call__()), I just thought it would make sense to keep rename() in it.
> I have no idea why __call__ isn't implemented by abstractvfs.

So what should we do here?

1) keep series as is
2) move rename in abstractvfs (keeping actual audit in `vfs`)
3) try to move __call__ in abstractbvfs (keep actual audit in `vfs`)
4) something else.
>
>>> Also, 'dst' is likely to be relative in abstractvfs.rename(), so we won't
>>> probably need to convert absolute 'dst' back to relative path.
>> What's your reasoning here? We directly call the underlying `rename`
>> method in the `vfs` class, and we know for sure that they can receive
>> absolute paths.
> Sorry, I misread the inheritance.
>
> FWIW, I think the vfs is designed to receive relative paths, though absolute
> path also works. It's probably wrong to access files out of the vfs.base.
There are various locations that use absolute paths (often within the
vfs itself). I don't think it is reasonable to update them and change
the API in the same series.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Dec. 22, 2018, 3:01 a.m.
On Fri, 21 Dec 2018 15:08:03 +0100, Boris FELD wrote:
> On 03/12/2018 12:03, Yuya Nishihara wrote:
> > On Sun, 2 Dec 2018 16:30:28 +0100, Boris FELD wrote:
> >> On 28/11/2018 13:04, Yuya Nishihara wrote:
> >>> On Mon, 26 Nov 2018 19:22:48 +0100, Boris Feld wrote:
> >>>> # HG changeset patch
> >>>> # User Boris Feld <boris.feld@octobus.net>
> >>>> # Date 1498961267 -7200
> >>>> #      Sun Jul 02 04:07:47 2017 +0200
> >>>> # Node ID afdc73b20bd1faeec1b278ef0a2ab8d1dda71eb8
> >>>> # Parent  cc7d970d99eb242dbe2d8e792a9212aabd06911f
> >>>> # EXP-Topic vfs.audit-rename
> >>>> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >>>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r afdc73b20bd1
> >>>> vfs: also audit rename
> >>> Queued the first 4 patches, thanks.
> >>>
> >>>> diff --git a/mercurial/vfs.py b/mercurial/vfs.py
> >>>> --- a/mercurial/vfs.py
> >>>> +++ b/mercurial/vfs.py
> >>>> @@ -436,6 +436,10 @@ class vfs(abstractvfs):
> >>>>  
> >>>>          return fp
> >>>>  
> >>>> +    def rename(self, src, dst, *args, **kwargs):
> >>>> +        self._auditpath(dst, 'w')
> >>>> +        return super(vfs, self).rename(src, dst, *args, **kwargs)
> >>> Can you move this to abstractvfs.rename()?
> >>>
> >>> I don't think it's worth to override rename() just for _auditpath(). Instead,
> >>> abstractvfs can implement _auditpath() that raises exception.
> >> I'm not sure what are your concerns and what would be the point of
> >> moving that one layer up. Currently, the _auditpath logic is exclusive
> >> to vfs. It comes from the `vfs.__call__` method that the `abstractvfs`
> >> class do not have. What makes it worthy to move it in `abstractvfs`?
> >>
> >> (I'm not opposed to doing so, I'm trying to understand the logic and
> >> responsibility of each class)
> > Since the abstractvfs provides most of the filesystem functions (except
> > __call__()), I just thought it would make sense to keep rename() in it.
> > I have no idea why __call__ isn't implemented by abstractvfs.
> 
> So what should we do here?
> 
> 1) keep series as is
> 2) move rename in abstractvfs (keeping actual audit in `vfs`)
> 3) try to move __call__ in abstractbvfs (keep actual audit in `vfs`)
> 4) something else.

As I said before, my preference is (2).

Patch

diff --git a/mercurial/vfs.py b/mercurial/vfs.py
--- a/mercurial/vfs.py
+++ b/mercurial/vfs.py
@@ -436,6 +436,10 @@  class vfs(abstractvfs):
 
         return fp
 
+    def rename(self, src, dst, *args, **kwargs):
+        self._auditpath(dst, 'w')
+        return super(vfs, self).rename(src, dst, *args, **kwargs)
+
     def symlink(self, src, dst):
         self.audit(dst)
         linkname = self.join(dst)