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

[Feature] - Extend AppConfig To Allow Flexibility On Bot Checks #460

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

probably-not
Copy link

WHY are these changes introduced?

There are situations where the app developer does not have full control over the underlying infrastructure (including how User-Agent headers are passed to the service). In the current version, the rejectBotRequest function is run unconditionally. This is a major problem, since this assumes that the developer has full control over the deployment infrastructure, which is typically not the case.

WHAT is this pull request doing?

This change adds two configurations:

  • extraAllowedUserAgents: can be used to exclude specific user agents from the isbot check
  • skipBotCheck: can be used to fully skip the bot check. This can be used in situations where the developer's underlying infrastructure already has bot checking at a higher layer in the stack, and the developer doesn't necessarily know what user agents will be passed through.

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have used yarn changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
  • I have added/updated tests for this change

I don't see any tests for this function? Tell me if there's something to add here

  • I have documented new APIs/updated the documentation for modified APIs (for public APIs)

Not sure which docs to change... please advise

…d skipBotCheck to allow fully skipping the bot check via configuration
@probably-not probably-not requested a review from a team as a code owner October 4, 2023 13:18
@probably-not
Copy link
Author

I just signed the CLA, if someone can re-run the check, it should pass now

@paulomarg
Copy link
Contributor

Hey, thank you for contributing! I agree that this is not ideal because we might be ignoring legitimate requests, but I would like to understand the use case a little bit more to figure out what kind of configurations we'd want to provide.

Personally, I would prefer to not make it possible to completely ignore the check (as it prevents unwanted calls to Shopify for every app), but I'm all for making it possible to ignore specific cases.

Could you please give me an example of a valid User-Agent that would trip isbot?

@probably-not
Copy link
Author

Hi @paulomarg, sorry for the delay in answering (been a bit busy on my end).

As a simple example that tripped my devs up - when using Amazon Cloudfront with an API Gateway and a Lambda answering API calls, if there is a misconfiguration somewhere along the line (i.e. one of the layers, the CDN, the Gateway not passing on all of the headers) then isbot will mark that the request is a bot no matter what (looks like all Amazon related user agents get marked from my tests).

Like I mentioned in the PR, this is a major problem when the developer is not in full control of the infrastructure. I have no problem if I just throw a machine online and point a domain name to it's public IP, but the second things get more involved (which is a certainty at companies with growing teams), then the dev has no control over the way the infrastructure reacts.

This, combined with the fact that the isbot blocking functionality is pretty much undocumented (I only came across it by looking through Github issues and the code itself), and the fact that there's 0 logging on why the request is being blocked (I even turned on full debug logs and got nothing in our application) makes this library impossible to work with as is.

@lizkenyon
Copy link
Contributor

Hey @probably-not!

Thank you for your patience with this. We would like to get this merged.

A couple of questions and requests for you!

  • Could you add some documentation for these new configuration settings?
  • Do we need both of the configuration settings, or would one be enough? I think it would make more sense to allow apps to turn off bot checks, and then they can add their own custom approach if they want to, so I was thinking whether it made more sense to just have a skipBotCheck setting.
  • Could you generate a changeset for this?
  • Could you add a test to cover the new configuration?

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

Successfully merging this pull request may close these issues.

None yet

3 participants