It turns out the user.is_authenticated check is redundant, since it's True for both anonymous users and logged in users, and API key users are handled prior to the check.
security: apply CSRF check to all non-GET requests
The automatic CSRF protection was broken for POST requests with no request payload parameters (but possibly containing request URI parameters); a security hole was narrowly avoided because the code base quite consistently checks the request method in the same way, and because of browser protection against PUT/DELETE CSRF attacks.
Since explicit is better than implicit, the better way of checking the HTTP request method is to simply check request.method, instead of checking if request.POST is non-empty, which is subtly different (it doesn't catch POST requests if all parameters are in the query string) and non-obvious (because it also applies to PUT requests).
The commit also fixes some tests which relied on the CSRF protection being broken. It does not fix all the controllers that still does the misleading request.POST check, but since the CSRF check has now been tightened, those are no longer a potential security issue.
pull requests: remove immediate invocation of the function defined in pullrequest_data.html
pullrequest_data.html behaved differently when included and when used as namespace. Keep it simple and just let it define the pullrequest_overview function.
pull requests: don't filter on repo when finding comments for a PR
Filtering on repo while finding comments for a PR would miss some. Fix that.
The existing data model is inconsistent; PRs already have two repos and programming errors could easily make the one on a comment wrong. (Revision comments do however need the repo reference.)
auth: use HMAC-SHA1 to calculate password reset token
The use of standard cryptographic primitives is always preferable, and in this case allows us not to worry about length extension attacks and possibly any number of issues that I'm not presently aware of.
This is a better implementation of password reset function, which doesn't involve sending a new password to the user's email address in clear text, and at the same time is stateless.
The old implementation generated a new password and sent it in clear text to whatever email assigned to the user currently, so that any user, possibly unauthenticated, could request a reset for any username or email. Apart from potential insecurity, this made it possible for anyone to disrupt users' workflow by repeatedly resetting their passwords.
The idea behind this implementation is to generate an authentication token which is dependent on the user state at the time before the password change takes place, so the token is one-time and can't be reused, and also to bind the token to the browser session.
The token is calculated as SHA1 hash of the following:
* user's identifier (number, not a name) * timestamp * hashed user's password * session identifier * per-application secret
We use numeric user's identifier, as it's fixed and doesn't change, so renaming users doesn't affect the mechanism. Timestamp is added to make it possible to limit the token's validness (currently hard coded to 24h), and we don't want users to be able to fake that field easily. Hashed user's password is needed to prevent using the token again once the password has been changed. Session identifier is an additional security measure to ensure someone else stealing the token can't use it. Finally, per-application secret is just another way to make it harder for an attacker to guess all values in an attempt to generate a valid token.
When the token is generated, an anonymous user is directed to a confirmation page where the timestamp and the usernames are already preloaded, so the user needs to specify the token. User can either click the link in the email if it's really them reading it, or to type the token manually.
Using the right token in the same session as it was requested directs the user to a password change form, where the user is supposed to specify a new password (twice, of course). Upon completing the form (which is POSTed) the password change happens and a notification mail is sent.
The test is updated to test the basic functionality with a bad and a good token, but it doesn't (yet) cover all code paths.
The original work from Andrew has been thorougly reviewed and heavily modified by Søren Løvborg.
email: send comment and pullrequest mails with the author's name in 'From'
When emails are sent for comments and pullrequest invitations, set the From header to: Author's Name (no-reply) <generic email address>
Using the name of the person that causes the email, makes the emails more useful and interpretable for the recipient of the emails. To avoid replies directly to the author, triggering an 'offline' email discussion that is not visible in the Kallithea interface, a generic 'no-reply' email address is used instead of the author's email address. This approach is assumed to be accepted by spam filters, as several other web services are using the same approach.
The sender used for other email types, e.g. password reset mails, is untouched and remains the value configured in app_email_from.
The sender used for the SMTP envelope is untouched as well.
Show an extra checkbox next to the first selected checkbox to be able to specify single revision ranges (which is different from the start of an open range).
When two revisions are selected, hide all other checkboxes to make it impossible to select more.
auth: avoid random auth_internal failures - add explicit import of auth_internal to user admin
auth_internal would often have been loaded by the custom auth module loader and available as auth_modules.auth_internal ... but sometimes it wasn't and navigating to Add User would fail with:
File '.../kallithea/controllers/admin/users.py', line 155 in new c.default_extern_type = auth_modules.auth_internal.KallitheaAuthPlugin.name AttributeError: 'module' object has no attribute 'auth_internal'
users: fix missing c.readonly in UsersController.update error rendering
The c.readonly attribute, while correctly set in UsersControllers.edit, was not assigned in UsersControllers.update, causing a template error if the form validation failed. This commit unifies template rendering in separate method to fix this and avoid future problems.
pullrequests: remove reviewer list during PR creation
There is not much use for it before the actual diff is shown ... and removing it also removes a bit of duplicated code that otherwise should be maintained in two places.
vcs: invalidate repo caches _after_ a push finishes
Caches were invalidated right after creating the result iterator, before actually applying the change. Caches would thus be refreshed too early and soon be outdated.
This bug was especially seen causing errors with missing revisions when creating PRs right after pushing.
Normalize phrasing and capitalization of repository locking messages. This also avoids the piecing together of sentence fragments in a way that can cause i18n headaches.
The alternative text should convey the same information as the image, something which the text "gravatar" does not. (In a context where the gravatar is used on its own, the username could be a useful alt-text - and title-text - but it's not apparent that this is ever the case.)
docs: improve documentation of beaker session configuration
beaker.session.auto is dropped; it defaults to false and there is no reason to ever set it true for Kallithea.
beaker.session.cookie_path and secure are dropped; like cookie_domain, they should automatically be set to the right value. * * * beaker.session.cookie_expires MUST have the default value of True to provide the default value of 'browser session lifetime' when not enabling 'remember' in the login box. The cookie life is hardcoded to 365 days when remember is selected.
comments: fix warning when unloading page with unsaved comments
e87baa8f1c5b broke the existing check. Instead, only set the comment-inline-form class when it actually is an inline form and use that class for finding comments.
comments: browser display of context around url #targets should only be used for diff comments
24d01c64c5f3 had the unintended side-effect of not only highlighting the linked line in file view but also the spacing above it. For now, instead of changing the markup, restrict the fancy #target offsets to comments.
pullrequests: avoid unnecessary scroll-bar on short lists of available updates with Chrome
The surrounding div will apparently take size after the content but miss that there also is 1 pixel border ... and when drawing it realizes that there isn't enough space and it adds scroll bars anyway.
Work around that by giving the surrounding box 1 pixel padding.
The canvas will be set to the right size when the page has loaded. Until the page has been loaded, Chrome has been seen to set the canvas to be so wide that it would cover other visible links, thus preventing navigating away from the page before it has been fully loaded.
This will make sure the canvas never takes up more space than intended.