You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@openmeetings.apache.org by so...@apache.org on 2026/10/22 15:08:07 UTC
(openmeetings) branch master updated: [OPENMEETINGS-2829] OM user pictureUri is properly validated
This is an automated email from the ASF dual-hosted git repository.
solomax pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/openmeetings.git
The following commit(s) were added to refs/heads/master by this push:
new 725ac73a2 [OPENMEETINGS-2829] OM user pictureUri is properly validated
725ac73a2 is described below
commit 725ac73a2b6d07317e97e7b252f936a8e00ce00f
Author: Maxim Solodovnik <so...@apache.org>
AuthorDate: Thu Oct 22 22:07:54 2026 +0700
[OPENMEETINGS-2829] OM user pictureUri is properly validated
---
.../openmeetings/db/util/ApplicationHelper.java | 17 ++++-
.../apache/openmeetings/web/room/RoomPanel.java | 21 ++++--
.../openmeetings/web/user/rooms/RoomsPanel.java | 40 ++---------
.../web/util/ProfileImageResourceReference.java | 67 +++++++++++++-----
.../webapp/WEB-INF/classes/openmeetings.properties | 3 +
.../openmeetings/web/util/TestProfileImage.java | 82 ++++++++++++++++++++++
6 files changed, 171 insertions(+), 59 deletions(-)
diff --git a/openmeetings-db/src/main/java/org/apache/openmeetings/db/util/ApplicationHelper.java b/openmeetings-db/src/main/java/org/apache/openmeetings/db/util/ApplicationHelper.java
index 8c3db65f8..bfe6e4113 100644
--- a/openmeetings-db/src/main/java/org/apache/openmeetings/db/util/ApplicationHelper.java
+++ b/openmeetings-db/src/main/java/org/apache/openmeetings/db/util/ApplicationHelper.java
@@ -24,7 +24,11 @@ import static org.apache.openmeetings.util.OpenmeetingsVariables.isInitComplete;
import static org.springframework.web.context.WebApplicationContext.ROOT_WEB_APPLICATION_CONTEXT_ATTRIBUTE;
import static org.springframework.web.context.support.WebApplicationContextUtils.getWebApplicationContext;
+import java.io.IOException;
import java.lang.reflect.InvocationTargetException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
import java.util.Enumeration;
import java.util.Map;
import java.util.UUID;
@@ -58,6 +62,7 @@ import org.springframework.web.context.support.XmlWebApplicationContext;
public class ApplicationHelper {
private static final Logger log = LoggerFactory.getLogger(ApplicationHelper.class);
private static final Object SYNC_OBJ = new Object();
+ private static final String TEMP_PREFIX = "_OM-temp-test_";
private ApplicationHelper() {}
@@ -68,7 +73,7 @@ public class ApplicationHelper {
try {
app = (WebApplication)getAppClass().getDeclaredConstructor().newInstance();
app.setName(String.format("--%s--", UUID.randomUUID())); //temporary name for temporary application
- final ServletContext sc = new MockServletContext(app, null);
+ final ServletContext sc = new MockServletContext(app, Files.createTempDirectory(TEMP_PREFIX).toString());
XmlWebApplicationContext xmlContext = new XmlWebApplicationContext();
xmlContext.setConfigLocation("classpath:applicationContext.xml");
xmlContext.setServletContext(sc);
@@ -109,7 +114,7 @@ public class ApplicationHelper {
};
app.setWicketFilter(filter);
}
- } catch (InstantiationException | IllegalAccessException | ClassNotFoundException | InvocationTargetException | NoSuchMethodException e) {
+ } catch (InstantiationException | IllegalAccessException | ClassNotFoundException | InvocationTargetException | NoSuchMethodException | IOException e) {
log.error("Failed to create Application");
}
}
@@ -178,6 +183,14 @@ public class ApplicationHelper {
public static void destroyApplication() {
WebApplication app = (WebApplication)Application.get(getWicketApplicationName());
+ Path root = Paths.get(app.getServletContext().getRealPath("/"));
+ if (root.getFileName().startsWith(TEMP_PREFIX)) {
+ try {
+ Files.deleteIfExists(root);
+ } catch (IOException e) {
+ log.error("Failed to delete temp root");
+ }
+ }
WebApplicationContext ctx = getWebApplicationContext(app.getServletContext());
app.internalDestroy(); //need to be called too
if (ctx != null) {
diff --git a/openmeetings-web/src/main/java/org/apache/openmeetings/web/room/RoomPanel.java b/openmeetings-web/src/main/java/org/apache/openmeetings/web/room/RoomPanel.java
index 1436d30eb..a563c04bd 100644
--- a/openmeetings-web/src/main/java/org/apache/openmeetings/web/room/RoomPanel.java
+++ b/openmeetings-web/src/main/java/org/apache/openmeetings/web/room/RoomPanel.java
@@ -598,7 +598,10 @@ public class RoomPanel extends BasePanel {
return;
}
if (!curClient.getUid().equals(c.getUid())) {
- handler.appendJavaScript(String.format("if (typeof(VideoManager) !== 'undefined') {VideoManager.mute('%s', %s);}", obj.getString("uid"), obj.getBoolean("mute")));
+ handler.appendJavaScript(String.format("""
+ if (typeof(VideoManager) !== 'undefined') {
+ VideoManager.mute('%s', %s);
+ }""", obj.getString("uid"), obj.getBoolean("mute")));
}
}
@@ -609,7 +612,10 @@ public class RoomPanel extends BasePanel {
// no luck
return;
}
- handler.appendJavaScript(String.format("if (typeof(VideoManager) !== 'undefined') {VideoManager.muteOthers('%s');}", uid));
+ handler.appendJavaScript(String.format("""
+ if (typeof(VideoManager) !== 'undefined') {
+ VideoManager.muteOthers('%s');
+ }""", uid));
}
private void onWbPutFile(TextRoomMessage m) {
@@ -629,11 +635,18 @@ public class RoomPanel extends BasePanel {
Client curClient = getClient();
if (r.isInterview() && curClient.hasRight(Right.MODERATOR)) {
if (streamProcessor.isRecording(r.getId())) {
- handler.appendJavaScript("if (typeof(WbArea) === 'object') {WbArea.setRecStarted(true);}");
+ handler.appendJavaScript("""
+ if (typeof(WbArea) === 'object') {
+ WbArea.setRecStarted(true);
+ }""");
} else if (streamProcessor.recordingAllowed(getClient())) {
boolean hasStreams = cm.streamByRoom(r.getId())
.anyMatch(cl -> !cl.getStreams().isEmpty());
- handler.appendJavaScript(String.format("if (typeof(WbArea) === 'object') {WbArea.setRecStarted(false);WbArea.setRecEnabled(%s);}", hasStreams));
+ handler.appendJavaScript(String.format("""
+ if (typeof(WbArea) === 'object') {
+ WbArea.setRecStarted(false);
+ WbArea.setRecEnabled(%s);
+ }""", hasStreams));
}
}
}
diff --git a/openmeetings-web/src/main/java/org/apache/openmeetings/web/user/rooms/RoomsPanel.java b/openmeetings-web/src/main/java/org/apache/openmeetings/web/user/rooms/RoomsPanel.java
index 6e6275bd4..6125b7bdd 100644
--- a/openmeetings-web/src/main/java/org/apache/openmeetings/web/user/rooms/RoomsPanel.java
+++ b/openmeetings-web/src/main/java/org/apache/openmeetings/web/user/rooms/RoomsPanel.java
@@ -18,18 +18,14 @@
*/
package org.apache.openmeetings.web.user.rooms;
-import java.io.File;
-import java.io.FileInputStream;
-import java.io.InputStream;
import java.util.List;
import org.apache.openmeetings.db.dao.room.RoomDao;
-import org.apache.openmeetings.db.dao.user.UserDao;
import org.apache.openmeetings.db.entity.basic.Client;
import org.apache.openmeetings.db.entity.room.Room;
import org.apache.openmeetings.web.app.ClientManager;
import org.apache.openmeetings.web.common.UserPanel;
-import org.apache.openmeetings.util.OmFileHelper;
+import org.apache.openmeetings.web.util.ProfileImageResourceReference;
import org.apache.wicket.ajax.AjaxRequestTarget;
import org.apache.wicket.markup.html.WebMarkupContainer;
import org.apache.wicket.markup.html.basic.Label;
@@ -37,9 +33,6 @@ import org.apache.wicket.markup.html.image.Image;
import org.apache.wicket.markup.html.list.ListItem;
import org.apache.wicket.markup.html.list.ListView;
import org.apache.wicket.model.Model;
-import org.apache.wicket.request.resource.ByteArrayResource;
-
-import org.apache.wicket.util.io.IOUtils;
import jakarta.inject.Inject;
@@ -55,8 +48,6 @@ public class RoomsPanel extends UserPanel {
private final List<Room> rooms;
private Long roomId = 0L;
- @Inject
- private UserDao userDao;
@Inject
private ClientManager cm;
@Inject
@@ -71,32 +62,9 @@ public class RoomsPanel extends UserPanel {
@Override
protected void populateItem(final ListItem<Client> item) {
Client client = item.getModelObject();
- final Long userId = client.getUserId();
- item.add(new Image("clientImage", new ByteArrayResource("image/png") {
- private static final long serialVersionUID = 1L;
-
- @Override
- protected ResourceResponse newResourceResponse(Attributes attributes) {
- ResourceResponse rr = super.newResourceResponse(attributes);
- rr.disableCaching();
- return rr;
- }
-
- @Override
- protected byte[] getData(Attributes attributes) {
- String uri = null;
- if (userId != null) {
- uri = userDao.get(userId > 0 ? userId : -userId).getPictureUri();
- }
- File img = OmFileHelper.getUserProfilePicture(userId, uri);
- try (InputStream is = new FileInputStream(img)) {
- return IOUtils.toByteArray(is);
- } catch (Exception e) {
- //no-op
- }
- return null;
- }
- }));
+ item.add(new Image("clientImage",
+ new ProfileImageResourceReference(),
+ ProfileImageResourceReference.getImgParams(client.getUser())));
item.add(new Label("clientLogin", client.getUser().getLogin()));
item.add(new Label("from", client.getConnectedSince()));
}
diff --git a/openmeetings-web/src/main/java/org/apache/openmeetings/web/util/ProfileImageResourceReference.java b/openmeetings-web/src/main/java/org/apache/openmeetings/web/util/ProfileImageResourceReference.java
index 34397c17e..75bcfd7dd 100644
--- a/openmeetings-web/src/main/java/org/apache/openmeetings/web/util/ProfileImageResourceReference.java
+++ b/openmeetings-web/src/main/java/org/apache/openmeetings/web/util/ProfileImageResourceReference.java
@@ -37,19 +37,38 @@ import jakarta.inject.Inject;
import jakarta.servlet.http.HttpServletResponse;
import static org.apache.openmeetings.util.OmFileHelper.PNG_MIME_TYPE;
-import static org.apache.openmeetings.util.OmFileHelper.SIP_USER_ID;
import java.io.File;
import java.io.FileInputStream;
import java.io.InputStream;
import java.net.URI;
+import java.net.URL;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.Set;
public class ProfileImageResourceReference extends ResourceReference {
private static final long serialVersionUID = 1L;
private static final Logger log = LoggerFactory.getLogger(ProfileImageResourceReference.class);
+ private static final Set<String> ALLOWED_SCHEMAS = new HashSet<>();
+ private static final URI EMPTY_URI = URI.create("");
@Inject
private UserDao userDao;
+ static {
+ Properties props = new Properties();
+ try (final InputStream is = ProfileImageResourceReference.class.getResourceAsStream("/openmeetings.properties")) {
+ props.load(is);
+ } catch (Exception e) {
+ log.error("Failed to load openmeetings.properties {}", e.getMessage());
+ }
+ String[] schemas = props.getProperty("valid.avatar.schemas", "https").split(",");
+ for (String schema : schemas) {
+ ALLOWED_SCHEMAS.add(schema);
+ }
+ }
+
public ProfileImageResourceReference() {
super(ProfileImageResourceReference.class, "profile");
Injector.get().inject(this);
@@ -59,24 +78,38 @@ public class ProfileImageResourceReference extends ResourceReference {
return getUrl(rc, userDao.get(userId));
}
+ public static PageParameters getImgParams(User u) {
+ return new PageParameters()
+ .add("id", u.getId())
+ .add("anticache", Optional.ofNullable(u.getUpdated())
+ .orElse(u.getInserted()).getTime());
+ }
+
public static String getUrl(RequestCycle rc, User u) {
- String uri = u.getPictureUri();
- if (isRelative(uri)) {
- File img = OmFileHelper.getUserProfilePicture(u.getId(), uri);
- uri = rc.urlFor(new ProfileImageResourceReference()
- , new PageParameters().add("id", u.getId()).add("anticache", img.lastModified())).toString();
- }
- return uri;
+ URI uri = sanitize(u);
+ return !uri.isAbsolute()
+ ? rc.urlFor(new ProfileImageResourceReference(), getImgParams(u)).toString()
+ : uri.toString();
}
- private static boolean isRelative(String uri) {
- boolean relative = true;
+ //package private for testing
+ static URI sanitize(User u) {
+ URI uri = EMPTY_URI;
try {
- relative = !URI.create(uri).isAbsolute();
+ uri = u == null ? EMPTY_URI : URI.create(u.getPictureUri());
+ if (!uri.isAbsolute()) {
+ return uri;
+ }
+ if (!ALLOWED_SCHEMAS.contains(uri.getScheme())) {
+ return EMPTY_URI;
+ }
+ //let's check if we can construct URL out of this
+ URL url = uri.toURL();
+ uri = url.toURI().normalize();
} catch (Exception e) {
- //no-op
+ uri = EMPTY_URI;
}
- return relative;
+ return uri;
}
@Override
@@ -84,7 +117,7 @@ public class ProfileImageResourceReference extends ResourceReference {
return new ByteArrayResource(PNG_MIME_TYPE) {
private static final long serialVersionUID = 1L;
private Long userId = null;
- private String uri = null;
+ private URI uri = null;
@Override
protected ResourceResponse newResourceResponse(Attributes attributes) {
@@ -93,7 +126,7 @@ public class ProfileImageResourceReference extends ResourceReference {
PageParameters params = attributes.getParameters();
try {
userId = params.get("id").toOptionalLong();
- uri = SIP_USER_ID.equals(userId) ? null : userDao.get(userId).getPictureUri();
+ uri = sanitize(userDao.get(userId));
} catch (Exception e) {
// no-op, junk filter
}
@@ -109,8 +142,8 @@ public class ProfileImageResourceReference extends ResourceReference {
@Override
protected byte[] getData(Attributes attributes) {
- if (isRelative(uri)) {
- File img = OmFileHelper.getUserProfilePicture(userId, uri);
+ if (!uri.isAbsolute()) {
+ File img = OmFileHelper.getUserProfilePicture(userId, uri.toString());
try (InputStream is = new FileInputStream(img)) {
return IOUtils.toByteArray(is);
} catch (Exception e) {
diff --git a/openmeetings-web/src/main/webapp/WEB-INF/classes/openmeetings.properties b/openmeetings-web/src/main/webapp/WEB-INF/classes/openmeetings.properties
index 17aac4feb..505b745f0 100644
--- a/openmeetings-web/src/main/webapp/WEB-INF/classes/openmeetings.properties
+++ b/openmeetings-web/src/main/webapp/WEB-INF/classes/openmeetings.properties
@@ -89,3 +89,6 @@ otp.issuer=
otp.ntp.server=pool.ntp.org
## milliseconds
otp.ntp.timeout=3000
+
+## valid schemas for external avatar URLs, comma separated
+valid.avatar.schemas=https
diff --git a/openmeetings-web/src/test/java/org/apache/openmeetings/web/util/TestProfileImage.java b/openmeetings-web/src/test/java/org/apache/openmeetings/web/util/TestProfileImage.java
new file mode 100644
index 000000000..8f3d9edb6
--- /dev/null
+++ b/openmeetings-web/src/test/java/org/apache/openmeetings/web/util/TestProfileImage.java
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License") + you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.openmeetings.web.util;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Date;
+import java.util.List;
+
+import org.apache.openmeetings.db.entity.user.User;
+import org.apache.openmeetings.db.util.ApplicationHelper;
+import org.apache.openmeetings.util.OpenmeetingsVariables;
+import org.apache.openmeetings.web.IsolatedTest;
+import org.apache.wicket.request.cycle.RequestCycle;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+@IsolatedTest
+class TestProfileImage {
+ @BeforeAll
+ static void setup() {
+ OpenmeetingsVariables.setWicketApplicationName("TestProfileImage");
+ ApplicationHelper.ensureRequestCycle(ApplicationHelper.ensureApplication());
+ }
+
+ @AfterAll
+ static void teardown() {
+ ApplicationHelper.destroyApplication();
+ }
+
+ private static List<String> invalidURLs() {
+ return List.of("../../../abracadabra"
+ , "/etc/hosts"
+ , "file:///etc/hosts"
+ , "ftp://username:password@ftp.domain.ext/folder/abracadabra"
+ , "smb://video/abracadabra"
+ , "\\\\video/abracadabra"
+ , "javascript:alert(document.cookie)"
+ , "emailto:jane@doe");
+ }
+
+ @ParameterizedTest
+ @MethodSource("invalidURLs")
+ void testSanitize(String url) {
+ User u = new User();
+ u.setId(666L);
+ u.setInserted(new Date());
+ u.setPictureUri(url);
+ assertFalse(ProfileImageResourceReference.sanitize(u).isAbsolute()
+ , "Invalid URL should be treated as relative");
+ }
+
+ @ParameterizedTest
+ @MethodSource("invalidURLs")
+ void testProfilePicture(String url) {
+ User u = new User();
+ u.setId(666L);
+ u.setInserted(new Date());
+ u.setPictureUri(url);
+ assertTrue(ProfileImageResourceReference.getUrl(RequestCycle.get(), u).startsWith("./profile/666")
+ , "There should be standard picture URL");
+ }
+}