Backend Security Audit fixes #352

oudeismetis · 2024-09-09T18:14:31Z

What this does

Had to do a security audit recently for one of our projects that uses this bootstrapper.
This PR includes fixes from that effort.

How to test

Add user steps to achieve desired functionality for this feature.

oudeismetis · 2024-09-09T18:19:12Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/serializers.py

@@ -16,6 +16,7 @@ class Meta:
            "last_name",
            "full_name",
        )
+        read_only_fields = ["email"]


Default behavior shouldn't allow changing of a users email just by having a valid token.
We should consider implement a proper endpoint for this, but it should require a user to re-enter their password and probably should trigger an email to the old email address when the change is made and possibly require them to verify they control the new email address. (currently this last one doesn't happen for registration either, but it probably should)

@paribaker I know you had thoughts about this one

oudeismetis · 2024-09-09T18:21:37Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/views.py

-    mixins.UpdateModelMixin,
-    mixins.DestroyModelMixin,
-):
+class UserViewSet(viewsets.GenericViewSet, mixins.RetrieveModelMixin):


We implement some of these with overrides below.
Oddly, by implementing update() and having the UpdateModelMixin above, we ended up with PATCH and POST endpoints that both handled updating, but had different rules and validation.
Most projects shouldn't need all of these (ex: listing all users). If they do, it's easy enough to add them, along with the proper security controls.

oudeismetis · 2024-09-09T18:22:10Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/views.py

+        user = self.get_object()
+        user.is_active = False
+        user.save()
+        return Response(status=status.HTTP_204_NO_CONTENT)


Comment above should be clear.
By default apps shouldn't just delete important data from the DB like this.

oudeismetis · 2024-09-09T18:22:31Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/settings.py

@@ -306,7 +309,6 @@
 if not IN_DEV:
    SECURE_SSL_REDIRECT = True
    SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https")
-    MIDDLEWARE += ["django.middleware.security.SecurityMiddleware"]


duplicate from another spot in this file.

oudeismetis · 2024-09-09T18:23:14Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/settings.py

-{% if cookiecutter.client_app.lower() != 'none' -%}
-CORS_ALLOWED_ORIGINS.append("http://localhost:8080")
-{% endif -%}
-


See further up.
This is now controlled by ALLOWED_HOSTS. Hardcoding values like localhost into this file is a security risk.

oudeismetis · 2024-09-09T18:24:29Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/urls.py

@@ -1,6 +1,9 @@
 from django.contrib import admin
 from django.urls import include, path

+admin.site.site_header = "{{ cookiecutter.project_name }} Admin"
+admin.site.site_title = "{{ cookiecutter.project_name }}"
+


Just a nice little addition so that managing multiple projects makes it more obvious which admin you are in.

Also, non-dev project stakeholders don't know (or need to care) what the "Django Admin" is. It's just the admin for the app.

oudeismetis · 2024-09-09T18:24:55Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/common/models.py

@@ -16,4 +16,4 @@ class Meta:
        abstract = True

    def __str__(self):
-        return "ah yes"
+        return "__str__ not defined for this model"


gentle nudge for devs to set this for their new models

oudeismetis · 2024-09-11T20:31:16Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/views.py

    queryset = User.objects.all()
    serializer_class = UserSerializer

    # No auth required to create user
    # Auth required for all other actions
-    permission_classes = (permissions.IsAuthenticated | CreateOnlyPermissions,)
+    permission_classes = (HasUserPermissions,)


IsAuthenticated was causing a problem here. It meant that a logged in user could do a GET on any user.
Changing this to a permission class that only lets them take action on themselves by default (unless they are a staff user)

Create still works fine.

There are automated tests now for all of this so should this break on a project, a dev would have to also remove the test to get around it.

oudeismetis · 2024-09-11T22:02:01Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/views.py

        """
+        user = self.request.user
+        return User.objects.filter(pk=user.pk)


@whusterj I removed the querset and replaced it with this call. It might not matter since we don't have list as an option here anymore. But probably a good default to have for projects that try adding a quick mixin here.

aaaand it's broken the tests. Will investigate in the morning.

I had to add the queryset back. Left a comment as to why

oudeismetis · 2024-09-12T14:52:11Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/tests.py

+    other_user = user_factory()
+    other_user.save()
+    res = api_client.get(f"/api/users/{other_user.pk}/")
+    assert res.status_code == status.HTTP_404_NOT_FOUND


Note that this is a 404. The other user does exist, but we don't want to make bad actors aware of that fact. So it's a 404.

oudeismetis · 2024-09-12T14:54:14Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/views.py

-    mixins.UpdateModelMixin,
-    mixins.DestroyModelMixin,
-):
+class UserViewSet(viewsets.GenericViewSet, mixins.RetrieveModelMixin):
    queryset = User.objects.all()


I tried to remove this but it raises an error:
AssertionError: basenameargument not specified, and could not automatically determine the name from the viewset, as it does not have a.queryset attribute.

I couldn't find any deep discussion on how to better use the queryset here. You can't auto-filter here by the logged in user or anything like that. All examples just have it as .all(). So this seems to be field you just HAVE to have.
But it doesn't degrade our security at all as additional filters are tacked on to this

Ah, in that case I think you could replace it with User.objects.none()

I like it, but that doesn't work. A couple of tests are failing now because the chain of filters starts by finding nothing.

setting it to just User.objects works though. And that would imply to devs that filters get layered on elsewhere

Yeah, I think that's a good place to land

oudeismetis · 2024-09-12T14:55:26Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/views.py

        """
