Skip to content

Commit 994c066

Browse files
authored
Merge pull request #33 from sgaisser/source_handler_check
Source handler check
2 parents c32f836 + 03c5542 commit 994c066

File tree

7 files changed

+350
-25
lines changed

7 files changed

+350
-25
lines changed

CHANGELOG.md

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
88

99
# [unreleased]
1010

11+
## Added
12+
13+
- Added `RestrictedFilestore` to limit the file access of the `NativeFilestore` to a specific
14+
directory.
15+
1116
## Fixed
1217

1318
- Correction for `InvalidDestinationId` exception arguments in destination handler.
14-
- Destination handler now only checks entity ID values when checking inserted packets
19+
- Destination handler now only checks entity ID values when checking inserted packets.
20+
- Source handler used an incorrect check if the file exists without the virtual filestore.
1521

1622
# [v0.4.0] 2024-11-08
1723

src/cfdppy/__init__.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
from spacepackets.cfdp import TransactionId
66

77
from .defs import CfdpIndication, CfdpState
8-
from .filestore import HostFilestore
8+
from .filestore import HostFilestore, VirtualFilestore
99
from .handler.common import PacketDestination, get_packet_destination
1010
from .mib import (
1111
IndicationCfg,
@@ -14,6 +14,7 @@
1414
RemoteEntityCfgTable,
1515
)
1616
from .request import PutRequest
17+
from .restricted_filestore import RestrictedFilestore
1718
from .user import CfdpUserBase
1819

1920
__all__ = [
@@ -27,6 +28,8 @@
2728
"PutRequest",
2829
"RemoteEntityCfg",
2930
"RemoteEntityCfgTable",
31+
"RestrictedFilestore",
3032
"TransactionId",
33+
"VirtualFilestore",
3134
"get_packet_destination",
3235
]

src/cfdppy/filestore.py

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import os
88
import platform
99
import shutil
10+
import subprocess
1011
from typing import TYPE_CHECKING, BinaryIO
1112

