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

Calling Accounts.config on the client only is an invisible security hole #828

Closed
jzgoda opened this Issue Mar 15, 2013 · 10 comments

Comments

Projects
None yet
4 participants
@jzgoda

jzgoda commented Mar 15, 2013

I'm not sure if this is a software issue, or just a documentation issue, but the Accounts.config option forbidClientAccountCreation does not work if it's only specified in the client or server, it needs to be specified in both. If in the client, it only removes the create account link, but calls from the JS console are still able to create new accounts. If it's placed in both, it works as the documentation says it should.

(FYI, I'm really new to Meteor, and come from a more of a Java/Actionscript background...so maybe any seasoned Javascript developer would have realized this.)

@cmather

This comment has been minimized.

Contributor

cmather commented Mar 20, 2013

Hi @jzgoda, Thanks for highlighting this. I don't think any seasoned javascript developer would have realized this :-). I'll bring this up as a documentation enhancement item. But in the mean time, is there an issue with moving this config code to a location that gets loaded on the client and the server? I actually haven't tried it, so let me know if you run into trouble. Is your project organized by folders or are you using the if (Meteor.isClient) {...} if (Meteor.isServer) {...} blocks?

@jzgoda

This comment has been minimized.

jzgoda commented Mar 20, 2013

My code is organized by folders, not the blocks.
I moved that code to a file that would be shared by both, and it works as expected. Should have thought of that (as I'm already doing that for Collection definitions), thanks for the suggestion!

@cmather

This comment has been minimized.

Contributor

cmather commented Mar 21, 2013

Hi @jzgoda, Sure thing. It's probably something that should be enhanced in the docs. Thanks for pointing out that it doesn't quite make sense! And glad you got it working.

@glasser

This comment has been minimized.

Member

glasser commented Mar 26, 2013

I feel like we should make this impossible to get wrong, instead of just something that needs more docs. After all, if you get it wrong it's a a subtle security hole! Similarly, my suggestion for fixing #765 also requires an Accounts.config call that appears to work if you only put it on the client but has a security hole if you don't put it on the server too.

We should make it an error to get it wrong, instead of a subtle security hole.

My suggestion is that we change Accounts.config to only run on the server, and make it use __meteor_runtime_config__ to publish to the client anything that needs to be there too. This means that if you put it in a client code block you get an error.

Another alternative would be figuring out how to tell in a package API call if it is called from dual client/server code, but that seems too magic.

@cmather

This comment has been minimized.

Contributor

cmather commented Mar 26, 2013

Yeah I agree.

Sent from my iPhone

On Mar 25, 2013, at 10:54 PM, David Glasser notifications@github.com wrote:

I feel like we should make this impossible to get wrong, instead of just something that needs more docs. After all, if you get it wrong it's a a subtle security hole! Similarly, my suggestion for fixing #765 also requires an Accounts.config call that appears to work if you only put it on the client but has a security hole if you don't put it on the server too.

We should make it an error to get it wrong, instead of a subtle security hole.

My suggestion is that we change Accounts.config to only run on the server, and make it use meteor_runtime_config to publish to the client anything that needs to be there too. This means that if you put it in a client code block you get an error.

Another alternative would be figuring out how to tell in a package API call if it is called from dual client/server code, but that seems too magic.


Reply to this email directly or view it on GitHub.

n1mmy added a commit that referenced this issue Apr 15, 2013

@glasser

This comment has been minimized.

Member

glasser commented Oct 2, 2013

A simpler solution (and more backwards-compatible) than "you have to put it in server-only code": on the server, if you call Accounts.config, then it sets a single "Accounts.config was called" flag in __meteor_runtime_config__.

On the client, if you call Accounts.config, and the flag isn't set, then throw/log an error that basically says "It looks like you called Accounts.config only on the client. It will not take effect." Maybe this is just a Meteor._debug and you only see it if you look at the client console.

glasser referenced this issue Oct 2, 2013

Allow passing function predicate for domain checks
Escape regexp construction
Don't use &= with Booleans
@glasser

This comment has been minimized.

Member

glasser commented Oct 4, 2013

Fixed in 6fcff6d.

@glasser glasser closed this Oct 4, 2013

@MiroHibler

This comment has been minimized.

MiroHibler commented Dec 8, 2014

I'm doing:

// Server:
Meteor.methods({
    'Accounts.config': function ( options ) {
        // A hack to avoid "Can't set `<options>` more than once" exception
        if ( __meteor_runtime_config__.accountsConfigCalled ) {
            throw new Meteor.Error( 500,
                'Options already set',
                'Options already set [500]'
            );
        }

        Accounts.config( options );
    }
}

// Client:
Meteor.call( 'Accounts.config', options, function ( error ) {
    if ( !error ) {
        Accounts.config( options );
    }
});

...and getting that (confusing) "Accounts.config was called on the client but not on the server..." warning.

Judging by 6fcff6d, __meteor_runtime_config__.accountsConfigCalled never gets set to true on the client thus causing this warning.

@glasser

This comment has been minimized.

Member

glasser commented Dec 8, 2014

I'm not sure why you're calling Accounts.config over DDP. That seems like a security hole.

@MiroHibler

This comment has been minimized.

MiroHibler commented Dec 8, 2014

I'm writing a package based on accounts-password package and wanted to get sendVerificationEmail and forbidClientAccountCreation values for the purpose of deciding on a different process flow.

I tend to avoid accessing private parameters (i.e. Accounts._config object) and didn't know of any other way ;)

So, I'm allowing for setting of Accounts.config() parameters (and catching them ;) within my package's init(options) method on the client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment