Skip to content

Commit c6d6137

Browse files
sl0thentr0pysl0thentr0py
committed
Don't gate request query strings and form data behind send_default_pii
1 parent 913c75e commit c6d6137

File tree

9 files changed

+83
-181
lines changed

9 files changed

+83
-181
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/lib/sentry/utils/http_tracing.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ def record_sentry_breadcrumb(request_info, response_status)
2020
level: :info,
2121
category: self.class::BREADCRUMB_CATEGORY,
2222
type: :info,
23-
data: { status: response_status, **request_info }
23+
data: { status: response_status, **request_info.compact }
2424
)
2525

2626
Sentry.add_breadcrumb(crumb)

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

Lines changed: 30 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -21,76 +21,35 @@
2121
end
2222
end
2323

24-
context "with config.send_default_pii = true" do
25-
before do
26-
Sentry.configuration.send_default_pii = true
27-
end
28-
29-
it "adds http breadcrumbs with query string & request body" do
30-
stub_normal_response
31-
32-
response = Net::HTTP.get_response(URI("http://example.com/path?foo=bar"))
33-
34-
expect(response.code).to eq("200")
35-
crumb = Sentry.get_current_scope.breadcrumbs.peek
36-
expect(crumb.category).to eq("net.http")
37-
expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path", query: "foo=bar", body: nil })
38-
39-
http = Net::HTTP.new("example.com")
40-
request = Net::HTTP::Get.new("/path?foo=bar")
41-
response = http.request(request)
42-
43-
expect(response.code).to eq("200")
44-
crumb = Sentry.get_current_scope.breadcrumbs.peek
45-
expect(crumb.category).to eq("net.http")
46-
expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path", query: "foo=bar", body: nil })
47-
48-
request = Net::HTTP::Post.new("/path?foo=bar")
49-
request.body = 'quz=qux'
50-
response = http.request(request)
51-
52-
expect(response.code).to eq("200")
53-
crumb = Sentry.get_current_scope.breadcrumbs.peek
54-
expect(crumb.category).to eq("net.http")
55-
expect(crumb.data).to eq(
56-
{ status: 200, method: "POST", url: "http://example.com/path", query: "foo=bar", body: 'quz=qux' }
57-
)
58-
end
59-
end
60-
61-
context "with config.send_default_pii = false" do
62-
before do
63-
Sentry.configuration.send_default_pii = false
64-
end
65-
66-
it "adds http breadcrumbs without query string & request body" do
67-
stub_normal_response
68-
69-
response = Net::HTTP.get_response(URI("http://example.com/path?foo=bar"))
70-
71-
expect(response.code).to eq("200")
72-
crumb = Sentry.get_current_scope.breadcrumbs.peek
73-
expect(crumb.category).to eq("net.http")
74-
expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path" })
75-
76-
http = Net::HTTP.new("example.com")
77-
request = Net::HTTP::Get.new("/path?foo=bar")
78-
response = http.request(request)
79-
80-
expect(response.code).to eq("200")
81-
crumb = Sentry.get_current_scope.breadcrumbs.peek
82-
expect(crumb.category).to eq("net.http")
83-
expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path" })
84-
85-
request = Net::HTTP::Post.new("/path?foo=bar")
86-
request.body = 'quz=qux'
87-
response = http.request(request)
88-
89-
expect(response.code).to eq("200")
90-
crumb = Sentry.get_current_scope.breadcrumbs.peek
91-
expect(crumb.category).to eq("net.http")
92-
expect(crumb.data).to eq({ status: 200, method: "POST", url: "http://example.com/path" })
93-
end
24+
it "adds http breadcrumbs with query string & request body" do
25+
stub_normal_response
26+
27+
response = Net::HTTP.get_response(URI("http://example.com/path?foo=bar"))
28+
29+
expect(response.code).to eq("200")
30+
crumb = Sentry.get_current_scope.breadcrumbs.peek
31+
expect(crumb.category).to eq("net.http")
32+
expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path", query: "foo=bar" })
33+
34+
http = Net::HTTP.new("example.com")
35+
request = Net::HTTP::Get.new("/path?foo=bar")
36+
response = http.request(request)
37+
38+
expect(response.code).to eq("200")
39+
crumb = Sentry.get_current_scope.breadcrumbs.peek
40+
expect(crumb.category).to eq("net.http")
41+
expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path", query: "foo=bar" })
42+
43+
request = Net::HTTP::Post.new("/path?foo=bar")
44+
request.body = 'quz=qux'
45+
response = http.request(request)
46+
47+
expect(response.code).to eq("200")
48+
crumb = Sentry.get_current_scope.breadcrumbs.peek
49+
expect(crumb.category).to eq("net.http")
50+
expect(crumb.data).to eq(
51+
{ status: 200, method: "POST", url: "http://example.com/path", query: "foo=bar", body: 'quz=qux' }
52+
)
9453
end
9554

9655
it "doesn't record breadcrumb for the SDK's request" do
@@ -115,7 +74,7 @@
11574
expect(response.code).to eq("200")
11675
crumb = Sentry.get_current_scope.breadcrumbs.peek
11776
expect(crumb.category).to eq("net.http")
118-
expect(crumb.data).to eq({ status: 200, method: "GET", url: "http://example.com/path" })
77+
expect(crumb.data).to eq({ status: 200, method: "GET", query: "foo=bar", url: "http://example.com/path" })
11978
end
12079
end
12180
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)