Skip to content
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

fix(Webhook): check whether user is null during patching #10049

Closed
wants to merge 1 commit into from
Closed

fix(Webhook): check whether user is null during patching #10049

wants to merge 1 commit into from

Conversation

rockerbacon
Copy link

Please describe the changes this PR makes and why it should be merged:
While fetching a webhook using the Client#fetchWebhook method, it is possible for the Discord API to send a response with some properties set to null and others not defined.
While creating a new instance of the Webhook class, the _patch method will check whether or not a property was returned using the in operator. An issue then arises when the property "user" is null: the check 'user' in data evaluates to true (the property is indeed in data, it's just set to the value "null") and a null value is then sent to CachedManager#_add, which attempts to access the property id in a null value.

The following is the relevant stack trace of the error I encountered:

TypeError: Cannot read properties of null (reading 'id')
    at UserManager._add (/home/loka/Documents/discord-bots/greatreads-discord-bot/node_modules/discord.js/src/managers/CachedManager.js:47:48)
    at Webhook._patch (/home/loka/Documents/discord-bots/greatreads-discord-bot/node_modules/discord.js/src/structures/Webhook.js:91:39)
    at new Webhook (/home/loka/Documents/discord-bots/greatreads-discord-bot/node_modules/discord.js/src/structures/Webhook.js:25:20)
    at Client.fetchWebhook (/home/loka/Documents/discord-bots/greatreads-discord-bot/node_modules/discord.js/src/client/Client.js:313:12)

And this is the offending Discord API response, obtained by logging the immediate response (Client.js ln312) to the console:

{
  data: {
    application_id: '<redacted>',
    avatar: null,
    channel_id: null,
    guild_id: null,
    id: '1184125670163488828',
    name: '<redacted>',
    type: 3,
    user: null
  }
}

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Copy link

vercel bot commented Dec 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Visit Preview Dec 13, 2023 11:38pm
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Dec 13, 2023 11:38pm

@Jiralite
Copy link
Member

Jiralite commented Dec 14, 2023

Pretty sure this is an issue with the API. The user of a webhook is not documented to be null.

We'll see what happens with discord/discord-api-docs#6576.

@rockerbacon
Copy link
Author

Thanks for the quick response.

If it is of any relevance: my use case is editing a message sent by my own bot at the end of an OAuth2 authorization process. Since the authorization system is designed to be a separate distributed system, our strategy was to store the message id, webhook id and webhook token in a central database and retrieve them later.
I worked around the issue by instantiating a WebhookClient directly, as documented in the discord.js guide.

Let me know if you'd require any further assistance with the issue.

@Qjuh
Copy link
Contributor

Qjuh commented Dec 14, 2023

You could also just instantiate an InteractionWebhook instance directly, no need to fetch it or make another Client (which your WebhookClient is) if you already have a logged in Client.

@Jiralite
Copy link
Member

Jiralite commented Dec 14, 2023

Looks like we got an official response: discord/discord-api-docs#6576 (comment)

Fetching these kinds of webhooks should return a 404, which means we shouldn't be encountering a null user. I'll be closing this.

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

Successfully merging this pull request may close these issues.

None yet

3 participants