search

freedomofpress / securethenews

An automated scanner and web dashboard for tracking TLS deployment across news organizations
https://securethe.news
GNU Affero General Public License v3.0
102 stars 25 forks source link

Fix bug in computing "Available over HTTPS" #74

Closed garrettr closed 7 years ago

garrettr commented 7 years ago

Parker noticed that abcnews.go.com was listed as being "Available over HTTPS" on Secure the News, but when accesssed in a web browser it appears to downgrade HTTPS to HTTP. We checked the headers with curl -I, which shows that it is not serving an HTTP redirect to the insecure origin; however, it may be using another technique to redirect (e.g. <meta redirect> or Javascript).

Without thoroughly investigating exactly what abcnews.go.com is doing, it is clear that is confusing pshtt, which returns "Downgrades HTTPS": null while simultaneously returning "Valid HTTPS": true. This appears to be a bug in pshtt. This issue is compounded by using the ! negation operator in the leaderboard template, because that will type coerce null to false, which causes Secure the News to incorrectly display "Availble over HTTPS".

This commit fixes the mistaken results displayed on the leaderboard template, and I will file a follow-up issue on pshtt to address the issue with the scan results.

conorsch commented 7 years ago

@garrettr Thanks for the super clear explanation on this, and hurray for upstream contributions! I will check this at the server and rescan ABC to confirm the grade changes.

garrettr commented 7 years ago

@conorsch This is only a frontend change, so there is no need to rescan. However, you will need to rebuild the frontend assets (I believe this is handled by the deployment automation) and you might need to bounce the Cloudflare cache to get the new JS running on the live site.

conorsch commented 7 years ago

@garrettr Thanks for the clarification. Yes, the automation runs the gulp build:production, which should be what we want. Will bust cache manually after deploy.

conorsch commented 7 years ago

We're most of the way there—the leaderboard shows the updated value (i.e. Available over HTTPS is now false):

abc-news-leaderboard

However the individual site page (https://securethe.news/sites/abc-news) is still showing true:

abs-news-individual-page

This is after manually purging all cached entities for the STN site.

garrettr commented 7 years ago

However the individual site page (https://securethe.news/sites/abc-news) is still showing true

@conorsch Ah, bitten by the duplication of that logic in JS and Python! I knew it would happen sooner or later. Give me a minute and I'll push another commit to fix.

garrettr commented 7 years ago

Upstream issue filed: https://github.com/dhs-ncats/pshtt/issues/49.

garrettr commented 7 years ago

After the change in API semantics in the upstream fix, "Downgrades HTTPS" may now only be True or False. We should upgrade pshtt on the server as soon as a new release is available. In the meantime, this fix should not be merged due to another related bug that was identified and fixed as part of #50 (see point 2., "If a site does have HTTPS, there were some cases where the previous code would incorrectly return null instead of false.")