feat: job bundle browser/sharing - #1181
Conversation
562da40 to
d9eb4aa
Compare
c4f510a to
0e6eb04
Compare
| Job bundles can be either: | ||
|
|
||
| - **Directories** — a folder containing `template.yaml` or `template.json` at the root, plus any scripts, data files, and `asset_references.yaml`. | ||
| - **Archives** — an `.ojd` file (zip format under the hood) containing a job bundle. The template can be at the archive root or inside a single wrapper directory. |
There was a problem hiding this comment.
This makes sense to me. I think it should be formalized in the OJD spec as part of this project.
There was a problem hiding this comment.
Yea, we've put off formalizing job bundles for a while now. Something we can get started
|
Thought from offline discussion: would be great to have a way to rename, update, or delete bundles from UI, especially for the queue view. |
|
Code Review Complete - I have reviewed PR 1181 (job bundle browser/sharing feature). CRITICAL ISSUE FOUND: The Share button uses .zip extension instead of .ojd at line 731 of submit_job_to_deadline_dialog.py. This breaks the browser S3 listing logic which filters by .ojd extension. Shared bundles will not appear in the Queue source. I have posted detailed inline comments on this and other minor issues. Overall the code is well-designed with good test coverage and proper security controls (zip slip protection, cache isolation). Once the extension bug is fixed the PR should be good to merge. |
|
CRITICAL BUG: File extension mismatch in Share button (submit_job_to_deadline_dialog.py line ~2147) The Share button uploads bundles with .zip extension but should use .ojd: Impact: Bundles shared via the UI will NOT appear in the browser dialog because S3BundleRepository only lists .ojd files. SECURITY: Good practices found:
MINOR ISSUES:
The critical bug must be fixed. Security is solid overall. |
|
Advisory Code Review - PR 1181 CRITICAL BUG FOUND: SECURITY ISSUES:
CORRECTNESS ISSUES: STRENGTHS:
VERDICT: Well-designed feature with one critical bug. Fix .zip->.ojd and add zip bomb protection, then ready to merge. Reviewed by: Claude Sonnet 4.5 |
|
Detailed Findings Issue 1: Critical Extension Bug Issue 2: Zip Bomb Protection Missing Issue 3: Encapsulation Violation Issue 4: ReDoS Risk Issue 5: Silent Truncation |
|
Review Complete I have reviewed all 3579 lines of changes in this PR. The feature is well-architected with good separation of concerns and comprehensive test coverage. Key Takeaways: MUST FIX:
SHOULD FIX:
NICE TO HAVE:
The path traversal protection is correctly implemented and the caching strategy is sound. With the critical bug fixed and zip bomb protection added, this feature is ready for production use. Questions or concerns about any finding? Please reply to this thread. |
|
Code Review Summary - I reviewed PR 1181 for the job bundle browser feature. Found 2 security issues: (1) Path traversal vulnerability on Windows in zip extraction validation at repository.py:49 - recommend using os.path.commonpath() (2) Symlink following in bundle upload at bundle_group.py:827 - os.walk() follows symlinks by default. Also found 1 bug: silent metadata truncation at bundle_group.py:815 without warnings. Overall the implementation is well-structured with good tests and caching design. Security issues should be addressed before merging. |
|
Collection of UX feedback: Bundle preview — spacing and sizing
Queue tab
Upload/download
History/source confusion
"Show hidden" is inconsistent across tabs (nit)
"Filter bundles" behavior
Show in Finder (idea)
Path input
Naming (nit/idea)
|
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
The bundle browser aborted an in-progress S3 download with QThread.terminate(), which kills the thread mid-request and can leave the boto3 client/socket inconsistent and the cache partially written. Add a cancel flag checked in the download progress callback that raises to unwind the transfer cleanly, wire it to the dialog's Cancel/close, and wait for the worker to finish before clearing the partial cache. All nine review items are now implemented, each with tests verified to fail on the pre-fix code and a clean fmt/lint/mypy/build. Nothing has been committed — let me know if you'd like me to stage and commit these (individually per issue, or grouped), or if there's anything else to adjust. Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
… there are none Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
done
done
done
done
Fixed the message. For what it's worth it was actually a credential issue. If you don't have bundles but have valid creds, it shows "No bundles", but it was stuck in "Loading" if you didn't have valid creds.
A good idea. There's a download button specifically for queue bundles now, we could have a feature request to upload local ones though.
It's based on AWS profile. Always has been. Could potentially to "Profile history" but I'm very much in favour of shorter succinct names.
Queue/Local match in my mind. We've created a Hide/Unhide mechanism for the queue via an S3 mechanism (something users don't need to understand). Local filesystems already have the concept of hidden folders/files so we leverage that instead.
While we don't expect users to really do anything in history (they can choose the folder but it's written to by us), you can definitely put hidden stuff in there. I don't want to change that because I value the consistency in the UI and switching between sources shouldn't change that.
Folders (Queue/Local) are lazy loaded because it can be expensive to perform all those operations. Locally, we're looking at any folder that contains a template.yaml to determine if it's a bundle which requires you to go a few layers down. We don't explore that or S3 fully unless a user starts going down those nested folders. I've made a change to preload 1 more layer down to maybe help make that feel a little better.
Fixed. Now it does it based on if you had actually selected something.
I had a similar idea in my head that I implemented. Decided to put a button in the preview to handle and show download size (Queue)
I made it look less editable, not sure if it looks better or not. I knowing what bundle/object i'm looking at is important to me, especially if I wanna do something outside the browser with it.
See reply above |
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>







