-
Notifications
You must be signed in to change notification settings - Fork 220
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.
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
Knowage 8043 #891
Knowage 8043 #891
Changes from 3 commits
da96b6a
997e9ab
e3c6a3c
aaa3d09
8a66b78
09ea635
fd75ee0
c560439
0fab752
2284ba0
f302cdb
b168ec2
9bb7574
71231ad
b7ecf4e
c0dc7cc
f0f1db5
45a0eff
6520e49
ca94b0d
fe68d3d
6d9fa38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,8 @@ | |
import org.json.JSONException; | ||
import org.json.JSONObject; | ||
|
||
import it.eng.knowage.commons.security.PathTraversalChecker; | ||
import it.eng.knowage.commons.security.exceptions.PathTraversalAttackException; | ||
import it.eng.spagobi.commons.SingletonConfig; | ||
import it.eng.spagobi.commons.bo.UserProfile; | ||
import it.eng.spagobi.commons.services.AbstractSpagoBIAction; | ||
|
@@ -107,6 +109,11 @@ public void doService() { | |
// checks for path traversal attacks | ||
private void checkRequiredFile(String fileName) { | ||
File targetDirectory = GeneralUtilities.getPreviewFilesStorageDirectoryPath(); | ||
try { | ||
PathTraversalChecker.get(fileName, targetDirectory.getName()); | ||
} catch (Exception e) { | ||
throw new PathTraversalAttackException("Error managing preview file for file: " + fileName); | ||
} | ||
FileUtils.checkPathTraversalAttack(fileName, targetDirectory); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this method and use PathTraversalChecker instead |
||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,6 @@ | |
import java.io.File; | ||
import java.io.FileOutputStream; | ||
import java.nio.file.Files; | ||
import java.nio.file.Paths; | ||
import java.text.SimpleDateFormat; | ||
import java.util.ArrayList; | ||
import java.util.Calendar; | ||
|
@@ -131,16 +130,13 @@ public Response getresourcePath(@QueryParam("templateName") String fileName, @Qu | |
String separator = File.separator; | ||
if (fileName.endsWith("?")) | ||
fileName = fileName.substring(0, fileName.length() - 1); | ||
|
||
java.nio.file.Path outputPath = Paths.get(SpagoBIUtilities.getResourcePath(), "dossier", String.valueOf(documentId), fileName); | ||
File file = outputPath.toFile(); | ||
String outPath = SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId + separator + fileName; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use PathTraversalChecker.get method: |
||
ResponseBuilder responseBuilder = null; | ||
|
||
File file = new File(outPath); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. merge this with the above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
JSONObject response = new JSONObject(); | ||
File dossierDir = new File(SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId + separator); | ||
try { | ||
PathTraversalChecker.isValidFileName(fileName); | ||
PathTraversalChecker.preventPathTraversalAttack(file, dossierDir); | ||
PathTraversalChecker.get(fileName, dossierDir.getName()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. inputs reversed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
byte[] bytes = Files.readAllBytes(file.toPath()); | ||
responseBuilder = Response.ok(bytes); | ||
responseBuilder.header("Content-Disposition", "attachment; filename=" + fileName); | ||
|
@@ -171,8 +167,7 @@ public Response checkPathFile(@QueryParam("templateName") String fileName, @Quer | |
File dossierDir = new File(SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId + separator); | ||
JSONObject response = new JSONObject(); | ||
try { | ||
PathTraversalChecker.isValidFileName(fileName); | ||
PathTraversalChecker.preventPathTraversalAttack(file, dossierDir); | ||
PathTraversalChecker.get(fileName, dossierDir.getName()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. inputs reversed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
Files.readAllBytes(file.toPath()); | ||
response.put("STATUS", "OK"); | ||
} catch (Exception e) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,7 @@ | |
import com.fasterxml.jackson.databind.JsonMappingException; | ||
|
||
import it.eng.knowage.commons.security.PathTraversalChecker; | ||
import it.eng.knowage.commons.security.exceptions.PathTraversalAttackException; | ||
import it.eng.qbe.dataset.QbeDataSet; | ||
import it.eng.spago.base.SourceBeanException; | ||
import it.eng.spago.error.EMFInternalError; | ||
|
@@ -1621,9 +1622,18 @@ private void renameAndMoveDatasetFile(String originalFileName, String newFileNam | |
String filePath = resourcePath + File.separatorChar + "dataset" + File.separatorChar + "files" + File.separatorChar + "temp" + File.separatorChar; | ||
String fileNewPath = resourcePath + File.separatorChar + "dataset" + File.separatorChar + "files" + File.separatorChar; | ||
|
||
File originalDatasetFile = new File(filePath + originalFileName); | ||
File newDatasetFile = new File(fileNewPath + newFileName + "." + fileType.toLowerCase()); | ||
if (originalDatasetFile.exists()) { | ||
File originalDatasetFile = null; | ||
File newDatasetFile = null; | ||
try { | ||
PathTraversalChecker.get(filePath, originalFileName); | ||
originalDatasetFile = new File(filePath + originalFileName); | ||
PathTraversalChecker.get(fileNewPath, newFileName + "." + fileType.toLowerCase()); | ||
newDatasetFile = new File(fileNewPath + newFileName + "." + fileType.toLowerCase()); | ||
} catch (Exception e) { | ||
throw new PathTraversalAttackException( | ||
"Error getting removind/moving dataset file for orginal file " + originalFileName + " and new file " + newFileName); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this part |
||
if (newDatasetFile != null && originalDatasetFile != null && originalDatasetFile.exists()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how files can be null There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
/* | ||
* This method copies the contents of the specified source file to the specified destination file. The directory holding the destination file is | ||
* created if it does not exist. If the destination file exists, then this method will overwrite it. | ||
|
@@ -1741,8 +1751,15 @@ private JSONObject getCkanDataSetConfig(SelfServiceDataSetDTO selfServiceDataSet | |
private void deleteDatasetFile(String fileName, String resourcePath, String fileType) { | ||
String filePath = resourcePath + File.separatorChar + "dataset" + File.separatorChar + "files" + File.separatorChar + "temp" + File.separatorChar; | ||
|
||
File datasetFile = new File(filePath + fileName); | ||
if (datasetFile.exists()) { | ||
File datasetFile = null; | ||
try { | ||
PathTraversalChecker.get(filePath, fileName); | ||
datasetFile = new File(filePath + fileName); | ||
} catch (Exception e) { | ||
throw new PathTraversalAttackException("Error deleting dataset file " + fileName); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove this catch block |
||
|
||
if (datasetFile != null && datasetFile.exists()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how files can be null There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
datasetFile.delete(); | ||
} | ||
} | ||
|
@@ -1796,10 +1813,7 @@ private Integer getCategoryCode(String category) { | |
|
||
try { | ||
ICategoryDAO categoryDao = DAOFactory.getCategoryDAO(); | ||
categories = categoryDao.getCategoriesForDataset() | ||
.stream() | ||
.map(Domain::fromCategory) | ||
.collect(toList()); | ||
categories = categoryDao.getCategoriesForDataset().stream().map(Domain::fromCategory).collect(toList()); | ||
} catch (Throwable t) { | ||
throw new SpagoBIRuntimeException("An unexpected error occured while loading categories types from database", t); | ||
} | ||
|
@@ -2174,10 +2188,7 @@ protected List<Integer> getCategories(IEngUserProfile profile) { | |
ICategoryDAO categoryDao = DAOFactory.getCategoryDAO(); | ||
|
||
// TODO : Makes sense? | ||
List<Domain> dialects = categoryDao.getCategoriesForDataset() | ||
.stream() | ||
.map(Domain::fromCategory) | ||
.collect(toList()); | ||
List<Domain> dialects = categoryDao.getCategoriesForDataset().stream().map(Domain::fromCategory).collect(toList()); | ||
if (dialects == null || dialects.size() == 0) { | ||
return null; | ||
} | ||
|
@@ -2191,10 +2202,7 @@ protected List<Integer> getCategories(IEngUserProfile profile) { | |
|
||
List<RoleMetaModelCategory> aRoleCategories = roledao.getMetaModelCategoriesForRole(role.getId()); | ||
List<RoleMetaModelCategory> resp = new ArrayList<>(); | ||
List<Domain> array = categoryDao.getCategoriesForDataset() | ||
.stream() | ||
.map(Domain::fromCategory) | ||
.collect(toList()); | ||
List<Domain> array = categoryDao.getCategoriesForDataset().stream().map(Domain::fromCategory).collect(toList()); | ||
for (RoleMetaModelCategory r : aRoleCategories) { | ||
for (Domain dom : array) { | ||
if (r.getCategoryId().equals(dom.getValueId())) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,8 @@ | |
import org.apache.log4j.Logger; | ||
import org.json.JSONObject; | ||
|
||
import it.eng.knowage.commons.security.PathTraversalChecker; | ||
import it.eng.knowage.commons.security.exceptions.PathTraversalAttackException; | ||
import it.eng.qbe.dataset.QbeDataSet; | ||
import it.eng.spago.base.SourceBean; | ||
import it.eng.spago.base.SourceBeanException; | ||
|
@@ -228,8 +230,15 @@ public ObjTemplate fromSDKTemplateToObjTemplate(SDKTemplate sdkTemplate) { | |
} | ||
if (dh != null && dh.getName() != null) { | ||
LOGGER.debug("Deleting attachment file ..."); | ||
File attachment = new File(dh.getName()); | ||
if (attachment.exists() && attachment.isFile()) { | ||
String fileName = dh.getName(); | ||
File attachment = null; | ||
try { | ||
PathTraversalChecker.isValidFileName(fileName); | ||
attachment = new File(fileName); | ||
} catch (Exception e) { | ||
throw new PathTraversalAttackException("Error converting SDKTemplate " + sdkTemplate + ". File name " + fileName + " is not valid."); | ||
} | ||
if (attachment != null && attachment.exists() && attachment.isFile()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how files can be null There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
boolean attachmentFileDeleted; | ||
try { | ||
attachmentFileDeleted = Files.deleteIfExists(attachment.toPath()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PathTraversalChecker should throw only PathTraversalAttackException exception, that is runtime exception, so we can remove the try catch bloch