Skip to content
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.

Sign up for GitHub

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

fix(logging): Fixed input validation for X-Cloud-Trace-Context; encoded spanID from XCTC header into hex string. #10979

Merged
merged 6 commits into from
Oct 15, 2024
Merged

fix(logging): Fixed input validation for X-Cloud-Trace-Context; encoded spanID from XCTC header into hex string. #10979

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
758a2d6
fix: Fixed input validation for X-Cloud-Trace-Context; encoded spanID
gkevinzheng Oct 9, 2024
8a7690e
Reverted input validation to pass as much of what passed prior to
gkevinzheng Oct 11, 2024
5846918
Merge branch 'main' into xctc-hex
gkevinzheng Oct 14, 2024
a707634
Fixed docstring
gkevinzheng Oct 15, 2024
4859b1a
Merge branch 'main' into xctc-hex
gkevinzheng Oct 15, 2024
8d96a1e
Merge branch 'main' into xctc-hex
gkevinzheng Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions logging/logging.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,15 +884,15 @@ var validXCloudTraceContext = regexp.MustCompile(
`(?:;o=(\d))?`)

func deconstructXCloudTraceContext(s string) (traceID, spanID string, traceSampled bool) {
// As per the format described at https://cloud.google.com/trace/docs/setup#force-trace
// "X-Cloud-Trace-Context: TRACE_ID/SPAN_ID;o=TRACE_TRUE"
// As per the format described at https://cloud.google.com/trace/docs/trace-context#legacy-http-header
// "X-Cloud-Trace-Context: TRACE_ID[/SPAN_ID][;o=TRACE_TRUE]"
// for example:
// "X-Cloud-Trace-Context: 105445aa7843bc8bf206b120001000/1;o=1"
// "X-Cloud-Trace-Context: 105445aa7843bc8bf206b12000100000/1;o=1"
//
// We expect:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be easier to understand if you phrase the comment as:

  // Parameters:
  //   s - ...
  //
  // Returns:
  //     ...

With respect to "We expect", it is unclear whether this is referring to the input or the output.

If it is intended to describe the input, then shouldn't the documentation for spanID say it is a decimal string?

// * traceID (optional): "105445aa7843bc8bf206b120001000"
// * spanID (optional): "1"
// * traceSampled (optional): true
// * traceID (optional, 32-bit hex string): "105445aa7843bc8bf206b12000100000"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trace ID is 128-bit, not 32-bit; it is represented as a 16-byte (32-char) array. This comment and the next one may be confusing the number of symbols in the string with the number of bits in the numeric value.

https://www.w3.org/TR/trace-context/#trace-id

// * spanID (optional, 16-bit hex string): "0000000000000001" (needs to be converted into 16 bit hex string)
// * traceSampled (optional, bool): true
matches := validXCloudTraceContext.FindStringSubmatch(s)

if matches != nil {
Expand All @@ -903,6 +903,15 @@ func deconstructXCloudTraceContext(s string) (traceID, spanID string, traceSampl
spanID = ""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to consider similar validation/conversion on the trace ID (to ensure that it isn't all-zero and to ensure that it is zero-padded if smaller than the W3C spec length).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to leave some clues when this happens?

}

if spanID != "" {
// Convert to 16 byte unsigned hex string
intSpanID, err := strconv.ParseUint(spanID, 10, 64)
if err != nil || intSpanID == 0 {
spanID = ""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to leave some clues when this happens?

} else {
spanID = fmt.Sprintf("%016x", intSpanID)
}
}
return
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional readability suggestion: consider returning the values explicitly rather than relying on named return values. It was a little confusing seeing this return without the list of variables that are returned.

}

Expand Down
54 changes: 41 additions & 13 deletions logging/logging_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,13 +311,13 @@ func TestToLogEntry(t *testing.T) {
HTTPRequest: &logging.HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/000000000000004a;o=1"}},
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b12000100000/000000000000001;o=1"}},
},
},
},
want: &logpb.LogEntry{
Trace: "projects/P/traces/105445aa7843bc8bf206b120001000",
SpanId: "000000000000004a",
Trace: "projects/P/traces/105445aa7843bc8bf206b12000100000",
SpanId: "0000000000000001",
TraceSampled: true,
},
}, {
Expand All @@ -327,28 +327,56 @@ func TestToLogEntry(t *testing.T) {
HTTPRequest: &logging.HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/000000000000004a;o=0"}},
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b12000100000/0000000000000001;o=0"}},
},
},
},
want: &logpb.LogEntry{
Trace: "projects/P/traces/105445aa7843bc8bf206b120001000",
SpanId: "000000000000004a",
Trace: "projects/P/traces/105445aa7843bc8bf206b12000100000",
SpanId: "0000000000000001",
TraceSampled: true,
},
}, {
name: "X-Trace-Context header with all fields; Span ID properly encoded in hexadecimal",
in: logging.Entry{
HTTPRequest: &logging.HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b12000100000/16;o=1"}},
},
},
},
want: &logpb.LogEntry{
Trace: "projects/P/traces/105445aa7843bc8bf206b12000100000",
SpanId: "0000000000000010",
TraceSampled: true,
},
}, {
name: "X-Trace-Context with Span ID too large",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any logging or other debug information that can be output in such a case?

in: logging.Entry{
HTTPRequest: &logging.HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b12000100000/99999999999999999999999999999999"}},
},
},
},
want: &logpb.LogEntry{
Trace: "projects/P/traces/105445aa7843bc8bf206b12000100000",
},
}, {
name: "X-Trace-Context header with all fields; TraceSampled from Header",
in: logging.Entry{
HTTPRequest: &logging.HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/000000000000004a;o=1"}},
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b12000100000/0000000000000001;o=1"}},
},
},
},
want: &logpb.LogEntry{
Trace: "projects/P/traces/105445aa7843bc8bf206b120001000",
SpanId: "000000000000004a",
Trace: "projects/P/traces/105445aa7843bc8bf206b12000100000",
SpanId: "0000000000000001",
TraceSampled: true,
},
}, {
Expand All @@ -368,25 +396,25 @@ func TestToLogEntry(t *testing.T) {
HTTPRequest: &logging.HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/;o=0"}},
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b12000100000/;o=0"}},
},
},
},
want: &logpb.LogEntry{
Trace: "projects/P/traces/105445aa7843bc8bf206b120001000",
Trace: "projects/P/traces/105445aa7843bc8bf206b12000100000",
},
}, {
name: "X-Trace-Context header with missing traceSampled aka ?o=*",
in: logging.Entry{
HTTPRequest: &logging.HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/0"}},
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b12000100000/0"}},
},
},
},
want: &logpb.LogEntry{
Trace: "projects/P/traces/105445aa7843bc8bf206b120001000",
Trace: "projects/P/traces/105445aa7843bc8bf206b12000100000",
},
}, {
name: "X-Trace-Context header with all blank fields",
Expand Down
Loading