https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au, | |martin.renvoize@openfifth.c | |o.uk Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200145 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200145&action=edit Bug 42404: Add BatchImportCoverImages background job This patch adds a background job that processes a zip file containing cover images and an idlink.txt/datalink.txt mapping file. Each line in the mapping file maps a biblio_id (or item_id) to an image filename. Features: - Supports both biblio_id and item_id targets - Replace mode (deletes existing images before import) - Uses Archive::Zip instead of system unzip - Path traversal protection on extracted files - Reports progress via the background jobs infrastructure Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/BackgroundJob/BatchImportCoverImages.t => SUCCESS: Tests pass! 3. Tests cover: - enqueue() sets correct job size, status, and queue - Single and multiple image imports - Replace mode behavior - Bad delimiter handling in mapping files - Cancelled job is skipped - Upload cleanup after processing 4. Sign off :-D Sponsored-by: ByWater Solutions -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42405 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42405 [Bug 42405] Add single cover image upload path -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42804 Attachment #200145|0 |1 is obsolete| | Attachment #200146|0 |1 is obsolete| | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42804 [Bug 42804] Replace svc/cover_images and tools/upload-cover-image.pl with REST API -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #4 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200151 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200151&action=edit Bug 42404: Add REST API endpoint for batch cover image import This patch adds a REST API endpoint for batch importing cover images: POST /api/v1/cover_images/batch It accepts a zip file via multipart/form-data and enqueues a BatchImportCoverImages background job to process it. Query parameters: - replace: boolean, delete existing images before import - target: enum (biblio_id, item_id), specifies what the first column of the mapping file represents Requires tools: upload_local_cover_images permission. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/cover_images.t => SUCCESS: Tests pass! 3. Tests cover: - Authentication required (401) - Authorization required (403) - Successful upload enqueues job (201) - Replace flag forwarded correctly - Missing file returns 400 4. Sign off :-D Sponsored-by: ByWater Solutions -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #6 from David Nind <david@davidnind.com> --- I'm not seeing a background job after these steps: 2. Upload a zip file with cover images via the upload tool 3. Navigate to Administration > Background jobs 4. Click on the batch_import_cover_images job Is there something else I need to do? Steps: 1. All three patches applied 2. yarn build 3. restart_all 4. All the tests mentioned pass 5. Cataloging > Tools > Upload local cover image -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|42804 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42804 [Bug 42804] Replace svc/cover_images and tools/upload-cover-image.pl with REST API -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #8 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- The only way to test this from the UI is by having bug 42804 applied too. This patchset only adds: * Background job with tests * API for submitting cover images in bulk and enqueueing the job, with tests You should probably just apply the patches and run the tests after doing: ```shell $ yarn api:bundle $ koha-plack --restart kohadev ``` You will have your change to see this in action later. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #10 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Martin Renvoize (ashimema) from comment #9)

I'd be tempted to return a 202 Accepted as apposed to a 201 Created here?

That's brillant. I went auto-pilot on following the current guidelines for POST, but it makes sense.

The task is transient isn't it.. the final result is the processed covers.. I'd also be tempted to set the Location headers to the job tracking API endpoint?

I think I just forgot it, you're right too! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #12 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Hi! (In reply to Martin Renvoize (ashimema) from comment #9)

I'd be tempted to return a 202 Accepted as apposed to a 201 Created here?

The task is transient isn't it.. the final result is the processed covers.. I'd also be tempted to set the Location headers to the job tracking API endpoint?

