Alon Bar-Lev has posted comments on this change.
Change subject: userportal,webadmin: fix comment from branding patch
......................................................................
Patch Set 3: (2 inline comments)
It is working and what I write is minor...
I just was surprise to this this in code...
You assume a filesystem structure of:
base/BRANDING_PACKAGE/path
While in future the BRANDING_PACKAGE may be moved or exist in several
locations... the fact that the above is true now does not mean that it will
continue to be correct.
What is correct is that the path is relative to the branding package location.
What I suggested is to provide the BRANDING_PACKAGE name as a separate query
parameter.
But this is not that important for now, and can be changed at any point in
future.
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/BrandingServlet.java
Line 101: * @param brandingRootPath The path to the root of the branding.
Cannot be null
Line 102: * @param path The path to translate.
Line 103: * @return A full absolute path for the passed in path.
Line 104: */
Line 105: String getFullPath(final File brandingRootPath, final String
path) {
Why do you keep string?
Line 106: String result = null;
Line 107: String mergedPath =
FileSystems.getDefault().getPath(brandingRootPath.getAbsolutePath(),
Line 108: path == null ? "": path).toString();
Line 109: if (path != null && ServletUtils.isSane(mergedPath)) {
Line 105: String getFullPath(final File brandingRootPath, final String
path) {
Line 106: String result = null;
Line 107: String mergedPath =
FileSystems.getDefault().getPath(brandingRootPath.getAbsolutePath(),
Line 108: path == null ? "": path).toString();
Line 109: if (path != null && ServletUtils.isSane(mergedPath)) {
why is the isSane and not what I suggested? The following is working for me:
String baseS = "/tmp/a/b/c/d";
String rightS = "../d/alon/balon/galon";
Path base = Paths.get(baseS);
Path right = Paths.get(rightS);
if (right.isAbsolute()) {
throw new GeneralSecurityException("Invalid path");
}
right = base.resolve(right);
if (!right.normalize().startsWith(base.normalize())) {
throw new GeneralSecurityException("Invalid path");
}
Line 110: // Return a result relative to the branding root path.
Line 111: result = mergedPath;
Line 112: } else {
Line 113: log.error("The path \"" + mergedPath + "\" is not sane");
//$NON-NLS-1$ //$NON-NLS-2$
--
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: 3
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
