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

Logic issue with is_redirect method #66

Open
egyptiankarim opened this issue Mar 30, 2017 · 2 comments
Open

Logic issue with is_redirect method #66

egyptiankarim opened this issue Mar 30, 2017 · 2 comments
Labels
bug This issue or pull request addresses broken functionality

Comments

@egyptiankarim
Copy link
Contributor

The way is_redirect is currently written, a domain will get flagged as a redirect if all endpoints are down or otherwise code 400-ing. For example, imagine domain.tld, which only has its https endpoint up and happens to be returning for a 404 for its index. That will return true for is_redirect, which is confusing (and happening a bunch where I am).

I'm curious if that whole logic block can be simplified to check and see if any of the end points are returning a code 300. That entire block starting on line 527 might reasonably be reduced to something like:

redirection_codes = range(300, 309)

return (https.status in redirection_codes) or (http.status in redirection_codes) or (httpswww.status in redirection_codes) or (httpwww.status in redirection_codes)

I'll submit a pull request and we can hash it out there.

@konklone
Copy link
Collaborator

From #67:

I think we should add a conditional to the calculation that ensures that at least one of the endpoints is an external redirect. Something like:

# ...
  and (
    httpwww.redirect_eventually_to_external or 
    http.redirect_eventually_to_external or 
    httpswww.redirect_eventually_to_external or 
    https.redirect_eventually_to_external
  )

I closed #67, and am moving the proposed resolution here for tracking.

@konklone
Copy link
Collaborator

konklone commented Jun 5, 2017

Just noting for myself and others that this is still an issue, and affects at least one second-level domain in exim.gov:

https://s3-us-gov-west-1.amazonaws.com/cg-4adefb86-dadb-4ecf-be3e-f1c7b4f6d084/live/cache/pshtt/exim.gov.json

EDIT: URL is now https://s3-us-gov-west-1.amazonaws.com/cg-4adefb86-dadb-4ecf-be3e-f1c7b4f6d084/live/cache/pshtt/exim.gov.json

@konklone konklone added the WSC label Aug 25, 2017
@konklone konklone added the bug This issue or pull request addresses broken functionality label Mar 25, 2018
@hillaryj hillaryj removed the WSC label Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request addresses broken functionality
Projects
None yet
Development

No branches or pull requests

3 participants