From 2fda45f67aa61aaa0107496db83d0a62a2340287 Mon Sep 17 00:00:00 2001 From: Robin Shen Date: Thu, 8 Sep 2022 22:07:27 +0800 Subject: [PATCH] Fix issue #912 - Attachment in confidential issue can be accessed by unauthorized users if issue uuid is leaked --- .../server/entitymanager/BuildManager.java | 3 +++ .../entitymanager/CodeCommentManager.java | 3 +++ .../impl/DefaultBuildManager.java | 9 +++++++ .../impl/DefaultCodeCommentManager.java | 8 +++++++ .../io/onedev/server/model/CodeComment.java | 4 ++-- .../java/io/onedev/server/model/Issue.java | 2 -- .../io/onedev/server/model/Milestone.java | 2 -- .../java/io/onedev/server/model/Project.java | 2 -- .../io/onedev/server/model/PullRequest.java | 2 -- .../web/resource/AttachmentResource.java | 24 +++++++++++++++---- 10 files changed, 44 insertions(+), 15 deletions(-) diff --git a/server-core/src/main/java/io/onedev/server/entitymanager/BuildManager.java b/server-core/src/main/java/io/onedev/server/entitymanager/BuildManager.java index f59d0e0ecf..4f1e4146e0 100644 --- a/server-core/src/main/java/io/onedev/server/entitymanager/BuildManager.java +++ b/server-core/src/main/java/io/onedev/server/entitymanager/BuildManager.java @@ -27,6 +27,9 @@ public interface BuildManager extends EntityManager { @Nullable Build find(String buildFQN); + + @Nullable + Build findByUUID(String uuid); @Nullable Build find(ProjectScopedNumber buildFQN); diff --git a/server-core/src/main/java/io/onedev/server/entitymanager/CodeCommentManager.java b/server-core/src/main/java/io/onedev/server/entitymanager/CodeCommentManager.java index 36d21754bc..ffd523629d 100644 --- a/server-core/src/main/java/io/onedev/server/entitymanager/CodeCommentManager.java +++ b/server-core/src/main/java/io/onedev/server/entitymanager/CodeCommentManager.java @@ -31,4 +31,7 @@ public interface CodeCommentManager extends EntityManager { void delete(Collection comments); + @Nullable + CodeComment findByUUID(String uuid); + } \ No newline at end of file diff --git a/server-core/src/main/java/io/onedev/server/entitymanager/impl/DefaultBuildManager.java b/server-core/src/main/java/io/onedev/server/entitymanager/impl/DefaultBuildManager.java index 36b278babc..e368142286 100644 --- a/server-core/src/main/java/io/onedev/server/entitymanager/impl/DefaultBuildManager.java +++ b/server-core/src/main/java/io/onedev/server/entitymanager/impl/DefaultBuildManager.java @@ -63,6 +63,7 @@ import io.onedev.server.model.BuildDependence; import io.onedev.server.model.BuildParam; import io.onedev.server.model.Group; import io.onedev.server.model.GroupAuthorization; +import io.onedev.server.model.Issue; import io.onedev.server.model.Project; import io.onedev.server.model.PullRequest; import io.onedev.server.model.Role; @@ -1016,5 +1017,13 @@ public class DefaultBuildManager extends BaseEntityManager implements Bui return getSession().createQuery(criteriaQuery).getResultList(); } } + + @Override + public Build findByUUID(String uuid) { + EntityCriteria criteria = newCriteria(); + criteria.add(Restrictions.eq(Issue.PROP_UUID, uuid)); + criteria.setCacheable(true); + return find(criteria); + } } diff --git a/server-core/src/main/java/io/onedev/server/entitymanager/impl/DefaultCodeCommentManager.java b/server-core/src/main/java/io/onedev/server/entitymanager/impl/DefaultCodeCommentManager.java index cd89aaea3f..8449d33b3d 100644 --- a/server-core/src/main/java/io/onedev/server/entitymanager/impl/DefaultCodeCommentManager.java +++ b/server-core/src/main/java/io/onedev/server/entitymanager/impl/DefaultCodeCommentManager.java @@ -303,4 +303,12 @@ public class DefaultCodeCommentManager extends BaseEntityManager im delete(comment); } + @Override + public CodeComment findByUUID(String uuid) { + EntityCriteria criteria = newCriteria(); + criteria.add(Restrictions.eq(CodeComment.PROP_UUID, uuid)); + criteria.setCacheable(true); + return find(criteria); + } + } diff --git a/server-core/src/main/java/io/onedev/server/model/CodeComment.java b/server-core/src/main/java/io/onedev/server/model/CodeComment.java index 85b1f8d61f..8fb88c7258 100644 --- a/server-core/src/main/java/io/onedev/server/model/CodeComment.java +++ b/server-core/src/main/java/io/onedev/server/model/CodeComment.java @@ -84,9 +84,9 @@ public class CodeComment extends AbstractEntity implements AttachmentStorageSupp public static final String PROP_REPLIES = "replies"; public static final String PROP_CHANGES = "changes"; - - public static final String PROP_ID = "id"; + public static final String PROP_UUID = "uuid"; + public static final List QUERY_FIELDS = Lists.newArrayList( NAME_CONTENT, NAME_REPLY, NAME_PATH, NAME_CREATE_DATE, NAME_UPDATE_DATE, NAME_REPLY_COUNT); diff --git a/server-core/src/main/java/io/onedev/server/model/Issue.java b/server-core/src/main/java/io/onedev/server/model/Issue.java index c2040eeaff..f939181795 100644 --- a/server-core/src/main/java/io/onedev/server/model/Issue.java +++ b/server-core/src/main/java/io/onedev/server/model/Issue.java @@ -156,8 +156,6 @@ public class Issue extends AbstractEntity implements Referenceable, AttachmentSt public static final String PROP_UUID = "uuid"; - public static final String PROP_ID = "id"; - public static final String PROP_NO_SPACE_TITLE = "noSpaceTitle"; public static final String PROP_CONFIDENTIAL = "confidential"; diff --git a/server-core/src/main/java/io/onedev/server/model/Milestone.java b/server-core/src/main/java/io/onedev/server/model/Milestone.java index 28182fda69..ff61902260 100644 --- a/server-core/src/main/java/io/onedev/server/model/Milestone.java +++ b/server-core/src/main/java/io/onedev/server/model/Milestone.java @@ -39,8 +39,6 @@ public class Milestone extends AbstractEntity { public static final int MAX_DESCRIPTION_LEN = 15000; - public static final String PROP_ID = "id"; - public static final String PROP_PROJECT = "project"; public static final String PROP_NAME = "name"; diff --git a/server-core/src/main/java/io/onedev/server/model/Project.java b/server-core/src/main/java/io/onedev/server/model/Project.java index 617006f839..a56aee77b3 100644 --- a/server-core/src/main/java/io/onedev/server/model/Project.java +++ b/server-core/src/main/java/io/onedev/server/model/Project.java @@ -201,8 +201,6 @@ public class Project extends AbstractEntity { public static final String PROP_DESCRIPTION = "description"; - public static final String PROP_ID = "id"; - public static final String PROP_FORKED_FROM = "forkedFrom"; public static final String PROP_PARENT = "parent"; diff --git a/server-core/src/main/java/io/onedev/server/model/PullRequest.java b/server-core/src/main/java/io/onedev/server/model/PullRequest.java index e17c6da26c..9c8ce4aeda 100644 --- a/server-core/src/main/java/io/onedev/server/model/PullRequest.java +++ b/server-core/src/main/java/io/onedev/server/model/PullRequest.java @@ -152,8 +152,6 @@ public class PullRequest extends AbstractEntity implements Referenceable, Attach public static final String PROP_LAST_MERGE_PREVIEW = "lastMergePreview"; - public static final String PROP_ID = "id"; - public static final String PROP_REVIEWS = "reviews"; public static final String PROP_BUILDS = "builds"; diff --git a/server-core/src/main/java/io/onedev/server/web/resource/AttachmentResource.java b/server-core/src/main/java/io/onedev/server/web/resource/AttachmentResource.java index cae97253db..ff94499ba7 100644 --- a/server-core/src/main/java/io/onedev/server/web/resource/AttachmentResource.java +++ b/server-core/src/main/java/io/onedev/server/web/resource/AttachmentResource.java @@ -16,10 +16,14 @@ import org.apache.wicket.request.mapper.parameter.PageParameters; import org.apache.wicket.request.resource.AbstractResource; import io.onedev.server.OneDev; +import io.onedev.server.entitymanager.BuildManager; +import io.onedev.server.entitymanager.CodeCommentManager; +import io.onedev.server.entitymanager.IssueManager; import io.onedev.server.entitymanager.ProjectManager; import io.onedev.server.entitymanager.PullRequestManager; +import io.onedev.server.model.Build; +import io.onedev.server.model.Issue; import io.onedev.server.model.Project; -import io.onedev.server.model.PullRequest; import io.onedev.server.security.SecurityUtils; import io.onedev.server.storage.AttachmentStorageManager; import io.onedev.server.util.CryptoUtils; @@ -50,11 +54,21 @@ public class AttachmentResource extends AbstractResource { String authorization = params.get(PARAM_AUTHORIZATION).toOptionalString(); if (authorization == null || !new String(CryptoUtils.decrypt(Base64.decodeBase64(authorization)), StandardCharsets.UTF_8).equals(group)) { - PullRequest request = OneDev.getInstance(PullRequestManager.class).findByUUID(group); - if (request != null && !SecurityUtils.canReadCode(project)) - throw new UnauthorizedException(); - else if (!SecurityUtils.canAccess(project)) + Issue issue; + Build build; + if (OneDev.getInstance(PullRequestManager.class).findByUUID(group) != null + || OneDev.getInstance(CodeCommentManager.class).findByUUID(group) != null) { + if (!SecurityUtils.canReadCode(project)) + throw new UnauthorizedException(); + } else if ((issue = OneDev.getInstance(IssueManager.class).findByUUID(group)) != null) { + if (!SecurityUtils.canAccess(issue)) + throw new UnauthorizedException(); + } else if ((build = OneDev.getInstance(BuildManager.class).findByUUID(group)) != null) { + if (!SecurityUtils.canAccess(build)) + throw new UnauthorizedException(); + } else if (!SecurityUtils.canAccess(project)) { throw new UnauthorizedException(); + } } String attachment = params.get(PARAM_ATTACHMENT).toString();