update Dockerfile and middleware for crypt4gh - #33
Open
vschnei wants to merge 3 commits into
Open
Conversation
…nce decryption logic, and adjust permissions
Reviewer's GuideUpdates the Crypt4GH middleware to operate correctly with read-only container mounts by switching to a crypt4gh-enabled base image, hardening file handling and permissions in the decryption workflow, and integrating the middleware with the AbstractMiddleware interface while filtering to .c4gh inputs and updating dependency management to Poetry 2.x format. Sequence diagram for CryptMiddleware apply_middleware flow with .c4gh filteringsequenceDiagram
actor Client
participant FlaskApp
participant CryptMiddleware
participant Config as FocaConfig
Client->>FlaskApp: HTTP request with JSON inputs
FlaskApp->>CryptMiddleware: apply_middleware(request)
CryptMiddleware->>CryptMiddleware: _set_original_input_paths(request)
loop for each input_body in request.json[inputs]
CryptMiddleware->>CryptMiddleware: validate path not starting with VOLUME_PATH
alt path endswith .c4gh
CryptMiddleware->>CryptMiddleware: append path to original_input_paths
else non .c4gh input
CryptMiddleware->>CryptMiddleware: skip path
end
end
CryptMiddleware->>CryptMiddleware: _change_executor_paths(request)
CryptMiddleware->>CryptMiddleware: _add_volume(request)
CryptMiddleware->>CryptMiddleware: _add_decryption_executor(request)
FlaskApp->>Config: read foca.custom.tes.service_list
Config-->>FlaskApp: tes service urls
FlaskApp->>CryptMiddleware: pass tes service urls
CryptMiddleware->>CryptMiddleware: tes_urls = unique list of urls
CryptMiddleware->>FlaskApp: set request.json[tes_urls]
CryptMiddleware-->>FlaskApp: modified request
FlaskApp-->>Client: response (after downstream processing)
Class diagram for updated CryptMiddleware integrationclassDiagram
class AbstractMiddleware {
}
class CryptMiddleware {
- list original_input_paths
- list tes_urls
+ CryptMiddleware()
+ apply_middleware(request)
- _set_original_input_paths(request)
- _change_executor_paths(request)
- _add_volume(request)
- _add_decryption_executor(request)
}
CryptMiddleware --|> AbstractMiddleware
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
decrypt_files,NamedTemporaryFile(delete=False)combined with multiple exception paths means temporary files are never cleaned up on failure (e.g.,ValueError), so consider wrapping the move/decrypt in atry/finallythat unlinksf_out.nameon error. - In
_set_original_input_paths,path = input_body.get("path")can beNone, which will causepath.startswith(...)andstr(path).lower().endswith(...)to fail; add a type/None check (or default to empty string) before calling string methods. - The updated
Dockerfileswitches tolvarin/crypt4gh:1.6but no longer copies or installs this project’s code into the image, so the built container will not contain or run the middleware unless you reintroduce the appropriateCOPY/install steps.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `decrypt_files`, `NamedTemporaryFile(delete=False)` combined with multiple exception paths means temporary files are never cleaned up on failure (e.g., `ValueError`), so consider wrapping the move/decrypt in a `try/finally` that unlinks `f_out.name` on error.
- In `_set_original_input_paths`, `path = input_body.get("path")` can be `None`, which will cause `path.startswith(...)` and `str(path).lower().endswith(...)` to fail; add a type/None check (or default to empty string) before calling string methods.
- The updated `Dockerfile` switches to `lvarin/crypt4gh:1.6` but no longer copies or installs this project’s code into the image, so the built container will not contain or run the middleware unless you reintroduce the appropriate `COPY`/install steps.
## Individual Comments
### Comment 1
<location> `crypt4gh_middleware/decrypt.py:62` </location>
<code_context>
- with open(file_path, "rb") as f_in, NamedTemporaryFile() as f_out:
+ if not file_path.name.endswith('.c4gh'):
+ continue
+ with open(file_path, "rb") as f_in, NamedTemporaryFile(delete=False) as f_out:
try:
decrypt(keys=key_tuples, infile=f_in, outfile=f_out) # Checks for magic
</code_context>
<issue_to_address>
**issue (bug_risk):** Temporary files are never cleaned up on decryption failure when using `delete=False`.
Using `NamedTemporaryFile(delete=False)` means any exception in `decrypt()` will leave the temp file on disk. Please either (a) wrap decrypt + move in try/finally and unlink `f_out.name` when the move doesn’t occur, or (b) keep `delete=True` and reopen the temp path for the move, so we don’t accumulate orphaned temp files in long‑running processes.
</issue_to_address>
### Comment 2
<location> `crypt4gh_middleware/decrypt.py:120` </location>
<code_context>
raise ValueError(f"Could not remove files: {directory} is not a directory.")
for file in directory.iterdir():
- subprocess.run(["rm", "-P", str(file)], check=True)
+ subprocess.run(["rm", "-R", str(file)], check=True)
logger.debug(f"Removed {file.name}")
</code_context>
<issue_to_address>
**🚨 issue (security):** Switching from `rm -P` to `rm -R` removes the secure-wipe behavior and conflicts with the function description.
The docstring still says files are "rewritten" before removal, but `rm -R` only deletes recursively and does not securely overwrite data like `rm -P`. If secure deletion is required, this is a security regression. Either restore secure wiping (e.g., overwrite files while still handling directories recursively) or update the docstring and function naming/description to remove the secure-wipe guarantee.
</issue_to_address>
### Comment 3
<location> `crypt4gh_middleware/middleware.py:80-81` </location>
<code_context>
"""
for input_body in request.json["inputs"]:
- if input_body["path"].startswith(VOLUME_PATH):
+ path = input_body.get("path")
+ if path.startswith(VOLUME_PATH):
raise PathNotAllowedException(f"{VOLUME_PATH} is not allowed in input path.")
- self.original_input_paths.append(input_body["path"])
</code_context>
<issue_to_address>
**issue:** `input_body.get("path")` can be `None`, causing an `AttributeError` on `.startswith`.
Using `get()` means `path` can now be `None`, so `path.startswith(...)` will fail if the key is missing. Either ensure the payload always includes a non-empty `"path"` (and document/enforce that), or guard against `None` before calling `.startswith`. Reverting to direct indexing is also fine if the key is truly mandatory.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
vschnei
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes issues with the Crypt4GH middleware that prevented it from working correctly in environments with read-only container mounts.
Changes Made
Dockerfile:
python:3.12tolvarin/crypt4gh:1.6for crypt4gh supportdecrypt.py:
.c4ghfile extension removal after successful decryptioncopy2whenmovefails on read-only filesystems.c4ghfiles during decryption processrm -Rcommand (was incorrectly using-Pflag)middleware.py:
AbstractMiddlewarefor integration.c4ghfilesDependencies:
poetry.lockwith new dependency groups formatRelated Issues
Fixes #32
Summary by Sourcery
Update Crypt4GH middleware and container image to work correctly with read-only mounts and ensure decrypted outputs are usable by downstream components.
New Features:
Bug Fixes:
Enhancements:
Build:
Chores: