From abe174d0c67e305e125fc63c8dbc21c291e4e36c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Sun, 5 Apr 2026 06:26:24 -0600 Subject: [PATCH] refactor: extract _check_open_perms to deduplicate permission checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The open/sysopen permission-check block was copy-pasted three times (in __open, _io_file_mock_open, and __sysopen) with subtle divergences. In __sysopen, the elsif branch checking parent-dir perms was dead code: contents was always defined at that point because the ENOENT check above already returned. This meant sysopen(FH, $path, O_WRONLY|O_CREAT) in a restricted parent directory silently succeeded — inconsistent with open(). Fix: capture $is_new before O_CREAT populates contents, extract a shared _check_open_perms() helper used by all three call sites. Also normalizes the autodie throw pattern (all now use _maybe_throw_autodie). Closes #329 Closes #330 Co-Authored-By: Claude Opus 4.6 --- lib/Test/MockFile.pm | 104 +++++++++++++++++-------------------------- t/perms.t | 35 +++++++++++++++ 2 files changed, 77 insertions(+), 62 deletions(-) diff --git a/lib/Test/MockFile.pm b/lib/Test/MockFile.pm index 6c5d4cf..229d8ea 100644 --- a/lib/Test/MockFile.pm +++ b/lib/Test/MockFile.pm @@ -783,6 +783,38 @@ sub _check_parent_perms { return _check_perms( $parent_mock, $access ); } +# _check_open_perms($mock_file, $abs_path, $rw, $is_new, $func_name, @orig_args) +# Unified permission check for open/sysopen. +# $is_new: true when the file is being created (contents was undef before O_CREAT). +# Returns 1 if allowed, 0 if denied (with $! and autodie set). +sub _check_open_perms { + my ( $mock_file, $abs_path, $rw, $is_new, $func_name, @orig_args ) = @_; + + return 1 unless defined $_mock_uid; + + if ( !$is_new ) { + # Existing file: check file permissions + my $need = 0; + $need |= 4 if $rw =~ /r/; + $need |= 2 if $rw =~ /w/; + if ( !_check_perms( $mock_file, $need ) ) { + $! = EACCES; + _maybe_throw_autodie( $func_name, @orig_args ); + return 0; + } + } + elsif ( $rw =~ /w/ ) { + # Creating new file: check parent dir write+execute + if ( !_check_parent_perms( $abs_path, 2 | 1 ) ) { + $! = EACCES; + _maybe_throw_autodie( $func_name, @orig_args ); + return 0; + } + } + + return 1; +} + my @_tmf_callers; # Packages where autodie was active when T::MF was imported. @@ -2815,27 +2847,7 @@ sub _io_file_mock_open { $rw .= 'a' if grep { $_ eq $mode } qw/>> +>>/; # Permission check (GH #3) - if ( defined $_mock_uid ) { - if ( defined $contents ) { - # Existing file: check file permissions - my $need = 0; - $need |= 4 if $rw =~ /r/; - $need |= 2 if $rw =~ /w/; - if ( !_check_perms( $mock_file, $need ) ) { - $! = EACCES; - _maybe_throw_autodie( 'open', @_ ); - return undef; - } - } - elsif ( $rw =~ /w/ ) { - # Creating new file: check parent dir write+execute - if ( !_check_parent_perms( $abs_path, 2 | 1 ) ) { - $! = EACCES; - _maybe_throw_autodie( 'open', @_ ); - return undef; - } - } - } + return undef unless _check_open_perms( $mock_file, $abs_path, $rw, !defined $contents, 'open', @_ ); # Tie the existing IO::File glob directly (don't create a new one) tie *{$fh}, 'Test::MockFile::FileHandle', $abs_path, $rw; @@ -3166,26 +3178,8 @@ sub __open (*;$@) { $rw .= 'w' if grep { $_ eq $mode } qw/+< +> +>> > >>/; $rw .= 'a' if grep { $_ eq $mode } qw/>> +>>/; - # Permission check (GH #3) — IO::File path must match __open - if ( defined $_mock_uid ) { - if ( defined $contents ) { - my $need = 0; - $need |= 4 if $rw =~ /r/; - $need |= 2 if $rw =~ /w/; - if ( !_check_perms( $mock_file, $need ) ) { - $! = EACCES; - _maybe_throw_autodie( 'open', @_ ); - return undef; - } - } - elsif ( $rw =~ /w/ ) { - if ( !_check_parent_perms( $abs_path, 2 | 1 ) ) { - $! = EACCES; - _maybe_throw_autodie( 'open', @_ ); - return undef; - } - } - } + # Permission check (GH #3) + return undef unless _check_open_perms( $mock_file, $abs_path, $rw, !defined $contents, 'open', @_ ); my $filefh = IO::File->new; tie *{$filefh}, 'Test::MockFile::FileHandle', $abs_path, $rw; @@ -3326,8 +3320,12 @@ sub __sysopen (*$$;$) { return undef; } + # Track whether we're creating a new file (before O_CREAT populates contents). + # Used by the permission check below — without this, the elsif branch is dead. + my $is_new = !defined $mock_file->{'contents'}; + # O_CREAT — POSIX open(2): creating a new file sets atime, mtime, and ctime. - if ( $sysopen_mode & O_CREAT && !defined $mock_file->{'contents'} ) { + if ( $sysopen_mode & O_CREAT && $is_new ) { $mock_file->{'contents'} = ''; my $now = time; $mock_file->{'atime'} = $now; @@ -3370,26 +3368,8 @@ sub __sysopen (*$$;$) { return undef; } - # Permission check (GH #3) - if ( defined $_mock_uid ) { - if ( defined $mock_file->{'contents'} ) { - my $need = 0; - $need |= 4 if $rw =~ /r/; - $need |= 2 if $rw =~ /w/; - if ( !_check_perms( $mock_file, $need ) ) { - $! = EACCES; - _maybe_throw_autodie( 'sysopen', @_ ); - return undef; - } - } - elsif ( $rw =~ /w/ ) { - if ( !_check_parent_perms( $mock_file->{'path'}, 2 | 1 ) ) { - $! = EACCES; - _maybe_throw_autodie( 'sysopen', @_ ); - return undef; - } - } - } + # Permission check (GH #3) — uses $is_new captured before O_CREAT + return undef unless _check_open_perms( $mock_file, $mock_file->{'path'}, $rw, $is_new, 'sysopen', @_ ); $abs_path //= $mock_file->{'path'}; diff --git a/t/perms.t b/t/perms.t index 664da5f..2b6c882 100644 --- a/t/perms.t +++ b/t/perms.t @@ -336,4 +336,39 @@ subtest 'open > on new file checks parent directory perms' => sub { } 1000, 1000; }; +# ========================================================================= +# sysopen O_CREAT — parent-dir permission check (GH #329) +# Before the fix, the elsif branch checking parent-dir perms was dead code +# in __sysopen, so sysopen(FH, '/restricted/new', O_WRONLY|O_CREAT) would +# succeed even in a read-only parent directory. +# ========================================================================= + +subtest 'sysopen O_CREAT checks parent dir permissions' => sub { + my $parent = Test::MockFile->new_dir( '/perms/restricted', { mode => 0555, uid => 1000, gid => 1000 } ); + my $child = Test::MockFile->file('/perms/restricted/newfile'); + + with_user { + ok( !sysopen( my $fh, '/perms/restricted/newfile', O_WRONLY | O_CREAT ), + 'sysopen O_CREAT fails in read-only parent dir' ); + is( $! + 0, EACCES, 'sysopen errno is EACCES for restricted parent' ); + } 2000, 2000; + + # Owner also cannot create in a 0555 directory (no write bit for anyone) + with_user { + ok( !sysopen( my $fh, '/perms/restricted/newfile', O_WRONLY | O_CREAT ), + 'sysopen O_CREAT fails for owner too when parent has no write bit' ); + is( $! + 0, EACCES, 'sysopen errno is EACCES for owner too' ); + } 1000, 1000; + + # With write permission on parent, O_CREAT should succeed + my $parent2 = Test::MockFile->new_dir( '/perms/writable', { mode => 0755, uid => 1000, gid => 1000 } ); + my $child2 = Test::MockFile->file('/perms/writable/newfile2'); + + with_user { + ok( sysopen( my $fh, '/perms/writable/newfile2', O_WRONLY | O_CREAT ), + 'sysopen O_CREAT succeeds in writable parent dir' ); + close $fh if $fh; + } 1000, 1000; +}; + done_testing();