Skip to content

Fix PDF document uploading #628

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

Merged
yingbull merged 2 commits into from
Sep 25, 2025
Merged

Fix PDF document uploading #628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8b8092
fix: use sanitized file name in eDoc, refactor ManageDocument2Action,…
sebastian-j-ibanez Sep 24, 2025
42eba16
fix: update ManageDocument2Action, use failure.jsp instead of error.j…
sebastian-j-ibanez Sep 24, 2025
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
14 changes: 5 additions & 9 deletions src/main/java/ca/openosp/openo/documentManager/actions/AddEditDocument2Action.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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;
Expand Down Expand Up @@ -251,17 +247,17 @@ 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());
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
newDoc.setDocPublic(this.getDocPublic());

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);
Expand Down Expand Up @@ -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);
Comment thread
sebastian-j-ibanez marked this conversation as resolved.
updateFileContent = true; // set update to true
}
}
Expand Down
134 changes: 114 additions & 20 deletions src/main/java/ca/openosp/openo/documentManager/actions/ManageDocument2Action.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -137,10 +137,25 @@ 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";
}
}

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);
addActionError("Invalid request. The requested operation could not be performed.");
return "error";
}

// Functional interface for our handlers.
Expand Down Expand Up @@ -311,12 +326,22 @@ 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");
addActionError("Document ID is missing. 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");
Expand All @@ -338,6 +363,10 @@ 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);
addActionError("Invalid document ID format. Please check the document ID and try again.");
return "error";
} catch (Exception e) {
MiscUtils.getLogger().error("Error", e);
}
Expand All @@ -356,19 +385,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");
Expand Down Expand Up @@ -615,6 +654,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"))) {
Expand Down Expand Up @@ -821,6 +870,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();
Expand Down Expand Up @@ -947,6 +999,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)) {
Expand Down Expand Up @@ -980,12 +1038,40 @@ 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._-]", "");

// 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);
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
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);
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
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);
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
String canonicalDest = finalDestFile.getCanonicalPath();
File saveDir = new File(savePath);
String canonicalSaveDir = saveDir.getCanonicalPath();

Expand All @@ -999,7 +1085,15 @@ 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 - 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 {

newDoc.setContentType("application/pdf");
Expand Down
1 change: 1 addition & 0 deletions src/main/webapp/WEB-INF/classes/struts.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1972,6 +1972,7 @@
<result name="success">/documentManager/undocument.jsp</result>
<result name="displaySingleDoc">/documentManager/MultiPageDocDisplay.jsp</result>
<result name="nextIncomingDoc">/documentManager/incomingDocs.jsp</result>
<result name="error">/failure.jsp</result>
</action>
<action name="DocumentDescriptionTemplate" class="ca.openosp.openo.provider.web.DocumentDescriptionTemplate2Action"/>
<action name="PrinterList" class="ca.openosp.openo.printer.PrinterList2Action"/>
Expand Down
Loading