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

Close the socket when authentication verification fails #262

Merged
merged 6 commits into from Nov 22, 2018

Conversation

@dominykas
Copy link
Contributor

dominykas commented Nov 22, 2018

Closes #260.

dominykas added 4 commits Nov 20, 2018
@@ -65,7 +66,8 @@ internals.schema = Joi.object({
]),
index: Joi.boolean(),
timeout: Joi.number().integer().min(1).allow(false),
maxConnectionsPerUser: Joi.number().integer().min(1).allow(false).when('index', { is: true, otherwise: Joi.valid(false) })
maxConnectionsPerUser: Joi.number().integer().min(1).allow(false).when('index', { is: true, otherwise: Joi.valid(false) }),
verify: Joi.number().integer()

This comment has been minimized.

Copy link
@dominykas

dominykas Nov 22, 2018

Author Contributor

@hueniverse you mentioned you'd like to see a min here? Aside from Joi trickery to achieve that (ref a key from another object) - is there really a need for that?

This comment has been minimized.

Copy link
@hueniverse

hueniverse Nov 22, 2018

Member

The point of making sure this is not less than the heartbeat rate is to set expectations. If you want to verify the connection faster than the heartbeat check, it will still only happen as fast as messages are passing through or the heartbeat check.

Also, verify is not very descriptive. I prefer minAuthVerifyIntervalMsec.

return true;
}

if (Date.now() - this.auth._verified < this._listener._settings.auth.verify) {

This comment has been minimized.

Copy link
@dominykas

dominykas Nov 22, 2018

Author Contributor

I'm not adding a way to disable this completely - should I? By e.g. setting verify: false?

This comment has been minimized.

Copy link
@hueniverse

hueniverse Nov 22, 2018

Member

Sounds reasonable. false works for me.

@@ -263,6 +264,11 @@ internals.Socket.prototype._lifecycle = function (request) {
throw Boom.badRequest('Message missing id');
}

if (!this._verifyAuth()) {

This comment has been minimized.

Copy link
@dominykas

dominykas Nov 22, 2018

Author Contributor

I only added this for received messages, because of the heartbeat. If the server receives no heartbeat - it will disconnect anyways. There is a chance of someone configuring a very long heartbeat timeout and keeping on pushing messages, but is that an issue?

hueniverse added 2 commits Nov 22, 2018
@hueniverse hueniverse merged commit 2dc88aa into hapijs:master Nov 22, 2018
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@hueniverse hueniverse self-assigned this Nov 22, 2018
@hueniverse hueniverse added this to the 9.0.3 milestone Nov 22, 2018
hueniverse added a commit that referenced this pull request Nov 23, 2018
@dominykas dominykas deleted the dominykas:auth-verify branch Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.