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

Update phishing config request logic #244

Merged
merged 1 commit into from
Jun 17, 2020
Merged

Conversation

whymarrh
Copy link
Contributor

This change updates the PhishingController config request logic to:

  1. Use the official GitHub API for repo contents (see Replace Infura blacklist endpoint #219)
  2. Handle 403 responses, as the GH API will rate limit with that status code

Note that the current query interval, 180000 ms, will be fewer than 60 requests per hour:

For unauthenticated requests, the rate limit allows for up to 60 requests per hour. Unauthenticated requests are associated with the originating IP address, and not the user making requests.

And, per the docs, 304 responses don't count towards the rate limit:

Making a conditional request and receiving a 304 response does not count against your Rate Limit, so we encourage you to use it whenever possible.

See also:

This change updates the `PhishingController` config request logic to:

1. Use the official GitHub API for repo contents (see #219)
2. Handle 403 responses, as the GH API will rate limit with that status code

Note that the current query interval, 180000 ms, will be fewer than 60 requests
per hour:

> For unauthenticated requests, the rate limit allows for up to 60 requests
> per hour. Unauthenticated requests are associated with the originating IP
> address, and not the user making requests.

And, per the docs, 304 responses don't count towards the rate limit:

> Making a conditional request and receiving a 304 response does not count
> against your Rate Limit, so we encourage you to use it whenever possible.

See also:

- [GitHub REST API v3 - Media Types](https://developer.github.com/v3/media/)
- [GitHub REST API v3 - Rate limiting](https://developer.github.com/v3/#rate-limiting)
- [GitHub REST API v3 - Conditional requests](https://developer.github.com/v3/#conditional-requests)
@whymarrh whymarrh marked this pull request as ready for review June 16, 2020 22:08
@whymarrh
Copy link
Contributor Author

And, per the docs, 304 responses don't count towards the rate limit:

Making a conditional request and receiving a 304 response does not count against your Rate Limit, so we encourage you to use it whenever possible.

Though this might not apply to unauthenticated requests:

curl -sSL \
--dump-header /dev/fd/1 \
--output /dev/null \
--header 'Accept: application/vnd.github.v3.raw+json' \
--header 'If-None-Match: "85c5475f72353f82a0f712c1e7c9a6478ab1c7a7"' \
https://api.github.com/repos/MetaMask/eth-phishing-detect/contents/src/config.json

Running this sees the X-Ratelimit-Remaining decrease. 🤷🏽‍♂️ The existing interval should be fine, though NAT will definitely cause co-located users to hit that rate limit.

@whymarrh whymarrh requested a review from a team June 16, 2020 22:11
@whymarrh
Copy link
Contributor Author

I've tested this with the extension and it works as expected.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@whymarrh whymarrh merged commit e44802b into develop Jun 17, 2020
@whymarrh whymarrh deleted the phishing-config-endpoint branch June 17, 2020 13:27
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
This change updates the `PhishingController` config request logic to:

1. Use the official GitHub API for repo contents (see #219)
2. Handle 403 responses, as the GH API will rate limit with that status code

Note that the current query interval, 180000 ms, will be fewer than 60 requests
per hour:

> For unauthenticated requests, the rate limit allows for up to 60 requests
> per hour. Unauthenticated requests are associated with the originating IP
> address, and not the user making requests.

And, per the docs, 304 responses don't count towards the rate limit:

> Making a conditional request and receiving a 304 response does not count
> against your Rate Limit, so we encourage you to use it whenever possible.

See also:

- [GitHub REST API v3 - Media Types](https://developer.github.com/v3/media/)
- [GitHub REST API v3 - Rate limiting](https://developer.github.com/v3/#rate-limiting)
- [GitHub REST API v3 - Conditional requests](https://developer.github.com/v3/#conditional-requests)
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
This change updates the `PhishingController` config request logic to:

1. Use the official GitHub API for repo contents (see #219)
2. Handle 403 responses, as the GH API will rate limit with that status code

Note that the current query interval, 180000 ms, will be fewer than 60 requests
per hour:

> For unauthenticated requests, the rate limit allows for up to 60 requests
> per hour. Unauthenticated requests are associated with the originating IP
> address, and not the user making requests.

And, per the docs, 304 responses don't count towards the rate limit:

> Making a conditional request and receiving a 304 response does not count
> against your Rate Limit, so we encourage you to use it whenever possible.

See also:

- [GitHub REST API v3 - Media Types](https://developer.github.com/v3/media/)
- [GitHub REST API v3 - Rate limiting](https://developer.github.com/v3/#rate-limiting)
- [GitHub REST API v3 - Conditional requests](https://developer.github.com/v3/#conditional-requests)
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

2 participants