Rest API should not return ids for read denied records

[6TELOIV]

When using the Rest API, relationship fields can return a mix of numbers and objects when the field contains records to which the user doesn't have read access.

To me, most of the time there is no use for an id of a record that the user cannot access (exceptions exist). Additionally, this could be a leak of sensitive data, depending on what the ID is used for.

All in all, this behavior results in

• boilerplate code to filter out the number values on the client
• extra data sent on the network
• potential leakage of sensitive data (unlikely, but depends on the usage)

I would propose that the Rest API should return only the fields to which the user has read access by default, with an option in configuration to opt-in to the current behavior (possibly allowUnreadableIds on the relationship field as a config option would cover it?)

[6TELOIV]

For anyone else running into this issue, here is function and type that you can use with the generated types to remove numbers from the array responses. This will remove all numbers from all array fields, so it should not be used if you know your query depth will not be deep enough, or you expect your response to include an array of numbers.

type RemoveNumberFromMixedArrays<T> = T extends (number | infer U)[] // Check if T is an array of number or some other type
  ? U[] // Transform it to an array of the other type, removing `number`
  : T extends object // If T is an object, recurse into its fields
    ? { [K in keyof T]: RemoveNumberFromMixedArrays<T[K]> }
    : T; // For all other types, leave them as-is

function removeNumbersFromMixedArrays<T>(
  value: T,
): RemoveNumberFromMixedArrays<T> {
  // recursively remove numbers from arrays
  if (Array.isArray(value)) {
    return value.filter(
      (v) => typeof v !== "number",
    ) as RemoveNumberFromMixedArrays<T>;
  }
  // recursively filter the values of an object
  if (typeof value === "object" && value !== null) {
    return Object.fromEntries(
      Object.entries(value).map(([key, value]) => [
        key,
        removeNumbersFromMixedArrays(value),
      ]),
    ) as RemoveNumberFromMixedArrays<T>;
  }
  return value as RemoveNumberFromMixedArrays<T>;
}

an example usage for me, using tanstack query, with a global Homepage that contains relationship fields.

useSuspenseQuery({
  queryKey: ["homepage"],
  queryFn: async () => {
    const res = await fetch(
      `${process.env.PAYLOAD_URL}/api/globals/homepage?depth=1`,
    );
    if (res.ok) {
      const body = (await res.json()) as Homepage;
      return removeNumbersFromMixedArrays(body);
    } else {
      throw (await res.json()) as { errors: { message: string }[] };
    }
  },
});

[6TELOIV]

With the change of not mixing number and object in the return array, the function could be removed, and the of res.json() changed to RemoveNumberFromMixedArrays<Homepage>.

[6TELOIV]

I've updated the post above to remove all numbers from arrays, as my responses should never include numbers in arrays.

I think something else would be needed to have a fully robust solution which knows the difference between a mixed array that only happens to have numbers, and an array that only stores numbers which are not ID's for an object (i.e. a hasMany number field).

I think a way to disambiguate this without entirely changing the current functionality would be to return objects with only the ID for read-denied fields. In that case, you could with certainty know the difference between an array of numbers and an array of read-denied objects, though I would prefer my original proposal of not including read-denied IDs at all for relationship fields.

// hasMany number field
[1, 2, 3, 4]
// relationship past depth limit OR with read access denied on records
[{id:1},{id:2},{id:3},{id:4}]

[DanRibbens]

I appreciate the code examples, but you are still making the fetch and removing the data on the client side, so this is not really any more secure if that is the goal.

A little context for why Payload works this the way it does, serving the id as a placeholder for a related document in relationship fields that cannot be read is important from the editor perspecitve. Without it you might be updating a document and eliminate the relationship inadvertanly. Moving the boundary of access of what can be read would be a major shift with larger ramifications than just how the API returns data to a project frontend.

You can always add read access to the relationship field itself in order to enforce that relationship IDs should not come through by config. That would be the payload preferred way to do this to not result in any data loss when saving content.

[6TELOIV]

I agree, the examples given do not improve security. They are just a way to "sanitize" the data so that component(s) in the frontend can expect a (typed) array of just objects, rather than a mixed array.

