Remove the built-in login/logout routes

shaunluttin commented 7 years ago

The problem with the logout route is that anybody can embed a hidden iframe pointing to this route, which would force a victim to log out without realizing it.

If you do that in a loop, this may result in a DOS as the victim would be permanently logged out.

Thank you @pinpointtownes

shaunluttin commented 7 years ago

Resolved with 1ca99cdaae89c75f82526aac540c8ea77b88a29c.

\cc @PinpointTownes

kevinchalet commented 7 years ago

Awesome, thanks @shaunluttin!

mttmccb commented 7 years ago

You might want to reconsider if the login side of it, I've modified the login route to handle to pass in the current route and the capture it in callback. The use case for this is you're either logged out or your session has expired and you request a page say /home and when you're logged you could attempt to redirect to that route instead of the root page. I haven't done it in a clean way as I'm doing that part outside the plugin, hence this state...

    public login(instruction: NavigationInstruction): Promise<any> {
        return this.userManager.signinRedirect({ data: instruction.queryParams });
    }

    public signInRedirectCallback(instruction: NavigationInstruction): Promise<any> {

        const callbackHandler = () => {
            return this.userManager.getUser().then((user) => {
                // getUser()
                // Sign in only if we do not already have a user;
                // otherwise, we receive a 'No matching state found in storage' error,
                // on a page refresh with stale state in the window.location.
                if (user === null || user === undefined) {
                    return this.userManager.signinRedirectCallback().then((oidcUser) =>
                      this.state.redirect = oidcUser ? oidcUser.state.url : '');
                }
            });
        };

        const postCallbackRedirect = () => {
            instruction.config.moduleId = this.openIdConnectConfiguration.loginRedirectModuleId;
        };

        return this.runHandlers(callbackHandler, postCallbackRedirect);
    }

If you want me to raise this as a separate issue let me know, not sure if I've explained it very well.

shaunluttin commented 7 years ago

\cc @PinpointTownes Thoughts? I.e. does keeping the /login route present any known security holes?

kevinchalet commented 7 years ago

@shaunluttin the login route is a bit less concerning, tho' it might be a potential vector attack if the authorization server includes a sort of anti-flooding protection for the authorization endpoint: repetitive requests sent from a malicious hidden iframe would trigger it, which would eventually prevent the user from logging in.

I do think that what you did in open-id-connect-user-block.ts to compensate the removal of the built-in routes was the right thing to do (i.e exposing helpers that allow triggering sign-in/sign-out using JS buttons/links).

aurelia-contrib / aurelia-open-id-connect

Remove the built-in login/logout routes #29