From e8b80923912e89c8cdad8697052daed1ec0aaa05 Mon Sep 17 00:00:00 2001 From: Sebastian Ibanez Date: Wed, 24 Sep 2025 12:03:38 -0400 Subject: [PATCH 1/2] fix: use sanitized file name in eDoc, refactor ManageDocument2Action, add documentManager error page --- .../actions/AddEditDocument2Action.java | 14 ++- .../actions/ManageDocument2Action.java | 87 ++++++++++++++++--- src/main/webapp/documentManager/error.jsp | 35 ++++++++ 3 files changed, 113 insertions(+), 23 deletions(-) create mode 100644 src/main/webapp/documentManager/error.jsp diff --git a/src/main/java/ca/openosp/openo/documentManager/actions/AddEditDocument2Action.java b/src/main/java/ca/openosp/openo/documentManager/actions/AddEditDocument2Action.java index 58537e4bcf8..b9c7e6cab5c 100644 --- a/src/main/java/ca/openosp/openo/documentManager/actions/AddEditDocument2Action.java +++ b/src/main/java/ca/openosp/openo/documentManager/actions/AddEditDocument2Action.java @@ -110,14 +110,11 @@ public String html5MultiUpload() throws Exception { newDoc.setProgramId(pp.getProgramId().intValue()); } - fileName = MiscUtils.sanitizeFileName(newDoc.getFileName()); // save local file; if (this.getDocFile().length() == 0) { - //errors.put("uploaderror", "documentManager.error.uploadError"); response.setHeader("oscar_error", props.getString("dms.addDocument.errorZeroSize")); response.sendError(500, props.getString("dms.addDocument.errorZeroSize")); return null; - //throw new FileNotFoundException(); } File file = writeLocalFile(Files.newInputStream(this.getDocFile().toPath()), fileName);// write file to local dir @@ -127,7 +124,6 @@ public String html5MultiUpload() throws Exception { return null; } - //newDoc.setContentType(this.getDocFile().getContentType()); if (fileName.endsWith(".PDF") || fileName.endsWith(".pdf")) { newDoc.setContentType("application/pdf"); // get number of pages when document is pdf; @@ -251,8 +247,8 @@ private boolean addDocument(HttpServletRequest request) { errors.put("uploaderror", "dms.error.uploadError"); throw new FileNotFoundException(); } - // original file name - String fileName1 = this.docFileFileName; + // sanitize the original file name first + String fileName1 = MiscUtils.sanitizeFileName(this.docFileFileName); EDoc newDoc = new EDoc(this.getDocDesc(), this.getDocType(), fileName1, "", this.getDocCreator(), this.getResponsibleId(), this.getSource(), 'A', this.getObservationDate(), "", "", this.getFunction(), this.getFunctionId()); newDoc.setDocPublic(this.getDocPublic()); @@ -260,8 +256,8 @@ private boolean addDocument(HttpServletRequest request) { newDoc.setAppointmentNo(Integer.parseInt(this.getAppointmentNo())); newDoc.setDocClass(this.getDocClass()); newDoc.setDocSubClass(this.getDocSubClass()); - // new file name with date attached - String fileName2 = MiscUtils.sanitizeFileName(newDoc.getFileName()); + // get the filename with timestamp prefix from EDoc (after preliminary processing) + String fileName2 = newDoc.getFileName(); // save local file File file = writeLocalFile(Files.newInputStream(docFile.toPath()), fileName2); @@ -394,7 +390,7 @@ private String editDocument(HttpServletRequest request) { { File docFile = this.getDocFile(); if (docFile != null && docFile.exists()) { - fileName = this.docFileFileName; + fileName = MiscUtils.sanitizeFileName(this.docFileFileName); updateFileContent = true; // set update to true } } diff --git a/src/main/java/ca/openosp/openo/documentManager/actions/ManageDocument2Action.java b/src/main/java/ca/openosp/openo/documentManager/actions/ManageDocument2Action.java index d7f41796a99..bc8093769be 100644 --- a/src/main/java/ca/openosp/openo/documentManager/actions/ManageDocument2Action.java +++ b/src/main/java/ca/openosp/openo/documentManager/actions/ManageDocument2Action.java @@ -137,10 +137,23 @@ public String execute() { return handler.handle(this); } catch (Exception e) { log.error("Error in " + method + "():", e); + return "error"; } } - return documentUpdate(); + // Only call documentUpdate() if no method is specified and required parameters are present + if (method == null || method.trim().isEmpty()) { + String documentId = request.getParameter("documentId"); + String documentDescription = request.getParameter("documentDescription"); + + // Check if this looks like a documentUpdate request + if (documentId != null || documentDescription != null) { + return documentUpdate(); + } + } + + log.error("No valid method found and insufficient parameters for documentUpdate. Method: " + method); + return "error"; } // Functional interface for our handlers. @@ -311,12 +324,21 @@ public String documentUpdate() { String observationDate = request.getParameter("observationDate");// :2008-08-22< String documentDescription = request.getParameter("documentDescription");// :test2< String documentId = request.getParameter("documentId");// :29< + // Also check for doc_no parameter (used by display method URLs) + if (documentId == null || documentId.trim().isEmpty()) { + documentId = request.getParameter("doc_no"); + } String docType = request.getParameter("docType");// :consult< if (!securityInfoManager.hasPrivilege(LoggedInInfo.getLoggedInInfoFromSession(request), "_edoc", "w", null)) { throw new SecurityException("missing required sec object (_edoc)"); } + if (documentId == null || documentId.trim().isEmpty()) { + log.error("Document ID is null or empty, cannot process document update"); + return "error"; + } + LogAction.addLog((String) request.getSession().getAttribute("user"), LogConst.ADD, LogConst.CON_DOCUMENT, documentId, request.getRemoteAddr()); String demog = request.getParameter("demog"); @@ -338,6 +360,9 @@ public String documentUpdate() { log.warn("Invalid provider number format: " + (proNo != null ? proNo.replaceAll("[\r\n]", "") : "null")); } } + } catch (NumberFormatException e) { + log.error("Invalid document ID format: " + documentId, e); + return "error"; } catch (Exception e) { MiscUtils.getLogger().error("Error", e); } @@ -356,19 +381,29 @@ public String documentUpdate() { documentDao.merge(d); } - try { - CtlDocument ctlDocument = ctlDocumentDao.getCtrlDocument(Integer.parseInt(documentId)); - if (ctlDocument != null) { - ctlDocument.getId().setModuleId(Integer.parseInt(demog)); - ctlDocumentDao.merge(ctlDocument); - // save a document created note - if (ctlDocument.isDemographicDocument()) { - // save note - saveDocNote(request, d.getDocdesc(), demog, documentId); + if (documentId != null && !documentId.trim().isEmpty()) { + try { + CtlDocument ctlDocument = ctlDocumentDao.getCtrlDocument(Integer.parseInt(documentId)); + if (ctlDocument != null) { + if (demog != null && !demog.trim().isEmpty()) { + ctlDocument.getId().setModuleId(Integer.parseInt(demog)); + ctlDocumentDao.merge(ctlDocument); + // save a document created note + if (ctlDocument.isDemographicDocument() && d != null) { + // save note + saveDocNote(request, d.getDocdesc(), demog, documentId); + } + } else { + log.warn("Demographics parameter is null or empty, skipping ctlDocument update"); + } } + } catch (NumberFormatException e) { + log.error("Invalid number format for documentId: " + documentId + " or demog: " + demog, e); + } catch (Exception e) { + MiscUtils.getLogger().error("Error", e); } - } catch (Exception e) { - MiscUtils.getLogger().error("Error", e); + } else { + log.warn("Document ID is null or empty, skipping ctlDocument operations"); } String providerNo = request.getParameter("providerNo"); @@ -615,6 +650,16 @@ public void viewDocPage() { LogAction.addLog((String) request.getSession().getAttribute("user"), LogConst.READ, LogConst.CON_DOCUMENT, doc_no, request.getRemoteAddr()); Document d = documentDao.getDocument(doc_no); + if (d == null) { + log.error("Document not found for ID: " + doc_no); + try { + response.sendError(HttpServletResponse.SC_NOT_FOUND, "Document not found"); + } catch (IOException e) { + log.error("Error sending error response", e); + } + return; + } + log.debug("Document Name :" + d.getDocfilename()); //if the file is not a pdf, use display function if (!(d.getContenttype().equals("application/pdf") || d.getDocfilename().endsWith(".pdf"))) { @@ -821,6 +866,9 @@ public void display() throws Exception { } } + if (remoteDocument == null || remoteDocumentContents == null) { + throw new IllegalStateException("Remote document not found or contents unavailable for document ID: " + doc_no); + } docxml = remoteDocument.getDocXml(); contentType = remoteDocument.getContentType(); @@ -947,6 +995,12 @@ public String addIncomingDocument() throws Exception { throw new IllegalArgumentException("Invalid parameters"); } + // Validate queueId and pdfDir to prevent directory traversal + if (queueId1.contains("..") || queueId1.contains("/") || queueId1.contains("\\") || + pdfDir.contains("..") || pdfDir.contains("/") || pdfDir.contains("\\")) { + throw new SecurityException("Invalid directory parameters"); + } + // Sanitize filename to prevent path traversal String sanitizedPdfName = FilenameUtils.getName(pdfName); if (!sanitizedPdfName.equals(pdfName)) { @@ -980,7 +1034,10 @@ public String addIncomingDocument() throws Exception { newDoc.setDocSubClass(docSubClass); newDoc.setDocPublic("0"); fileName = newDoc.getFileName(); - destFilePath = savePath + fileName; + // Sanitize the filename to match what the file system will actually create + String sanitizedFileName = fileName.replaceAll("[^a-zA-Z0-9._-]", ""); + newDoc.setFileName(sanitizedFileName); + destFilePath = savePath + sanitizedFileName; String doc_no = ""; // Validate destination path is within allowed directory @@ -999,7 +1056,9 @@ public String addIncomingDocument() throws Exception { boolean success = f1.renameTo(new File(destFilePath)); if (!success) { log.error("Not able to move " + f1.getName() + " to " + destFilePath); - // File was not successfully moved + // File was not successfully moved - return error to prevent orphaned database entries + request.setAttribute("errorMessage", "Failed to save document file. Please try again."); + return "error"; } else { newDoc.setContentType("application/pdf"); diff --git a/src/main/webapp/documentManager/error.jsp b/src/main/webapp/documentManager/error.jsp new file mode 100644 index 00000000000..f91906272ed --- /dev/null +++ b/src/main/webapp/documentManager/error.jsp @@ -0,0 +1,35 @@ +<%-- + Simple error page for document management errors +--%> + + + + Document Error + + + + +
+
+