Implemented these in a follow-up. I also wrote an entry on the wiki about this, with a `DRAFT` status so we vote it next dev meeting: https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#.5BDRAFT.5D_RES... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #200150|0 |1 is obsolete| | Attachment #200151|0 |1 is obsolete| | Attachment #200152|0 |1 is obsolete| | Attachment #200611|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #14 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200833 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200833&action=edit Bug 42404: Add REST API endpoint for batch cover image import This patch adds a REST API endpoint for batch importing cover images: POST /api/v1/cover_images/batch It accepts a zip file via multipart/form-data and enqueues a BatchImportCoverImages background job to process it. Query parameters: - replace: boolean, delete existing images before import - target: enum (biblio_id, item_id), specifies what the first column of the mapping file represents Requires tools: upload_local_cover_images permission. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/cover_images.t => SUCCESS: Tests pass! 3. Tests cover: - Authentication required (401) - Authorization required (403) - Successful upload enqueues job (201) - Replace flag forwarded correctly - Missing file returns 400 4. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #16 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200835 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200835&action=edit Bug 42404: (follow-up) Return 202 Accepted with Location header The batch cover image import endpoint enqueues a background job for processing. A 201 Created is incorrect because no resource is durably created at response time - the work is deferred. This patch changes the response to 202 Accepted, which is the correct HTTP status for async job submission. Additionally, the Location header is set to the job tracking endpoint so clients can poll for completion status without out-of-band knowledge. Changes: - Return 202 instead of 201 from batch_import controller - Set Location header to /api/v1/jobs/{job_id} - Update OpenAPI spec to document 202 response and Location header - Update tests to assert 202 status and Location header Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/cover_images.t => SUCCESS: Tests pass! 3. Verify the OpenAPI spec is valid: k$ cd /kohadevbox/koha && perl t/db_dependent/api/v1/spec.t => SUCCESS: Spec validates! 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #18 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200837 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200837&action=edit Bug 42404: (follow-up) Reject symlinks in image file paths The existing path-traversal check guards against filenames that resolve outside the extraction directory, but did not guard against symlinks that point to arbitrary host filesystem paths (e.g. /etc/passwd). Add an explicit -l check for each resolved image path before passing it to GD::Image->new(); symlinks are treated as path_traversal errors and the image is skipped. The mapping-file symlink checks introduced earlier are therefore consistent across all file reads in the job. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #20 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200839 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200839&action=edit Bug 42404: (follow-up) Reuse item object to avoid redundant fetch in replace mode In item_id target mode, the item was fetched once to obtain the biblionumber, then fetched again inside the replace block via Koha::Items->find(). Store the first result and pass it through to avoid the N+1 pattern. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #22 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200841 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200841&action=edit Bug 42404: (follow-up) Validate IDs are numeric before database lookup IDs from the mapping file are used directly in Koha::Items->find() and Koha::Biblios->find(). Add an explicit check that the value is all digits before passing it to the ORM layer so that non-numeric values produce a clear 'invalid_id' message rather than an ORM exception or silent failure. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #24 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200843 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200843&action=edit Bug 42404: (follow-up) Die on upload directory creation failure The mkdir call in _store_upload silently continued if the directory could not be created, leading to a confusing move_to failure later. Die immediately with a descriptive message; the caller's catch block will return a 500 to the client. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42891 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #26 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201278 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201278&action=edit Bug 42404: Add BatchImportCoverImages background job This patch adds a background job that processes a zip file containing cover images and an idlink.txt/datalink.txt mapping file. Each line in the mapping file maps a biblio_id (or item_id) to an image filename. Features: - Supports both biblio_id and item_id targets - Replace mode (deletes existing images before import) - Uses Archive::Zip instead of system unzip - Path traversal protection on extracted files - Reports progress via the background jobs infrastructure Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/BackgroundJob/BatchImportCoverImages.t => SUCCESS: Tests pass! 3. Tests cover: - enqueue() sets correct job size, status, and queue - Single and multiple image imports - Replace mode behavior - Bad delimiter handling in mapping files - Cancelled job is skipped - Upload cleanup after processing 4. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #28 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201280 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201280&action=edit Bug 42404: (follow-up) Add background job detail template This patch adds the TT include used by the background jobs detail page to display the results of a batch cover image import job. Test plan: 1. Apply patch 2. Upload a zip file with cover images via the upload tool 3. Navigate to Administration > Background jobs 4. Click on the batch_import_cover_images job => SUCCESS: Job detail page shows imported/failed counts and messages 5. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #30 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201282 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201282&action=edit Bug 42404: (follow-up) Fix GD exception handling, logger, and empty mapping detection - Wrap GD::Image->new() in eval{} so a die from a malformed image is caught per-image rather than aborting the entire job via the outer catch block; images_failed and step() are now always updated correctly on GD errors. - Add Koha::Logger->get->error/warn calls in all catch blocks so operators have server-side detail while users see a sanitized message. - Add _finish_failed() helper that correctly stores messages and the report before setting status to 'failed', fixing the pre-existing issue where early-exit paths called status('failed')->store without calling finish(). - After mapping parsing, fail the job (not silently succeed with 0 imported) when no valid mappings are found, including the case where all lines had unrecognised delimiters. - Update tests accordingly. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #32 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201284 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201284&action=edit Bug 42404: (follow-up) Wrap per-image replace+store in a transaction Without a transaction, the delete-then-store sequence in replace mode is non-atomic: if ->store() fails the old images are already gone and cannot be recovered. Wrap each image's delete (optional) and store in a txn_do() block so both either succeed together or roll back together. Each image is processed independently so a failure on one does not affect others. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #34 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201286 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201286&action=edit Bug 42404: (follow-up) Handle move failure and empty filename in _store_upload - Check the return value of move_to(); die if the file could not be written so the caller's catch block returns a 500 rather than silently creating an UploadedFile record pointing to a missing file. - After filename sanitization, fall back to 'upload.zip' if all characters were stripped, preventing an empty path component. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #36 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201288 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201288&action=edit Bug 42404: (follow-up) Canonicalize extraction directory before path-traversal check The path-traversal guard compares Cwd::abs_path(\$full_path) against \$dirname using a regex anchor. If \$dirname itself contains unresolved symlink components the comparison could be fooled. Resolve \$dirname via Cwd::abs_path() immediately after extraction (once the directory exists on disk) so both sides of the comparison are fully canonicalized. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #38 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201290 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201290&action=edit Bug 42404: (follow-up) Ensure upload file is cleaned up on unexpected exception The upload variable was declared inside the outer try block, making it invisible to any cleanup code after the try/catch. Move the declaration outside the try so a guard after the try/catch can delete the upload if an unexpected exception fired before the normal cleanup path ran. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43016 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43016 [Bug 43016] [OMNIBUS] Server resource protection -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #41 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #8)

The only way to test this from the UI is by having bug 42804 applied too.

This patchset only adds:

* Background job with tests * API for submitting cover images in bulk and enqueueing the job, with tests

The dependency tree is a bit of a mess... can't make bug 42804 a dependency of this one because bug 42804 depends on bug 42405 but bug 42404 blocks bug 42405... It's not at all clear what the order of testing these bugs should be... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #43 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #41)

(In reply to Tomás Cohen Arazi (tcohen) from comment #8)

...

The only way to test this from the UI is by having bug 42804 applied too.

This patchset only adds:

* Background job with tests * API for submitting cover images in bulk and enqueueing the job, with tests

The dependency tree is a bit of a mess... can't make bug 42804 a dependency of this one because bug 42804 depends on bug 42405 but bug 42404 blocks bug 42405...

It's not at all clear what the order of testing these bugs should be...

After reviewing all the bugs and the dependency tree... I think I understand what you're trying to do here. Trying to build up the components in separate bug reports before tying them all together. Yeah that makes sense conceptually. Makes things a bit tougher to test, but I get it now... I don't love the idea of signing off on a bug report just because it's passing tests though... I've used up most of my testing time trying to figure out how to test it, so I think I'll have to come back another time... -- You are receiving this mail because: You are watching all bug changes.