Skip to content

fix: dashboard patch api should not allow 'id' in updates #909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mGolestan98
Copy link
Contributor

@mGolestan98 mGolestan98 commented Jun 4, 2025

Problem

The dashboard PATCH API endpoint was using DashboardSchema.partial() for request body validation, which allowed the id field to be included in update requests. This was inconsistent with the updateDashboard controller method, which was already correctly typed to accept schema excluding the id.

Copy link

changeset-bot bot commented Jun 4, 2025

⚠️ No Changeset found

Latest commit: e0c870f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jun 4, 2025

@mGolestan98 is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

@teeohhem teeohhem requested review from wrn14897 and dhable June 5, 2025 16:50
@mGolestan98
Copy link
Contributor Author

mGolestan98 commented Jun 5, 2025

actually 2 things that i just realized:

  1. The id field isn’t used in the Mongo schema—only _id is—so it shouldn’t cause any issues. However, _id still appears in the request body and should be stripped out.
  2. I attempted to use Zod to strip unknown parameters not defined in the schema, but it doesn’t seem to be working as expected. I’ll dig into this further after work.

I'm converting this PR to a draft for now.

@mGolestan98 mGolestan98 marked this pull request as draft June 5, 2025 20:03
@dhable
Copy link
Contributor

dhable commented Jun 5, 2025

I think the problem is that zod is allowing undefined fields by default. Appending .strict() now causes the UI update to the dashboard name to fail because the id is being included:

  validateRequest({
    params: z.object({
      id: objectIdSchema,
    }),
    body: DashboardWithoutIdSchema.partial().strict(),
  })

Response after editing dashboard title:

[
  {
    "type": "Body",
    "errors": {
      "issues": [
        {
          "code": "unrecognized_keys",
          "keys": [
            "_id",
            "team",
            "createdAt",
            "updatedAt",
            "__v",
            "id"
          ],
          "path": [],
          "message": "Unrecognized key(s) in object: '_id', 'team', 'createdAt', 'updatedAt', '__v', 'id'"
        }
      ],
      "name": "ZodError"
    }
  }
]

@mGolestan98
Copy link
Contributor Author

mGolestan98 commented Jun 6, 2025

Thanks @dhable. I think using strict would require significant code changes in the frontend apps to sanitize request parameters for all API requests which I believe that would be a harder path to take;

Instead, I’d suggest stripping unknown parameters in all backend API routes using the processRequest function from zod-express-middleware, rather than using validateRequest. With processRequest, that would apply someSchema.strip() and pass the cleaned data to the controller function which validateRequest doesn't.

Also, since zod-express-middleware is deprecated and the functionality we need is pretty minimal, it might make more sense to implement this functionality in the api codebase and just use that - something similar that already i found to exist for validating request headers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants