snoopdave commented on code in PR #205:
URL: https://github.com/apache/roller/pull/205#discussion_r4177473855
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/EntryEdit.java:
##########
@@ -298,12 +321,212 @@ String save() {
} catch (Exception e) {
log.error("Error saving new entry", e);
+ if (!entrySaved) {
+ 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;
+ }
+ 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()));
+ mediaManager.createMediaFile(getActionWeblog(), media, errors);
Review 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.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/EntryEdit.java:
##########
@@ -298,12 +321,212 @@ String save() {
} catch (Exception e) {
log.error("Error saving new entry", e);
+ if (!entrySaved) {
+ 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;
+ }
+ 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()));
+ mediaManager.createMediaFile(getActionWeblog(), media, errors);
+ if (errors.getErrorCount() > 0) {
+ addMediaErrors(errors);
+ return html;
+ }
+ createdImages.add(media);
+ 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 {
+ mediaManager.removeMediaFile(getActionWeblog(), image);
+ } catch (WebloggerException cleanupError) {
+ log.warn("Could not remove an image from a failed entry save",
cleanupError);
+ }
+ }
+ if (!createdImages.isEmpty()) {
+ try {
+ WebloggerFactory.getWeblogger().flush();
Review 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.
##########
app/src/main/java/org/apache/roller/weblogger/util/InlineImageData.java:
##########
@@ -0,0 +1,238 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.roller.weblogger.util;
+
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.List;
+import java.util.Locale;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.roller.weblogger.config.WebloggerConfig;
+
+/** Image data URLs accepted in entry content and their locations in HTML. */
+public final class InlineImageData {
+
+ private static final Log log = LogFactory.getLog(InlineImageData.class);
+
+ static final String MAX_FIELD_BYTES_PROPERTY =
"weblog.inlineImages.maxFieldBytes";
+
+ // 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[^<>]*>");
Review 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
