Skip to content

ROL-2184: Preserve pasted entry images when publishing #205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
snoopdave wants to merge 10 commits into
base: roller-6.1.x
Choose a base branch
from
Open

ROL-2184: Preserve pasted entry images when publishing #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8da9237
Preserve pasted entry images when publishing
snoopdave Oct 3, 2026
97db6d2
Make the inline image field limit configurable
snoopdave Oct 3, 2026
ff66545
Fix inline image parsing and add tests
snoopdave Oct 3, 2026
6140216
Roll back failed entry edits before removing their uploaded images
snoopdave Oct 4, 2026
4aa2335
Report an inline image over the field limit as too large
snoopdave Oct 4, 2026
adeb6fc
Explain a form POST that is too large for the server
snoopdave Oct 4, 2026
03911f9
Show a plain page for a form that is too large
snoopdave Oct 4, 2026
7333029
Tell the author how to get back to a form that was too large
snoopdave Oct 4, 2026
c7a34f1
Keep the apostrophe in the inline image upload limit message
snoopdave Oct 4, 2026
72bc34b
Merge branch 'roller-6.1.x' into fix-inline-entry-images-6.1.x
snoopdave Oct 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,22 @@

## 6.1.7

### Improvements

- **Pasted entry images are kept when you publish**
([ROL-2184](https://issues.apache.org/jira/browse/ROL-2184)). PNG, JPEG and
GIF images pasted or dragged into the rich text editor are saved as media
files when the author can upload. Otherwise they are kept inline.
- `weblog.inlineImages.preferInline=true` keeps images inline even when
uploads are available.
- `weblog.inlineImages.maxFieldBytes` (default 60000) limits a content or
summary field that has inline images. On MySQL, change the entry columns to
`MEDIUMTEXT` before you raise it.
- Bundled themes allow `data:` images. Custom themes with their own Content
Security Policy need `data:` in `img-src`.
- Pasted images travel in the form POST as base64. Tomcat's `maxPostSize`
defaults to 2 MB, so larger pastes are refused with a "form is too large"
message; raise `maxPostSize` to accept them.
### Behaviour changes worth reading before upgrading

- **XML parsing uses Apache Commons Secure XML.** Roller now bundles
Expand Down
25 changes: 25 additions & 0 deletions app/src/main/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.roller.weblogger.util.I18nMessages;

/**
* Filter checks all POST request for presence of valid salt value and rejects those without
Expand All @@ -38,6 +40,12 @@
public class ValidateSaltFilter implements Filter {
private static final Log log = LogFactory.getLog(ValidateSaltFilter.class);

/**
* Request attribute Tomcat sets when it does not parse the parameters of a
* request; its value names the reason.
*/
static final String PARSE_FAILED_REASON = "org.apache.catalina.parameter_parse_failed_reason";

@Override
public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain) throws IOException, ServletException {
Expand All @@ -54,6 +62,18 @@ public void doFilter(ServletRequest request, ServletResponse response,
try {
SaltValidator.requireSubmittedSalt(httpReq);
} catch (ServletException e) {
if (isPostTooLarge(httpReq)) {
// The container dropped the form, salt included, because it
// is over its maximum POST size. Pasted images make this
// likely, so say why instead of reporting a security error.
log.warn("Refused a POST to " + httpReq.getServletPath()
+ " that is larger than the server's maximum POST size");
((HttpServletResponse) response).sendError(
HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE,
I18nMessages.getMessages(httpReq.getLocale())
.getString("error.postTooLarge"));
return;
}
if (log.isDebugEnabled()) {
log.debug("Valid salt value not found on POST to URL : "
+ httpReq.getServletPath());
Expand All @@ -73,6 +93,11 @@ public void init(FilterConfig filterConfig) throws ServletException {
public void destroy() {
}

static boolean isPostTooLarge(HttpServletRequest request) {
Object reason = request.getAttribute(PARSE_FAILED_REASON);
return reason != null && "POST_TOO_LARGE".equals(reason.toString());
}

private boolean isStrutsAction(HttpServletRequest request) {
String servletPath = request.getServletPath();
return servletPath != null && servletPath.endsWith(".rol");
Expand Down
242 changes: 242 additions & 0 deletions app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/EntryEdit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,36 @@

package org.apache.roller.weblogger.ui.struts2.editor;

import java.io.ByteArrayInputStream;
import java.math.BigDecimal;
import java.nio.charset.StandardCharsets;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.UUID;

import org.apache.commons.lang3.StringUtils;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.roller.util.DateUtil;
import org.apache.roller.util.RollerConstants;
import org.apache.roller.weblogger.WebloggerException;
import org.apache.roller.weblogger.business.MediaFileManager;
import org.apache.roller.weblogger.business.WebloggerFactory;
import org.apache.roller.weblogger.business.WeblogEntryManager;
import org.apache.roller.weblogger.business.plugins.PluginManager;
import org.apache.roller.weblogger.business.plugins.entry.WeblogEntryPlugin;
import org.apache.roller.weblogger.business.search.IndexManager;
import org.apache.roller.weblogger.config.WebloggerConfig;
import org.apache.roller.weblogger.config.WebloggerRuntimeConfig;
import org.apache.roller.weblogger.pojos.GlobalPermission;
import org.apache.roller.weblogger.pojos.MediaFile;
import org.apache.roller.weblogger.pojos.MediaFileDirectory;
import org.apache.roller.weblogger.pojos.WeblogCategory;
import org.apache.roller.weblogger.pojos.WeblogEntry;
import org.apache.roller.weblogger.pojos.WeblogEntry.PubStatus;
Expand All @@ -47,7 +58,10 @@
import org.apache.roller.weblogger.ui.core.plugins.WeblogEntryEditor;
import org.apache.roller.weblogger.ui.struts2.util.UIAction;
import org.apache.roller.weblogger.util.EnclosureMetadata;
import org.apache.roller.weblogger.util.InlineImageData;
import org.apache.roller.weblogger.util.MailUtil;
import org.apache.roller.weblogger.util.RollerMessages;
import org.apache.roller.weblogger.util.RollerMessages.RollerMessage;
import org.apache.roller.weblogger.util.cache.CacheManager;
import org.apache.struts2.convention.annotation.AllowedMethods;
import org.apache.struts2.interceptor.validation.SkipValidation;
Expand Down Expand Up @@ -200,7 +214,15 @@ String save() {
return failedSave();
}

String submittedText = getBean().getText();
String submittedSummary = getBean().getSummary();
List<MediaFile> createdImages = new ArrayList<>();
boolean entrySaved = false;
try {
if (!prepareInlineImages(createdImages)) {
return failedSave();
}

WeblogEntryManager weblogEntryManager = WebloggerFactory.getWeblogger()
.getWeblogEntryManager();

Expand Down Expand Up @@ -264,6 +286,7 @@ String save() {
log.debug("Saving entry");
weblogEntryManager.saveWeblogEntry(weblogEntry);
WebloggerFactory.getWeblogger().flush();
entrySaved = true;

// notify search of the new entry
if (weblogEntry.isPublished()) {
Expand Down Expand Up @@ -298,12 +321,231 @@ String save() {

} catch (Exception e) {
log.error("Error saving new entry", e);
if (!entrySaved) {
// The entry may already hold the failed edit, and the
// cleanup below commits its own transaction. Roll the
// failed edit back first so that commit cannot save it.
WebloggerFactory.getWeblogger().release();
getBean().setText(submittedText);
getBean().setSummary(submittedSummary);
removeCreatedImages(WebloggerFactory.getWeblogger()
.getMediaFileManager(), createdImages);
}
addError("generic.error.check.logs");
}
}
return failedSave();
}

/**
* Uploads data images in the submitted text and summary as media files, or
* keeps them inline when uploads are unavailable. Adds an action error and
* returns false if the entry cannot be saved. Media files it creates are
* added to createdImages so a later failure can remove them.
*/
// Package-private so EntryEditInlineImagesTest can drive it directly.
boolean prepareInlineImages(List<MediaFile> createdImages)
throws WebloggerException {
String submittedText = getBean().getText();
String submittedSummary = getBean().getSummary();
Map<String, InlineImageData.Image> images = new HashMap<>();
List<InlineImageData.Source> textImages = InlineImageData.findSources(submittedText);
List<InlineImageData.Source> summaryImages = InlineImageData.findSources(submittedSummary);
boolean keepInline = WebloggerConfig.getBooleanProperty(
"weblog.inlineImages.preferInline")
|| !WebloggerRuntimeConfig.getBooleanProperty("uploads.enabled")
|| !getActionWeblog().hasUserPermission(
getAuthenticatedUser(), WeblogPermission.POST);
long maxUploadBytes = 0;
if (!keepInline && (!textImages.isEmpty() || !summaryImages.isEmpty())) {
maxUploadBytes = (long) (RollerConstants.ONE_MB_IN_BYTES
* new BigDecimal(WebloggerRuntimeConfig.getProperty(
"uploads.file.maxsize")).doubleValue());
}
if (!validateInlineImages(textImages, images, keepInline, maxUploadBytes)
|| !validateInlineImages(summaryImages, images, keepInline,
maxUploadBytes)) {
return false;
}
if (!images.isEmpty()) {
if (keepInline) {
String inlineText = normalizeInlineSources(submittedText,
textImages);
String inlineSummary = normalizeInlineSources(submittedSummary,
summaryImages);
if (!inlineFieldFits(inlineText, textImages)
|| !inlineFieldFits(inlineSummary, summaryImages)) {
return false;
}
getBean().setText(inlineText);
getBean().setSummary(inlineSummary);
} else {
MediaFileManager mediaManager = WebloggerFactory.getWeblogger()
.getMediaFileManager();
MediaFileDirectory directory = mediaManager
.getDefaultMediaFileDirectory(getActionWeblog());
if (directory == null) {
directory = mediaManager.createDefaultMediaFileDirectory(
getActionWeblog());
}
Map<String, String> mediaUrls = new HashMap<>();
getBean().setText(replaceInlineImages(submittedText,
textImages, images, mediaUrls, directory,
mediaManager, createdImages));
if (!hasActionErrors()) {
getBean().setSummary(replaceInlineImages(submittedSummary,
summaryImages, images, mediaUrls, directory,
mediaManager, createdImages));
}
if (hasActionErrors()) {
getBean().setText(submittedText);
getBean().setSummary(submittedSummary);
removeCreatedImages(mediaManager, createdImages);
return false;
}
}
}
return true;
}

private boolean validateInlineImages(List<InlineImageData.Source> sources,
Map<String, InlineImageData.Image> images, boolean keepInline,
long maxUploadBytes) {
for (InlineImageData.Source source : sources) {
if (!keepInline && InlineImageData.exceedsUploadLimit(
source.getValue(), maxUploadBytes)) {
addError("weblogEdit.inlineImageUploadTooLarge");
return false;
}
// parse() also refuses an inline image over the field limit; report
// that as too large, not as an invalid image
if (keepInline && source.getValue().length() > InlineImageData.maxFieldBytes()) {
addError("weblogEdit.inlineImageTooLarge");
return false;
}
InlineImageData.Image image = keepInline
? InlineImageData.parse(source.getValue())
: InlineImageData.parseForUpload(source.getValue(),
maxUploadBytes);
if (image == null) {
addError("weblogEdit.inlineImageInvalid");
return false;
}
images.put(source.getValue(), image);
}
return true;
}

private boolean inlineFieldFits(String html, List<InlineImageData.Source> sources) {
if (!sources.isEmpty() && html.getBytes(StandardCharsets.UTF_8).length
> InlineImageData.maxFieldBytes()) {
addError("weblogEdit.inlineImageTooLarge");
return false;
}
return true;
}

private String normalizeInlineSources(String html,
List<InlineImageData.Source> sources) {
if (html == null || sources.isEmpty()) {
return html;
}
StringBuilder result = new StringBuilder(html.length());
int cursor = 0;
for (InlineImageData.Source source : sources) {
result.append(html, cursor, source.getStart());
result.append("src=\"").append(source.getValue()).append('"');
cursor = source.getEnd();
}
result.append(html, cursor, html.length());
return result.toString();
}

private String replaceInlineImages(String html,
List<InlineImageData.Source> sources,
Map<String, InlineImageData.Image> images,
Map<String, String> mediaUrls, MediaFileDirectory directory,
MediaFileManager mediaManager, List<MediaFile> createdImages)
throws WebloggerException {
if (html == null || sources.isEmpty()) {
return html;
}
StringBuilder result = new StringBuilder(html.length());
int cursor = 0;
for (InlineImageData.Source source : sources) {
String url = mediaUrls.get(source.getValue());
if (url == null) {
InlineImageData.Image image = images.get(source.getValue());
String name = "entry-image-" + UUID.randomUUID() + "."
+ image.getExtension();
RollerMessages errors = new RollerMessages();
if (!WebloggerFactory.getWeblogger().getFileContentManager()
.canSave(getActionWeblog(), name, image.getContentType(),
image.getBytes().length, errors)) {
addMediaErrors(errors);
return html;
}
MediaFile media = new MediaFile();
media.setName(name);
media.setWeblog(getActionWeblog());
media.setDirectory(directory);
media.setLength(image.getBytes().length);
media.setContentType(image.getContentType());
media.setInputStream(new ByteArrayInputStream(image.getBytes()));
// Track the upload before creating it: createMediaFile commits
// the record before it writes the file, so a failed write can
// still leave a record to remove.
createdImages.add(media);
mediaManager.createMediaFile(getActionWeblog(), media, errors);

@snoopdave snoopdave Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐞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.

@snoopdave snoopdave Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖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.

if (errors.getErrorCount() > 0) {
addMediaErrors(errors);
return html;
}
url = media.getPermalink();
mediaUrls.put(source.getValue(), url);
}
result.append(html, cursor, source.getStart());
result.append("src=\"").append(url).append('"');
cursor = source.getEnd();
}
result.append(html, cursor, html.length());
return result.toString();
}

private void addMediaErrors(RollerMessages errors) {
for (Iterator<RollerMessage> it = errors.getErrors(); it.hasNext();) {
RollerMessage message = it.next();
String[] args = message.getArgs();
addError(message.getKey(), args == null
? Collections.emptyList() : java.util.Arrays.asList(args));
}
}

private void removeCreatedImages(MediaFileManager mediaManager,
List<MediaFile> createdImages) {
for (MediaFile image : createdImages) {
try {
// Look the upload up again: the save may have rolled back and
// released the session, and an attempted upload may never
// have been stored.
MediaFile stored = mediaManager.getMediaFile(image.getId());
if (stored != null) {
mediaManager.removeMediaFile(stored.getWeblog(), stored);
}
} catch (WebloggerException cleanupError) {
log.warn("Could not remove an image from a failed entry save", cleanupError);
}
}
if (!createdImages.isEmpty()) {
try {
WebloggerFactory.getWeblogger().flush();

@snoopdave snoopdave Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐞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.

@snoopdave snoopdave Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖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.

} catch (WebloggerException cleanupError) {
log.warn("Could not flush image cleanup after a failed entry save",
cleanupError);
}
}
}

EnclosureMetadata validateEnclosure() {
if (StringUtils.isEmpty(getBean().getEnclosureURL())) {
return null;
Expand Down
Loading
Loading