snoopdave commented on code in PR #205:
URL: https://github.com/apache/roller/pull/205#discussion_r4177537799
##########
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:
🤖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.
##########
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:
🤖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.
##########
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:
🤖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.
--
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]