Document Error

+

+ <% + String errorMessage = (String) request.getAttribute("errorMessage"); + if (errorMessage != null) { + out.print(errorMessage); + } else { + out.print("An error occurred while processing the document. The document may not exist or there was a problem with the file upload."); + } + %> +

+ +
+ +
+
+
+ + \ No newline at end of file From 42eba16a6635e17c0714d64a75f06094c8758292 Mon Sep 17 00:00:00 2001 From: Sebastian Ibanez Date: Wed, 24 Sep 2025 14:24:50 -0400 Subject: [PATCH 2/2] fix: update ManageDocument2Action, use failure.jsp instead of error.jsp, update ManageDocument in struts.xml --- .../actions/ManageDocument2Action.java | 51 ++++++++++++++++--- src/main/webapp/WEB-INF/classes/struts.xml | 1 + src/main/webapp/documentManager/error.jsp | 35 ------------- 3 files changed, 44 insertions(+), 43 deletions(-) delete mode 100644 src/main/webapp/documentManager/error.jsp diff --git a/src/main/java/ca/openosp/openo/documentManager/actions/ManageDocument2Action.java b/src/main/java/ca/openosp/openo/documentManager/actions/ManageDocument2Action.java index bc8093769be..ec511456106 100644 --- a/src/main/java/ca/openosp/openo/documentManager/actions/ManageDocument2Action.java +++ b/src/main/java/ca/openosp/openo/documentManager/actions/ManageDocument2Action.java @@ -114,10 +114,10 @@ public class ManageDocument2Action extends ActionSupport { // Static initializer used to set the actions of the map for the execute function static { ACTIONS.put("refileDocumentAjax", ctx -> ctx.refileDocumentAjax()); - ACTIONS.put("viewDocPage", ctx -> { ctx.viewDocPage(); return "viewDocPage"; }); - ACTIONS.put("display", ctx -> { ctx.display(); return "display"; }); - ACTIONS.put("viewAnnotationAcknowledgementTickler", ctx -> { ctx.viewAnnotationAcknowledgementTickler(); return "viewAnnotationAcknowledgementTickler"; }); - ACTIONS.put("viewDocumentDescription", ctx -> { ctx.viewDocumentDescription(); return "viewDocumentDescription"; }); + ACTIONS.put("viewDocPage", ctx -> { ctx.viewDocPage(); return null; }); + ACTIONS.put("display", ctx -> { ctx.display(); return null; }); + ACTIONS.put("viewAnnotationAcknowledgementTickler", ctx -> { ctx.viewAnnotationAcknowledgementTickler(); return null; }); + ACTIONS.put("viewDocumentDescription", ctx -> { ctx.viewDocumentDescription(); return null; }); // Enable calling the method to remove providers ACTIONS.put("removeLinkFromDocument", new ActionHandler() { public String handle(ManageDocument2Action action) { @@ -137,6 +137,7 @@ public String execute() { return handler.handle(this); } catch (Exception e) { log.error("Error in " + method + "():", e); + addActionError("An error occurred while processing the document. Please try again or contact your system administrator."); return "error"; } } @@ -153,6 +154,7 @@ public String execute() { } log.error("No valid method found and insufficient parameters for documentUpdate. Method: " + method); + addActionError("Invalid request. The requested operation could not be performed."); return "error"; } @@ -336,6 +338,7 @@ public String documentUpdate() { if (documentId == null || documentId.trim().isEmpty()) { log.error("Document ID is null or empty, cannot process document update"); + addActionError("Document ID is missing. Cannot process document update."); return "error"; } @@ -362,6 +365,7 @@ public String documentUpdate() { } } catch (NumberFormatException e) { log.error("Invalid document ID format: " + documentId, e); + addActionError("Invalid document ID format. Please check the document ID and try again."); return "error"; } catch (Exception e) { MiscUtils.getLogger().error("Error", e); @@ -1036,13 +1040,38 @@ public String addIncomingDocument() throws Exception { fileName = newDoc.getFileName(); // Sanitize the filename to match what the file system will actually create String sanitizedFileName = fileName.replaceAll("[^a-zA-Z0-9._-]", ""); + + // Ensure sanitized filename is not empty and has a minimum length + if (sanitizedFileName.trim().isEmpty() || sanitizedFileName.length() < 1) { + // Generate a fallback filename with timestamp + String timestamp = String.valueOf(System.currentTimeMillis()); + sanitizedFileName = "document_" + timestamp + ".dat"; + } + + // Ensure filename uniqueness by checking if file already exists + File destFile = new File(savePath + sanitizedFileName); + String originalSanitized = sanitizedFileName; + int counter = 1; + while (destFile.exists()) { + String nameWithoutExt = originalSanitized; + String extension = ""; + int lastDot = originalSanitized.lastIndexOf("."); + if (lastDot > 0) { + nameWithoutExt = originalSanitized.substring(0, lastDot); + extension = originalSanitized.substring(lastDot); + } + sanitizedFileName = nameWithoutExt + "_" + counter + extension; + destFile = new File(savePath + sanitizedFileName); + counter++; + } + newDoc.setFileName(sanitizedFileName); destFilePath = savePath + sanitizedFileName; String doc_no = ""; // Validate destination path is within allowed directory - File destFile = new File(destFilePath); - String canonicalDest = destFile.getCanonicalPath(); + File finalDestFile = new File(destFilePath); + String canonicalDest = finalDestFile.getCanonicalPath(); File saveDir = new File(savePath); String canonicalSaveDir = saveDir.getCanonicalPath(); @@ -1056,8 +1085,14 @@ public String addIncomingDocument() throws Exception { boolean success = f1.renameTo(new File(destFilePath)); if (!success) { log.error("Not able to move " + f1.getName() + " to " + destFilePath); - // File was not successfully moved - return error to prevent orphaned database entries - request.setAttribute("errorMessage", "Failed to save document file. Please try again."); + // File was not successfully moved - attempt to delete temp file to prevent orphaned files + boolean deleted = f1.delete(); + if (!deleted) { + log.warn("Failed to delete temporary file: " + f1.getAbsolutePath()); + } + String documentId = request.getParameter("documentId"); + log.error("Failed to save document file for document ID: " + documentId); + addActionError("Failed to save document file. Please try again or contact your system administrator."); return "error"; } else { diff --git a/src/main/webapp/WEB-INF/classes/struts.xml b/src/main/webapp/WEB-INF/classes/struts.xml index 1fdef5ec863..8d2f3088e78 100644 --- a/src/main/webapp/WEB-INF/classes/struts.xml +++ b/src/main/webapp/WEB-INF/classes/struts.xml @@ -1972,6 +1972,7 @@ /documentManager/undocument.jsp /documentManager/MultiPageDocDisplay.jsp /documentManager/incomingDocs.jsp + /failure.jsp diff --git a/src/main/webapp/documentManager/error.jsp b/src/main/webapp/documentManager/error.jsp deleted file mode 100644 index f91906272ed..00000000000 --- a/src/main/webapp/documentManager/error.jsp +++ /dev/null @@ -1,35 +0,0 @@ -<%-- - Simple error page for document management errors ---%> - - - - Document Error - - - - -
-
-

Document Error

-

- <% - String errorMessage = (String) request.getAttribute("errorMessage"); - if (errorMessage != null) { - out.print(errorMessage); - } else { - out.print("An error occurred while processing the document. The document may not exist or there was a problem with the file upload."); - } - %> -

- -
- -
-
-
- - \ No newline at end of file