Alon Bar-Lev has posted comments on this change.

Change subject: userportal,webadmin: fix comment from branding patch
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File 
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/ServletUtils.java
Line 196:         if (path != null) {
Line 197:             // Make sure path never begins with a '/'
Line 198:             mergedPath += path.startsWith("/") ? path.substring(1) : 
path;
Line 199:         }
Line 200:         return mergedPath;
What is the difference between this and new File(base, path)?

As base is from configuration, and path from the user we should check the path 
to not containing '..' or special characters.

Probably the following will work, no?

 Path basePath = FileSystems.getDefault().getPath(base).normalize();
 Path pathPath = FileSystems.getDefault().getPath(base, path).normalize();
 if (!pathPath.startsWith(basePath) {
     throw new SecurityException(...);
 }
Line 201:     }


....................................................
File 
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/BrandingServlet.java
Line 48:         // Get the requested path:
Line 49:         String path = request.getPathInfo();
Line 50: 
Line 51:         // Locate the requested file:
Line 52:         String fullPath = 
getFullPath(brandingManager.getBrandingRootPath().getAbsolutePath(), path);
I think that using Path or File all over is best than strings.
Line 53:         if (fullPath != null) {
Line 54:             File file = new File(fullPath);
Line 55:             if (!file.exists() || !file.canRead() || 
file.isDirectory()) {
Line 56:                 log.error("Unable to retrieve file: " + 
file.getAbsolutePath() //$NON-NLS-1$


-- 
To view, visit http://gerrit.ovirt.org/15702
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fad1cda014713724f294fe8454fb5f36870c137
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to