From 0150655891537a8de3f3debb5ec5c49f4a6de146 Mon Sep 17 00:00:00 2001 From: minherz Date: Sun, 5 Dec 2021 11:19:23 +0200 Subject: [PATCH] fix: enforce w3c trace context value validation (#777) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: enforce w3c trace context value validation validate length and characters of the w3c trace context value. see https://www.w3.org/TR/trace-context/#traceparent-header-field-values refactor unit testing to bring invalid w3c trace context tests to separate file Fixes #774 * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * chore: fix bug in formatting validation change length of the span field from 12 to 16 according to the standard * chore: refactor w3c format regex Co-authored-by: Owl Bot --- README.md | 4 +- .../com/google/cloud/logging/Context.java | 33 +++++------ .../com/google/cloud/logging/ContextTest.java | 44 ++------------ .../cloud/logging/InvalidContextTest.java | 57 +++++++++++++++++++ 4 files changed, 78 insertions(+), 60 deletions(-) create mode 100644 google-cloud-logging/src/test/java/com/google/cloud/logging/InvalidContextTest.java diff --git a/README.md b/README.md index 537b48984..82929e763 100644 --- a/README.md +++ b/README.md @@ -58,13 +58,13 @@ implementation 'com.google.cloud:google-cloud-logging' If you are using Gradle without BOM, add this to your dependencies ```Groovy -implementation 'com.google.cloud:google-cloud-logging:3.5.0' +implementation 'com.google.cloud:google-cloud-logging:3.5.1' ``` If you are using SBT, add this to your dependencies ```Scala -libraryDependencies += "com.google.cloud" % "google-cloud-logging" % "3.5.0" +libraryDependencies += "com.google.cloud" % "google-cloud-logging" % "3.5.1" ``` ## Authentication diff --git a/google-cloud-logging/src/main/java/com/google/cloud/logging/Context.java b/google-cloud-logging/src/main/java/com/google/cloud/logging/Context.java index e88980d36..97867cdee 100644 --- a/google-cloud-logging/src/main/java/com/google/cloud/logging/Context.java +++ b/google-cloud-logging/src/main/java/com/google/cloud/logging/Context.java @@ -18,11 +18,17 @@ import com.google.cloud.logging.HttpRequest.RequestMethod; import com.google.common.base.MoreObjects; -import com.google.common.base.Strings; import java.util.Objects; +import java.util.regex.Matcher; +import java.util.regex.Pattern; /** Class to hold context attributes including information about {@see HttpRequest} and tracing. */ public class Context { + // validate W3C trace context value on load according to the existing version format. + // see https://www.w3.org/TR/trace-context/#traceparent-header-field-values for details. + private static final Pattern W3C_TRACE_CONTEXT_FORMAT = + Pattern.compile( + "^00-(?!00000000000000000000000000000000)[0-9a-f]{32}-(?!0000000000000000)[0-9a-f]{16}-[0-9a-f]{2}$"); private final HttpRequest request; private final String traceId; private final String spanId; @@ -142,26 +148,15 @@ public Builder loadCloudTraceContext(String cloudTrace) { */ public Builder loadW3CTraceParentContext(String traceParent) throws IllegalArgumentException { if (traceParent != null) { - String[] fields = traceParent.split("-"); - if (fields.length > 3) { - String versionFormat = fields[0]; - if (!versionFormat.equals("00")) { - throw new IllegalArgumentException("Not supporting versionFormat other than \"00\""); - } - } else { + Matcher validator = W3C_TRACE_CONTEXT_FORMAT.matcher(traceParent.toLowerCase()); + if (!validator.matches()) { throw new IllegalArgumentException( - "Invalid format of the header value. Expected \"00-traceid-spanid-arguments\""); - } - String traceId = fields[1]; - if (!traceId.isEmpty()) { - setTraceId(traceId); - } - if (!Strings.isNullOrEmpty(traceId)) { - String spanId = fields[2]; - if (!spanId.isEmpty()) { - setSpanId(spanId); - } + "Invalid format of the header value. The value does not match W3C Trace Context version \"00\""); } + String[] fields = traceParent.split("-"); + setTraceId(fields[1]); + setSpanId(fields[2]); + // fields[3] contains flag(s) } return this; } diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/ContextTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/ContextTest.java index 72c91236a..b9168ec8b 100644 --- a/google-cloud-logging/src/test/java/com/google/cloud/logging/ContextTest.java +++ b/google-cloud-logging/src/test/java/com/google/cloud/logging/ContextTest.java @@ -132,50 +132,16 @@ public void testParsingCloudTraceContext() { @Test public void testParsingW3CTraceParent() { - final String TRACEPARENT_NO_TRACE = "00--SPAN_ID-FLAGS"; - final String TRACEPARENT_TRACE_ONLY = "00-" + TEST_TRACE_ID + "--SPAN_ID-FLAGS"; - final String TRACEPARENT_TRACE_FULL = "00-" + TEST_TRACE_ID + "-" + TEST_SPAN_ID + "-FLAGS"; + final String W3C_TEST_TRACE_ID = "12345678901234567890123456789012"; + final String W3C_TEST_SPAN_ID = "1234567890123456"; + final String W3C_TRACE_CONTEXT = "00-" + W3C_TEST_TRACE_ID + "-" + W3C_TEST_SPAN_ID + "-00"; Context.Builder builder = Context.newBuilder(); builder.loadW3CTraceParentContext(null); assertTraceAndSpan(builder.build(), null, null); - builder.loadW3CTraceParentContext(TRACEPARENT_NO_TRACE); - assertTraceAndSpan(builder.build(), null, null); - builder.loadW3CTraceParentContext(TRACEPARENT_TRACE_ONLY); - assertTraceAndSpan(builder.build(), TEST_TRACE_ID, null); - builder.loadW3CTraceParentContext(TRACEPARENT_TRACE_FULL); - assertTraceAndSpan(builder.build(), TEST_TRACE_ID, TEST_SPAN_ID); - } - - @Test(expected = IllegalArgumentException.class) - public void testEmptyW3CTraceParent() { - Context.Builder builder = Context.newBuilder(); - builder.loadW3CTraceParentContext(""); - } - - @Test(expected = IllegalArgumentException.class) - public void testInvalidFormatW3CTraceParent() { - Context.Builder builder = Context.newBuilder(); - builder.loadW3CTraceParentContext("TRACE_ID/SPAN_ID;o=TRACE_TRUE"); - } - - @Test(expected = IllegalArgumentException.class) - public void testInvalidFormatW3CTraceParent1Dash() { - Context.Builder builder = Context.newBuilder(); - builder.loadW3CTraceParentContext("00-TRACE_ID"); - } - - @Test(expected = IllegalArgumentException.class) - public void testInvalidFormatW3CTraceParentWithoutFlag() { - Context.Builder builder = Context.newBuilder(); - builder.loadW3CTraceParentContext("00-TRACE_ID-SPAN_ID-"); - } - - @Test(expected = IllegalArgumentException.class) - public void testInvalidVersionW3CTraceParent() { - Context.Builder builder = Context.newBuilder(); - builder.loadW3CTraceParentContext("01-TRACE_ID-SPAN_ID-FLAGS"); + builder.loadW3CTraceParentContext(W3C_TRACE_CONTEXT); + assertTraceAndSpan(builder.build(), W3C_TEST_TRACE_ID, W3C_TEST_SPAN_ID); } private void assertTraceAndSpan(Context context, String expectedTraceId, String expectedSpanId) { diff --git a/google-cloud-logging/src/test/java/com/google/cloud/logging/InvalidContextTest.java b/google-cloud-logging/src/test/java/com/google/cloud/logging/InvalidContextTest.java new file mode 100644 index 000000000..4ae5cd063 --- /dev/null +++ b/google-cloud-logging/src/test/java/com/google/cloud/logging/InvalidContextTest.java @@ -0,0 +1,57 @@ +/* + * Copyright 2021 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.logging; + +import java.util.Arrays; +import java.util.Collection; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; + +@RunWith(Parameterized.class) +public class InvalidContextTest { + @Parameters + public static Collection data() { + final String[] INVALID_W3C_TRACE_CONTEXTS = { + "", + "abc/efg", + "01-something", + "00-123456789012345678901234567890", + "00-12345678901234567890123456789012", + "00-12345678901234567890123456789012345", + "00-12345678901234567890123456789012-123456789012345", + "00-12345678901234567890123456789012-1234567890123456", + "00-12345678901234567890123456789012-12345678901234567", + "00-12345678901234567890123456789012-1234567890123456-1", + "00-12345678901234567890123456789012-1234567890123456-123" + }; + return Arrays.asList(INVALID_W3C_TRACE_CONTEXTS); + } + + private final String traceContext; + + public InvalidContextTest(String traceContext) { + this.traceContext = traceContext; + } + + @Test(expected = IllegalArgumentException.class) + public void testAssertionInvalidContext() { + Context.Builder builder = Context.newBuilder(); + builder.loadW3CTraceParentContext(traceContext); + } +}