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

feat: fault tolerant body handling #747

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

shevernitskiy
Copy link
Contributor

@shevernitskiy shevernitskiy commented Jan 20, 2025

The issue was originally described here: #613

Short Summary:
We assume that the body will be valid JSON. However, it can be empty or contain invalid JSON in some unusual cases. This can cause the server to crash due to an uncaught error triggered by JSON parsing in some of the adapters.

It was suggested to wrap it in a function, but I think that is unnecessary. We already have a promise and just need to safely resolve it to a value that we can then validate.

I suggest catching all promises where a crash can occur and returning an empty object. In webhookCallback, we can perform basic validation by checking the update_id field. In the case of an invalid body, we should return a 400 status to the client. Here, we need to add an extra callback to the adapter interface.

I added some e2e test cases to check for 400 and 401 errors, but it takes some time (~300-350ms on my machine).

As of TODO: investigate deno bug that happens when this console logging is removed - I didn't notice any issues. Maybe it's fixed now?

Closes: #697

deno.jsonc Outdated Show resolved Hide resolved
@KnorpelSenf KnorpelSenf merged commit fe4a4de into grammyjs:v2 Jan 20, 2025
@KnorpelSenf
Copy link
Member

Closes: #697

This is not true, it's still a property on the adapters rather than a function

@@ -239,6 +243,7 @@ export type WorktopAdapter = (req: {

/** AWS lambda serverless functions */
const awsLambda: LambdaAdapter = (event, _context, callback) => ({
// TODO: add safe parse workaround
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it a function will btw fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just commited, but again late for 2 secs before you merged:)

function safeJsonParse(value: string, fallback = {} as any): any {
    try {
        return JSON.parse(value);
    } catch (_err) {
        return fallback;
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah

@shevernitskiy
Copy link
Contributor Author

Closes: #697

This is not true, it's still a property on the adapters rather than a function

but function wrapper is not needed now

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