I had not considered the ramifications for the editor experience. Had you seen my recent reply above, which proposes moving from a mixed array of IDs and objects, to objects with only IDs and objects populated with data? This would let you keep the existing functionality while making it possible to more easily differentiate between this data on the frontend.

Lastly, I had not considered the read access on the relationship field. What would that look like? As far as I was aware, read control for a field is all-or-nothing. I assumed maybe I could do some check in a beforeRead of afterRead hook, but was unsure about how I would hook into the access control.

If it helps clarify, the situation I'm dealing with is a global with a relationship field to a colleciton of alerts. The alerts are always readable by users, but are read-denied before some publication date. This results in the API returning something like:

{
  "id": 1,
  "alerts": [
    5,
    {
      "id": 7,
      "title": "An alert"
      "content": "etc etc"
    },
    9,
    15
  ]
}

[6TELOIV]

I was unable to use access control to do what I desired. However, I did come up with a method to do so with an afterRead hook. But, it seems duplicative of how the API opreates alread, i.e. I'm doing extra calls to the DB to resolve the request.

Please advise if you know of a better approach to achieve the desired behavior.

export const HomePage: GlobalConfg = {
  /* ... other config ... */
  fields: [
    {
      name: 'alerts',
      type: 'relationship',
      relationTo: 'alerts',
      hasMany: true,
      hooks: {
        afterRead: [
          async ({ value, req: { user, payload } }) => {
            // in case of null value for the field
            if (!value) {
              return value
            }
            // otherwise, it will be an array of ids
            // filter out the ids to which the user doesn't have access
            const filteredValues = await Promise.all(
              (value as number[]).map(async (id) => {
                const totalDocs = (
                  await payload.count({
                    collection: 'alerts',
                    where: {
                      id: {
                        equals: id,
                      },
                    },
                    user,
                    overrideAccess: false,
                    disableErrors: true,
                  })
                ).totalDocs
                if (totalDocs === 0) {
                  return null
                }
                return id
              }),
            )
            return filteredValues.filter((v) => v !== null)
          },
        ],
      },
    },
  ]
}

[6TELOIV]

An improved generic version, for reuse across multiple SingleRelationship fields.

import { FieldHook, SingleRelationshipField } from 'payload'

export const removeUnreadableIds: FieldHook = async ({ value, req: { user, payload }, field }) => {
  // in case of null value for the field
  if (!value) {
    return value
  }
  // otherwise, it will be an array of ids
  // filter out the ids to which the user doesn't have access
  const filteredValues = await Promise.all(
    (value as number[]).map(async (id) => {
      const totalDocs = (
        await payload.count({
          collection: (field as SingleRelationshipField).relationTo,
          where: {
            id: {
              equals: id,
            },
          },
          user,
          overrideAccess: false,
          disableErrors: true,
        })
      ).totalDocs
      if (totalDocs === 0) {
        return null
      }
      return id
    }),
  )
  return filteredValues.filter((v) => v !== null)
}

[Lippur]

I find it quite difficult to understand why the preservation of relationship data in the editor requires the ID to be returned as a number, rather than an object with just the ID { "id": 7 }. Returning objects consistently would allow preserving relationships just as well, so it cannot really be an argument for returning the ID numbers mixed in with objects. It pushes the whole burden of handling the mixed data onto the developer, just to facilitate a specific use case that could also be achieved equally well without all of this burden. Doesn't make a whole lot of sense.

[6TELOIV]

Here is a better type for recursively removing number options from the generated payload types. I use this in my code for with tanstack query and am returning a value from the API which I know based on usage will not include id values for relationship fields:

export type Deep<T> = unknown extends T
  ? T // don't modify T if it's any or unknown; doing so will cause infinite recursion
  : number extends T // if T can be a number
    ? Exclude<T, number | undefined | null> extends never // and can only be a number, null, or undefined
      ? T // Then don't modify it. This number does not represent an ID from payload.
      : Deep<Exclude<T, number>> // Otherwise, remove the number (ID) option and recurse
    : T extends object // if T can't be a number and is an object,
      ? { [K in keyof T]: Deep<T[K]> } // recurse
      : T extends (infer U)[] // if T can't be a number and is an array
        ? Deep<U>[] // recurse
        : T; // otherwise, return T

