Add browser-native file operations to Hop Web - #8276
Conversation
|
@michaaels : thanks a lot for the pull request and bringing forward what is really a great idea. I ran a code review on the PR and found a few minor issues. Let us know if you want any help fixing these, the broken unit test or anything else! |
mattcasters
left a comment
mattcasters
left a comment
There was a problem hiding this comment.
Great work! Let's make sure the file a user is working on is properly saved on the server at all times and properly flagged as changed.
| String safeName = safeFilename(filename, ""); | ||
| Path file = getSessionTempDirectory().resolve(UUID.randomUUID().toString() + "-" + safeName); | ||
| Files.copy(uploadedFile, file); | ||
| IHopFileTypeHandler handler = HopGui.getInstance().fileDelegate.fileOpen(file.toString()); |
There was a problem hiding this 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.
| 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)); |
There was a problem hiding this 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”.
| 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)); |
There was a problem hiding this 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().
| return; | ||
| } | ||
|
|
||
| Path uploadedFile = Path.of(uploadedPath); |
There was a problem hiding this 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.
| 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); |
There was a problem hiding this 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.
| * File#toURI()} emits {@code file:/C:/...}; normalize that form while leaving UNC and non-file | ||
| * URIs untouched. | ||
| */ | ||
| private static String toFileUri(File file) { |
There was a problem hiding this 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.
| } | ||
|
|
||
| @Test | ||
| void sanitizesDownloadHeaders() { |
There was a problem hiding this 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”.
|
|
||
| <properties> | ||
| <rap.version>4.4.0</rap.version> | ||
| <commons.fileupload2.version>2.0.0-M5</commons.fileupload2.version> |
There was a problem hiding this 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.
| root = HopGui.ID_MAIN_MENU, | ||
| id = HopGui.ID_MAIN_MENU_FILE_USER_EXPORT_PROJECT, | ||
| label = "i18n::HopGui.Menu.File.ExportProjectZip", | ||
| image = "export.svg", |
There was a problem hiding this 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.
| 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 |
There was a problem hiding this 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.
|
Thank you for your comments; I'm currently working on the corrections. |
File Browser ZIP import extracts into a hop-web-user-files session temp folder that is deleted when the dialog closes. Do not persist that path, ignore an already-stored temp last-used value, and remember a real folder only if the user browsed away from the upload.
2 participants
Summary
Adds browser-native file operations to Hop Web while preserving the existing server-side
Fileworkflow.File Browsercategory for creating, uploading, downloading, and exporting pipelines/workflows and projects.CloseandClose Allonly inFile, and shows SVG export only while a pipeline or workflow is active.hop-misc-importonce in the standard plugin assembly so the action is available in the standard web distribution, and removes it from the optional marketplace catalog.Compatibility
File Browseractions.Security notes
Uploads are scoped to a RAP UI session. Download handlers require both the owning HTTP session and UI session, use one-time expiring tokens, and delete temporary files after use or session disposal. Kettle ZIP extraction rejects absolute/traversal paths, duplicates, overlong/deep paths, excessive entry counts, per-entry and aggregate expansion limits, and high compression ratios.
Review follow-up
ui/images/zipfile.svgicon.Validation
All commands below used Java 21.0.10.
-pl assemblies/web -am -Dspotless.skip=true -DskipTests -DskipITs package: passed (10 modules).-pl assemblies/client -am -Pskip-uitest -Dspotless.skip=true -DskipTests -DskipITs package: passed (284 modules). This broader check compiled production/test sources and packaged the distribution; it did not execute the broad test suite.git diff --check: passed.rapandplugins/misc/import: passed with zero unapproved files. A full root clean/install/RAT validation is not claimed./ui, bound to loopback. An earlier Chrome session confirmed GUI startup and captured screenshots without console or HTTP errors.Remaining manual/browser validation
The current browser MCP reports both Chrome and the integrated browser unavailable. End-to-end SVG download/content validation, menu screenshots in all open/close/project/reload states, the browser upload widget, and the wider RAP dialog/JEE-vs-SWT smoke test are not yet certified. Unit/service tests do not replace those checks. The review threads remain for reviewer confirmation.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean install apache-rat:checkto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.git rebase -i.addresses #123), if applicable.To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.