Undocumented caveat that might lead to access control vulnerability in draft & version enabled collections

[WilsonLe]

Undocumented caveat that might lead to access control vulnerability.

In collections with draft and version enabled, update access control needs to handle both cases where user is updating by ID or performing bulk updates (in other words, whether or not args.id is defined).

If the user's access control does not handle cases where args.id is defined, only returning query constraints, assuming that returning query constraints is sufficient to handle both updateById and bulk update (which is true for non-draft and non-versioned collection), then there's a vulnerability: user can bypass the update access control by updateById, (i.e. `PATCH /api/pages/page-id).

Users can bypass the update access control because when user specifies an id in their update request, the implementation of getLatestCollectionVersion.ts, with version and draft enabled collections, Payload naively finds the latest version of the to-be-updated document without regarding to access control, and naively return the latest version without considering access control.

export const getLatestCollectionVersion = async <T extends TypeWithID = any>({
  id,
  config,
  payload,
  query,
  req,
}: Args): Promise<T> => {
  let latestVersion: TypeWithVersion<T>

  if (config.versions?.drafts) {
    // NAIVELY FINDS BY ID
    const { docs } = await payload.db.findVersions<T>({
      collection: config.slug,
      req,
      sort: '-updatedAt',
      where: { parent: { equals: id } },
    })
    ;[latestVersion] = docs
  }

  const doc = await payload.db.findOne<T>({ ...query, req })
  
  // COLLECTION WITHOUT VERSION DRAFT ENABLED USES query TO HANDLE ACCESS CONTROL
  if (!latestVersion || (docHasTimestamps(doc) && latestVersion.updatedAt < doc.updatedAt)) {
    return doc
  }

  // COLLECTION WITH VERSION DRAFT ENABLED NAIVELY RETURNS doc
  return {
    ...latestVersion.version,
    id,
    createdAt: latestVersion.createdAt,
    updatedAt: latestVersion.updatedAt,
  }
}

Of course, this vulnerabilty can be negated if developers handles cases where args.id is defined, but that's not always the case. This caveat is not documented anywhere. The only reasons I came accross this weird caveat is having robust test cases.

Edit: this caveat also applies for delete access control of draft & version enabled collections

[BrianJM]

Note: it's best to report vulnerabilities directly to the Payload team so they can resolve the defect prior to public disclosure.

@WilsonLe Can you provide a link to a reproduction?

Can this be reproduced in one of the test suites?

The only reasons I came accross this weird caveat is having robust test cases.

The test suites do need some work regarding regression - they not cover all documented features or reported defects. This is an area I'd like to contribute to. If you have written more comprehensive test, perhaps they can be added to the repo?

[DanRibbens]

I haven't tried to reproduce this yet though it makes sense conceptually.

My question would be what should happen if a user tries to update a document where access to a later draft would be limited. This doesn't allow for the most up to date changes to be merged with the data being saved. How would we handle this without it looking like a bug that causes data loss between users with different access privileges?

Also, should getlatestdraft invoke the read or update access method?

I think the way we have it is likely not going to change and perhaps documenting it as you said is the only way forward. I'm interested in other feedback.

[paulpopus]

Since this needs a wider discussion/feedback, I'm converting this to a discussion