Data fetching (REST) with any user-permissions relations does not work (as if ignored completely)

Bug report

Required System information

Node.js version: 18.14.0
NPM version: 9.3.1
Strapi version: 4.11.1 (but this seems to be the problem for longer time)
Database: PostgreSQL
Operating system: MacOS Monterey
Is your project Javascript or Typescript: Javascript

Describe the bug

I have multiple DB tables that have "user" fields which are relations to users-permissions.

All tables with issues have this relation:

"user": {
      "type": "relation",
      "relation": "oneToOne",
      "target": "plugin::users-permissions.user"
    },

This tables are for example orders on the web-shop, service records for devices and so on. All related to logged-in users.
I discovered that filtering by these relations are not working at all anymore and all my registered users could see ALL orders - good thing that I still have small number of registered users, but I presume, someone could have the same issue on very large system!

Also - population does not work on same fields - if I use "populate=*" or "populate[0]=user" in query - this does nothing.

I noticed that all other relations seem to work - filtering + population. So it seems, only relation to users-permissions is broken.
But this is quite a big issue.

I need to note that these queries did not change for over 8 months in my code so I did not even have this on the radar that it could be a problem. Now I need to write some new tests.

Steps to reproduce the behavior

Simply try REST query on the table where you want to filter by related ID - for example, if I execute this when using 4.11.1:
http://localhost:1337/api/orders?filters[$and][0][user][id][$eq]=30&sort[id]=DESC&populate[0]=user&filters[id]=366
I get all orders and I should only get the ones related to user with ID 30.
Also - none of the records have "user" populated in response.

Same thing happens on all my tables with relation to users-permissions.

Expected behavior

Filtering by related table should work.
Population should work

Code snippets

By dumping ctx.request.query into my log - I can see that REST query seems correct:

[2023-06-13 20:40:30.095] http: GET /api/orders?filters[$and][0][user][id][$eq]=30&sort[id]=DESC&populate[0]=user (171 ms) 200
[2023-06-13 20:40:35.580] orders-find-query {
  "filters": {
    "$and": [
      {
        "user": {
          "id": {
            "$eq": "30"
          }
        }
      }
    ]
  },
  "sort": {
    "id": "DESC"
  },
  "populate": [
    "user"
  ]
}

Additional context

I went back to vesion 4.6.2 just to test - and there the filtering works but population does not. I did not go back to where population works also.
If seems that from version 4.8.1 both filtering and population on these relations does not work. So, as said, this seems to be an issue for longer time.

UPDATE: I remembered that I've had similar issue with different problem - and it was resolved when I gave the right to "find" on that relation. This is the case here also. When I do that (allow authenticated users), filter works - but population still does not. Only when I also allow "findOne", population works.

This resolved my immediate issue, but we still have a huge problem. When find/findOne is not allowed, result should surely not be ignoring the filter and dumping everything out - should it? I would hugely prefer some error message like "You are filtering by field/relation that you do not have access to" (or similar).

Also, if I allow "find", anyone with valid JWT token can list all users - which is another can of worms, but probably solvable in a different manner (policies maybe) - will look into that.

I had to create my own controllers for the content types that had a user relation. In my cases, I needed to give the users the access to view their created content (e.g. their events).

This change came some thing 4.7.x ish. If the fields could be populated user data could be leaked.

For example this controller was for my events that only the users themselves need (api/event/controllers/auth-user-events.ts)
(And yes the code could probably be better)

export = {
  async findEvents(ctx) {
    const { user } = ctx.state;

    if (!user) {
      return ctx.unauthorized("You must be logged in to perform this action.");
    }

    try {
      const query = {
        filters: {
          user: {
            id: {
              $eq: user.id,
            },
          },
        },
        populate: ["categories"], // populate some stuff here if you want
      };

      ctx.query = { ...ctx.query, ...query };

      const contentType = strapi.contentType("api::event.event");

      let entities = await strapi.entityService.findMany(
        contentType.uid,
        ctx.query
      );

      return {
        data: entities,
      };
    } catch (error) {
      return ctx.internalServerError("Oops! Something went wrong.");
    }
  },
};

This would be a router (api/eventsroutes/auth-user-events.ts)

export default {
  routes: [
    {
      method: "GET",
      path: "/auth-user-events",
      handler: "auth-user-event.findEvents",
    },
  ],
};

If other users in the frontend would have to search for that user, it is not possible. But if the user also has a open Profile that can be found the base controller filters would work.

@danielmuxel

Thank you for the reply @danielmuxel! I am aware of this option, and I already have a lot of custom controllers and methods. This just caught me a bit of guard for simple filtering/population. This is, as it seems, the way ;)

