Skip to content

H5json #420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

H5json #420

wants to merge 31 commits into from

Conversation

jreadey
Copy link
Member

@jreadey jreadey commented Apr 23, 2025

Use h5json package for typing and objids

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 23, 2025
View reviewed changes
mattjala
mattjala reviewed May 7, 2025
View reviewed changes
mattjala
mattjala reviewed May 7, 2025
View reviewed changes
mattjala
mattjala reviewed May 7, 2025
View reviewed changes
tests/unit/array_util_test.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we now depend on hdf5-json to do this testing, it might be a good idea to include hdf5-json's tests as a step in the CI

mattjala
mattjala previously approved these changes May 7, 2025
View reviewed changes
Copy link
Contributor

@mattjala mattjala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides a few minor comments and questions, this is good to go in. I'll try to get the outstanding PRs on hdf5-json reviewed this week so that we can avoid having HSDS depend on a specific branch.

mattjala
mattjala reviewed May 7, 2025
View reviewed changes
hsds/group_sn.py Outdated
created = link_item["created"]
# allow "pre-dated" attributes if recent enough
predate_max_time = config.get("predate_max_time", default=10.0)
if now - created > predate_max_time:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comparison seems backwards. If I understand correctly, the difference between current time and creation time should need to be under the max time, not above it

github-advanced-security[bot]
github-advanced-security bot found potential problems May 15, 2025
View reviewed changes
hsds/util/fileClient.py
dirpath = pp.normpath(dirpath)
log.debug(f"normpath: {dirpath}")

if not pp.isdir(dirpath):

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.

Copilot Autofix

AI about 1 month ago

To fix the issue, we need to ensure that the dirpath variable in the _mkdir method is validated to confirm it is within the safe root directory (self._root_dir). This can be achieved by:

  1. Normalizing the dirpath using os.path.normpath.
  2. Verifying that the normalized dirpath starts with self._root_dir. If it does not, raise an exception to prevent unsafe directory creation.

The _mkdir method should be updated to include this validation step before proceeding to create the directory.

Suggested changeset 1
hsds/util/fileClient.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/hsds/util/fileClient.py b/hsds/util/fileClient.py
--- a/hsds/util/fileClient.py
+++ b/hsds/util/fileClient.py
@@ -183,2 +183,8 @@
 
+            # Ensure dirpath is within the root directory
+            if not dirpath.startswith(self._root_dir):
+                msg = f"Attempt to create directory outside root: {dirpath}"
+                log.error(msg)
+                raise HTTPBadRequest(reason=msg)
+
             if not pp.isdir(dirpath):
EOF
@@ -183,2 +183,8 @@

# Ensure dirpath is within the root directory
if not dirpath.startswith(self._root_dir):
msg = f"Attempt to create directory outside root: {dirpath}"
log.error(msg)
raise HTTPBadRequest(reason=msg)

if not pp.isdir(dirpath):
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
hsds/util/fileClient.py

if not pp.isdir(dirpath):
log.debug(f"mkdir({dirpath})")
mkdir(dirpath)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.

Copilot Autofix

AI about 1 month ago

To fix the issue, we need to ensure that the constructed dirpath is validated to confirm it is within the intended root directory (self._root_dir). This can be achieved by:

  1. Normalizing the path using os.path.normpath.
  2. Verifying that the normalized path starts with self._root_dir.

The _getFilePath method should be updated to include this validation. Additionally, the _mkdir method should rely on the validated path returned by _getFilePath.

Suggested changeset 1
hsds/util/fileClient.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/hsds/util/fileClient.py b/hsds/util/fileClient.py
--- a/hsds/util/fileClient.py
+++ b/hsds/util/fileClient.py
@@ -59,4 +59,8 @@
     def _getFilePath(self, bucket, key=""):
-        filepath = pp.join(self._root_dir, bucket, key)
-        return pp.normpath(filepath)
+        filepath = pp.normpath(pp.join(self._root_dir, bucket, key))
+        if not filepath.startswith(self._root_dir):
+            msg = f"Invalid path: {filepath} is outside the root directory"
+            log.error(msg)
+            raise HTTPBadRequest(reason=msg)
+        return filepath
 
@@ -180,4 +184,4 @@
         try:
-            dirpath = pp.normpath(dirpath)
-            log.debug(f"normpath: {dirpath}")
+            log.debug(f"mkdir: validating path {dirpath}")
+            dirpath = self._getFilePath(dirpath, key="")  # Validate path
 
EOF
@@ -59,4 +59,8 @@
def _getFilePath(self, bucket, key=""):
filepath = pp.join(self._root_dir, bucket, key)
return pp.normpath(filepath)
filepath = pp.normpath(pp.join(self._root_dir, bucket, key))
if not filepath.startswith(self._root_dir):
msg = f"Invalid path: {filepath} is outside the root directory"
log.error(msg)
raise HTTPBadRequest(reason=msg)
return filepath

@@ -180,4 +184,4 @@
try:
dirpath = pp.normpath(dirpath)
log.debug(f"normpath: {dirpath}")
log.debug(f"mkdir: validating path {dirpath}")
dirpath = self._getFilePath(dirpath, key="") # Validate path

Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
@mattjala
Copy link
Contributor

mattjala commented Jun 3, 2025

@jreadey Linter issues are preventing CI from running on this right now

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

@mattjala mattjala mattjala left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jreadey @mattjala