Usage (assuming HomePage has 1 nested level of relationships only. API depth would have to be set explicitly for other queries):

import { useQuery } from "@tanstack/react-query";
import { HomePage } from "path/to/payload-types";
import type { Deep } from "path/to/deep";

/**
 * Gets the homepage global from the API
 */
export default function useHomepage() {
  return useQuery<Deep<HomePage>>({
    queryKey: ["homepage"],
    queryFn: async () => {
      const res = await fetch(
        `${process.env.PAYLOAD_URL}/api/globals/homePage`,
      );
      if (res.ok) {
        return await res.json();
      } else {
        throw {
          ...(await res.json()),
          code: res.status,
        };
      }
    },
  });
}

[SimonVreman]

I agree with @6TELOIV's proposal (not the original, which would omit data) to return either an object containing the ID; or the full document. I'd like to shed some light on the current approaches I have seen, and some considerations in consistency.

Type narrowing

A union type between a primitive and an object is quite bad, it is a hassle to have to resolve. This makes UI code more difficult to read and business logic more prone to errors. I have seen some wonky checks and messy conditional trees arise. This is no longer a problem when implementing the proposal: a simple null check would suffice, which can easily be chained in-line and results in better readability.

API interchangeability

Consistency in the API responses across the various solutions that are supported would be a big advantage. I can think of some more minor arguments, but a concrete case I have experienced is perhaps the most useful. With the release of Payload 3, it has become incredibly easy to fetch data server side through the Payload Local API. However, to reduce round trip delays in client heavy applications, it becomes interesting to also fetch client side, but only to revalidate. A NextJS example:

• During SSR, fetch using the Local API
• During CSR, revalidate using the REST or GraphQL API
• On form submission, e.g using a server action, optimistic return using the Local API
  I've come to appreciate this flow a lot, though I have opted to drop the usage of GraphQL due to the type inconsistencies being to much of a hassle, and cluttering my codebase. The GraphQL API does follow the proposed ID pattern, whereas the other API's do not.

Determining partial document

Of course, this proposal has downsides. Say you wish to render a card for every result received, but not when access is denied. When filtering these results, it is no longer good enough to check if the result is of the object type. One way would be to check some arbitrary, hopefully always present, property like createdAt. Though I argue the correct course of action here is to filter based on the UI context: i.e. skip all results without a title, or some approach that makes sense based on what you intend to show.

Observed approaches

1. The most nasty, but I think the most common, has been to assume that the relation cannot be a primitive, and just type cast it to the object. This is a recipe for bugs, and can occur pretty much everywhere: it is, in my opinion, too difficult to keep a mental map of all the current and future responses to justify type casting. I suppose the approach takes by @6TELOIV is a better variation, by using the type on a query level, where you have more context. But I still think this removes all the red lines without removing the actual type errors.
2. Type narrowing, a correct approach but very ugly to execute everywhere. This can be cleaned up a lot by creating some utility functions, which I do, but this is far from ideal.

Suggested approach

Implementing the proposal ;). But I can see that this is a major change anyways, so for now I am personally doing it using some utility functions (assuming uuid, not numeric):

// Overload to preserve nullability
export function entityId(entity: string | { id: string }): string
export function entityId(entity?: string | { id: string } | null): string | null
export function entityId(entity?: string | { id: string } | null): string | null {
  if (entity == null) return null
  return typeof entity === 'string' ? entity : entity.id
}

export function relation<T extends { id: string }>(relation?: T | string | null) {
  if (relation == null || typeof relation === 'string') return null
  return relation as T
}

Which would then be used as follows:

const post: Post = await fetchPost(postId);
const groupId: string = entityId(post.group); // or null, if group is optional
const group: Group | null = relation(post.group);

[6TELOIV]

I like your method a lot, although I still don't like the overhead of having to call relation to "clean" the returned data on each relationship field.

I think moving from a union type to an object with only an ID is a good way to go.

I am trying to think of a way to indicate that the data was unreadable in some way. Maybe populated: true or populated: false? With proper type definitions, you would just have to check the value populated and then TS would have the right types for your other fields (either never or the actual type).

This provides benefits over just returning an object with an id field, as that method effectively makes all of your other fields optional, even if they are required, making you write unnecessary undefined checks on the frontend.