Copilot commented on code in PR #1805: URL: https://github.com/apache/struts/pull/1805#discussion_r3630364624
########## core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java: ########## @@ -0,0 +1,198 @@ +/* + * 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.struts2.dispatcher.multipart; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsException; + +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.UUID; + +/** + * In-memory backed {@link UploadedFile} for small multipart uploads that Commons FileUpload kept + * in memory ({@code DiskFileItem.isInMemory() == true}). + * + * <p>The content is held as a byte array and is written to a temporary file only the first time a + * caller demands a {@link File} through {@link #getContent()} or {@link #getAbsolutePath()} (lazy + * materialization). Callers reading through {@link #getInputStream()} never touch the disk. The + * temporary file uses the secure {@code upload_<uuid>.tmp} naming and ignores the user-supplied + * original filename.</p> + * + * <p><strong>Clustered deployments:</strong> the target temporary path is resolved on the node that + * created this instance. If an un-materialized instance is serialized (for example, session + * replication) and deserialized on another node, a later {@link #getContent()} materializes to that + * originating node's path, which may not exist on the new node. Read via {@link #getInputStream()}, + * which never touches disk, when content must survive cross-node replication.</p> + * + * @since 7.3.0 + */ +public class StrutsInMemoryUploadedFile implements UploadedFile { + + private static final long serialVersionUID = 1L; + + private static final Logger LOG = LogManager.getLogger(StrutsInMemoryUploadedFile.class); + + private final byte[] content; + private final File targetFile; + private final String contentType; + private final String originalName; + private final String inputName; + + private volatile transient File materializedFile; + + private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String contentType, + String originalName, String inputName) { + this.content = content; + String name = "upload_" + UUID.randomUUID().toString().replace("-", "_") + ".tmp"; + this.targetFile = saveDir.resolve(name).toFile(); + this.contentType = contentType; + this.originalName = originalName; + this.inputName = inputName; + } + + private synchronized File materialize() { + if (materializedFile == null) { + try { + Files.write(targetFile.toPath(), content); + } catch (IOException e) { + try { + Files.deleteIfExists(targetFile.toPath()); + } catch (IOException suppressed) { + e.addSuppressed(suppressed); + } + throw new StrutsException("Could not materialize in-memory uploaded file: " + targetFile.getName(), e); + } + materializedFile = targetFile; + LOG.debug("Materialized in-memory uploaded item to {}", targetFile.getAbsolutePath()); + } + return materializedFile; + } Review Comment: `Files.write(targetFile.toPath(), content)` will create-or-truncate an existing path, which (in the worst case) can enable overwrite/symlink-style attacks if something manages to pre-create that filename in the upload dir. Consider opening the file with `StandardOpenOption.CREATE_NEW` (fail if it exists) and aligning the creation semantics with the existing secure temp-file creation approach used elsewhere (e.g., create atomically and avoid overwriting). This also makes the “materialize exactly once” contract more robust under unexpected filesystem state. ########## core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java: ########## @@ -0,0 +1,198 @@ +/* + * 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.struts2.dispatcher.multipart; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsException; + +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.UUID; + +/** + * In-memory backed {@link UploadedFile} for small multipart uploads that Commons FileUpload kept + * in memory ({@code DiskFileItem.isInMemory() == true}). + * + * <p>The content is held as a byte array and is written to a temporary file only the first time a + * caller demands a {@link File} through {@link #getContent()} or {@link #getAbsolutePath()} (lazy + * materialization). Callers reading through {@link #getInputStream()} never touch the disk. The + * temporary file uses the secure {@code upload_<uuid>.tmp} naming and ignores the user-supplied + * original filename.</p> + * + * <p><strong>Clustered deployments:</strong> the target temporary path is resolved on the node that + * created this instance. If an un-materialized instance is serialized (for example, session + * replication) and deserialized on another node, a later {@link #getContent()} materializes to that + * originating node's path, which may not exist on the new node. Read via {@link #getInputStream()}, + * which never touches disk, when content must survive cross-node replication.</p> + * + * @since 7.3.0 + */ +public class StrutsInMemoryUploadedFile implements UploadedFile { + + private static final long serialVersionUID = 1L; + + private static final Logger LOG = LogManager.getLogger(StrutsInMemoryUploadedFile.class); + + private final byte[] content; + private final File targetFile; + private final String contentType; + private final String originalName; + private final String inputName; + + private volatile transient File materializedFile; + + private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String contentType, + String originalName, String inputName) { + this.content = content; + String name = "upload_" + UUID.randomUUID().toString().replace("-", "_") + ".tmp"; + this.targetFile = saveDir.resolve(name).toFile(); + this.contentType = contentType; + this.originalName = originalName; + this.inputName = inputName; + } Review Comment: The class claims thread-safety/lazy caching, but it stores the caller-provided `byte[]` by reference. If that array is mutated after construction (or shared across threads), `getInputStream()`, `length()`, and eventual materialization can observe inconsistent content, violating immutability and thread-safety expectations. Recommend defensively copying the byte array on construction (and optionally rejecting `null` content early) so `isMissing()` can reflect actual availability and you avoid `NullPointerException` paths in `getInputStream()`/`length()`. ########## core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java: ########## @@ -0,0 +1,198 @@ +/* + * 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.struts2.dispatcher.multipart; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsException; + +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.UUID; + +/** + * In-memory backed {@link UploadedFile} for small multipart uploads that Commons FileUpload kept + * in memory ({@code DiskFileItem.isInMemory() == true}). + * + * <p>The content is held as a byte array and is written to a temporary file only the first time a + * caller demands a {@link File} through {@link #getContent()} or {@link #getAbsolutePath()} (lazy + * materialization). Callers reading through {@link #getInputStream()} never touch the disk. The + * temporary file uses the secure {@code upload_<uuid>.tmp} naming and ignores the user-supplied + * original filename.</p> + * + * <p><strong>Clustered deployments:</strong> the target temporary path is resolved on the node that + * created this instance. If an un-materialized instance is serialized (for example, session + * replication) and deserialized on another node, a later {@link #getContent()} materializes to that + * originating node's path, which may not exist on the new node. Read via {@link #getInputStream()}, + * which never touches disk, when content must survive cross-node replication.</p> + * + * @since 7.3.0 + */ +public class StrutsInMemoryUploadedFile implements UploadedFile { + + private static final long serialVersionUID = 1L; + + private static final Logger LOG = LogManager.getLogger(StrutsInMemoryUploadedFile.class); + + private final byte[] content; Review Comment: The class claims thread-safety/lazy caching, but it stores the caller-provided `byte[]` by reference. If that array is mutated after construction (or shared across threads), `getInputStream()`, `length()`, and eventual materialization can observe inconsistent content, violating immutability and thread-safety expectations. Recommend defensively copying the byte array on construction (and optionally rejecting `null` content early) so `isMissing()` can reflect actual availability and you avoid `NullPointerException` paths in `getInputStream()`/`length()`. ########## core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java: ########## @@ -181,20 +174,17 @@ protected void processNormalFormField(DiskFileItem item, Charset charset) throws * <ol> * <li>Validating the file name and field name are not null/empty</li> * <li>Determining if the file is stored in memory or on disk</li> - * <li>For in-memory files: creating a temporary file and copying content</li> + * <li>For in-memory files: wrapping the content in a {@link StrutsInMemoryUploadedFile} + * that only writes to disk lazily, on demand</li> * <li>For disk files: using the existing file directly</li> * <li>Creating an {@link UploadedFile} abstraction</li> * <li>Adding the file to the uploaded files collection</li> * </ol> - * - * <p>Temporary files created for in-memory uploads are automatically - * tracked for cleanup. Any errors during temporary file creation are - * logged and added to the error list for user feedback.</p> - * + * * @param item the disk file item representing the uploaded file - * @see #cleanUpTemporaryFiles() + * @throws IOException if an error occurs reading the in-memory item's content */ - protected void processFileField(DiskFileItem item, String saveDir) { + protected void processFileField(DiskFileItem item, String saveDir) throws IOException { Review Comment: In the in-memory branch, `item.get()` is now on the hot path with no local error-to-`errors` translation (the previous eager-write path explicitly converted `IOException` into a `LocalizedMessage`). If `item.get()` throws, parsing may now fail with an exception rather than producing a collected upload error like other failure modes. If the intent is “only materialization failures move to consumption-time,” consider catching `IOException` specifically around `item.get()` and mapping it to the existing upload error mechanism (or update surrounding error-handling docs to reflect the new behavior). ########## core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java: ########## @@ -0,0 +1,198 @@ +/* + * 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.struts2.dispatcher.multipart; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsException; + +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.UUID; + +/** + * In-memory backed {@link UploadedFile} for small multipart uploads that Commons FileUpload kept + * in memory ({@code DiskFileItem.isInMemory() == true}). + * + * <p>The content is held as a byte array and is written to a temporary file only the first time a + * caller demands a {@link File} through {@link #getContent()} or {@link #getAbsolutePath()} (lazy + * materialization). Callers reading through {@link #getInputStream()} never touch the disk. The + * temporary file uses the secure {@code upload_<uuid>.tmp} naming and ignores the user-supplied + * original filename.</p> + * + * <p><strong>Clustered deployments:</strong> the target temporary path is resolved on the node that + * created this instance. If an un-materialized instance is serialized (for example, session + * replication) and deserialized on another node, a later {@link #getContent()} materializes to that + * originating node's path, which may not exist on the new node. Read via {@link #getInputStream()}, + * which never touches disk, when content must survive cross-node replication.</p> + * + * @since 7.3.0 + */ +public class StrutsInMemoryUploadedFile implements UploadedFile { + + private static final long serialVersionUID = 1L; + + private static final Logger LOG = LogManager.getLogger(StrutsInMemoryUploadedFile.class); + + private final byte[] content; + private final File targetFile; + private final String contentType; + private final String originalName; + private final String inputName; + + private volatile transient File materializedFile; + + private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String contentType, + String originalName, String inputName) { + this.content = content; + String name = "upload_" + UUID.randomUUID().toString().replace("-", "_") + ".tmp"; + this.targetFile = saveDir.resolve(name).toFile(); + this.contentType = contentType; + this.originalName = originalName; + this.inputName = inputName; + } + + private synchronized File materialize() { + if (materializedFile == null) { + try { + Files.write(targetFile.toPath(), content); + } catch (IOException e) { + try { + Files.deleteIfExists(targetFile.toPath()); + } catch (IOException suppressed) { + e.addSuppressed(suppressed); + } + throw new StrutsException("Could not materialize in-memory uploaded file: " + targetFile.getName(), e); + } + materializedFile = targetFile; + LOG.debug("Materialized in-memory uploaded item to {}", targetFile.getAbsolutePath()); + } + return materializedFile; + } + + @Override + public InputStream getInputStream() { + return new ByteArrayInputStream(content); + } + + @Override + public boolean isMissing() { + return false; + } + + @Override + public Long length() { + return (long) content.length; + } Review Comment: The class claims thread-safety/lazy caching, but it stores the caller-provided `byte[]` by reference. If that array is mutated after construction (or shared across threads), `getInputStream()`, `length()`, and eventual materialization can observe inconsistent content, violating immutability and thread-safety expectations. Recommend defensively copying the byte array on construction (and optionally rejecting `null` content early) so `isMissing()` can reflect actual availability and you avoid `NullPointerException` paths in `getInputStream()`/`length()`. ########## core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java: ########## @@ -215,40 +205,14 @@ protected void processFileField(DiskFileItem item, String saveDir) { List<UploadedFile> values = uploadedFiles.computeIfAbsent(fieldName, k -> new ArrayList<>()); if (item.isInMemory()) { - LOG.debug(() -> "Creating temporary file representing in-memory uploaded item: " + normalizeSpace(item.getFieldName())); - try { - File tempFile = createTemporaryFile(item.getName(), Path.of(saveDir)); - - // Track the temporary file for explicit cleanup - temporaryFiles.add(tempFile); - - // Write the in-memory content to the temporary file - try (java.io.FileOutputStream fos = new java.io.FileOutputStream(tempFile)) { - fos.write(item.get()); - } - - UploadedFile uploadedFile = StrutsUploadedFile.Builder - .create(tempFile) - .withOriginalName(item.getName()) - .withContentType(item.getContentType()) - .withInputName(item.getFieldName()) - .build(); - values.add(uploadedFile); - - if (LOG.isDebugEnabled()) { - LOG.debug("Created temporary file for in-memory uploaded item: {} at {}", - normalizeSpace(item.getName()), tempFile.getAbsolutePath()); - } - } catch (IOException e) { - LOG.warn("Failed to create temporary file for in-memory uploaded item: {}", - normalizeSpace(item.getName()), e); - - // Add the error to the errors list for proper user feedback - LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(), new Object[]{item.getName()}); - if (!errors.contains(errorMessage)) { - errors.add(errorMessage); - } - } + LOG.debug(() -> "Keeping in-memory uploaded item without writing to disk: " + normalizeSpace(item.getFieldName())); + UploadedFile uploadedFile = StrutsInMemoryUploadedFile.Builder + .create(item.get(), Path.of(saveDir)) + .withOriginalName(item.getName()) + .withContentType(item.getContentType()) + .withInputName(item.getFieldName()) + .build(); + values.add(uploadedFile); Review Comment: In the in-memory branch, `item.get()` is now on the hot path with no local error-to-`errors` translation (the previous eager-write path explicitly converted `IOException` into a `LocalizedMessage`). If `item.get()` throws, parsing may now fail with an exception rather than producing a collected upload error like other failure modes. If the intent is “only materialization failures move to consumption-time,” consider catching `IOException` specifically around `item.get()` and mapping it to the existing upload error mechanism (or update surrounding error-handling docs to reflect the new behavior). -- 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]
