SED-4816 Introduce AttachmentStorage#676
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new AttachmentStorage interface and its implementations (LocalAttachmentStorage and StepStreamingResourceManager) to decouple attachment handling from the general ResourceManager. Various components, including ExportHandler, JUnitXmlReportBuilder, and ReportNodeAttachmentManager, have been refactored to use this new abstraction. The review feedback highlights several critical and high-severity issues, including a path traversal vulnerability in LocalAttachmentStorage due to unsanitized filenames, a potential NullPointerException in PlanRunnerResult when attachmentStorage is null, and fragile directory creation logic. Additionally, improvements are suggested for input validation on attachment IDs, safer type casting of the execution context, pre-compiling regular expressions for performance, and using more portable file path constructors.
david-stephan
left a comment
david-stephan
left a comment
There was a problem hiding this comment.
approved, with minor comment
| String className = executionContext == null ? "null" : executionContext.getClass().getName(); | ||
| return new SkippedAttachmentMeta(filename, mimeType, "UNEXPECTED: Invalid execution context type of class " + className); | ||
| } | ||
| ExecutionContext context = (ExecutionContext) Objects.requireNonNull(executionContext); |
There was a problem hiding this comment.
don't think requireNonNull is needed here as it should enter the if condition above.
| mimeType = sanitizeMimeType(mimeType); | ||
| boolean supportsLineAccess = isLineAccessSupported(mimeType); | ||
| StreamingResourceMetadata metadata = new StreamingResourceMetadata(filename, mimeType, supportsLineAccess); | ||
| String resourceId = registerNewResource(metadata, uploadContextId); |
There was a problem hiding this comment.
not related to this PR and should definitely not be refactored as part of it, but we should have avoided to use the word "resource" in the context of streaming as it's quite confusing with the legacy Step "resources"
2 participants
No description provided.