files: fix Selection Link for multi-line selections
It broke in dacdea9fda2a when wrong casing in the translation lookup caused an undefined value which didn't show up in the UI and thus made the link unusable.
files: treat messages about 'File too big' as sentences and add a dot.
Make it more easy to read: File is too big to display Show full annotation anyway by treating them as two sentences: File is too big to display. Show full annotation anyway.
Similar messages about 'Changeset too big to display' in the changeset and pullrequest code are wrapped in an <h4> tag. Follow the same style when showing files or annotations.
files: support annotation on files larger than cut_off_limit
When requesting the annotation for a file larger than the cut_off_limit configured in the ini file, the only current option is to click the useless 'show as raw' (which is not an annotation).
Replace it with a link 'Show full annotation anyway' instead.
Commit b0774d79c7c95ec14ec6d23389d85ed544dd4b50 broke the 'Compare branches' button on the repository branches page, when attempting to replace a Yahoo UI click handler with jQuery.
Commit b0774d79c7c95ec14ec6d23389d85ed544dd4b50 broke the 'Compare bookmarks' button on the repository bookmarks page, when attempting to replace a Yahoo UI click handler with jQuery.
auth: note that we never emit authuser "cookies" for the default user
The only place where we set "authuser" in the session is in log_in_user, which is called only by the internal auth system and by auth plugins. The internal auth system cannot log a user in as the default user, because the default user doesn't have a password (and cannot have a password assigned). Auth plugins cannot log a user in as the default user, because the user doesn't have the right extern_type. As such, it's a bug if log_in_user is ever called with the default user (which this commit documents with an assert).
This realization makes the is_authenticated field of the authuser cookie redundant, as it's always True. It also emphasizes that is_default_user and is_authenticated are mutually exclusive.
auth: avoid setting AuthUser.is_authenticated for unauthenticated users
AuthUser.is_authenticated could be True for three reasons: because the user "was" the default user, because the user was authenticated by session cookie, or because the user was just authenticated by an auth module (including the internal auth module). In the last case, a session cookie is emitted (even when using container auth), so the last two cases are closely related.
This commit do that unauthenticated users (the first case) only get the is_default_user attribute set, and that the is_authenticated attribute only is set for authenticated users (for the second and third case).
This complicates some expressions, but allows others to be simplified. More importantly, it makes the code more explicit, and makes the "is_authenticated" name mean what it says.
(This will temporarily make the is_authenticated session value look even more weird than before.)
This makes makes a number of checks more readable.
The username of the default user is presently hardcoded to "default" (in db.User.DEFAULT_USER); this is currently what defines the default user, and this commit doesn't change that. (Even if the check that defines is_default_user is a comparison between user IDs and not usernames, the anonymous_user object used in the comparison is loaded by looking up the user named "default".)
All redirect does is to log "Generating 302 redirect" with logging the actual location and raise a WebOb HTTPFound exception, and the logging is redundant, as WebOb exceptions and their status codes are already logged.
Instead, just raise the exception directly, which is both explicit and simpler (and finally, gets rid of "return redirect" which never really returns).
All abort does is to look up the matching WebOb exception and raising that; so just raise it directly. WebOb exception names are also more readable than HTTP error codes. (And finally, don't "return abort", since abort never returns.)
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.