Skip to content

Exclude sensitive attributes in serializable_hash by default

Thong Kuah requested to merge hardening_prevent_encrypted_values_from_serializable_hash into master

What does this MR do and why?

  1. Add all attr_encrypted fields serializable_hash :except option
  2. Add all add_authentication_token_field fields to serializable_hash :except option
  3. Provide prevent_from_serialization :some_sensitive_value utility to manually add fields that were not automatically added above

Note This does not address all problems with serializable_hash / to_json / as_json but it solves one obvious weakness.

Related issues: https://gitlab.com/gitlab-org/gitlab/-/issues/353408, https://gitlab.com/gitlab-org/gitlab/-/issues/353608, https://gitlab.com/gitlab-com/gl-security/security-operations/sirt/operations/-/issues/2010

Moved from https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/2235

Manual testing

Enable the feature flag:

Feature.enable :prevent_sensitive_fields_from_serializable_hash
  1. Project Master:
[1] pry(main)> Project.first.serializable_hash.keys.include? "runners_token"
=> true

On this branch, it's false

  1. WebHook Master:
[1] pry(main)> WebHook.first.serializable_hash.keys.include? "token"
  WebHook Load (0.7ms)  SELECT "web_hooks".* FROM "web_hooks" ORDER BY "web_hooks"."id" ASC LIMIT 1 /*application:console,db_config_name:main_replica,line:(pry):1:in `__pry__'*/
=> true

On this branch, it's false

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

Numbered steps to set up and validate the change are strongly suggested.

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 Thong Kuah

Merge request reports