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: handler middleware is overwritten when combined with group or global middleware #588

Merged

Conversation

mkuznets
Copy link
Contributor

@mkuznets mkuznets commented Apr 3, 2023

Summary

When handler-scoped middlewares are combined with global- or group-scoped middlewares, the concatenation is now done using a newly allocated slice instead of append() to the existing slice.

Motivation

Here's how handler-scoped middleware was initialised before:

// m is a slice of handler-scoped middleware

if len(b.group.middleware) > 0 {
	m = append(b.group.middleware, m...)
}

handler := func(c Context) error {
	return applyMiddleware(h, m...)(c)
}

If b.group.middleware has extra capacity to fit the elements from m, the append will place the elements of m right next to the elements of b.group.middleware without allocating new memory. The expanded slice will be captured by the handler.

However, if we add another handler with a middleware, the append will use the same extra capacity again, overwriting the handler-scoped middleware added before.

The same issue applies when group-scoped middleware is combined with the global one.

Example:

bot, _ := telebot.NewBot(telebot.Settings{Offline: true, Synchronous: true})
// At least three middlewares to trigger extra capacity in the middleware slice
bot.Use(middleware.AutoRespond())
bot.Use(middleware.IgnoreVia())
bot.Use(middleware.Logger(log.Default()))

bot.Handle("/secret", func(c telebot.Context) error { panic("unauthorised access") }, middleware.Whitelist(42))

// middleware.Blacklist clobbers middleware.Whitelist from the previous handler
bot.Handle("/public", func(c telebot.Context) error { return c.Send("Hello, world!") }, middleware.Blacklist())

u := telebot.Update{
	Message: &telebot.Message{
		Text:   "/secret",
		Sender: &telebot.User{ID: 1337},
	},
}

bot.ProcessUpdate(u) // panic: unauthorised access

Tests

  • Added two tests that demonstrate the issue
  • Added an extra test to verify the call order of middlewares. This is to ensure that my changes did not affect anything.

@mkuznets mkuznets changed the title Fix handler middleware clobbering when combining it with group or global middleware Fix: handler middleware is overwritten when combined with group or global middleware Apr 3, 2023
3JoB added a commit to 3JoB/telebot that referenced this pull request Apr 4, 2023
@roreng
Copy link

roreng commented Aug 3, 2023

Encountered a similar issue.
Thank you, @mkuznets, for providing the solution.

@demget @tucnak, could you please review this pull request?

@demget demget added this to the v3.2 milestone Sep 20, 2023
@demget demget changed the base branch from v3 to v3-middleware-fix November 2, 2023 22:59
@demget demget merged commit bb76a9a into tucnak:v3-middleware-fix Nov 2, 2023
@demget
Copy link
Collaborator

demget commented Nov 2, 2023

Such a great work, @mkuznets. Thank you and sorry for the long wait.

Cases like this show how knowing the language internals is significant, as well as having qualitative tests...

I moved the append-thing to the separate function and prettied the tests, check it out: #628

@mkuznets mkuznets deleted the mkuznets/fix-middleware-clobbering branch November 2, 2023 23:27
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.

None yet

3 participants