Skip to content

Tighten up `remove_old` while backing up GitLab

Description

  • The Backup::Manager class currently removes old backups like this (extraneous lines removed):

    next unless file =~ /(\d+)(?:_\d{4}_\d{2}_\d{2}(_\d+\.\d+\.\d+.*)?)?_gitlab_backup\.tar$/

timestamp = $1.to_i

if Time.at(timestamp) < (Time.now - keep_time)
    FileUtils.rm(file)
end

  • If for some reason the regex match passes, it is conceivable that a file that shouldn't be deleted could get deleted.

  • We need to protect against this, and be absolutely confident that this can't occur - only files exactly matching a given pattern should be deleted.

Things to Improve

  • Tighten the regex. Possibly match for a specific number of digits for the timestamp, rather than \d+. Also start the regex with ^.
  • Improve the test suite so we're confident that no files that aren't supposed to be deleted could be deleted by an improper match
  • Potentially move from a regex to something more procedural, with multiple guards
  • EE appends -ee to tags, which isn't currently tested (context)

References

/cc @DanielDent @axil

Edited by username-removed-407765