1213
from crcmod.predefined import PredefinedCrc
@@ -310,24 +311,28 @@ def list_directory(
310311
:param recursive:
311312
:return:
312313
"""
314+
if not dir_name.exists() or not dir_name.is_dir():
315+
_LOGGER.warning(f"{dir_name} does not exist or is not a directory")
316+
return FilestoreResponseStatusCode.NOT_PERFORMED
317+
318+
if platform.system() == "Linux" or platform.system() == "Darwin":
319+
cmd = ["ls", "-al"]
320+
elif platform.system() == "Windows":
321+
cmd = ["dir"]
322+
else:
323+
_LOGGER.warning(f"Unknown OS {platform.system()}, do not know how to list directory")
324+
return FilestoreResponseStatusCode.NOT_PERFORMED
325+
313326
open_flag = "a" if target_file.exists() else "w"
314327
with open(target_file, open_flag) as of:
315-
if platform.system() == "Linux" or platform.system() == "Darwin":
316-
cmd = "ls -al"
317-
elif platform.system() == "Windows":
318-
cmd = "dir"
319-
else:
320-
_LOGGER.warning(
321-
f"Unknown OS {platform.system()}, do not know how to list directory"
322-
)
323-
return FilestoreResponseStatusCode.NOT_PERFORMED
324328
of.write(f"Contents of directory {dir_name} generated with '{cmd}':\n")
325-
curr_path = os.getcwd()
326-
os.chdir(dir_name)
327-
os.system( # noqa S605 TODO this is dangerous as the user has control over the target_file
328-
f'{cmd} >> "{target_file}"'
329-
)
330-
os.chdir(curr_path)
329+
try:
330+
# cmd is not modified by user input and dir_name has been checked above
331+
result = subprocess.run(cmd, check=True, capture_output=True, cwd=dir_name) # noqa S603
332+
except (subprocess.CalledProcessError, OSError) as e:
333+
_LOGGER.error(f"Failed to list directory {dir_name}: {e}")
334+
return FilestoreResponseStatusCode.NOT_PERFORMED
335+
of.write(result.stdout.decode())
331336
return FilestoreResponseStatusCode.SUCCESS
332337

333338
def _verify_checksum(self, checksum_type: ChecksumType) -> None:

src/cfdppy/handler/source.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -573,7 +573,7 @@ def _prepare_file_params(self) -> None:
573573
self._params.fp.metadata_only = True
574574
else:
575575
assert self._put_req.source_file is not None
576-
if not self._put_req.source_file.exists():
576+
if not self.user.vfs.file_exists(self._put_req.source_file):
577577
# TODO: Handle this exception in the handler, reset CFDP state machine
578578
raise SourceFileDoesNotExist(self._put_req.source_file)
579579
file_size = self.user.vfs.file_size(self._put_req.source_file)

src/cfdppy/restricted_filestore.py

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
"""Wrapper to restrict filestore access to a specific directory.
2+
3+
This class will limit the filestore access to a specific directory.
4+
All relative paths will be relative to this directory.
5+
All absolute paths will be converted to subpaths of the restricted path e.g.
6+
/tmp/file.txt -> /restricted_path/tmp/file.txt
7+
8+
This is not a security feature but a convenience feature to limit filestore
9+
access to a specific directory.
10+
"""
11+
12+
from __future__ import annotations # Python 3.9 compatibility for | syntax
13+
14+
from typing import TYPE_CHECKING
15+
16+
from cfdppy.filestore import NativeFilestore
17+
18+
if TYPE_CHECKING:
19+
from pathlib import Path
20+
21+
from spacepackets.cfdp import ChecksumType, FilestoreResponseStatusCode
22+
23+
24+
class RestrictedFilestore(NativeFilestore):
25+
"""Wrapper to restrict filestore access to a specific directory."""
26+
27+
def __init__(self, restricted_path: Path):
28+
"""Create a new RestrictedFilestore instance.
29+
30+
The path is used to restrict all paths as relative to this path.
31+
Absolute paths will be converted to subpaths of the restricted path
32+
keeping the original path structure.
33+
34+
:param restricted_path: Path to restrict the filestore to
35+
"""
36+
super().__init__()
37+
self.restricted_path = restricted_path
38+
39+
def __make_local(self, file: Path) -> Path:
40+
"""Make file paths subfolders of the restricted path.
41+
42+
:param file: File to make relative to the restricted path
43+
:return: New Path
44+
"""
45+
if not file.is_relative_to(self.restricted_path):
46+
if file.is_absolute():
47+
return self.restricted_path.joinpath(file.relative_to(file.anchor))
48+
return self.restricted_path.joinpath(file)
49+
return file
50+
51+
def read_data(self, file: Path, offset: int | None, read_len: int | None = None) -> bytes:
52+
"""Read data from file."""
53+
return super().read_data(self.__make_local(file), offset, read_len)
54+
55+
def is_directory(self, path: Path) -> bool:
56+
"""Check if path is a directory."""
57+
return super().is_directory(self.__make_local(path))
58+
59+
def filename_from_full_path(self, path: Path) -> str | None:
60+
"""Get filename from full path."""
61+
return super().filename_from_full_path(self.__make_local(path))
62+
63+
def file_exists(self, path: Path) -> bool:
64+
"""Check if file exists."""
65+
return super().file_exists(self.__make_local(path))
66+
67+
def truncate_file(self, file: Path) -> None:
68+
"""Truncate file."""
69+
return super().truncate_file(self.__make_local(file))
70+
71+
def file_size(self, file: Path) -> int:
72+
"""Get file size."""
73+
return super().file_size(self.__make_local(file))
74+
75+
def write_data(self, file: Path, data: bytes, offset: int | None) -> None:
76+
"""Write data to file."""
77+
return super().write_data(self.__make_local(file), data, offset)
78+
79+
def create_file(self, file: Path) -> FilestoreResponseStatusCode:
80+
"""Create file."""
81+
return super().create_file(self.__make_local(file))
82+
83+
def delete_file(self, file: Path) -> FilestoreResponseStatusCode:
84+
"""Delete file."""
85+
return super().delete_file(self.__make_local(file))
86+
87+
def rename_file(self, _old_file: Path, _new_file: Path) -> FilestoreResponseStatusCode:
88+
"""Rename file."""
89+
return super().rename_file(self.__make_local(_old_file), self.__make_local(_new_file))
90+
91+
def replace_file(self, _replaced_file: Path, _source_file: Path) -> FilestoreResponseStatusCode:
92+
"""Replace file."""
93+
return super().replace_file(
94+
self.__make_local(_replaced_file), self.__make_local(_source_file)
95+
)
96+
97+
def create_directory(self, _dir_name: Path) -> FilestoreResponseStatusCode:
98+
"""Create directory."""
99+
return super().create_directory(self.__make_local(_dir_name))
100+
101+
def remove_directory(
102+
self, dir_name: Path, recursive: bool = False
103+
) -> FilestoreResponseStatusCode:
104+
"""Remove directory."""
105+
return super().remove_directory(dir_name=self.__make_local(dir_name), recursive=recursive)
106+
107+
def list_directory(
108+
self, _dir_name: Path, _file_name: Path, _recursive: bool = False
109+
) -> FilestoreResponseStatusCode:
110+
"""List directory contents."""
111+
return super().list_directory(
112+
self.__make_local(_dir_name), self.__make_local(_file_name), _recursive
113+
)
114+
115+
def calculate_checksum(
116+
self,
117+
checksum_type: ChecksumType,
118+
file_path: Path,
119+
size_to_verify: int,
120+
segment_len: int = 4096,
121+
) -> bytes:
122+
"""Calculate checksum of file.
123+
124+
:param checksum_type: Type of checksum
125+
:param file_path: Path to file
126+
:param size_to_verify: Size to check in bytes
127+
:param segment_len: Length of segments to calculate checksum for
128+
:return: checksum as bytes
129+
"""
130+
return super().calculate_checksum(
131+
checksum_type, self.__make_local(file_path), size_to_verify, segment_len
132+
)
133+
134+
def verify_checksum(
135+
self,
136+
checksum: bytes,
137+
checksum_type: ChecksumType,
138+
file_path: Path,
139+
size_to_verify: int,
140+
segment_len: int = 4096,
141+
) -> bool:
142+
"""Verify checksum of file."""
143+
return super().verify_checksum(
144+
checksum=checksum,
145+
checksum_type=checksum_type,
146+
file_path=self.__make_local(file_path),
147+
size_to_verify=size_to_verify,
148+
segment_len=segment_len,
149+
)

tests/test_filestore.py

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
import os.path
2+
import shutil
23
import struct
34
import tempfile
45
from pathlib import Path
6+
from unittest import TestCase
57

6-
from pyfakefs.fake_filesystem_unittest import TestCase
8+
from spacepackets.cfdp import ChecksumType
79

810
from cfdppy.crc import calc_modular_checksum
911
from cfdppy.filestore import FilestoreResult, NativeFilestore
@@ -31,16 +33,15 @@
3133

3234
class TestCfdpHostFilestore(TestCase):
3335
def setUp(self):
34-
self.setUpPyfakefs()
35-
self.temp_dir = tempfile.gettempdir()
36+
self.temp_dir = tempfile.mkdtemp()
3637
self.test_file_name_0 = Path(f"{self.temp_dir}/cfdp_unittest0.txt")
3738
self.test_file_name_1 = Path(f"{self.temp_dir}/cfdp_unittest1.txt")
3839
self.test_dir_name_0 = Path(f"{self.temp_dir}/cfdp_test_folder0")
3940
self.test_dir_name_1 = Path(f"{self.temp_dir}/cfdp_test_folder1")
4041
self.test_list_dir_name = Path(f"{self.temp_dir}/list-dir-test.txt")
4142
self.filestore = NativeFilestore()
4243

43-
self.file_path = Path(f"{tempfile.gettempdir()}/crc_file")
44+
self.file_path = Path(f"{self.temp_dir}/crc_file")
4445
with open(self.file_path, "wb") as file:
4546
file.write(EXAMPLE_DATA_CFDP)
4647
# Kind of re-writing the modular checksum impl here which we are trying to test, but the
@@ -63,6 +64,9 @@ def setUp(self):
6364

6465
self.expected_checksum_for_example = struct.pack("!I", full_sum)
6566

67+
def tearDown(self):
68+
shutil.rmtree(self.temp_dir)
69+
6670
def test_creation(self):
6771
res = self.filestore.create_file(self.test_file_name_0)
6872
self.assertTrue(res == FilestoreResult.CREATE_SUCCESS)
@@ -140,6 +144,11 @@ def test_list_dir(self):
140144
def test_modular_checksum(self):
141145
self.assertEqual(calc_modular_checksum(self.file_path), self.expected_checksum_for_example)
142146

143-
def tearDown(self):
144-
if self.file_path.exists():
145-
os.remove(self.file_path)
147+
def test_zero_length_checksum(self):
148+
with self.assertRaises(ValueError):
149+
self.filestore.calculate_checksum(
150+
checksum_type=ChecksumType.CRC_32,
151+
file_path=self.file_path,
152+
size_to_verify=10,
153+
segment_len=0,
154+
)

0 commit comments

Comments
 (0)