Skip to content

Verify LFS OID

Joern Schneeweisz requested to merge joernchen-master-patch-47750 into master

What does this MR do and why?

Currently the oid on our LfsObjects aren't verified for their format. This MR introduces a regex check for /\A\h{64}\z/ to ensure the oids are well-formed.

Screenshots or screen recordings

These are strongly recommended to assist reviewers and reduce the time to merge your change.

How to set up and validate locally

In the rails console for an invalid oid:

[1] pry(main)> LfsObject.create!(oid: "notvalid",size: 1234,file: nil)
Creating scope :verification_succeeded. Overwriting existing method LfsObject.verification_succeeded.
Creating scope :verification_failed. Overwriting existing method LfsObject.verification_failed.
Creating scope :checksummed. Overwriting existing method LfsObject.checksummed.
Creating scope :not_checksummed. Overwriting existing method LfsObject.not_checksummed.
Creating scope :available_verifiables. Overwriting existing method LfsObject.available_verifiables.
  TRANSACTION (0.1ms)  BEGIN /*application:console,db_config_name:main,line:(pry):1:in `__pry__'*/
  LfsObject Exists? (2.3ms)  SELECT 1 AS one FROM "lfs_objects" WHERE "lfs_objects"."oid" = 'notvalid' LIMIT 1 /*application:console,db_config_name:main,line:(pry):1:in `__pry__'*/
  TRANSACTION (0.1ms)  ROLLBACK /*application:console,db_config_name:main,line:/lib/gitlab/database.rb:370:in `rollback'*/
ActiveRecord::RecordInvalid: Validation failed: Oid is invalid
from /home/joern/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/activerecord-6.1.6.1/lib/active_record/validations.rb:80:in `raise_validation_error'
[2] pry(main)> 

For a valid oid:

[2] pry(main)> LfsObject.create!(oid: "a"*64,size: 1234,file: nil)
  TRANSACTION (0.3ms)  BEGIN /*application:console,db_config_name:main,line:(pry):2:in `__pry__'*/
  LfsObject Exists? (0.4ms)  SELECT 1 AS one FROM "lfs_objects" WHERE "lfs_objects"."oid" = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' LIMIT 1 /*application:console,db_config_name:main,line:(pry):2:in `__pry__'*/
  LfsObject Create (67.5ms)  INSERT INTO "lfs_objects" ("oid", "size", "created_at", "updated_at", "file") VALUES ('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', 1234, '2022-08-01 15:01:36.087915', '2022-08-01 15:01:36.087915', 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa') RETURNING "id" /*application:console,db_config_name:main,line:(pry):2:in `__pry__'*/
  LfsObject Update (1.3ms)  UPDATE "lfs_objects" SET "file_store" = 1 WHERE "lfs_objects"."id" = 19 /*application:console,db_config_name:main,line:/app/models/concerns/file_store_mounter.rb:24:in `update_file_store'*/
  LfsObject Exists? (0.3ms)  SELECT 1 AS one FROM "lfs_objects" WHERE "lfs_objects"."file_store" = 1 AND "lfs_objects"."id" = 19 LIMIT 1 /*application:console,db_config_name:main,line:/ee/app/models/concerns/geo/verifiable_model.rb:19:in `save_verification_details'*/
  Geo::LfsObjectState Load (10.7ms)  SELECT "lfs_object_states".* FROM "lfs_object_states" WHERE "lfs_object_states"."lfs_object_id" = 19 LIMIT 1 /*application:console,db_config_name:main,line:/ee/app/models/ee/lfs_object.rb:64:in `lfs_object_state'*/
  Geo::LfsObjectState Create (0.5ms)  INSERT INTO "lfs_object_states" ("lfs_object_id") VALUES (19) RETURNING "lfs_object_id" /*application:console,db_config_name:main,line:/ee/app/models/concerns/geo/verifiable_model.rb:28:in `save_verification_details'*/
  TRANSACTION (0.2ms)  COMMIT /*application:console,db_config_name:main,line:/lib/gitlab/database.rb:364:in `commit'*/
  GeoNode Exists? (0.4ms)  SELECT 1 AS one FROM "geo_nodes" LIMIT 1 /*application:console,db_config_name:main,line:/ee/lib/gitlab/geo.rb:86:in `block in enabled?'*/
=> #<LfsObject:0x000055eedd55d058
 id: 19,
 oid: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
 size: 1234,
 created_at: Mon, 01 Aug 2022 15:01:36.087915577 UTC +00:00,
 updated_at: Mon, 01 Aug 2022 15:01:36.087915577 UTC +00:00,
 file: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
 file_store: 1,
 verification_checksum: nil>

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Joern Schneeweisz

Merge request reports