-
-
Notifications
You must be signed in to change notification settings - Fork 540
feat(data-collection): Add sensitive key-value collection filter #3025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
sl0thentr0py
wants to merge
2
commits into
neel/data-collection/base
from
neel/data-collection/sensitive
+199
−19
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
92 changes: 92 additions & 0 deletions
92
sentry-ruby/lib/sentry/data_collection/key_value_collection.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Sentry | ||
| class DataCollection | ||
| # Configuration for key-value data collection. | ||
| class KeyValueCollection | ||
| FILTERED_VALUE = "[Filtered]" | ||
|
|
||
| # Keys from this list are ALWAYS filtered, regardless of :mode | ||
| # TODO-neel-data cookies have separate list, see JS | ||
| SENSITIVE_DENY_LIST = %w[ | ||
| auth | ||
| token | ||
| secret | ||
| session | ||
| password | ||
| passwd | ||
| pwd | ||
| key | ||
| jwt | ||
| bearer | ||
| sso | ||
| saml | ||
| csrf | ||
| xsrf | ||
| credentials | ||
| sid | ||
| identity | ||
| cookie | ||
| set-cookie | ||
| ].freeze | ||
|
|
||
| # `mode` controls whether values are collected: | ||
| # - `:off` disables collection. | ||
| # - `:deny_list` collects values except those matching `terms`. | ||
| # - `:allow_list` collects only values matching `terms`. | ||
| # @return [:off, :deny_list, :allow_list] | ||
| attr_accessor :mode | ||
|
|
||
| # `terms` contains the keys or patterns used by the selected mode. | ||
| # @return [Array<String>, nil] | ||
| attr_reader :terms | ||
|
|
||
| def initialize(mode:, terms:) | ||
| @mode = mode | ||
| self.terms = terms | ||
| end | ||
|
|
||
| def terms=(terms) | ||
| @terms = terms&.map { |term| term.to_s.downcase }&.reject { |term| term.strip.empty? } | ||
| end | ||
|
|
||
| # Applies this collection configuration without changing the input hash. | ||
| # Keys are retained whenever the category is collected; values that are not | ||
| # safe to send are replaced with FILTERED_VALUE. | ||
| # | ||
| # @param values [Hash] key-value data to filter | ||
| # @return [Hash] a new filtered hash, or an empty hash when collection is off | ||
| def filter(values) | ||
| return {} if mode == :off | ||
|
|
||
| values.each_with_object({}) do |(key, value), filtered| | ||
| filtered[key] = safe_value?(key) ? value : FILTERED_VALUE | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def safe_value?(key) | ||
| key_downcase = key.to_s.downcase | ||
| return false if sensitive?(key_downcase) | ||
|
|
||
| case mode | ||
| when :deny_list | ||
| !matches_any_term?(key_downcase) | ||
| when :allow_list | ||
| matches_any_term?(key_downcase) | ||
| else | ||
| false | ||
| end | ||
| end | ||
|
|
||
| def sensitive?(key) | ||
| SENSITIVE_DENY_LIST.any? { |term| key.include?(term) } | ||
| end | ||
|
sl0thentr0py marked this conversation as resolved.
|
||
|
|
||
| def matches_any_term?(key) | ||
| @terms&.any? { |term| key.include?(term) } | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| end | ||
| end | ||
| end | ||
| end | ||
105 changes: 105 additions & 0 deletions
105
sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "sentry/data_collection/key_value_collection" | ||
|
|
||
| RSpec.describe Sentry::DataCollection::KeyValueCollection do | ||
| subject(:collection) { described_class.new(mode: mode, terms: terms) } | ||
|
|
||
| let(:values) do | ||
| { | ||
| "Authorization" => "secret-token", | ||
| "page" => "2", | ||
| "display_name" => "Ada", | ||
| 42 => "numeric key" | ||
| } | ||
| end | ||
| let(:mode) { :deny_list } | ||
| let(:terms) { nil } | ||
|
|
||
| describe "#filter" do | ||
| it "uses the collection configuration" do | ||
| expect(collection.filter(values)).to eq( | ||
| "Authorization" => "[Filtered]", | ||
| "page" => "2", | ||
| "display_name" => "Ada", | ||
| 42 => "numeric key" | ||
| ) | ||
| end | ||
|
|
||
| context "when mode is :off" do | ||
| let(:mode) { :off } | ||
|
|
||
| it "does not collect keys or values" do | ||
| expect(collection.filter(values)).to eq({}) | ||
| end | ||
| end | ||
|
|
||
| context "when mode is :deny_list" do | ||
| it "matches sensitive terms partially and case-insensitively" do | ||
| expect(described_class.new(mode: :deny_list, terms: nil).filter( | ||
| { "X-Auth-Token" => "a", "ACCESS_SECRET_VALUE" => "b", "random_field" => "c" } | ||
| )).to eq( | ||
| "X-Auth-Token" => "[Filtered]", | ||
| "ACCESS_SECRET_VALUE" => "[Filtered]", | ||
| "random_field" => "c" | ||
| ) | ||
| end | ||
|
|
||
| it "applies additional deny terms" do | ||
| expect(described_class.new(mode: :deny_list, terms: ["USER", :internal]).filter( | ||
| { "user_id" => "1", "internal" => "value" } | ||
| )).to eq( | ||
| "user_id" => "[Filtered]", | ||
| "internal" => "[Filtered]" | ||
| ) | ||
| end | ||
|
|
||
| it "normalizes terms assigned after initialization" do | ||
| collection.terms = ["USER"] | ||
|
|
||
| expect(collection.filter("user_id" => "1")).to eq("user_id" => "[Filtered]") | ||
| end | ||
|
|
||
| it "covers every built-in sensitive term" do | ||
| values = Sentry::DataCollection::KeyValueCollection::SENSITIVE_DENY_LIST.to_h do |term| | ||
| ["prefix-#{term.upcase}-suffix", "value"] | ||
| end | ||
|
|
||
| expect(collection.filter(values).values.uniq).to eq(["[Filtered]"]) | ||
| end | ||
| end | ||
|
|
||
| context "when mode is :allow_list" do | ||
| let(:mode) { :allow_list } | ||
| let(:terms) { ["page", "display"] } | ||
|
|
||
| it "filters values whose keys are not allowed" do | ||
| expect(collection.filter(values)).to eq( | ||
| "Authorization" => "[Filtered]", | ||
| "page" => "2", | ||
| "display_name" => "Ada", | ||
| 42 => "[Filtered]" | ||
| ) | ||
| end | ||
|
|
||
| it "still filters sensitive keys listed in the allow list" do | ||
| expect(described_class.new(mode: :allow_list, terms: ["token", "public"]).filter( | ||
| { "token" => "secret", "public" => "value" } | ||
| )).to eq("token" => "[Filtered]", "public" => "value") | ||
| end | ||
|
|
||
| it "does not allow every key when terms include nil or blank values" do | ||
| expect(described_class.new(mode: :allow_list, terms: [nil, "", " "]).filter( | ||
| { "public" => "value", "page" => "2" } | ||
| )).to eq("public" => "[Filtered]", "page" => "[Filtered]") | ||
| end | ||
| end | ||
|
|
||
| it "does not mutate the input" do | ||
| original = values.dup | ||
| collection.filter(values) | ||
|
|
||
| expect(values).to eq(original) | ||
| end | ||
| end | ||
| end |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.