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

Add nag notice and new script for checking headers #136

Merged
merged 10 commits into from May 6, 2019

Conversation

Projects
None yet
4 participants
@dshanske
Copy link
Member

commented Apr 24, 2019

This adds in a script that will nag you to run it until you do. Hopefully this will help educate people on the dangers of not passing Auth headers.

@dshanske dshanske requested a review from pfefferle Apr 24, 2019

Show resolved Hide resolved indieauth.php Outdated
Show resolved Hide resolved languages/indieauth.pot Outdated
@chrisaldrich

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Manually testing this on my site generally seems okay, but I think it contains a logic error because it is returning what must be a false positive.

I see the message in the admin UI and can click on the test which returns the message "Alternate Header Found. You are good to go." However, when attempting to actually log into Monocle, I get the same 403 error saying that it couldn't find the bearer token, and it won't let me log in. So obviously I'm not "good to go."

From a UI perspective something like "Your headers are properly configured and accessible." may be better than the "You are good to go" which may be harder for non-English speakers. Additionally wrapping that message in an anchor that will redirect to their admin UI might be nice.

(Originally posted on https://boffosocko.com/2019/04/25/55750053/)

@dshanske

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2019

The alternative header is REDIRECT_HTTP_AUTHORIZATION used by some servers. I didn't in my code ensure that the bearer token is being returned and should... just that the header existed. May ask for another check after some changes

@dshanske

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2019

@chrisaldrich Can you check with the new code? It now checks not only for the existence of the header but that it contains the test payload

@chrisaldrich

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@dshanske, I’ve removed the prior version and reinstalled d5b91a0. I don’t get the same original notification (I’m suspecting because the original stored a value in my db that unflags the nag). However, when rerunning the diagnostic script at /wp-admin/admin.php?page=indieauth I get the same positive response that “Alternate Header Found. You should be able to use all clients.” Sadly, I’m still getting the same old 403 when attempting to log into Monocle.

(Original posted at https://boffosocko.com/2019/04/25/55750053/#comment-259564)

@dshanske

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

@pfefferle Could you please review and permit merging on this? I realized that I was focusing on a token retrieval issue in the client code, not the token verification code. This is now fixed, and the the test must be run for all new users to ensure they check.

@pfefferle

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Will do later today!

@dshanske

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Thanks... I know the header issue affects a lot of people who use shared hosting and other projects are getting issues opened.

@pfefferle

This comment has been minimized.

Copy link
Member

commented May 2, 2019

In general: I would prefer a dedicated test-page, like provided by the OpenID plugin, that runs all needed checks.

Bildschirmfoto 2019-05-02 um 09 40 27

@dshanske

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

I could build it into the settings page, but then it would run the check every time you retrieved the page... which I suppose is fine. What do you think?

@pfefferle

This comment has been minimized.

Copy link
Member

commented May 2, 2019

We could add an extra settings page for the tests and add a link to the actual/main settings page.

@@ -0,0 +1,17 @@
<?php
echo '<strong>';

This comment has been minimized.

Copy link
@pfefferle

pfefferle May 2, 2019

Member

this is a template file, I would prefer to not echo the HTML tags.

public function admin_notices() {
if ( ! get_option( 'indieauth_header_check', 0 ) ) {
echo '<div class="notice notice-warning">';
_e( 'In order to ensure IndieAuth tokens will work please perform this check:', 'indieauth' );

This comment has been minimized.

Copy link
@pfefferle

pfefferle May 2, 2019

Member

please wrap the message in a <p>, to match the WordPress style.

@pfefferle

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Can we use getenv("auth_head") to check if the auth-header is disabled?

-- https://stackoverflow.com/a/34080802/1731334

@dshanske

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Again, seems to be the same sort of solution that requires a check.

@dshanske

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

@pfefferle If yhou look at the Stack OVerflow, that still requires you edit htaccess, so therefore the solution of adding the header is better.

dshanske added some commits May 5, 2019

Tweak new auth test to run embedded inside the settings page and allo…
…w for a more detailed diagnostic return in future
@dshanske

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

@pfefferle Can you review again? This time, in addition to the diagnostic script(which I want to access not logged in as people keep filing issues and I can check server support without them or compromising anything), the script is run from the settings page and embeds a large failure notice in the page if it doesn't work. Otherwise, it will still nag to check using the original script.

@dshanske

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Addresses #135 #91 #131

@dshanske dshanske merged commit 44246d9 into indieweb:master May 6, 2019

1 of 2 checks passed

codeclimate 3 issues to fix
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.