Skip to content

Finish work to prevent reading the body of x-sendfile responses

Problem Statement

We need to prevent accidental reading of X-Sendfile response bodies in Ruby, which can cause major performance issues by loading large files (like LFS objects, job artifacts) into memory instead of letting the reverse proxy handle file delivery.

Background

This issue completes the work started in !128869. When Rails uses send_file with X-Sendfile headers, the reverse proxy should deliver the file directly to the client. However, if code attempts to read the response body, Rails will load the entire file into memory, which is problematic for large files.

Related Issues

  • #419239 (closed) - LFS Pull Timeout with Large Files (closed, but this fix helps prevent similar issues)
  • #420738 (closed) - Related performance issue
  • #415885 - Move rest of lib/gitlab/utils code into gitlab-utils gem
  • Epic: &10834 - Git LFS Performance Improvements
  • Epic: &10869 - GitLab Monorepo Gems

Current State

MR !128869 contains a working implementation that:

  • Patches ActionDispatch::Response to use a custom FileBody class
  • Raises an exception when attempting to read X-Sendfile response bodies
  • Includes comprehensive test coverage
  • Has a feature flag block_xsendfile_response_body_parsing (disabled by default)

⚠️ Architectural Concerns

Important feedback from @tkuah (Maintainer, Aug 24, 2023):

FYI @robotmay_gitlab - we are looking to move utils out to gitlab-utils gem (see #415885 and &10869). Not sure if this FileBody class is strictly a util class.

Nonetheless, I notice there's a monkey-patch to ActionDispatch. Where possible, I recommend all monkey-patches to be isolated to a gem under gems so we can clearly have explicit dependencies (and also prevent cyclic dependency issues) - makes our application code more maintainable.

This suggests the current implementation may need architectural changes before merging.

What Needs to be Done

  1. Address architectural concerns - Determine if FileBody class should move to gitlab-utils gem or gems/ directory
  2. Isolate monkey-patch - Move ActionDispatch monkey-patch to appropriate gem location per maintainer feedback
  3. Rebase and fix the MR pipeline - The MR hasn't been updated since 2023 and pipeline is failing
  4. Address any review feedback - Complete any outstanding review comments
  5. Update feature flag rollout plan - Determine rollout strategy for the feature flag
  6. Verify test coverage - Ensure all edge cases are covered
  7. Documentation - Add any necessary documentation about the change

Acceptance Criteria

  • Architectural approach approved by maintainers (addressing @tkuah's feedback)
  • ActionDispatch monkey-patch properly isolated (likely in gems/ directory)
  • FileBody class location determined (utils gem vs gems directory)
  • MR !128869 pipeline passes
  • All review feedback addressed
  • Feature flag rollout plan defined
  • Tests pass and cover edge cases
  • No regressions in LFS or file download functionality

Implementation Details

The solution works by:

  1. Patching Rails' ActionDispatch::Response#send_file method
  2. Using a custom Gitlab::Utils::FileBody class that detects X-Sendfile requests
  3. Raising AttemptedToReadBodyOfSendFileResponseError when body reading is attempted
  4. Protecting critical code paths that might accidentally read response bodies

Note: The implementation approach may need to change based on architectural feedback.

Weight Estimate

6 (originally 2 as estimated by @robotmay_gitlab, but architectural changes, potential changes in code base since original work done two years ago, and an engineer new to the problem getting up to speed increases this).

Specifically, I think this probably needs to be broken down into 2 distinct weight 3 issues:

Phase 1: Research and Gem Creation (Weight 3)

  • Review and understand the existing MR !128869 implementation
  • Research the gitlab-utils gem structure and requirements
  • Determine the best architectural approach based on maintainer feedback
  • Create the gem structure with the FileBody class and monkey-patch
  • Address architectural concerns about where components should live

This is weight 3 because we don't do this kind of work all the time, there will be a learning curve, and the amount of existing context to take in is significant.

Phase 2: Integration and Implementation (Weight 3)

  • Integrate the gem into the main application
  • Update feature flag implementation
  • Address any integration issues
  • Complete testing and documentation
  • Ensure no regressions in LFS or file download functionality

This may end up being closer to a 2 but again, the large amount of context to deal with here makes it more realistic as a 3.

Edited by 🤖 GitLab Bot 🤖