+        return super().get_queryset().for_user(self.request.user)


This get_queryset doesn't do anything right now. But if you were to add the list mixin or add a def list() below then it'll do the job of only returning that logged in user for that list (or all users in a list for staff users)

Are you sure? I think get_queryset is also called for update methods

whusterj

Just one question on final review. If the answer is "yes," then this is good to go.

whusterj · 2024-09-12T17:15:00Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/admin.py

@@ -57,7 +65,7 @@ class CustomUserAdmin(UserAdmin):
    ordering = []

    def permissions(self, obj):
-        return ", ".join([g.name for g in obj.groups.all()])
+        return [g.name for g in obj.groups.all()]


@oudeismetis - I trust that you checked that this works? This is changing the return type from str to list[str].

correct. I pulled this from a project where I made the change.
The join isn't needed and happens for you

Just re-tested on a locally generated my_project.

Great! This is good to go then.

whusterj · 2024-09-12T17:15:08Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/models.py

+            return self.none()
+        elif user.is_staff:
+            return self.all()
+        return self.filter(pk=user.pk)


paribaker · 2024-09-12T19:28:07Z

{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/permissions.py

-        if view.action == "create":
-            return True
-        return False
+class HasUserPermissions(permissions.BasePermission):


Q, is there a situation where request.user might be None? if so wont that cause a 500 error crash

Should not happen ever

Notice that the viewset where this is used also handles user creation requests. There are automated tests that cover that case.

I do think I've had to handle some sort of none case in the past, but I haven't seen that in a long while. But should that bug arise it would be a small/quick/obvious fix

I think the Authentication Middleware always adds user to the request. If the client is unauthenticated, then it sets request.user to an instance of AnonymousUser. But as Ed said, it should never be None or falsey. Could only happen if that middleware is removed.

That tracks. Otherwise is_anonymous wouldn't be very useful. That might have been a bug in older versions that people reported

ah nvm this is used on the object level!

Backend Security Audit fixes

b585295

oudeismetis added the In Progress label Sep 9, 2024

oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-352 September 9, 2024 18:16 Inactive

oudeismetis commented Sep 9, 2024

View reviewed changes

oudeismetis added 2 commits September 9, 2024 14:31

missing import

3204c7e

linting

ca03c46

oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-352 September 9, 2024 18:37 Inactive

fix broken test

cc77cd0

oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-352 September 9, 2024 18:51 Inactive

oudeismetis added Ready For Review In Progress and removed In Progress Ready For Review labels Sep 9, 2024

Permission checks were bad

73b18a1

oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-352 September 11, 2024 20:20 Inactive

oudeismetis added Ready For Review and removed In Progress labels Sep 11, 2024

oudeismetis commented Sep 11, 2024

View reviewed changes

update queryset

810ea86

oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-352 September 11, 2024 21:55 Inactive

oudeismetis commented Sep 11, 2024

View reviewed changes

model manager for user

6b17024

oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-352 September 12, 2024 13:47 Inactive

fix queryset confusion

69aabe8

oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-352 September 12, 2024 14:42 Inactive

oudeismetis commented Sep 12, 2024

View reviewed changes

make queryset let specific

c7d7b0b

oudeismetis temporarily deployed to tn-spa-bootstrapper-pr-352 September 12, 2024 15:19 Inactive

whusterj approved these changes Sep 12, 2024

View reviewed changes

oudeismetis merged commit 8ac3f59 into main Sep 12, 2024
17 of 18 checks passed

oudeismetis deleted the security-audit-fixes branch September 12, 2024 18:35

paribaker reviewed Sep 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend Security Audit fixes #352

Backend Security Audit fixes #352

oudeismetis commented Sep 9, 2024 •

edited

Loading

oudeismetis Sep 9, 2024

oudeismetis Sep 9, 2024

oudeismetis Sep 9, 2024

oudeismetis Sep 9, 2024

oudeismetis Sep 9, 2024

oudeismetis Sep 9, 2024

oudeismetis Sep 9, 2024

oudeismetis Sep 9, 2024

whusterj Sep 9, 2024

oudeismetis Sep 11, 2024

oudeismetis Sep 11, 2024

oudeismetis Sep 11, 2024

oudeismetis Sep 12, 2024

oudeismetis Sep 12, 2024

oudeismetis Sep 12, 2024

oudeismetis Sep 12, 2024

whusterj Sep 12, 2024

oudeismetis Sep 12, 2024

oudeismetis Sep 12, 2024

whusterj Sep 12, 2024

oudeismetis Sep 12, 2024

paribaker Sep 12, 2024

whusterj left a comment

whusterj Sep 12, 2024

oudeismetis Sep 12, 2024

oudeismetis Sep 12, 2024

whusterj Sep 12, 2024

whusterj Sep 12, 2024

paribaker Sep 12, 2024

oudeismetis Sep 12, 2024

oudeismetis Sep 12, 2024

whusterj Sep 12, 2024

oudeismetis Sep 12, 2024

paribaker Sep 12, 2024

		"""
		return super().get_queryset().for_user(self.request.user)

Backend Security Audit fixes #352

Backend Security Audit fixes #352

Conversation

oudeismetis commented Sep 9, 2024 • edited Loading

What this does

How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whusterj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oudeismetis commented Sep 9, 2024 •

edited

Loading