Is this resolved? I have two users in User table. And having one user has multiple hotels. I have assigned 2 hotels to one user and 1 hotel to another user. Now I have to fetch a list of hotels assigned to logged in user. How can I do that?

@tnavadiya

@tnavadiya As far as I see - when calling via REST API, the issue is still there. But, like @danielmuxel noted, if you do it yourself via extended or custom controller, then you can get the data for specific user.

The thing is:

The REST API's default controllers use sanitizeOutput() under the hood which I think will remove any private attributes and relations you don't currently have permission for from the output.

That's why if you create a custom controller which uses strapi.service, strapi.entityService or strapi.db.query to do your find request, and if you do not call sanitizeOutput() manually, those user relations will be returned.

This is somewhat documented here:
https://docs.strapi.io/dev-docs/backend-customization/controllers#sanitization-when-utilizing-controller-factories

Your only option currently is to give the find permission so that the relation does not get thrown away by sanitizeOutput. Of course this opens a door for people to load all users. You could create a custom policy which prevents direct access to the user.find route for everyone to close that door again.

There is currently an intention to completely replace the existing users-permissions plugin. Please leave your upvote here to increase priority:
https://feedback.strapi.io/feature-requests/p/field-level-permissions-end-user-management

@GregorSondermeier

I did not like allowing public find/findOne for users as this is way too big of a security issue - so I used custom controller, since I only need filtering by user, not the user data (by relation) itself - so I do not populate that at all.

But you are 100% correct about data sanitization @GregorSondermeier - if you get the data like this, sanitation is a must.
Not hard to call, but you need to remember it :)

For us, upgrading from 4.5.2 => 4.10.6 caused this issue and resulted in some of our Rest APIs returning a lot more data than they should have. Eventually fixed it the way mentioned in the comments above.

Given that this issue causes a filter criteria to be silently removed, it can cause inadvertent leak of data. From that perspective:

The severity : low seems inadequate.
It would be ideal to document this somewhere to help prevent this for teams upgrading in the future. (happy to contribute to document if someone can point me to the right place).

As a subset to my previous response this request sanitization happens within the controller (core controllers), and generally if you are building a is owner type API you will want to use route policies to strictly enforce what they have access to in addition to the permission set by user and permissions.

Likewise when you have to enable the find or find one permissions you'll want to protect the users and permissions endpoint by creating a policy or a middleware for that.

We could probably construct some of these by defaults and ship them with the application and just allow users to turn them on or enable them but we would need to do a bit of discovery on that.

If you're using the entity service directly then these request sanitization of both filters and population do not apply meaning you can use the entity service within a policy or a route middleware to do accurate comparisons between the request state (user/role id) against the entity that they're trying to perform the action against.

Keep in mind that the construction of these policies is extremely subjective and for is owner-based policies we basically can't build a generic one for everyone, but we could provide sample code that would put you in the right direction depending on the way your content models are structured.

@geeky-biz

@geeky-biz To confirm where you filtering on private fields? since that is correct that was changed for security reasons. see https://www.ghostccamm.com/blog/multi_strapi_vulns/#cve-2023-22894-leaking-sensitive-user-information-by-filtering-on-private-fields-in-strapi-versions-471

If not where are you filtering on that has this issue?

