notifications: mark notifications to self "pre-read"
When a user e.g. comments on its own pull request, that user receives a notification about its own comment. This is slightly dubious behavior, but at least brings a level of continuity to the notification history.
However, at the very least, the notification should not show as unread.
cache: when invalidating a cache, always just delete all 'live cache' records instead of marking them inactive
Keep it simple. Adding the record again might be slightly more expensive than just updating the active flag but instead we get get a simpler model and automatic cleanup without using the cache-keys paster command.
forms: don't use secure forms with authentication token for GET requests
The token is secret and should never be used in forms posted with GET which are URL encoded. aef21d16a262 was too aggresive in using secure forms everywhere and did thus also incorrectly use them for forms posted with GET.
Some token leakage was reported by Gjoko Krstic <gjoko@zeroscience.mk> of Zero Science Lab.
login: use server-relative URLs in came_from correctly
Using h.url to combine came_from with query parameters caused the SCRIPT_NAME to incorrectly be prepended to came_from, even though it was already present. This was not a problem if the Kallithea instance was served directly from the server root ('/') as is common, but broke setups where Kallithea was served from a prefix.
The login controller uses the came_from query argument to determine the page to continue to after login.
Previously, came_from specified only the URL path (obtained using h.url.current), and any URL query parameters were passed along as separate (additional) URL query parameters; to obtain the final redirect target, h.url was used to combine came_from with the request.GET.
As of this changeset, came_from specifies both the URL path and query string (obtained using request.path_qs), which means that came_from can be used directly as the redirect target (as always, WebOb handles the task of expanding the server relative path to a fully qualified URL). The mangling of request.GET can also be removed.
The login code appended arbitrary, user-supplied query parameters to URLs by calling the Routes URLGenerator (h.url) with user-supplied keyword arguments. This construct is unfortunate, since url only appends _unknown_ keyword arguments as query parameters, and the parameter names could overlap with known keyword arguments, possibly affecting the generated URL in various ways. This changeset removes this usage from the login code, but other instances remain.
(In practice, the damage is apparently limited to causing an Internal Server Error when going to e.g. "/_admin/login?host=foo", since WebOb returns Unicode strings and URLGenerator only allows byte strings for these keyword arguments.)
Even though only server-relative came_from URLs were ever generated, the login controller allowed fully qualified URLs (URLs including scheme and server). To avoid an open HTTP redirect (CWE-601), the code included logic to prevent redirects to other servers. By requiring server-relative URLs, this logic can simply be removed.
Note: SCRIPT_NAME is still not validated and it is thus possible to redirect from one app to another on the same netloc.
example.com is explicitly reserved for this purpose. Using that means we won't accidentally hammer a real server or real email address if an example value escapes into the wild, e.g. in an automated test.
The domain "kallithea.example.com" has been used throughout to refer to the example Kallithea server.
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.