Skip to content

Commit

Permalink
API V3: avoid leaking information through expandable fields (#11381)
Browse files Browse the repository at this point in the history
* API V3: avoid leaking information through expandable fields

There are still more things to do, but first I'm removing the ones
that aren't documented, so this shouldn't be a breaking change,
and the information from those fields can already be obtained
from the main object, and if another user with lower permissions was relying on those
fields, that was actually a bug.

Ref readthedocs/readthedocs-corporate#1736.

Expandable fields may look cool, but once you have to deal with
permissions, they are a pain. Sorry, had to vent.

* Missing class inheritance
  • Loading branch information
stsewd authored Jul 9, 2024
1 parent 7c86e2f commit 3456794
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 10 deletions.
19 changes: 15 additions & 4 deletions readthedocs/api/v3/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -778,18 +778,23 @@ class Meta:

expandable_fields = {
# NOTE: this has to be a Model method, can't be a
# ``SerializerMethodField`` as far as I know
# ``SerializerMethodField`` as far as I know.
# NOTE: this lists public versions only.
"active_versions": (
VersionSerializer,
{
"many": True,
},
),
# NOTE: we use a serializer without expandable fields to avoid
# leaking information about the organization through the project.
"organization": (
"readthedocs.api.v3.serializers.OrganizationSerializer",
"readthedocs.api.v3.serializers.OrganizationSerializerWithoutExpandableFields",
# NOTE: we cannot have a Project with multiple organizations.
{"source": "organizations.first"},
),
# NOTE: we are leaking the slugs of all teams linked to this project
# to anyone with access to this prtoject. It's only the slug, but still.
"teams": (
serializers.SlugRelatedField,
{
Expand Down Expand Up @@ -1165,7 +1170,7 @@ class Meta:
}


class OrganizationSerializer(FlexFieldsModelSerializer):
class OrganizationSerializerWithoutExpandableFields(FlexFieldsModelSerializer):
created = serializers.DateTimeField(source="pub_date")
modified = serializers.DateTimeField(source="modified_date")
owners = UserSerializer(many=True)
Expand All @@ -1187,8 +1192,14 @@ class Meta:
"_links",
)


class OrganizationSerializerWithExpandableFields(
OrganizationSerializerWithoutExpandableFields
):
class Meta(OrganizationSerializerWithoutExpandableFields.Meta):
expandable_fields = {
"projects": (ProjectSerializer, {"many": True}),
# TODO: we are leaking all teams and its members to anyone who is
# an admin member of the organization.
"teams": (TeamSerializer, {"many": True}),
}

Expand Down
11 changes: 5 additions & 6 deletions readthedocs/api/v3/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
BuildSerializer,
EnvironmentVariableSerializer,
NotificationSerializer,
OrganizationSerializer,
OrganizationSerializerWithExpandableFields,
ProjectCreateSerializer,
ProjectSerializer,
ProjectUpdateSerializer,
Expand Down Expand Up @@ -646,7 +646,7 @@ class OrganizationsViewSetBase(
# Also note that Read the Docs for Business expose this endpoint already.

model = Organization
serializer_class = OrganizationSerializer
serializer_class = OrganizationSerializerWithExpandableFields
queryset = Organization.objects.none()
permission_classes = (IsAuthenticated,)

Expand All @@ -659,6 +659,7 @@ class OrganizationsProjectsViewSet(
APIv3Settings,
NestedViewSetMixin,
OrganizationQuerySetMixin,
FlexFieldsMixin,
ReadOnlyModelViewSet,
):
model = Project
Expand All @@ -667,10 +668,8 @@ class OrganizationsProjectsViewSet(
queryset = Project.objects.all()
serializer_class = ProjectSerializer
permission_classes = [IsAuthenticated & IsOrganizationAdminMember]
permit_list_expands = [
"organization",
"organization.teams",
]
# We don't need to expand the organization, it's already known.
permit_list_expands = []

def get_view_name(self):
return f"Organizations Projects {self.suffix}"
Expand Down

0 comments on commit 3456794

Please sign in to comment.