Adds check for Google Crawler attempting to crawl Auth Routes #824
Conversation
There was a problem hiding this comment.
Seeing the same code copy/pasted twice in the same file make me think we should create a method for it.
It would also facilitate the implementation of the final solution (throwing vs 418) since it would be implemented only once.
With that in mind, I would argue that throwing should be used for something unexpected that we can't recover from. Here it feels to me that since we can respond with 418 (? I am a teapot ???) and we know it will happen I would vote for the response code.
However did you try to see what is the behaviour from the bot? Does it "understand" 418 ?
I personally love the respond with the 418 most. And I agree with avoid a throw, I normally subscribe to the "never write exceptions" camp. Ironically it was a Goggle that had convinced me. I think whether or not the bot understands the response isn't of importance to us. The main reason for the throw was to eject from the function and stop the auth process. Ending the request with a response I think sounds better based on your comment as well as the chat with Kevin. Regarding the duplication, I'm on the fence. Trying to be more comfortable with duplication with AHA |
|
@cquemin I changed my mind and extracted it. I'll have to add the response to it but now we can compare |
I think it looks a bit better now :D |
There was a problem hiding this comment.
Instead of 418, we should return a 410 status code so Google stops coming back to these endpoints: https://www.searchenginejournal.com/google-404-status/254429/
…e wrapped in error handling
…e wrapped in error handling
…e wrapped in error handling
| if (isbot(request.headers['User-Agent'])) { | ||
| logForBot({request, log, func: 'callback'}); | ||
| throw new ShopifyErrors.BotActivityDetected( | ||
| 'Invalid OAuth callback initiated by bot', | ||
| ); | ||
| } |
There was a problem hiding this comment.
@teddyhwang New callback bot handling.
Your last comment was on an out dated diff. Lots of back and forth on this with @mkevinosullivan and @paulomarg to decide the best course of action.
This is where we've landed.
Next step is to update the express app middleware to handle this error.
abharvey
changed the title
abharvey
changed the title
WHY are these changes introduced?
FixesRelated to #686Users were seeing logs with multiple auth requests coming in failing on missing cookies. We determined that in Chrome google's crawler was sending additional requests to the auth and auth/callback endpoints for our test app with the user-agent "GoogleOther" which would not have cookies set nor shop and other parameters available.
WHAT is this pull request doing?
Adds a check against the google crawler user agent and for missing shop params, then throws an error (based on needed discussion)
Type of change
Checklist
yarn changesetto create a draft changelog entry (do NOT update theCHANGELOG.mdfile manually)