feat: implement native unzip for plugin installation performance impr…#2367
feat: implement native unzip for plugin installation performance impr…#2367RohitKushvaha01 wants to merge 1 commit into
Conversation
Greptile SummaryThis PR replaces the JavaScript-based, batched per-file extraction (JSZip +
Confidence Score: 4/5The core extraction path is functionally correct and the native unzip includes zip-slip protection; the main concerns are a small unreachable check in the Java code and the silent change in install-state semantics. The happy-path install flow works correctly end-to-end. The issues found are a dead branch in the Java security check (protected by an earlier guard so no real exposure), doubled memory usage due to JSZip parsing before the temp file is written, and a shift from SHA-256 hashes to constant "1" values in the persisted install state. None of these cause broken installs or data loss, but the state-semantics change could surprise future contributors relying on the stored values. src/lib/installPlugin.js deserves a second look — specifically the memory profile (JSZip in-memory + raw bytes on disk simultaneously) and the new install-state value format. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant JS as installPlugin.js
participant JSZip
participant FS as fsOperation (CACHE_STORAGE)
participant Cordova as Cordova Bridge
participant Java as System.java (unzip)
participant PluginDir as Plugin Directory
JS->>FS: readFile(pluginUrl) → ArrayBuffer
JS->>JSZip: loadAsync(plugin)
JSZip-->>JS: parsed zip (manifest + file list)
JS->>JS: "validate & patch pluginJson"
JS->>FS: createFile(tempZipName, plugin)
Note over FS: Raw zip written to CACHE_STORAGE
JS->>Cordova: system.unzip(tempZipFile, pluginDir)
Cordova->>Java: unzip(zipPath, destPath)
Java->>Java: ZipSlip check (canonical path)
Java->>PluginDir: extract all entries
Java-->>Cordova: callback.success()
Cordova-->>JS: resolve()
JS->>PluginDir: writeFile(plugin.json, patched JSON)
JS->>JS: populate state.updatedStore from zip entries
JS->>JS: state.save()
JS->>PluginDir: deleteRedundantFiles()
JS->>FS: delete(tempZipFile)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant JS as installPlugin.js
participant JSZip
participant FS as fsOperation (CACHE_STORAGE)
participant Cordova as Cordova Bridge
participant Java as System.java (unzip)
participant PluginDir as Plugin Directory
JS->>FS: readFile(pluginUrl) → ArrayBuffer
JS->>JSZip: loadAsync(plugin)
JSZip-->>JS: parsed zip (manifest + file list)
JS->>JS: "validate & patch pluginJson"
JS->>FS: createFile(tempZipName, plugin)
Note over FS: Raw zip written to CACHE_STORAGE
JS->>Cordova: system.unzip(tempZipFile, pluginDir)
Cordova->>Java: unzip(zipPath, destPath)
Java->>Java: ZipSlip check (canonical path)
Java->>PluginDir: extract all entries
Java-->>Cordova: callback.success()
Cordova-->>JS: resolve()
JS->>PluginDir: writeFile(plugin.json, patched JSON)
JS->>JS: populate state.updatedStore from zip entries
JS->>JS: state.save()
JS->>PluginDir: deleteRedundantFiles()
JS->>FS: delete(tempZipFile)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| name = name.replace('\\', '/'); | ||
|
|
||
| // Skip unsafe absolute paths | ||
| if (name.startsWith("/") || name.contains("://") || name.startsWith("\\\\") || (name.length() > 1 && name.charAt(1) == ':')) { |
There was a problem hiding this comment.
startsWith("\\\\") is unreachable
name has already been fully normalised by name.replace('\\', '/') on the line above, so no backslashes can remain. The name.startsWith("\\\\") branch will never be true. A UNC path like \\server\share becomes //server/share after the replace and is already blocked by the earlier name.startsWith("/") guard, so the protection still holds — but the second check is dead code and can mislead readers into thinking it provides independent coverage.
| for (const file of files) { | ||
| const entry = zip.files[file]; | ||
| let correctFile = file.replace(/\\/g, "/"); | ||
| const isDirEntry = entry.dir || correctFile.endsWith("/"); | ||
|
|
||
| await fsOperation(fileUrl).writeFile(data); | ||
| } catch (error) { | ||
| console.error(`Error processing file ${file}:`, error); | ||
| if (isUnsafeAbsolutePath(file)) { | ||
| ignoredUnsafeEntries.add(file); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| // Process in batches | ||
| for (let i = 0; i < files.length; i += limit) { | ||
| const batch = files.slice(i, i + limit); | ||
| await Promise.allSettled(batch.map(processFile)); | ||
| correctFile = sanitizeZipPath(correctFile, isDirEntry); | ||
| if (!correctFile) continue; | ||
|
|
||
| // Allow UI thread to breathe | ||
| await new Promise((r) => setTimeout(r, 0)); | ||
| if (!isDirEntry) { | ||
| state.updatedStore[correctFile.toLowerCase()] = "1"; | ||
| } | ||
| } |
There was a problem hiding this comment.
"1"
The old code stored a SHA-256 content hash in updatedStore via state.isUpdated(). The new loop writes "1" for every non-directory entry. After state.save() this persists to disk, so any future code path that reads the stored value expecting a real hash (e.g. a delta-update check in a future install) will instead see "1" and treat everything as changed. The existing state.exists() path works fine since it only tests key presence, but the semantics of the state file have silently shifted from "hash of last-installed content" to "was ever installed".
|
Closed in favour of #2376 |
github-project-automation
Bot
moved this from Backlog
to Done
in The Code Board - Acode
1 participant
No description provided.