Skip to content

Commit

Permalink
Update server validation logic.
Browse files Browse the repository at this point in the history 
This PR removes .ogg file check (supported only by very old servers). Other enhancements in server validation logic -
* Reject domains with no organizations. 
* Convert validation methods to async await
* Add messages.js for returning error message strings.

Fixes: zulip#596, zulip#573.
  • Loading branch information
@kanishk98
kanishk98 committed Jun 18, 2019
1 parent dd78421 commit 415aa89
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 70 deletions.
133 changes: 63 additions & 70 deletions app/renderer/js/utils/domain-util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const escape = require('escape-html');
const Logger = require('./logger-util');

const RequestUtil = require(__dirname + '/../utils/request-util.js');
const Messages = require(__dirname + '/../../../resources/messages.js');

const logger = new Logger({
file: `domain-util.log`,
Expand Down Expand Up @@ -101,19 +102,50 @@ class DomainUtil {
return false;
}

async checkCertError(domain, serverConf, error, silent) {
if (silent) {
// since getting server settings has already failed
return serverConf;
} else {
// Report error to sentry to get idea of possible certificate errors
// users get when adding the servers
logger.reportSentry(new Error(error));
const certErrorMessage = Messages.certErrorMessage(domain, error);
const certErrorDetail = Messages.certErrorDetail();

const response = await dialog.showMessageBox({
type: 'warning',
buttons: ['Yes', 'No'],
defaultId: 1,
message: certErrorMessage,
detail: certErrorDetail
});
if (response === 0) {
// set ignoreCerts parameter to true in case user responds with yes
serverConf.ignoreCerts = true;
try {
return await this.getServerSettings(domain, serverConf.ignoreCerts);
} catch (err) {
if (error === Messages.noOrgsError(domain)) {
throw new Error(error);
}
return serverConf;
}
} else {
throw new Error('Untrusted certificate.');
}
}
}

// ignoreCerts parameter helps in fetching server icon and
// other server details when user chooses to ignore certificate warnings
checkDomain(domain, ignoreCerts = false, silent = false) {
async checkDomain(domain, ignoreCerts = false, silent = false) {
if (!silent && this.duplicateDomain(domain)) {
// Do not check duplicate in silent mode
return Promise.reject('This server has been added.');
throw new Error('This server has been added.');
}

domain = this.formatUrl(domain);
const checkDomain = {
url: domain + '/static/audio/zulip.ogg',
...RequestUtil.requestOptions(domain, ignoreCerts)
};

const serverConf = {
icon: defaultIconUrl,
Expand All @@ -122,70 +154,29 @@ class DomainUtil {
ignoreCerts
};

return new Promise((resolve, reject) => {
request(checkDomain, (error, response) => {
// If the domain contains following strings we just bypass the server
const whitelistDomains = [
'zulipdev.org'
];

// make sure that error is an error or string not undefined
// so validation does not throw error.
error = error || '';

const certsError = error.toString().includes('certificate');
if (!error && response.statusCode < 400) {
// Correct
this.getServerSettings(domain, serverConf.ignoreCerts).then(serverSettings => {
resolve(serverSettings);
}, () => {
resolve(serverConf);
});
} else if (domain.indexOf(whitelistDomains) >= 0 || certsError) {
if (silent) {
this.getServerSettings(domain, serverConf.ignoreCerts).then(serverSettings => {
resolve(serverSettings);
}, () => {
resolve(serverConf);
});
} else {
// Report error to sentry to get idea of possible certificate errors
// users get when adding the servers
logger.reportSentry(new Error(error));
const certErrorMessage = `Do you trust certificate from ${domain}? \n ${error}`;
const certErrorDetail = `The organization you're connecting to is either someone impersonating the Zulip server you entered, or the server you're trying to connect to is configured in an insecure way.
\nIf you have a valid certificate please add it from Settings>Organizations and try to add the organization again.
\nUnless you have a good reason to believe otherwise, you should not proceed.
\nYou can click here if you'd like to proceed with the connection.`;

dialog.showMessageBox({
type: 'warning',
buttons: ['Yes', 'No'],
defaultId: 1,
message: certErrorMessage,
detail: certErrorDetail
}, response => {
if (response === 0) {
// set ignoreCerts parameter to true in case user responds with yes
serverConf.ignoreCerts = true;
this.getServerSettings(domain, serverConf.ignoreCerts).then(serverSettings => {
resolve(serverSettings);
}, () => {
resolve(serverConf);
});
} else {
reject('Untrusted Certificate.');
}
});
}
} else {
const invalidZulipServerError = `${domain} does not appear to be a valid Zulip server. Make sure that \
\n (1) you can connect to that URL in a web browser and \n (2) if you need a proxy to connect to the Internet, that you've configured your proxy in the Network settings \n (3) its a zulip server \
\n (4) the server has a valid certificate, you can add custom certificates in Settings>Organizations`;
reject(invalidZulipServerError);
try {
return await this.getServerSettings(domain, serverConf.ignoreCerts);
} catch (err) {
// If the domain contains following strings we just bypass the server
const whitelistDomains = [
'zulipdev.org'
];

// make sure that error is an error or string not undefined
// so validation does not throw error.
const error = err || '';

const certsError = error.toString().includes('certificate');
if (domain.indexOf(whitelistDomains) >= 0 || certsError) {
try {
return await this.checkCertError(domain, serverConf, error, silent);
} catch (err) {
throw err;
}
});
});
} else {
throw Messages.invalidZulipServerError(domain);
}
}
}

getServerSettings(domain, ignoreCerts = false) {
Expand All @@ -207,9 +198,11 @@ class DomainUtil {
alias: escape(data.realm_name),
ignoreCerts
});
} else {
reject(Messages.noOrgsError(domain));
}
} else {
reject('Zulip server version < 1.6.');
reject(response);
}
});
});
Expand Down
27 changes: 27 additions & 0 deletions app/resources/messages.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
class Messages {
invalidZulipServerError(domain) {
return `${domain} does not appear to be a valid Zulip server. Make sure that
\n • You can connect to that URL in a web browser.\
\n • If you need a proxy to connect to the Internet, that you've configured your proxy in the Network settings.\
\n • It's a Zulip server. (The oldest supported version is 1.6).\
\n • The server has a valid certificate. (You can add custom certificates in Settings > Organizations).`;
}

noOrgsError(domain) {
return `${domain} does not have any organizations added.\
\nPlease contact your server administrator.`;
}

certErrorMessage(domain, error) {
return `Do you trust certificate from ${domain}? \n ${error}`;
}

certErrorDetail() {
return `The organization you're connecting to is either someone impersonating the Zulip server you entered, or the server you're trying to connect to is configured in an insecure way.
\nIf you have a valid certificate please add it from Settings>Organizations and try to add the organization again.
\nUnless you have a good reason to believe otherwise, you should not proceed.
\nYou can click here if you'd like to proceed with the connection.`;
}
}

module.exports = new Messages();

0 comments on commit 415aa89

Please sign in to comment.