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(filter): Allow generic Slackbot to pass #947

Merged
merged 5 commits into from
Mar 15, 2021
Merged

Conversation

BYK
Copy link
Member

@BYK BYK commented Mar 11, 2021

@BYK BYK requested review from a team, jan-auer and untitaker March 11, 2021 21:20
@BYK BYK enabled auto-merge (squash) March 11, 2021 21:23
@BYK
Copy link
Member Author

BYK commented Mar 11, 2021

Eh, turns out this is not as straightforward as I thought. The generic bots? term eliminates the legit Slackbot 1.0 agent and the RegEx crate we use don't have lookaround support to only match when bot is not preceded with Slack.

The only two options I see are:

  1. Remove the generic term (this has been there for 5+ years, scary)
  2. Add a second layer that uses an allowlist and add Slackbot 1.0 there for by-pass

@jan-auer @untitaker thoughts?

@jan-auer
Copy link
Member

jan-auer commented Mar 12, 2021

Also adding @lobsterkatie since she's been adding filters over time. I have no context on implications of these filters.

For context, this was originally added due to getsentry/sentry#5284 in getsentry/sentry#5285

@jan-auer
Copy link
Member

jan-auer commented Mar 12, 2021

@BYK could you add more test cases for the bots we still want to block?

@BYK
Copy link
Member Author

BYK commented Mar 12, 2021

@BYK could you add more test cases for the bots we still want to block?

They are already there, that's why I didn't touch the tests much.

@lobsterkatie
Copy link
Member

Happy to help but I have no special context on this. Over time in support, people would write in, asking for things to be added. I'd post in the engineering channel and get someone to give it a thumbs-up, and then make a PR adding to the file. It wasn't a super common thing, though.

@getsentry getsentry deleted a comment from lobsterkatie Mar 15, 2021
@untitaker
Copy link
Member

Wouldn't it be possible to enumerate linkexpanding and imgproxy instead?

@untitaker
Copy link
Member

Answer: We have a generic bots filter that would match slackbot anyway.

Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

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

I think we won't get around having two regexes, but one last comment before we merge.

relay-filter/src/web_crawlers.rs Outdated Show resolved Hide resolved
@BYK BYK merged commit ab5b9f2 into master Mar 15, 2021
@BYK BYK deleted the byk/fix/relax-slackbot branch March 15, 2021 12:10
jan-auer added a commit that referenced this pull request Mar 15, 2021
* master:
  fix(common): Use zero-copy deserialization when FromStr is implemented (#950)
  fix(filter): Allow generic Slackbot to pass (#947)
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.

Legit errors from Slack commands were filtered out
4 participants