Skip to content

Commit 55bbe4d

Browse files
sl0thentr0pysl0thentr0py
committed
Don't gate request query strings and form data behind send_default_pii
1 parent 7d6edaf commit 55bbe4d

File tree

8 files changed

+57
-114
lines changed

8 files changed

+57
-114
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
- Remove `config.async` [#1894](https://github.com/getsentry/sentry-ruby/pull/1894)
66
- Migrate from to_hash to to_h ([#2351](https://github.com/getsentry/sentry-ruby/pull/2351))
7+
- Query strings and form data in requests are no longer controlled by `send_default_pii` ([#2452](https://github.com/getsentry/sentry-ruby/pull/2452))
78

89
## Unreleased
910

‎sentry-ruby/lib/sentry/faraday.rb

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,12 @@ def instrument(op_name, env, &block)
5656
private
5757

5858
def extract_request_info(env)
59-
url = env[:url].scheme + "://" + env[:url].host + env[:url].path
60-
result = { method: env[:method].to_s.upcase, url: url }
61-
62-
if Sentry.configuration.send_default_pii
63-
result[:query] = env[:url].query
64-
result[:body] = env[:body]
65-
end
66-
67-
result
59+
{
60+
method: env[:method].to_s.upcase,
61+
url: env[:url].scheme + "://" + env[:url].host + env[:url].path,
62+
query: env[:url].query,
63+
body: env[:body]
64+
}
6865
end
6966
end
7067
end

‎sentry-ruby/lib/sentry/interfaces/request.rb

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,12 @@ def initialize(env:, send_default_pii:, rack_env_whitelist:)
5353

5454
request = ::Rack::Request.new(env)
5555

56-
if send_default_pii
57-
self.data = read_data_from(request)
58-
self.cookies = request.cookies
59-
self.query_string = request.query_string
60-
end
61-
6256
self.url = request.scheme && request.url.split("?").first
6357
self.method = request.request_method
58+
self.data = read_data_from(request)
59+
self.query_string = request.query_string
60+
61+
self.cookies = request.cookies if send_default_pii
6462

6563
self.headers = filter_and_format_headers(env, send_default_pii)
6664
self.env = filter_and_format_env(env, rack_env_whitelist)

‎sentry-ruby/lib/sentry/net/http.rb

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -69,14 +69,12 @@ def extract_request_info(req)
6969
uri = req.uri || URI.parse(URI::DEFAULT_PARSER.escape("#{use_ssl? ? 'https' : 'http'}://#{hostname}#{req.path}"))
7070
url = "#{uri.scheme}://#{uri.host}#{uri.path}" rescue uri.to_s
7171

72-
result = { method: req.method, url: url }
73-
74-
if Sentry.configuration.send_default_pii
75-
result[:query] = uri.query
76-
result[:body] = req.body
77-
end
78-
79-
result
72+
{
73+
method: req.method,
74+
url: url,
75+
query: uri.query,
76+
body: req.body
77+
}
8078
end
8179
end
8280
end

‎sentry-ruby/spec/sentry/breadcrumb/http_logger_spec.rb

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -63,15 +63,15 @@
6363
Sentry.configuration.send_default_pii = false
6464
end
6565

66-
it "adds http breadcrumbs without query string & request body" do
66+
it "adds http breadcrumbs without request body" do
6767
stub_normal_response
6868

6969
response = Net::HTTP.get_response(URI("http://example.com/path?foo=bar"))
7070

7171
expect(response.code).to eq("200")
7272
crumb = Sentry.get_current_scope.breadcrumbs.peek
7373
expect(crumb.category).to eq("net.http")
74-
expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path" })
74+
expect(crumb.data).to eq({ status: 200, method: "GET", query: "foo=bar", url: "http://example.com/path" })
7575

7676
http = Net::HTTP.new("example.com")
7777
request = Net::HTTP::Get.new("/path?foo=bar")
@@ -80,7 +80,7 @@
8080
expect(response.code).to eq("200")
8181
crumb = Sentry.get_current_scope.breadcrumbs.peek
8282
expect(crumb.category).to eq("net.http")
83-
expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path" })
83+
expect(crumb.data).to eq({ status: 200, method: "GET", query: "foo=bar", url: "http://example.com/path" })
8484

8585
request = Net::HTTP::Post.new("/path?foo=bar")
8686
request.body = 'quz=qux'
@@ -89,7 +89,7 @@
8989
expect(response.code).to eq("200")
9090
crumb = Sentry.get_current_scope.breadcrumbs.peek
9191
expect(crumb.category).to eq("net.http")
92-
expect(crumb.data).to eq({ status: 200, method: "POST", url: "http://example.com/path" })
92+
expect(crumb.data).to eq({ status: 200, method: "POST", query: "foo=bar", url: "http://example.com/path" })
9393
end
9494
end
9595

@@ -115,7 +115,7 @@
115115
expect(response.code).to eq("200")
116116
crumb = Sentry.get_current_scope.breadcrumbs.peek
117117
expect(crumb.category).to eq("net.http")
118-
expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path" })
118+
expect(crumb.data).to eq({ status: 200, method: "GET", query: "foo=bar", url: "http://example.com/path" })
119119
end
120120
end
121121
end

‎sentry-ruby/spec/sentry/event_spec.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,8 @@
104104
headers: { 'Host' => 'localhost', 'X-Request-Id' => 'abcd-1234-abcd-1234' },
105105
method: 'POST',
106106
url: 'http://localhost/lol',
107+
query_string: 'biz=baz',
108+
data: { 'foo' => 'bar' }
107109
)
108110
expect(event.to_h[:tags][:request_id]).to eq("abcd-1234-abcd-1234")
109111
expect(event.to_h[:user][:ip_address]).to eq(nil)

‎sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb

Lines changed: 9 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -139,22 +139,22 @@ def to_s
139139
end
140140
end
141141

142-
it "doesn't store request body by default" do
143-
env.merge!("REQUEST_METHOD" => "POST", ::Rack::RACK_INPUT => StringIO.new("data=ignore me"))
142+
it "stores form data" do
143+
env.merge!("REQUEST_METHOD" => "POST", ::Rack::RACK_INPUT => StringIO.new("data=catch me"))
144144

145-
expect(subject.data).to eq(nil)
145+
expect(subject.data).to eq({ "data" => "catch me" })
146146
end
147147

148-
it "doesn't store request body by default" do
149-
env.merge!(::Rack::RACK_INPUT => StringIO.new("ignore me"))
148+
it "stores query string" do
149+
env.merge!("QUERY_STRING" => "token=xxxx")
150150

151-
expect(subject.data).to eq(nil)
151+
expect(subject.query_string).to eq("token=xxxx")
152152
end
153153

154-
it "doesn't store query_string by default" do
155-
env.merge!("QUERY_STRING" => "token=xxxx")
154+
it "stores request body" do
155+
env.merge!(::Rack::RACK_INPUT => StringIO.new("catch me"))
156156

157-
expect(subject.query_string).to eq(nil)
157+
expect(subject.data).to eq("catch me")
158158
end
159159

160160
context "with config.send_default_pii = true" do
@@ -166,24 +166,6 @@ def to_s
166166
expect(subject.cookies).to eq("cookies!")
167167
end
168168

169-
it "stores form data" do
170-
env.merge!("REQUEST_METHOD" => "POST", ::Rack::RACK_INPUT => StringIO.new("data=catch me"))
171-
172-
expect(subject.data).to eq({ "data" => "catch me" })
173-
end
174-
175-
it "stores query string" do
176-
env.merge!("QUERY_STRING" => "token=xxxx")
177-
178-
expect(subject.query_string).to eq("token=xxxx")
179-
end
180-
181-
it "stores request body" do
182-
env.merge!(::Rack::RACK_INPUT => StringIO.new("catch me"))
183-
184-
expect(subject.data).to eq("catch me")
185-
end
186-
187169
it "stores Authorization header" do
188170
env.merge!("HTTP_AUTHORIZATION" => "Basic YWxhZGRpbjpvcGVuc2VzYW1l")
189171

‎sentry-ruby/spec/sentry/net/http_spec.rb

Lines changed: 24 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -46,67 +46,30 @@
4646
end
4747
end
4848

49-
context "with config.send_default_pii = true" do
50-
before do
51-
Sentry.configuration.send_default_pii = true
52-
end
53-
54-
it "records the request's span with query string in data" do
55-
stub_normal_response
56-
57-
transaction = Sentry.start_transaction
58-
Sentry.get_current_scope.set_span(transaction)
59-
60-
response = Net::HTTP.get_response(URI("http://example.com/path?foo=bar"))
61-
62-
expect(response.code).to eq("200")
63-
expect(transaction.span_recorder.spans.count).to eq(2)
64-
65-
request_span = transaction.span_recorder.spans.last
66-
expect(request_span.op).to eq("http.client")
67-
expect(request_span.origin).to eq("auto.http.net_http")
68-
expect(request_span.start_timestamp).not_to be_nil
69-
expect(request_span.timestamp).not_to be_nil
70-
expect(request_span.start_timestamp).not_to eq(request_span.timestamp)
71-
expect(request_span.description).to eq("GET http://example.com/path")
72-
expect(request_span.data).to eq({
73-
"http.response.status_code" => 200,
74-
"url" => "http://example.com/path",
75-
"http.request.method" => "GET",
76-
"http.query" => "foo=bar"
77-
})
78-
end
79-
end
80-
81-
context "with config.send_default_pii = false" do
82-
before do
83-
Sentry.configuration.send_default_pii = false
84-
end
49+
it "records the request's span with query string in data" do
50+
stub_normal_response
8551

86-
it "records the request's span without query string" do
87-
stub_normal_response
52+
transaction = Sentry.start_transaction
53+
Sentry.get_current_scope.set_span(transaction)
8854

89-
transaction = Sentry.start_transaction
90-
Sentry.get_current_scope.set_span(transaction)
55+
response = Net::HTTP.get_response(URI("http://example.com/path?foo=bar"))
9156

92-
response = Net::HTTP.get_response(URI("http://example.com/path?foo=bar"))
93-
94-
expect(response.code).to eq("200")
95-
expect(transaction.span_recorder.spans.count).to eq(2)
57+
expect(response.code).to eq("200")
58+
expect(transaction.span_recorder.spans.count).to eq(2)
9659

97-
request_span = transaction.span_recorder.spans.last
98-
expect(request_span.op).to eq("http.client")
99-
expect(request_span.origin).to eq("auto.http.net_http")
100-
expect(request_span.start_timestamp).not_to be_nil
101-
expect(request_span.timestamp).not_to be_nil
102-
expect(request_span.start_timestamp).not_to eq(request_span.timestamp)
103-
expect(request_span.description).to eq("GET http://example.com/path")
104-
expect(request_span.data).to eq({
105-
"http.response.status_code" => 200,
106-
"url" => "http://example.com/path",
107-
"http.request.method" => "GET"
108-
})
109-
end
60+
request_span = transaction.span_recorder.spans.last
61+
expect(request_span.op).to eq("http.client")
62+
expect(request_span.origin).to eq("auto.http.net_http")
63+
expect(request_span.start_timestamp).not_to be_nil
64+
expect(request_span.timestamp).not_to be_nil
65+
expect(request_span.start_timestamp).not_to eq(request_span.timestamp)
66+
expect(request_span.description).to eq("GET http://example.com/path")
67+
expect(request_span.data).to eq({
68+
"http.response.status_code" => 200,
69+
"url" => "http://example.com/path",
70+
"http.request.method" => "GET",
71+
"http.query" => "foo=bar"
72+
})
11073
end
11174

11275
it "supports non-ascii characters in the path" do
@@ -348,7 +311,8 @@ def verify_spans(transaction)
348311
expect(request_span.data).to eq({
349312
"http.response.status_code" => 200,
350313
"url" => "http://example.com/path",
351-
"http.request.method" => "GET"
314+
"http.request.method" => "GET",
315+
"http.query" => "foo=bar"
352316
})
353317

354318
request_span = transaction.span_recorder.spans[2]
@@ -361,7 +325,8 @@ def verify_spans(transaction)
361325
expect(request_span.data).to eq({
362326
"http.response.status_code" => 404,
363327
"url" => "http://example.com/path",
364-
"http.request.method" => "GET"
328+
"http.request.method" => "GET",
329+
"http.query" => "foo=bar"
365330
})
366331
end
367332

0 commit comments

Comments
 (0)