mattcasters commented on code in PR #8276: URL: https://github.com/apache/hop/pull/8276#discussion_r3943604641
########## rap/src/main/java/org/apache/hop/ui/hopgui/UserFileTransfer.java: ########## @@ -0,0 +1,297 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hop.ui.hopgui; + +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import jakarta.servlet.http.HttpSession; +import java.io.IOException; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.time.Duration; +import java.util.Comparator; +import java.util.Map; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.stream.Stream; +import org.eclipse.rap.rwt.RWT; +import org.eclipse.rap.rwt.client.service.UrlLauncher; +import org.eclipse.rap.rwt.service.ServiceHandler; +import org.eclipse.rap.rwt.service.ServiceManager; +import org.eclipse.rap.rwt.service.UISession; +import org.eclipse.swt.SWT; +import org.eclipse.swt.widgets.FileDialog; +import org.eclipse.swt.widgets.Shell; + +/** Browser-backed file selection and download support for Hop Web. */ +final class UserFileTransfer { + + @FunctionalInterface + interface UploadListener { + void uploaded(String filename, Path uploadedFile) throws Exception; + + default void error(Exception exception) {} + } + + static final long DEFAULT_UPLOAD_TIME_LIMIT = Duration.ofMinutes(5).toMillis(); + static final long DOWNLOAD_TTL_NANOS = Duration.ofMinutes(10).toNanos(); + static final int MAX_PENDING_DOWNLOADS = 16; + + private static final String DOWNLOAD_SERVICE_PREFIX = UserFileTransfer.class.getName() + "."; + + private final Shell shell; + private final ServiceManager serviceManager; + private final UISession uiSession; + private final String uiSessionId; + private final String httpSessionId; + private final String downloadServiceId = DOWNLOAD_SERVICE_PREFIX + UUID.randomUUID(); + private final Path transferDirectory; + private final Path uploadDirectory; + private final Path downloadDirectory; + private final Map<String, Download> downloads = new ConcurrentHashMap<>(); + private final AtomicBoolean disposed = new AtomicBoolean(); + + UserFileTransfer(Shell shell, Path sessionTempDirectory) throws IOException { + this.shell = shell; + serviceManager = RWT.getServiceManager(); + uiSession = RWT.getUISession(); + uiSessionId = uiSession.getId(); + httpSessionId = uiSession.getHttpSession().getId(); + transferDirectory = + Files.createDirectory(sessionTempDirectory.resolve("transfer-" + UUID.randomUUID())); + uploadDirectory = Files.createDirectory(transferDirectory.resolve("uploads")); + downloadDirectory = Files.createDirectory(transferDirectory.resolve("downloads")); + serviceManager.registerServiceHandler(downloadServiceId, new DownloadServiceHandler()); + uiSession.addUISessionListener(event -> dispose()); + } + + void open(String acceptedExtensions, long uploadSizeLimit, UploadListener listener) { + if (isDisposed()) { + listener.error(new IOException("The browser file transfer session is closed.")); + return; + } + + Path requestDirectory = null; + try { + requestDirectory = + Files.createDirectory(uploadDirectory.resolve(UUID.randomUUID().toString())); + FileDialog dialog = new FileDialog(shell, SWT.OPEN); + dialog.setUploadDirectory(requestDirectory.toFile()); + dialog.setUploadSizeLimit(uploadSizeLimit); + dialog.setUploadTimeLimit(DEFAULT_UPLOAD_TIME_LIMIT); + if (acceptedExtensions != null && !acceptedExtensions.isBlank()) { + dialog.setFilterExtensions(acceptedExtensions.split(",")); + } + + String uploadedPath = dialog.open(); + if (uploadedPath == null || uploadedPath.isBlank()) { + if (!dialog.getExceptions().isEmpty()) { + throw new IOException( + "The browser could not upload the selected file.", dialog.getExceptions().get(0)); + } + return; + } + + Path uploadedFile = Path.of(uploadedPath); Review Comment: **[suggestion]** After RAP `FileDialog.open()`, the code accepts whatever absolute path RAP reports (`Path.of(uploadedPath)`) as long as it is a regular file under the size limit. It never checks that the file is inside `requestDirectory`. RAP `FileUploadProcessor` does `FilenameUtils.getName()` before `DiskFileUploadReceiver` writes `new File(uploadDir, fileName)`, so `../` in the client filename is usually stripped, but names like `..` and a future RAP/FileUpload change would write/read outside the per-request directory. `deleteTree(requestDirectory)` would also miss a file that landed elsewhere. **Suggestion:** Resolve the uploaded path with `NOFOLLOW_LINKS`, require `uploadedFile.startsWith(requestDirectory.toAbsolutePath().normalize())` (and that it is a direct child), and use only a sanitized basename for the listener’s `filename`. Reject/delete anything outside that directory. ########## plugins/misc/import/src/main/java/org/apache/hop/imports/kettle/KettleImportDialog.java: ########## @@ -725,7 +732,10 @@ private void doImport() { box.open(); } } catch (Exception e) { - new ErrorDialog(shell, "Error", "Error importing", e); + String title = BaseMessages.getString(PKG, "KettleImportDialog.Error.Title"); + String message = BaseMessages.getString(PKG, "KettleImportDialog.Error.Message"); + LogChannel.UI.logError(message, e); + new ErrorDialog(shell, title, message, new HopException(message)); Review Comment: **[bug]** The same pattern is applied in `doImport()`: the real failure is logged, then the dialog is given a fresh `HopException` and `KettleImportDialog.Error.Message` (“The Kettle/PDI import failed. See the server log for details.”). Import from the File menu on desktop is now opaque; the Details pane only shows the wrapper. **Suggestion:** Same split as the HopImportGuiPlugin dialog change. Pass the original exception into `ErrorDialog` unless `EnvironmentUtils.isWeb()`. ########## ui/src/main/resources/org/apache/hop/ui/hopgui/messages/messages_es_ES.properties: ########## @@ -97,6 +97,19 @@ HopGui.Menu.File=&Fichero HopGui.Menu.File.Close=&Cerrar HopGui.Menu.File.Close.All=Cerrar Todo HopGui.Menu.File.ExportToSVG=Exportar a SVG +HopGui.Menu.File.ExportProjectZip=Exportar proyecto ZIP +HopGui.Menu.File.ImportKettleZip=Importar ZIP de Kettle/PDI +HopGui.Menu.File.User=File Browser Review Comment: **[nit]** `HopGui.Menu.File.User` and `HopGui.FileBrowser.Error.Title` are left as English “File Browser” in the Spanish bundle. Neighboring File Browser strings were translated. **Suggestion:** Translate both keys (e.g. “Navegador de archivos”) so the new top-level menu is not mixed-language. ########## rap/src/main/java/org/apache/hop/ui/hopgui/HopWebUserFilePlugin.java: ########## @@ -0,0 +1,584 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hop.ui.hopgui; + +import java.io.FilterInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.lang.reflect.InvocationTargetException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Comparator; +import java.util.HashSet; +import java.util.IdentityHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import java.util.stream.Stream; +import java.util.zip.ZipEntry; +import java.util.zip.ZipInputStream; +import org.apache.commons.io.FilenameUtils; +import org.apache.commons.lang3.StringUtils; +import org.apache.hop.core.Const; +import org.apache.hop.core.exception.HopException; +import org.apache.hop.core.gui.plugin.GuiPlugin; +import org.apache.hop.core.gui.plugin.GuiRegistry; +import org.apache.hop.core.gui.plugin.menu.GuiMenuElement; +import org.apache.hop.core.gui.plugin.menu.GuiMenuItem; +import org.apache.hop.core.security.Permission; +import org.apache.hop.core.xml.XmlHandler; +import org.apache.hop.i18n.BaseMessages; +import org.apache.hop.pipeline.PipelineMeta; +import org.apache.hop.pipeline.PipelineSvgPainter; +import org.apache.hop.ui.core.dialog.EnterStringDialog; +import org.apache.hop.ui.core.dialog.ErrorDialog; +import org.apache.hop.ui.core.security.HopSecurityUi; +import org.apache.hop.ui.hopgui.file.IHopFileTypeHandler; +import org.apache.hop.ui.hopgui.file.pipeline.HopGuiPipelineGraph; +import org.apache.hop.ui.hopgui.file.workflow.HopGuiWorkflowGraph; +import org.apache.hop.workflow.WorkflowMeta; +import org.apache.hop.workflow.WorkflowSvgPainter; +import org.eclipse.rap.rwt.RWT; +import org.eclipse.rap.rwt.service.UISession; + +/** Hop Web menu actions for opening and saving files on the user's computer. */ +@GuiPlugin(name = "Hop Web user files") +public class HopWebUserFilePlugin { + + private static final Class<?> PKG = HopWebUserFilePlugin.class; + + public static final String ID_MAIN_MENU_FILE_USER = "15000-menu-file-user"; + + private static final String PROJECT_EXPORT_MENU_ID = "10055-menu-file-export-to-svg"; + private static final String KETTLE_IMPORT_MENU_ID = "10060-menu-tools-import"; + private static final String SESSION_TEMP_DIRECTORY = + HopWebUserFilePlugin.class.getName() + ".tempDirectory"; + private static final String HOP_FILE_EXTENSIONS = ".hpl,.hwf"; + private static final long MAX_HOP_FILE_SIZE = 25L * 1024 * 1024; + private static final long MAX_COMPRESSED_ZIP_SIZE = 100L * 1024 * 1024; + private static final long MAX_UNCOMPRESSED_ZIP_SIZE = 250L * 1024 * 1024; + private static final long MAX_UNCOMPRESSED_ZIP_ENTRY_SIZE = 64L * 1024 * 1024; + private static final long MIN_COMPRESSION_RATIO_SIZE = 1024L * 1024; + private static final int MAX_COMPRESSION_RATIO = 100; + private static final int MAX_ZIP_ENTRIES = 10_000; + private static final int MAX_ZIP_ENTRY_DEPTH = 32; + private static final int MAX_ZIP_ENTRY_NAME_LENGTH = 4096; + + private final Map<IHopFileTypeHandler, String> userFileNames = new IdentityHashMap<>(); + private UserFileTransfer transfer; + private boolean sessionCleanupRegistered; + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = ID_MAIN_MENU_FILE_USER, + label = "i18n::HopGui.Menu.File.User", + parentId = HopGui.ID_MAIN_MENU) + public void menuFileUser() { + // Category only. + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_NEW, + label = "i18n::HopGui.Menu.File.New", + image = "ui/images/add.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void newFile() { + HopGui.getInstance().menuFileNew(); + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_OPEN, + label = "i18n::HopGui.Menu.File.Open", + image = "ui/images/open.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void openFile() { + if (!HopSecurityUi.check(Permission.FILE_VIEW)) { + return; + } + try { + transfer() + .open( + HOP_FILE_EXTENSIONS, + MAX_HOP_FILE_SIZE, + new UserFileTransfer.UploadListener() { + @Override + public void uploaded(String filename, Path uploadedFile) { + openUploadedFile(filename, uploadedFile); + } + + @Override + public void error(Exception exception) { + showError("HopGui.FileBrowser.Error.Upload", exception); + } + }); + } catch (Exception e) { + showError("HopGui.FileBrowser.Error.Upload", e); + } + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_SAVE, + label = "i18n::HopGui.Menu.File.Save", + image = "ui/images/save.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void saveFile() { + downloadActiveFile(false); + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_SAVE_AS, + label = "i18n::HopGui.Menu.File.SaveAs", + image = "ui/images/save-as.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void saveFileAs() { + downloadActiveFile(true); + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_EXPORT_TO_SVG, + label = "i18n::HopGui.Menu.File.ExportToSVG", + image = "ui/images/image.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void exportToSvg() { + if (!HopSecurityUi.check(Permission.FILE_EXPORT)) { + return; + } + try { + HopGuiPipelineGraph pipelineGraph = HopGui.getActivePipelineGraph(); + if (pipelineGraph != null) { + String name = safeFilename(pipelineGraph.getPipelineMeta().getName(), ".svg"); + String svg = + PipelineSvgPainter.generatePipelineSvg( + pipelineGraph.getPipelineMeta(), 1.0f, pipelineGraph.getVariables()); + transfer().download(name, "image/svg+xml", svg.getBytes(StandardCharsets.UTF_8)); + return; + } + + HopGuiWorkflowGraph workflowGraph = HopGui.getActiveWorkflowGraph(); + if (workflowGraph != null) { + String name = safeFilename(workflowGraph.getWorkflowMeta().getName(), ".svg"); + String svg = + WorkflowSvgPainter.generateWorkflowSvg( + workflowGraph.getWorkflowMeta(), 1.0f, workflowGraph.getVariables()); + transfer().download(name, "image/svg+xml", svg.getBytes(StandardCharsets.UTF_8)); + return; + } + // The menu item is hidden until a pipeline or workflow is active. Keep this guard quiet as + // well in case a stale keyboard shortcut or menu event reaches the action. + return; + } catch (Exception e) { + showError("HopGui.FileBrowser.Error.ExportSvg", e); + } + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_EXPORT_PROJECT, + label = "i18n::HopGui.Menu.File.ExportProjectZip", + image = "export.svg", Review Comment: **[nit]** File Browser → Export Project ZIP uses `image = "export.svg"`. That file only exists in the projects plugin (`plugins/misc/projects/src/main/resources/export.svg`). The RAP classloader will miss it and fall back to `no_image.svg`. Other new items correctly use `ui/images/…`. **Suggestion:** Point at an icon that lives in `ui/images` (for example `ui/images/zipfile.svg` or `ui/images/download.svg`), matching `importFromKettleZip`. ########## plugins/misc/import/src/main/java/org/apache/hop/imports/gui/HopImportGuiPlugin.java: ########## @@ -44,23 +50,36 @@ public static HopImportGuiPlugin getInstance() { root = HopGui.ID_MAIN_MENU, id = ID_MAIN_MENU_FILE_IMPORT, label = "i18n::HopGuiImport.Menu.Item", - image = "kettle-logo.svg", + image = "ui/images/kettle-logo.svg", parentId = HopGui.ID_MAIN_MENU_FILE, separator = true) @GuiKeyboardShortcut(control = true, key = 'i', global = true) @GuiOsxKeyboardShortcut(command = true, key = 'i', global = true) public void menuToolsImport() { + menuToolsImport(null); + } + + /** Opens the Kettle/PDI import dialog with an optional source folder selected up front. */ + public void menuToolsImport(String sourceFolder) { + if (!HopSecurityUi.check(Permission.FILE_CREATE) + || !HopSecurityUi.check(Permission.METADATA_WRITE)) { + return; + } HopGui hopGui = HopGui.getInstance(); try { // Import using this Kettle import plugin... // KettleImport kettleImport = new KettleImport(); kettleImport.init(hopGui.getVariables(), hopGui.getLog()); + kettleImport.setInputFolderName(sourceFolder); KettleImportDialog dialog = new KettleImportDialog(hopGui.getShell(), hopGui.getVariables(), kettleImport); dialog.open(); } catch (Exception e) { - new ErrorDialog(hopGui.getShell(), "Error", "Error importing from Kettle", e); + String title = BaseMessages.getString(PKG, "HopGuiImport.Error.Title"); + String message = BaseMessages.getString(PKG, "HopGuiImport.Error.Message"); + hopGui.getLog().logError(message, e); + new ErrorDialog(hopGui.getShell(), title, message, new HopException(message)); Review Comment: **[bug]** Failures now log the real exception, then open `ErrorDialog` with `new HopException(message)` (no cause). The English text is “Unable to open the Kettle/PDI import. See the server log for details.” This plugin is used from File → Import on desktop as well as from File Browser on web. Desktop users no longer see the underlying error in the dialog, and there is no server log. **Suggestion:** Keep the generic, cause-stripped dialog for Hop Web only (as `ProjectsGuiPlugin.exportProject` already does with `showConfirmation ? e : new HopException(...)`). On desktop pass `e` through and use a message that does not say “server log”. ########## ui/src/main/java/org/apache/hop/ui/hopgui/HopGui.java: ########## @@ -2349,11 +2372,64 @@ public void handleFileCapabilities( mainToolbarWidgets.enableToolbarItem( fileType, handler, ID_MAIN_TOOLBAR_SAVE_AS, IHopFileType.CAPABILITY_SAVE_AS); + mainMenuWidgets.enableMenuItem( + fileType, handler, ID_MAIN_MENU_FILE_USER_SAVE, IHopFileType.CAPABILITY_SAVE, changed); Review Comment: **[suggestion]** File Browser → Save is enabled with the same `changed` flag as server File → Save. For a download action that is the wrong extra condition: after File Browser → Open of an unchanged pipeline, Save is greyed out and only Save As (an `EnterStringDialog`, then download) works. The error copy already calls this a download (`HopGui.FileBrowser.Error.Save=Unable to download the active file`). **Suggestion:** Enable File Browser Save whenever the active handler can serialize a pipeline/workflow (and `FILE_SAVE` is allowed), not only when the tab is dirty. Keep the dirty flag for the server File menu. ########## rap/src/test/java/org/apache/hop/ui/hopgui/HopWebUserFilePluginTest.java: ########## @@ -0,0 +1,149 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hop.ui.hopgui; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.ByteArrayOutputStream; +import java.lang.reflect.Method; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.UUID; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; +import org.apache.hop.core.exception.HopException; +import org.apache.hop.core.gui.plugin.menu.GuiMenuElement; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class HopWebUserFilePluginTest { + + @TempDir Path tempDirectory; + + @Test + void browserActionsAreRegisteredInsideTheFileUserMenu() throws Exception { + assertEquals( + HopGui.ID_MAIN_MENU, + HopWebUserFilePlugin.class + .getDeclaredMethod("menuFileUser") + .getAnnotation(GuiMenuElement.class) + .parentId()); + assertUserFileMenuAction("newFile", "15010-menu-file-user-new"); + assertUserFileMenuAction("openFile", "15020-menu-file-user-open"); + assertUserFileMenuAction("saveFile", HopGui.ID_MAIN_MENU_FILE_USER_SAVE); + assertUserFileMenuAction("saveFileAs", HopGui.ID_MAIN_MENU_FILE_USER_SAVE_AS); + assertUserFileMenuAction("exportToSvg", HopGui.ID_MAIN_MENU_FILE_USER_EXPORT_TO_SVG); + assertUserFileMenuAction("exportProjectZip", HopGui.ID_MAIN_MENU_FILE_USER_EXPORT_PROJECT); + assertUserFileMenuAction("importFromKettleZip", HopGui.ID_MAIN_MENU_FILE_USER_IMPORT_KETTLE); + assertEquals( + "ui/images/kettle-logo.svg", + HopWebUserFilePlugin.class + .getDeclaredMethod("importFromKettleZip") + .getAnnotation(GuiMenuElement.class) + .image()); + } + + @Test + void extractsSingleProjectFolder() throws Exception { + Path zip = zip("my-project/example.ktr", "kettle"); + Path extractionDirectory = Files.createDirectory(tempDirectory.resolve("extract-project")); + + Path source = HopWebUserFilePlugin.extractZip(zip, extractionDirectory); + + assertEquals(extractionDirectory.resolve("my-project"), source); + assertEquals("kettle", Files.readString(source.resolve("example.ktr"), StandardCharsets.UTF_8)); + } + + @Test + void rejectsPathsOutsideExtractionDirectory() throws Exception { + Path zip = zip("../outside.ktr", "unsafe"); + Path extractionDirectory = Files.createDirectory(tempDirectory.resolve("extract-traversal")); + + HopException exception = + assertThrows( + HopException.class, () -> HopWebUserFilePlugin.extractZip(zip, extractionDirectory)); + assertFalse(Files.exists(tempDirectory.resolve("outside.ktr"))); + assertFalse(exception.getMessage().contains("outside.ktr")); + } + + @Test + void rejectsWindowsStylePathTraversal() throws Exception { + Path zip = zip("..\\outside.ktr", "unsafe"); + Path extractionDirectory = Files.createDirectory(tempDirectory.resolve("extract-windows")); + + assertThrows( + HopException.class, () -> HopWebUserFilePlugin.extractZip(zip, extractionDirectory)); + assertFalse(Files.exists(tempDirectory.resolve("outside.ktr"))); + } + + @Test + void rejectsAbsoluteAndDriveQualifiedPaths() throws Exception { + Path unixZip = zip("/outside.ktr", "unsafe"); + Path driveZip = zip("C:/outside.ktr", "unsafe"); + Path unixDirectory = Files.createDirectory(tempDirectory.resolve("extract-absolute")); + Path driveDirectory = Files.createDirectory(tempDirectory.resolve("extract-drive")); + + assertThrows(HopException.class, () -> HopWebUserFilePlugin.extractZip(unixZip, unixDirectory)); + assertThrows( + HopException.class, () -> HopWebUserFilePlugin.extractZip(driveZip, driveDirectory)); + } + + @Test + void rejectsHighlyCompressedZipBomb() throws Exception { + Path zip = zip("project/repeated.ktr", "0".repeat(2 * 1024 * 1024)); + Path extractionDirectory = Files.createDirectory(tempDirectory.resolve("extract-ratio")); + + assertThrows( + HopException.class, () -> HopWebUserFilePlugin.extractZip(zip, extractionDirectory)); + } + + @Test + void sanitizesDownloadHeaders() { Review Comment: **[suggestion]** ZIP traversal/bomb tests and `contentDisposition` sanitization are useful, but nothing asserts the download invariants the PR claims: one-time tokens, HTTP+UI session match → 404 otherwise, TTL eviction, max 16 pending, or upload path containment. `sanitizesDownloadHeaders` only checks the header helper. **Suggestion:** Add tests around `UserFileTransfer` (package-private) for `safeHeaderFilename`/`contentDisposition` edge cases already there, plus token consume-once, foreign-session 404, and “uploaded path must stay under the request directory”. ########## rap/src/main/java/org/apache/hop/ui/hopgui/HopWebUserFilePlugin.java: ########## @@ -0,0 +1,584 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hop.ui.hopgui; + +import java.io.FilterInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.lang.reflect.InvocationTargetException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Comparator; +import java.util.HashSet; +import java.util.IdentityHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import java.util.stream.Stream; +import java.util.zip.ZipEntry; +import java.util.zip.ZipInputStream; +import org.apache.commons.io.FilenameUtils; +import org.apache.commons.lang3.StringUtils; +import org.apache.hop.core.Const; +import org.apache.hop.core.exception.HopException; +import org.apache.hop.core.gui.plugin.GuiPlugin; +import org.apache.hop.core.gui.plugin.GuiRegistry; +import org.apache.hop.core.gui.plugin.menu.GuiMenuElement; +import org.apache.hop.core.gui.plugin.menu.GuiMenuItem; +import org.apache.hop.core.security.Permission; +import org.apache.hop.core.xml.XmlHandler; +import org.apache.hop.i18n.BaseMessages; +import org.apache.hop.pipeline.PipelineMeta; +import org.apache.hop.pipeline.PipelineSvgPainter; +import org.apache.hop.ui.core.dialog.EnterStringDialog; +import org.apache.hop.ui.core.dialog.ErrorDialog; +import org.apache.hop.ui.core.security.HopSecurityUi; +import org.apache.hop.ui.hopgui.file.IHopFileTypeHandler; +import org.apache.hop.ui.hopgui.file.pipeline.HopGuiPipelineGraph; +import org.apache.hop.ui.hopgui.file.workflow.HopGuiWorkflowGraph; +import org.apache.hop.workflow.WorkflowMeta; +import org.apache.hop.workflow.WorkflowSvgPainter; +import org.eclipse.rap.rwt.RWT; +import org.eclipse.rap.rwt.service.UISession; + +/** Hop Web menu actions for opening and saving files on the user's computer. */ +@GuiPlugin(name = "Hop Web user files") +public class HopWebUserFilePlugin { + + private static final Class<?> PKG = HopWebUserFilePlugin.class; + + public static final String ID_MAIN_MENU_FILE_USER = "15000-menu-file-user"; + + private static final String PROJECT_EXPORT_MENU_ID = "10055-menu-file-export-to-svg"; + private static final String KETTLE_IMPORT_MENU_ID = "10060-menu-tools-import"; + private static final String SESSION_TEMP_DIRECTORY = + HopWebUserFilePlugin.class.getName() + ".tempDirectory"; + private static final String HOP_FILE_EXTENSIONS = ".hpl,.hwf"; + private static final long MAX_HOP_FILE_SIZE = 25L * 1024 * 1024; + private static final long MAX_COMPRESSED_ZIP_SIZE = 100L * 1024 * 1024; + private static final long MAX_UNCOMPRESSED_ZIP_SIZE = 250L * 1024 * 1024; + private static final long MAX_UNCOMPRESSED_ZIP_ENTRY_SIZE = 64L * 1024 * 1024; + private static final long MIN_COMPRESSION_RATIO_SIZE = 1024L * 1024; + private static final int MAX_COMPRESSION_RATIO = 100; + private static final int MAX_ZIP_ENTRIES = 10_000; + private static final int MAX_ZIP_ENTRY_DEPTH = 32; + private static final int MAX_ZIP_ENTRY_NAME_LENGTH = 4096; + + private final Map<IHopFileTypeHandler, String> userFileNames = new IdentityHashMap<>(); + private UserFileTransfer transfer; + private boolean sessionCleanupRegistered; + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = ID_MAIN_MENU_FILE_USER, + label = "i18n::HopGui.Menu.File.User", + parentId = HopGui.ID_MAIN_MENU) + public void menuFileUser() { + // Category only. + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_NEW, + label = "i18n::HopGui.Menu.File.New", + image = "ui/images/add.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void newFile() { + HopGui.getInstance().menuFileNew(); + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_OPEN, + label = "i18n::HopGui.Menu.File.Open", + image = "ui/images/open.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void openFile() { + if (!HopSecurityUi.check(Permission.FILE_VIEW)) { + return; + } + try { + transfer() + .open( + HOP_FILE_EXTENSIONS, + MAX_HOP_FILE_SIZE, + new UserFileTransfer.UploadListener() { + @Override + public void uploaded(String filename, Path uploadedFile) { + openUploadedFile(filename, uploadedFile); + } + + @Override + public void error(Exception exception) { + showError("HopGui.FileBrowser.Error.Upload", exception); + } + }); + } catch (Exception e) { + showError("HopGui.FileBrowser.Error.Upload", e); + } + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_SAVE, + label = "i18n::HopGui.Menu.File.Save", + image = "ui/images/save.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void saveFile() { + downloadActiveFile(false); + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_SAVE_AS, + label = "i18n::HopGui.Menu.File.SaveAs", + image = "ui/images/save-as.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void saveFileAs() { + downloadActiveFile(true); + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_EXPORT_TO_SVG, + label = "i18n::HopGui.Menu.File.ExportToSVG", + image = "ui/images/image.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void exportToSvg() { + if (!HopSecurityUi.check(Permission.FILE_EXPORT)) { + return; + } + try { + HopGuiPipelineGraph pipelineGraph = HopGui.getActivePipelineGraph(); + if (pipelineGraph != null) { + String name = safeFilename(pipelineGraph.getPipelineMeta().getName(), ".svg"); + String svg = + PipelineSvgPainter.generatePipelineSvg( + pipelineGraph.getPipelineMeta(), 1.0f, pipelineGraph.getVariables()); + transfer().download(name, "image/svg+xml", svg.getBytes(StandardCharsets.UTF_8)); + return; + } + + HopGuiWorkflowGraph workflowGraph = HopGui.getActiveWorkflowGraph(); + if (workflowGraph != null) { + String name = safeFilename(workflowGraph.getWorkflowMeta().getName(), ".svg"); + String svg = + WorkflowSvgPainter.generateWorkflowSvg( + workflowGraph.getWorkflowMeta(), 1.0f, workflowGraph.getVariables()); + transfer().download(name, "image/svg+xml", svg.getBytes(StandardCharsets.UTF_8)); + return; + } + // The menu item is hidden until a pipeline or workflow is active. Keep this guard quiet as + // well in case a stale keyboard shortcut or menu event reaches the action. + return; + } catch (Exception e) { + showError("HopGui.FileBrowser.Error.ExportSvg", e); + } + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_EXPORT_PROJECT, + label = "i18n::HopGui.Menu.File.ExportProjectZip", + image = "export.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void exportProjectZip() { + if (!HopSecurityUi.check(Permission.FILE_EXPORT)) { + return; + } + Path zipFile = null; + try { + zipFile = Files.createTempFile(getSessionTempDirectory(), "hop-project-", ".zip"); + Files.deleteIfExists(zipFile); + invokeGuiPlugin( + PROJECT_EXPORT_MENU_ID, + "exportProject", + new Class<?>[] {String.class, boolean.class}, + zipFile.toString(), + false); + if (!Files.exists(zipFile)) { + return; + } + + String projectName = + HopGui.getInstance().getVariables().getVariable("HOP_PROJECT_NAME", "hop-project"); + transfer().download(safeFilename(projectName, ".zip"), "application/zip", zipFile); + } catch (Exception e) { + showError("HopGui.FileBrowser.Error.ExportProject", e); + } finally { + if (zipFile != null) { + try { + Files.deleteIfExists(zipFile); + } catch (IOException ignored) { + // Best effort after the browser download has been prepared. + } + } + } + } + + @GuiMenuElement( + root = HopGui.ID_MAIN_MENU, + id = HopGui.ID_MAIN_MENU_FILE_USER_IMPORT_KETTLE, + label = "i18n::HopGui.Menu.File.ImportKettleZip", + image = "ui/images/kettle-logo.svg", + parentId = ID_MAIN_MENU_FILE_USER) + public void importFromKettleZip() { + if (!HopSecurityUi.check(Permission.FILE_CREATE) + || !HopSecurityUi.check(Permission.METADATA_WRITE)) { + return; + } + try { + transfer() + .open( + ".zip", + MAX_COMPRESSED_ZIP_SIZE, + new UserFileTransfer.UploadListener() { + @Override + public void uploaded(String filename, Path uploadedFile) { + openKettleImport(filename, uploadedFile); + } + + @Override + public void error(Exception exception) { + showError("HopGui.FileBrowser.Error.ImportKettle", exception); + } + }); + } catch (Exception e) { + showError("HopGui.FileBrowser.Error.ImportKettle", e); + } + } + + private UserFileTransfer transfer() throws IOException { + if (transfer == null || transfer.isDisposed()) { + transfer = new UserFileTransfer(HopGui.getInstance().getShell(), getSessionTempDirectory()); + if (!sessionCleanupRegistered) { + RWT.getUISession().addUISessionListener(event -> userFileNames.clear()); + sessionCleanupRegistered = true; + } + } + return transfer; + } + + private void openUploadedFile(String filename, Path uploadedFile) { + try { + if (!HopSecurityUi.check(Permission.FILE_VIEW)) { + return; + } + String extension = FilenameUtils.getExtension(filename); + if (!"hpl".equalsIgnoreCase(extension) && !"hwf".equalsIgnoreCase(extension)) { + throw new HopException("Only Hop pipeline and workflow files can be opened here."); + } + String safeName = safeFilename(filename, ""); + Path file = getSessionTempDirectory().resolve(UUID.randomUUID().toString() + "-" + safeName); + Files.copy(uploadedFile, file); + IHopFileTypeHandler handler = HopGui.getInstance().fileDelegate.fileOpen(file.toString()); Review Comment: **[bug]** File Browser → Open copies the upload into a RAP session temp directory and then `fileOpen()`s that path. Hop treats it as a normal server file: toolbar/File → Save writes back to `/tmp/hop-web-user-files-…`, recent-file audit stores that path, and `getSessionTempDirectory()` deletes the tree when the UI session ends. A user who opens a pipeline from their laptop and hits the familiar Save button will think the work is stored, then lose it on logoff/timeout. File Browser → Save only downloads a copy and does not clear the dirty flag or rebind the filename, so Close still prompts and the default Save path remains the doomed temp file. **Suggestion:** After a browser open, treat the handler as untitled (empty filename) so File/toolbar Save goes through Save As to a real project/VFS path, or intercept Save when the handler is in `userFileNames` and download instead. Do not persist session-temp paths in last-opened/recent files. After a successful File Browser download, `clearChanged()` so Close does not send the user back into the temp-file Save path. ########## core/src/main/java/org/apache/hop/core/vfs/HopVfs.java: ########## @@ -483,6 +483,22 @@ private static FileObject resolveWith( } } + /** + * Commons VFS expects a URI with an absolute path for Windows drive letters. Java's {@link + * File#toURI()} emits {@code file:/C:/...}; normalize that form while leaving UNC and non-file + * URIs untouched. + */ + private static String toFileUri(File file) { Review Comment: **[suggestion]** Every scheme-less path now goes through `File.toURI()` plus a Windows `file:/C:` → `file:///C:` rewrite. That is a process-wide VFS behavior change, not web-only. The new test covers a Linux path with spaces and `#`; the drive-letter rewrite (the reason for `toFileUri`) is untested. **Suggestion:** Add a unit test that feeds `toFileUri`/`getFileObject` a `file:/C:/…` style URI (can be string-level if the job isn’t on Windows) and confirm `HopVfs.getFilename` round-trips spaces, `#`, and drive letters. Watch any callers that compared native paths to `FileObject` URIs. ########## assemblies/web/pom.xml: ########## @@ -29,10 +29,29 @@ <name>Hop Assemblies Web</name> <properties> - <rap.version>4.4.0</rap.version> + <commons.fileupload2.version>2.0.0-M5</commons.fileupload2.version> Review Comment: **[suggestion]** The web WAR now ships `commons-fileupload2-*-2.0.0-M5` because RAP 4.7’s fileupload bundle depends on that milestone. Aligning `assemblies/web` RAP from 4.4.0 to 4.7.0 with `hop-ui-rap` is necessary for `FileDialog` upload APIs, but M5 is still a milestone on an ASF release train, and the RWT runtime bump is three minors. **Suggestion:** Call out in the PR that FileUpload 2.0.0-M5 is RAP-required, and smoke-test Hop Web beyond the new menu (existing RAP dialogs, file upload widget, JEE vs SWT compatibility mode). Prefer a non-milestone FileUpload if RAP will take it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
