Skip to content

Wrap redirect uri invalid error as RoutingError

Mark Chao requested to merge 214493-invaid-uri into master

What does this MR do?

Currently a large number of invalid requests are generating many errors on Sentry. To reduce this noise, we wrap errors caused by invalid url redirecting in as RoutingError which would be ignored.

Note that this can't be tested in request/feature spec, because invalid URL would cause testing setup code to fail before it reaches Rails. Here I extracted the logic as its own class so it can be tested.

To test this:

  1. We have to modify the GDK a bit. In lib/gdk/config_settings.rb, remove GITLAB_TRACING and GITLAB_TRACING_URL. These two variables are used in https://gitlab.com/gitlab-org/labkit, which would trigger URI.parse on its own. Tracing is not used in production.
  2. better_errors would display an exception's cause (and apparently they fixed it somehow recently), so it would appear the error raised was unchanged. We have to disable better_errors by calling BetterErrors::Middleware::ALLOWED_IPS.clear in initializers.
  3. Visit http://localhost:3000/twitter/typeahead-js/wikis/home\[

I didn't go the rack way of filtering everything at the beginning, because I feel that would cause unnecessary parsing. Plus, we seems to also accept incorrectly encoded URLs in some places.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Edited by 🤖 GitLab Bot 🤖

Merge request reports