Conversation
Add weblog.inlineImages.maxFieldBytes (default 60000). Document the database column sizes that bound it.
Stop an img tag match at the next '<' so scanning stays linear, and read img attributes in order so src text inside another attribute's quoted value is not taken for the source. Move entry image handling into prepareInlineImages() so it can be tested, add tests for parsing, the sanitizer and the editor save paths, and add the 6.1.7 change log entry.
snoopdave
left a comment
snoopdave
left a comment
There was a problem hiding this comment.
🐞Codex Issue: PR-Review: 3 inline findings (1 Blocking, 2 Important). Reviewed ff66545 against roller-6.1.x. All 23 focused JDK 11 tests passed; targeted failure and parsing probes exposed the reported gaps. Browser and database integration checks were not run.
| } | ||
| if (!createdImages.isEmpty()) { | ||
| try { | ||
| WebloggerFactory.getWeblogger().flush(); |
There was a problem hiding this comment.
🐞Codex Issue: Blocking: Roll back the failed entry edit before committing media cleanup. For an existing entry, getBean().copyTo(weblogEntry) mutates the managed entity before operations that can throw, such as resolving a category deleted since the edit form was loaded. The catch block restores only the bean, then this flush() commits the same EntityManager transaction (JPAPersistenceStrategy.flush() calls commit()). This can save the failed title/text/status changes while deleting the uploaded images referenced by that text. A focused save-path probe with a missing category reached this cleanup flush with the new title and rewritten media URL still on the entry, despite returning an error. Roll back/release the failed edit transaction, then remove committed uploads in a separate transaction. Add a persistence regression test asserting that a failed edit preserves the stored entry and leaves no new media.
There was a problem hiding this comment.
🤖Claude: fixed (not yet pushed). When the save fails before the entry is stored, the catch block now calls Weblogger.release() first. That rolls back the transaction that holds the failed edit and closes the session. The uploads are then removed in a new transaction: each one is looked up again by id and removed with its own managed weblog.
Regression test failedEditIsRolledBackBeforeUploadsAreRemoved uses a category that no longer exists, so copyTo changes the entry and then throws. It checks that release() runs before removeMediaFile, that flush() runs only once and only after the rollback, and that saveWeblogEntry is never called. It fails on the current branch head and passes with the fix. This is a mock-level test of the transaction order, not a database test: no Derby-backed EntryEdit test harness exists to build on.
| media.setLength(image.getBytes().length); | ||
| media.setContentType(image.getContentType()); | ||
| media.setInputStream(new ByteArrayInputStream(image.getBytes())); | ||
| mediaManager.createMediaFile(getActionWeblog(), media, errors); |
There was a problem hiding this comment.
🐞Codex Issue: Important: Include partially created media in failure cleanup. JPAMediaFileManagerImpl.createMediaFile() stores and commits the media record before calling saveFileContent(). If the file write throws, execution never reaches createdImages.add(media), so the outer catch cannot remove the current image. A failure-injection probe confirmed zero cleanup calls for that image. Handle the partial-success case explicitly, either by making media creation clean up its committed record on failure or by tracking and removing a persisted attempted upload. Cover a file write failure after metadata commit, including the first image in an entry.
There was a problem hiding this comment.
🤖Claude: fixed (not yet pushed). replaceInlineImages now adds the media file to createdImages before it calls createMediaFile, so a record committed before a failed file write is still tracked. Cleanup looks each upload up by id and skips any that were never stored.
New tests: uploadWhoseFileWriteFailsIsStillRemoved stores the record, then throws on the first and only image, and checks that the record is removed. It fails on the current branch head and passes with the fix. attemptedUploadThatWasNeverStoredIsSkipped checks that cleanup does not try to remove an upload that was never stored.
| // MySQL's TEXT column holds 65,535 bytes. Leave room for the rest of an entry. | ||
| static final int DEFAULT_MAX_FIELD_BYTES = 60000; | ||
|
|
||
| private static final Pattern IMAGE_TAG = Pattern.compile("(?is)<img\\b[^<>]*>"); |
There was a problem hiding this comment.
🐞Codex Issue: Important: Scan image tags with awareness of quoted attribute values. The [^<>]* pattern rejects a valid tag containing < in an attribute and stops at > even inside quotes. For example, findSources() returns zero for both <img alt="a > b" src="data:image/png;base64,..."> and <img src="data:image/png;base64,..." alt="a < b"> when supplied a valid PNG. prepareInlineImages() then reports success without converting the image or enforcing the inline field limit. A larger pasted image can remain in the entry and be stripped during rendering or fail to fit the database field. Use a scanner that respects quotes while retaining linear behavior, and add tests with angle brackets before and after src.
There was a problem hiding this comment.
🤖Claude: fixed by replacing the <img\\b[^<>]*> regex with a single forward scan (not yet pushed). It finds each <img tag start, then reads the attributes in order. Quoted values may contain < and >, and only a > outside quotes ends the tag. A tag that never closes stops the scan, so the work stays linear.
New tests cover angle brackets before and after src (alt="a > b", alt="a < b"), <imgx not counting as an img tag, and a 200,000-tag string inside a quoted value under the existing 2-second timeout. The angle-bracket test fails on the current branch head and passes with the fix. InlineImageDataTest 12/12, EntryEditInlineImagesTest 10/10, HTMLSanitizerInlineImageTest 3/3 and EntryEditEnclosureTest 3/3 pass on JDK 11.
- Release the session when a save fails before the entry is stored, so the cleanup transaction cannot commit the failed edit. Look each upload up again by id before removing it. - Track an upload before creating it, so a record committed before a failed file write is still removed. - Scan img tags with a quote-aware linear scanner instead of a regex, so angle brackets in quoted attribute values do not hide a data source.
With images kept inline, InlineImageData.parse() refuses a data URL longer than weblog.inlineImages.maxFieldBytes, and the caller reported every refusal as an unsupported or invalid image. Check the length first and report weblogEdit.inlineImageTooLarge.
A pasted image travels in the entry form as base64. When the form is over the container's maximum POST size (Tomcat: 2 MB by default), Tomcat drops every parameter, the salt check fails, and the author saw a 500 Security Violation. When Tomcat reports POST_TOO_LARGE, answer 413 with a message that says what happened, and document maxPostSize.
The generic error page called it an unexpected exception that had been logged. Use a page with a heading and the message.
Struts formats action errors with MessageFormat, which drops a single apostrophe, so the message read "the sites media upload size limit".
|
Manually tested at c7a34f1 on the 6.1.7 integration build (Tomcat 9, JDK 11, MySQL 8). Pass.
Testing added these fixes: |
1 participant
Fixes ROL-2184.
Summary
Desktop images pasted or dragged into the rich text editor remain visible while editing but disappear from published entries because their
data:sources are removed during rendering.This change converts PNG, JPEG, and GIF data images to Roller media files when uploads are available to the author. If uploads are disabled or the author cannot upload, it keeps validated images inline. Saving an older entry applies the same handling to images already in its text or summary.
It also adds
weblog.inlineImages.preferInlinefor operators who want inline storage even when uploads are available, limits inline content to 60,000 UTF-8 bytes per field by default (configurable withweblog.inlineImages.maxFieldBytes; MySQL needsMEDIUMTEXTcolumns to go higher), permits validated data images through the HTML sanitizer, and updates bundled theme image policies and the user guide.Verification
mvn -pl app -am teston JDK 11: 345 tests, 0 failures, 1 skipped.InlineImageDataTest(parsing, signatures, size limits, source positions, linear scanning on adversarial input),HTMLSanitizerInlineImageTest(PNG, JPEG and GIF data kept; SVG, HTML and mismatched data removed), andEntryEditInlineImagesTest(upload, keep inline, field limit, invalid image, upload limit, cleanup after a refused upload).roller-6.1.x. Browser verification was not done.Notes
Custom themes with their own Content Security Policy need
data:inimg-srcto display inline images.Supersedes #203, which targeted
master.Targets
roller-6.1.xfor 6.1.7. It will be cherry-picked tomasterafter the 6.1.7 release.