62 resolve 301 permanent redirects #97
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Owner
conorgil
commented
Sep 24, 2018
conorgil
commented
- Includes data updates from twofactorauth.org
- Resolves 301 permanent redirects (Resolve URL redirects where possible #62)
- Resolves 302 temporary redirects that only upgrade from HTTP to HTTPS
This commit take the existing JSON for the site configs and alphabetizes it by origin. It does not make ANY other changes. The whole purpose for this is that moving forward I am going to alphabetize the keys to make the diffs easier to reivew. I want to make it easy to tell which changes have come down from twofactorauth.org and which changes I have made locally.
This makes it easier to execute local scripts during development. For example: npm run ts-node path/to/some/temporary/dev/script.ts
This will make it easier to identify when an entry upgrades from HTTP to HTTPS since the diff will be inline instead of showing up as a deletion and addition elsewhere in the file.
The function isRedirectHttpToHttps() was not returning false as a default, which was causing incorrect resolution of URLs. I added a max redirect count while debugging the incorrect resolution. It is not strictly necessary anymore since I fixed the bug in isRedirectHttpToHttps(), but might as well leave it as a fail safe to avoid loops. Before saving to file, I sort the keys alphabetically while ignoring the scheme. This makes it easier to visually parse the diff to see which changes have been made recently. For example, if a URL gets upgraded from HTTP to HTTPS, then it will remain in the same spot in the list and show a small diff adding the 's' to the scheme. This doesn't solve all cases of reading the diff though. For example, it is still difficult to quickly identify URLs that have been upgraded based on resolving from a naked domain to the www domain because the show up in the diff as a deletion of the naked domain and an entry of the www domain elsewhere in the list.
This data has not been updated since it was initially added on April 6, 2018 (d0878fe). This update incorporates the latest data from twofactorauth.org. It also resolves all 301 redirects and some 302 redirects. This means that the actual origin in the dataset matches the origin in the browser when a user navigates to a site, which will allow 2FAN to perform a simple string compare to determine whether the current site is in the dataset or not.
The field img is not used at all, so we can remove it to save space. The field url is not used because the key of the map is the URL.
The other data updates are from running the generation script again. I don't know why those changes weren't picked up last time the script ran, but they appear to be accurate.
How can the tab not exist? I do not know. The docs likely say something about this situation, but this fix takes care of it regardless.
It was way too error prone to pass around strings because apparently I switch between working with url.origin in some places and url.hostname in others. By passing around the URL, the calling code doesn't need to worry about passing the wrong thing. The function can take care of that internally and make sure that it always gets the right value from the URL.
This was another main place where I used strings instead of URLs and it really really matters here. In storage, I deal with hostnames NOT origins and it was way too easy to accidentally pass in the url.origin value here and screw things up. Why hostnames here instead of origins? Well, sites might upgrade from HTTP to HTTPS and if that happens, I want the same site settings to apply. Therefore, I store settings under the hostname, which does not include the URL scheme (http/https) like the origin does. I deal with origins in the actual matching of the current page URL because I want to require HTTPS wherever possible. Though, it might make more sense to match on the hostname instead of origin because if the site is HTTP, it could still support 2FA? Hrmmm.... gotta think on that one. And yes, a git comment is not the place to figure this out, but leaving my note here anyways. Deal.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.