We fetch deals that were assigned to users. Via our middleware, we had added a filter condition(({'users' : {id : {'$eq': id}}}) to return only those deals that were assigned to the requesting user (for /api/deals). This worked perfectly on Strapi 4.5.2. Upon upgrade to 4.10.6 - every user started to see all the deals.

I now understand that this happened because we had so far not given this role access to user.find. We have fixed this by giving this access & blocking the user.find via a policy.

But, like I mentioned, a data-leak like this can have far-reaching consequences. And, I foresee many others going through an experience like ours.

The id is not a private field

FYI we are regrouping early next week to address this topic. Thank you for bringing this to our attention 👍

@alexandrebodin

Hey all,

We conducted an internal meeting/discussion around this today and came up with a few action items and things we plan to do in the short-term/long-term. This meeting involved myself, @alexandrebodin, @Convly, and @innerdvations .

Short Term

Introduce a change (debatable if breaking since the breaking change we did previously is what opened this can of worms) to implement a 403 error if a user tries to filter or sort on something they shouldn't have access to - expected ETA is by the end of August for this to be merged and released (need to chat with @marcoautiero for exact timeline)
Add additional documentation around creating a policy to properly secure the parent content-types via a policy when you enable the find permission including how to do this for the native users-permissions user routes
Add additional documentation around how to do manual sanitization within the core controllers if you do need advanced logic that goes beyond just a simple policy

Long Term (TBD)

Introduce a new plugin that incorporates Field level permissions (could also be action level permissions with field level controls) that has both a backend and UI part. AKA completely rebuild users-permissions in a new package.

I want to be transparent and open about the new users-permissions plugin, we do not have a timeline for this yet. I need to speak to @Aurelsicoko as it's a decision that has to come from our product team and not our engineering team and it would be a massive amount of work that we need to justify. With v5 development in full swing our initial v5 development scope is filled and locked so at bare minimum we wouldn't even be able to start this part until after probably Q2 2024.

In summary, we tried to prevent adding breaking changes with security fixes and we inadvertently did so already so we are adding another that would put us back in-line with being "secure by default". What I want to do is once we start working on this default 403 response PR, we will release a few experimental/beta releases and ask to have a few people assist us in testing this out to ensure it meets your expectations. I know it's not a complete fix the problems mentioned here but we will work towards that in the long term.

Also thank you for raising this topic, I would share the recording of our meeting but we discussed several things here and some of them we can't share.

Thanks

@alexandrebodin

Hey all,

We conducted an internal meeting/discussion around this today and came up with a few action items and things we plan to do in the short-term/long-term. This meeting involved myself, @alexandrebodin, @Convly, and @innerdvations .

Short Term

Introduce a change (debatable if breaking since the breaking change we did previously is what opened this can of worms) to implement a 403 error if a user tries to filter or sort on something they shouldn't have access to - expected ETA is by the end of August for this to be merged and released (need to chat with @marcoautiero for exact timeline)

Add additional documentation around creating a policy to properly secure the parent content-types via a policy when you enable the find permission including how to do this for the native users-permissions user routes

Add additional documentation around how to do manual sanitization within the core controllers if you do need advanced logic that goes beyond just a simple policy

Hi @derrickmehaffy,

I believe that I am not the only one that would prefer to get an access error instead of silent data-leak :)
So, in my eyes, your short-term plan seems like a logical and concise solution to an issue at hand - especially the thing about documenting - since that way everyone can easily resolve this kind of things.

Thank you for taking serious time to find a way to resolve this important issue.

Yep, it would probably be a generic 400, not a 403.
But other than that, pretty good summary 👌

@ntarandek

@ntarandek of course, quite welcome 🤗

Transparency and Active care are two of our core company values and we prefer to extend these values to our users and not just internally.

I have a chat with @marcoautiero on Thursday so hopefully I'll have updated timelines then.

Update on this, we just merged this PR into another branch but we expect this fix to be released in v4.13.0 on Aug 30th.

This will forcefully throw an error if someone attempts to filter on something they shouldn't have access to which is a break change but we have no other choice: #17693

This was patched in v4.13.1 so I'll go ahead and mark it as closed

The Release Notes mention "REST" only, but all our GraphQL Calls which fetch an image are now broken as well.

Error: Missing schema in validateQuery

Public Endpoint, no user-permissions involved.

Below query breaks as soon as I add image.

  query DemoPackages{
    demoPackages {
      data {
        id
        __typename
        attributes {
          title
          image {
            data {
              id
              attributes {
                name
                formats
              }
            }
          }
        }
      }
    }
  }

Someone else on discord has the same issue: https://discord.com/channels/811989166782021633/1147035990049968199

possible fix: #17895

This issue has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/4-13-1-graphql-error-missing-schema-in-validatequery-for-images/31743/1

Navigation Menu

Repository navigation

Data fetching (REST) with any user-permissions relations does not work (as if ignored completely) #17001

Bug report

Required System information

Describe the bug

Steps to reproduce the behavior

Expected behavior

Code snippets

Additional context

10 remaining items

Short Term

Long Term (TBD)

Short Term

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Data fetching (REST) with any user-permissions relations does not work (as if ignored completely) #17001

Description

Bug report

Required System information

Describe the bug

Steps to reproduce the behavior

Expected behavior

Code snippets

Additional context

Activity

ntarandek commented on Jun 14, 2023

danielmuxel commented on Jun 14, 2023

ntarandek commented on Jun 14, 2023

tnavadiya commented on Jul 10, 2023

ntarandek commented on Jul 11, 2023

GregorSondermeier commented on Jul 11, 2023

ntarandek commented on Jul 11, 2023

geeky-biz commented on Jul 18, 2023

10 remaining items

derrickmehaffy commented on Jul 19, 2023

geeky-biz commented on Jul 19, 2023

Boegie19 commented on Jul 19, 2023

alexandrebodin commented on Jul 19, 2023

derrickmehaffy commented on Jul 25, 2023

Short Term

Long Term (TBD)

ntarandek commented on Jul 25, 2023

Short Term

Convly commented on Jul 25, 2023

derrickmehaffy commented on Jul 25, 2023

derrickmehaffy commented on Aug 21, 2023

derrickmehaffy commented on Aug 30, 2023

LucaNerlich commented on Sep 3, 2023

strapi-bot commented on Sep 3, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions