Usually when we use `try` we actually mean `try!`, which is the same as
the safe navigation operator. However, there are a few cases where we
actually mean to execute a method if the object responds to that method.
In those cases using `try` would actually be OK, but in order to avoid
confusion as to whether we mean to check for `respond_to?` or we mean to
use safe navigation, I'm removing all usages of `try`.
We were converting markdown to HTML every time we saved a record, which
has the same problems as sanitizing HTML before saving it to the
database, particularly because the body of a legislation draft is stored
in a translations table.
Performance-wise this isn't a problem: converting a text with more than
200_000 characters takes about a milisecond on my machine.
Note we need to modify a migration generated by globalize, since the
method `create_translation_table!` would fail now that we don't define
`translates :body_html` in the model.
Sanitizing descriptions before saving a record has a few drawbacks:
1. It makes the application rely on data being safe in the database. If
somehow dangerous data enters the database, the application will be
vulnerable to XSS attacks
2. It makes the code complicated
3. It isn't backwards compatible; if we decide to disallow a certain
HTML tag in the future, we'd need to sanitize existing data.
On the other hand, sanitizing the data in the view means we don't need
to triple-check dangerous HTML has already been stripped when we see the
method `auto_link_already_sanitized_html`, since now every time we use
it we sanitize the text in the same line we call this method.
We could also sanitize the data twice, both when saving to the database
and when displaying values in the view. However, doing so wouldn't make
the application safer, since we sanitize text introduced through
textarea fields but we don't sanitize text introduced through input
fields.
Finally, we could also overwrite the `description` method so it
sanitizes the text. But we're already introducing Globalize which
overwrites that method, and overwriting it again is a bit too confusing
in my humble opinion. It can also lead to hard-to-debug behaviour.
The jQuery html() function does not filter <script> tags, so if somehow
an attacker introduced a <script> in the translation, we would be
vulnerable to a XSS attack.
Note using $.parseHTML wouldn't solve the problem, since it doesn't
filter attributes in image tags.
Since changing the text of the part which doesn't have the count wasn't
very clean, I've added another <span> tag for the part with the
description, and so we can use jQuery's text() function to replace it.
Using html() makes it possible to insert <script> tags in the DOM, and
in this case we aren't supposed to be inserting any HTML.
I haven't found a way to focus on a field with Capybara, then add a
character, and focus on another field. So I've manually triggered the
change event in the test.
In general, we always use relative URLs (using `_path`), but sometimes
we were accidentally using absolute URLs (using `_url`). It's been
reported i might cause some isuses if accepting both HTTP and HTTPS
connections, although we've never seen the case.
In any case, this change makes the code more consistent and makes the
generated HTML cleaner.
The main reason to use it was the `rel` attribute for previous/next
pages not being indexed correctly by certain search engines when using a
relative URL. However, AFAIK that only applied to `<link>` tags, not to
`<a>` tags, and only if a `<base>` tag was defined.
In any case, it looks like the same search engines don't use the `rel`
attribute for previous/next to index pages anymore.
There were some confusing definitions regarding the valuation of budget
investments.
In the controller, `CommentableActions` was included, which includes the
update action.
In the abilities, a valuator was given permission to update an
investment.
However, the action to update an investment didn't work because there is
no route defined to do so.
The ability was defined so valuators could access the "edit" action,
which will not call the "update" action but the "valuate" action. Since
internally "edit" and "update" use the same permission, it worked.
But then we added permission for regular users to update budget
investments, and these permissions were allowing valuators to update
another user's investment.
After this change, everything seems to work properly since we check
authorization in the controller itself instead of using abilities.
We're going to upgrade our ruby version, and we need these tasks.
Note we now get a warning caused by `rvm1:install:ruby` invoking
`deploy:updating`. It doesn't seem to be an issue because we don't add
any hooks to `deploy:updating`, and neither do the rest of the gems we
use.
Old CONSUL nginx configurations will probably have a reference to a
unicorn socket. Making that file a symbolic link to a puma socket makes
it possible for the application to keep working without updating the
nginx configuration file.
Puma was adding commands to `rvm_map_bins`, which meant RMV1 wasn't
using the default value of `rvm1_map_bins`.
Changing the order we use to require `rmv1/capistrano3` and
`capistrano/puma` did not fix the issue.
Puma is the server we use in the development environment, so this way we
don't need to maintain two servers. Furthermore, puma seems to offer a
few advantages over unicorn (like multithreading) and no disadvantages.
Our current unicorn task wasn't working in some cases. We also had a
version in the `capistrano` branch, which was the one we recommended.
However, that version assumed RVM, a certain ruby version and a certain
deploy folder were used. This version uses `bundle exec` and variables
like `release_path`, so it does not depend on any specific
configuration.
Even if we're replacing unicorn with puma, I wanted to make this change
in case we need it as a reference in the future.
