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

Limit avatar download attempts #1579

Merged
merged 11 commits into from Apr 11, 2018

Conversation

Projects
None yet
2 participants
@jcansdale
Contributor

jcansdale commented Apr 3, 2018

This PR limits the number of attempts that will be made to download avatar when a host requires authentication (which isn't currently supported for images).

GitHub Enterprise returns a 200/OK with a content type of text/html when authentication is required to download an avatar.

What this PR does

  • When an avatar returns a non-image content type, remember this for subsequent calls to the same host

Reviewers

Todo

  • We should refine the type and/or number of exceptions that can be thrown before a host is blacklisted to make it more robust.
    • Changed to only remember exceptions caused by hosts returning non-image content

Fixes #1547

@jcansdale jcansdale changed the title from [wip] Limit avatar download attempts to Limit avatar download attempts Apr 3, 2018

@jcansdale jcansdale requested review from sguthals and grokys Apr 4, 2018

@jcansdale jcansdale removed the request for review from sguthals Apr 4, 2018

@grokys

grokys approved these changes Apr 10, 2018

My only problem with this is that it will still be throwing an exception for each requested avatar. That could be avoided by putting the blacklisting into AvatarProvider. However given our schedule and that this is a verified solution to a relatively serious problem, I'm 👍.

jcansdale and others added some commits Apr 10, 2018

@grokys grokys merged commit b755fca into master Apr 11, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@grokys grokys deleted the 1547/limit-avatar-download-attempts branch Apr 11, 2018

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