What was the problem/requirement? (What/Why)
When using
deadline bundle gui-submit --browse, the "Load a different job bundle" button, or "SubmitJob Bundle" menu item in Deadline Cloud monitor, users are presented with a native OS folder picker.
This is inadequate because:
location
What was the solution? (How)
Replace the native folder picker with a custom job bundle browser dialog.
It provides:
quick search
submitted bundles)
.ojdarchives (zip format) stored in the queue's job attachment bucketunder a
job-bundles/prefix — no extra configuration neededaccess, or export locally
parallelized API calls
Key design decisions:
.ojdarchives (not loose folder bundles) — this gives a single canonical sharingformat with atomic uploads and simple cache validation
{{Param.X}}intact) so users seethe template identity rather than a specific parameterization
from_configparallelizes the GetQueue API call with S3 credential resolution for faster cold startsAdditionally there are new CLI commands:
deadline bundle list [path]deadline bundle list --queuedeadline bundle upload <dir>deadline bundle download <name>deadline bundle info <name>deadline bundle hide <name>deadline bundle unhide <name>deadline bundle cache cleandeadline bundle cache updateExamples:
New configuration:
settings.job_bundle_default_directoryAlso exposed in the Deadline Cloud settings dialog (Settings → General settings) as a "Job bundle
directory" picker.
What is the impact of this change?
--browseflag and "Load a different job bundle" button now open the new dialog insteadof the native picker
.ojdarchive format introduced as the standard for shared bundleslist_shared_bundles,upload_bundle,download_bundle) enable AI assistants to manageshared bundles
How was this change tested?
Was this change documented?
Design document at
docs/design/job-bundle-browser.mdcovers the full design. CLI commands include helptext. Code has docstrings on all public classes and methods. README updated with MCP examples for bundle
operations.
Does this PR introduce new dependencies?
Is this a breaking change?
No
Does this change impact security?
The S3 archive cache at
~/.deadline/cache/job-bundles/stores extracted bundle contents. Archives arevalidated before extraction to prevent path traversal attacks (absolute paths and
../entries arerejected, using
os.path.commonpathwithos.path.realpathfor cross-platform safety). Symlinks areskipped during archiving (
os.walk(followlinks=False)). The cache directory follows the same permissionmodel as the existing
~/.deadline/directory.S3 access uses the same boto3 session/profile as the rest of deadline-cloud — no new credential flows.
Bundle visibility updates use S3 conditional writes (
If-Match) for safe concurrent access.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this
contribution, under the terms of your choice.