Skip to content

Add 15s timeout to GitLab UrlBlocker

Harsh Chouraria requested to merge hchouraria-url-blocker-getaddrinfo-timeout into master

What does this MR do?

The URL blocker checks for resolvable addresses of the URLs it receives.

In some cases, bad/stale URLs do not resolve in a finite time, and lead to a hang of the call for over the rack/puma timeout levels.

The Addrinfo.getaddrinfo call supports passing a timeout since Ruby 2.7.0, and this change applies a 15 seconds timeout to the operation.

The "arbitrary" 15 seconds value choice here copies the value also used for "resolver" work in the app/services/verify_pages_domain_service.rb file. With 15s, upto 4 bad resolving URLs could be checked without triggering the default rack/puma timeout.

I've defined it as a constant just in case it requires quick updating.

Screenshots (strongly suggested)

Sanitized sample error log when there's a bad URL being checked as part of project badges:

{
  "method": "GET",
  "path": "/group/project",
  "format": "html",
  "controller": "ProjectsController",
  "action": "show",
  "status": 500,
  "time": "2021-01-26T10:30:10.621Z",
  "gitaly_calls": 3,
  "gitaly_duration_s": 0.012923,
  "redis_calls": 13,
  […]
  "queue_duration_s": 0.002724,
  "cpu_s": 0.28,
  "exception.class": "Rack::Timeout::RequestTimeoutException",
  "exception.message": "Request ran for longer than 60000ms",
  "exception.backtrace": [
    "lib/gitlab/url_blocker.rb:111:in `getaddrinfo'",
    "lib/gitlab/url_blocker.rb:111:in `get_address_info'",
    "lib/gitlab/url_blocker.rb:48:in `validate!'",
    "app/validators/addressable_url_validator.rb:83:in `validate_each'",
    "app/models/badge.rb:43:in `build_rendered_url'",
    "app/models/badge.rb:31:in `rendered_link_url'",
    "app/models/badges/project_badge.rb:10:in `rendered_link_url'",
    "app/views/projects/_home_panel.html.haml:85",
    "app/views/projects/_home_panel.html.haml:84",
    "app/views/projects/show.html.haml:11",
    "app/controllers/application_controller.rb:134:in `render'",
    […]
  ],
}

Source: customer ticket (internal link)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Biggest risk is that the code will not work on versions lower than Ruby 2.7.

Testing is not included as simulating a low level resolver timeout does not appear to be straight-forward from within an rspec unit test. But I'm open to ideas if I've missed a simple way to simulate it.

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Peter Leitzen

Merge request reports