From b59838c03ae395a024fe5776f29639afed2f4e9d Mon Sep 17 00:00:00 2001 From: Rishabh Rathod Date: Thu, 23 May 2024 13:45:16 +0530 Subject: [PATCH] fix: apiKey security issue (#33528) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Fixes #30009 ### Summary: This PR addresses the issue of masking sensitive information in query parameters or headers based on the authentication type selected by the user. The changes ensure that sensitive data is properly masked before sending back as response. ### Changes: - RequestCaptureFilter.java Added logic to check the authentication type and mask the appropriate query parameters or headers. ### Testing: Verified that the masking functionality works as expected for API_KEY authentication types. ## Automation /test datasource ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 9a7fc9cc1942ddc61c54f6ae9451706ad527f49c > Cypress dashboard url: Click here! ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- .../helpers/RequestCaptureFilter.java | 81 ++++++++++++++++--- .../helpers/RestAPIActivateUtils.java | 5 +- .../com/external/plugins/GraphQLPlugin.java | 5 +- .../com/external/plugins/RestApiPlugin.java | 5 +- .../external/plugins/RestApiPluginTest.java | 78 ++++++++++++++++++ 5 files changed, 159 insertions(+), 15 deletions(-) diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RequestCaptureFilter.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RequestCaptureFilter.java index db7d0bb3b8..6e5fecf8ca 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RequestCaptureFilter.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RequestCaptureFilter.java @@ -2,6 +2,9 @@ package com.appsmith.external.helpers.restApiUtils.helpers; import com.appsmith.external.models.ActionConfiguration; import com.appsmith.external.models.ActionExecutionRequest; +import com.appsmith.external.models.ApiKeyAuth; +import com.appsmith.external.models.AuthenticationDTO; +import com.appsmith.external.models.DatasourceConfiguration; import com.appsmith.external.models.Property; import com.fasterxml.jackson.databind.ObjectMapper; import lombok.Getter; @@ -18,11 +21,13 @@ import org.springframework.web.reactive.function.client.ExchangeFunction; import reactor.core.publisher.Mono; import java.net.URI; +import java.net.URISyntaxException; import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.StringJoiner; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; @@ -39,6 +44,7 @@ public class RequestCaptureFilter implements ExchangeFilterFunction { private ClientRequest request; private final ObjectMapper objectMapper; private final BodyReceiver bodyReceiver = new BodyReceiver(); + private final String MASKED_VALUE = "****"; public RequestCaptureFilter(ObjectMapper objectMapper) { this.objectMapper = objectMapper; @@ -50,8 +56,44 @@ public class RequestCaptureFilter implements ExchangeFilterFunction { return next.exchange(request); } + private URI createURIWithMaskedQueryParam(URI uriToMask, String queryParamKeyToMask) { + + String query = uriToMask.getQuery(); + if (query == null) { + return uriToMask; + } + String[] queryParams = query.split("&"); + StringJoiner newQuery = new StringJoiner("&"); + for (String queryParam : queryParams) { + String[] keyValuePair = queryParam.split("="); + + if (queryParamKeyToMask.equals(keyValuePair[0])) { + // fix when value is not present + if (keyValuePair.length > 1) { + keyValuePair[1] = MASKED_VALUE; + } + } + newQuery.add(keyValuePair[0] + "=" + keyValuePair[1]); + } + + try { + return new URI( + uriToMask.getScheme(), + uriToMask.getUserInfo(), + uriToMask.getHost(), + uriToMask.getPort(), + uriToMask.getPath(), + newQuery.toString(), + uriToMask.getRawFragment()); + } catch (URISyntaxException e) { + return uriToMask; + } + } + public ActionExecutionRequest populateRequestFields( - ActionExecutionRequest existing, boolean isBodySentWithApiRequest) { + ActionExecutionRequest existing, + boolean isBodySentWithApiRequest, + DatasourceConfiguration datasourceConfiguration) { final ActionExecutionRequest actionExecutionRequest = new ActionExecutionRequest(); if (request == null) { @@ -60,14 +102,34 @@ public class RequestCaptureFilter implements ExchangeFilterFunction { return actionExecutionRequest; } - actionExecutionRequest.setUrl(request.url().toString()); - actionExecutionRequest.setHttpMethod(request.method()); + AtomicBoolean isMultipart = new AtomicBoolean(false); + + String headerToMask = ""; + + String URLString = request.url().toString(); + + AuthenticationDTO authentication = datasourceConfiguration.getAuthentication(); + + // if authenticationType is api_key then check the addTo method and mask the api_key set by user accordingly. + if (authentication instanceof ApiKeyAuth auth) { + if (auth.getAddTo() != null) { + if (auth.getAddTo().equals(ApiKeyAuth.Type.HEADER)) { + headerToMask = auth.getLabel(); + } + if (auth.getAddTo().equals(ApiKeyAuth.Type.QUERY_PARAMS)) { + URI maskedURI = createURIWithMaskedQueryParam(request.url(), auth.getLabel()); + URLString = maskedURI.toString(); + } + } + } + String finalHeaderToMask = headerToMask; MultiValueMap headers = CollectionUtils.toMultiValueMap(new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH)); - AtomicBoolean isMultipart = new AtomicBoolean(false); request.headers().forEach((header, value) -> { - if (HttpHeaders.AUTHORIZATION.equalsIgnoreCase(header) || "api_key".equalsIgnoreCase(header)) { - headers.add(header, "****"); + if (HttpHeaders.AUTHORIZATION.equalsIgnoreCase(header) + || "api_key".equalsIgnoreCase(header) + || finalHeaderToMask.equals(header)) { + headers.add(header, MASKED_VALUE); } else { headers.addAll(header, value); } @@ -76,8 +138,10 @@ public class RequestCaptureFilter implements ExchangeFilterFunction { isMultipart.set(true); } }); - actionExecutionRequest.setHeaders(objectMapper.valueToTree(headers)); + actionExecutionRequest.setUrl(URLString); + actionExecutionRequest.setHeaders(objectMapper.valueToTree(headers)); + actionExecutionRequest.setHttpMethod(request.method()); actionExecutionRequest.setRequestParams(existing.getRequestParams()); actionExecutionRequest.setExecutionParameters(existing.getExecutionParameters()); actionExecutionRequest.setProperties(existing.getProperties()); @@ -90,7 +154,6 @@ public class RequestCaptureFilter implements ExchangeFilterFunction { } else { actionExecutionRequest.setBody(existing.getBody()); } - return actionExecutionRequest; } @@ -137,7 +200,7 @@ public class RequestCaptureFilter implements ExchangeFilterFunction { .filter(property -> property.getKey() != null) .forEach(property -> bodyDataMap.put(property.getKey(), property.getValue())); actionExecutionRequest.setBody(bodyDataMap); - } else if (MediaType.MULTIPART_FORM_DATA_VALUE.equals(reqContentType.get())) { + } else if (MULTIPART_FORM_DATA_VALUE.equals(reqContentType.get())) { final List bodyFormData = actionConfiguration.getBodyFormData(); Map bodyDataMap = new HashMap<>(); bodyFormData diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RestAPIActivateUtils.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RestAPIActivateUtils.java index 11e353afe4..88a94d2323 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RestAPIActivateUtils.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RestAPIActivateUtils.java @@ -71,7 +71,8 @@ public class RestAPIActivateUtils { ObjectMapper objectMapper, Set hintMessages, ActionExecutionResult errorResult, - RequestCaptureFilter requestCaptureFilter) { + RequestCaptureFilter requestCaptureFilter, + DatasourceConfiguration datasourceConfiguration) { return httpCall(client, httpMethod, uri, requestBody, 0) .flatMap(clientResponse -> clientResponse.toEntity(byte[].class)) .map(stringResponseEntity -> { @@ -94,7 +95,7 @@ public class RestAPIActivateUtils { // Set the request fields boolean isBodySentWithApiRequest = requestBody == null ? false : true; result.setRequest(requestCaptureFilter.populateRequestFields( - actionExecutionRequest, isBodySentWithApiRequest)); + actionExecutionRequest, isBodySentWithApiRequest, datasourceConfiguration)); result.setStatusCode(statusCode.toString()); diff --git a/app/server/appsmith-plugins/graphqlPlugin/src/main/java/com/external/plugins/GraphQLPlugin.java b/app/server/appsmith-plugins/graphqlPlugin/src/main/java/com/external/plugins/GraphQLPlugin.java index 1d9e737f3c..14b61d10a7 100644 --- a/app/server/appsmith-plugins/graphqlPlugin/src/main/java/com/external/plugins/GraphQLPlugin.java +++ b/app/server/appsmith-plugins/graphqlPlugin/src/main/java/com/external/plugins/GraphQLPlugin.java @@ -281,11 +281,12 @@ public class GraphQLPlugin extends BasePlugin { objectMapper, hintMessages, errorResult, - requestCaptureFilter) + requestCaptureFilter, + datasourceConfiguration) .onErrorResume(error -> { boolean isBodySentWithApiRequest = requestBodyObj == null ? false : true; errorResult.setRequest(requestCaptureFilter.populateRequestFields( - actionExecutionRequest, isBodySentWithApiRequest)); + actionExecutionRequest, isBodySentWithApiRequest, datasourceConfiguration)); errorResult.setIsExecutionSuccess(false); if (!(error instanceof AppsmithPluginException)) { error = new AppsmithPluginException( diff --git a/app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java b/app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java index f485d26189..9fe83b1890 100644 --- a/app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java +++ b/app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java @@ -202,11 +202,12 @@ public class RestApiPlugin extends BasePlugin { objectMapper, hintMessages, errorResult, - requestCaptureFilter) + requestCaptureFilter, + datasourceConfiguration) .onErrorResume(error -> { boolean isBodySentWithApiRequest = requestBodyObj == null ? false : true; errorResult.setRequest(requestCaptureFilter.populateRequestFields( - actionExecutionRequest, isBodySentWithApiRequest)); + actionExecutionRequest, isBodySentWithApiRequest, datasourceConfiguration)); errorResult.setIsExecutionSuccess(false); log.debug( "An error has occurred while trying to run the API query for url: {}, path : {}", diff --git a/app/server/appsmith-plugins/restApiPlugin/src/test/java/com/external/plugins/RestApiPluginTest.java b/app/server/appsmith-plugins/restApiPlugin/src/test/java/com/external/plugins/RestApiPluginTest.java index ddca086cde..26d22cdcc4 100644 --- a/app/server/appsmith-plugins/restApiPlugin/src/test/java/com/external/plugins/RestApiPluginTest.java +++ b/app/server/appsmith-plugins/restApiPlugin/src/test/java/com/external/plugins/RestApiPluginTest.java @@ -2652,4 +2652,82 @@ public class RestApiPluginTest { }) .verifyComplete(); } + + @Test + public void testMaskingOfAPIKeyAddedToQueryParams() { + DatasourceConfiguration dsConfig = new DatasourceConfiguration(); + String baseUrl = String.format("http://%s:%s", mockEndpoint.getHostName(), mockEndpoint.getPort()); + dsConfig.setUrl(baseUrl); + + mockEndpoint.enqueue(new MockResponse().setBody("{}").addHeader("Content-Type", "application/json")); + + AuthenticationDTO authenticationDTO = + new ApiKeyAuth(ApiKeyAuth.Type.QUERY_PARAMS, "secret", null, "secret_value"); + dsConfig.setAuthentication(authenticationDTO); + + ActionConfiguration actionConfig = new ActionConfiguration(); + actionConfig.setHeaders(List.of( + new Property("content-type", "application/json"), + new Property(HttpHeaders.AUTHORIZATION, "auth-value"))); + actionConfig.setAutoGeneratedHeaders(List.of(new Property("content-type", "application/json"))); + actionConfig.setHttpMethod(HttpMethod.POST); + + String requestBody = "{\"key\":\"value\"}"; + actionConfig.setBody(requestBody); + + final APIConnection apiConnection = + pluginExecutor.datasourceCreate(dsConfig).block(); + + Mono resultMono = + pluginExecutor.executeParameterized(apiConnection, new ExecuteActionDTO(), dsConfig, actionConfig); + StepVerifier.create(resultMono) + .assertNext(result -> { + assertTrue(result.getIsExecutionSuccess()); + assertNotNull(result.getRequest().getBody()); + final String resultURL = result.getRequest().getUrl(); + assertEquals(baseUrl + "?secret=****", resultURL); + }) + .verifyComplete(); + } + + @Test + public void testMaskingOfAPIKeyAddedToHeaders() { + DatasourceConfiguration dsConfig = new DatasourceConfiguration(); + String baseUrl = String.format("http://%s:%s", mockEndpoint.getHostName(), mockEndpoint.getPort()); + dsConfig.setUrl(baseUrl); + + mockEndpoint.enqueue(new MockResponse().setBody("{}").addHeader("Content-Type", "application/json")); + + AuthenticationDTO authenticationDTO = new ApiKeyAuth(ApiKeyAuth.Type.HEADER, "secret", null, "secret_value"); + dsConfig.setAuthentication(authenticationDTO); + + ActionConfiguration actionConfig = new ActionConfiguration(); + actionConfig.setHeaders(List.of( + new Property("content-type", "application/json"), + new Property(HttpHeaders.AUTHORIZATION, "auth-value"))); + actionConfig.setAutoGeneratedHeaders(List.of(new Property("content-type", "application/json"))); + actionConfig.setHttpMethod(HttpMethod.POST); + + String requestBody = "{\"key\":\"value\"}"; + actionConfig.setBody(requestBody); + + final APIConnection apiConnection = + pluginExecutor.datasourceCreate(dsConfig).block(); + + Mono resultMono = + pluginExecutor.executeParameterized(apiConnection, new ExecuteActionDTO(), dsConfig, actionConfig); + StepVerifier.create(resultMono) + .assertNext(result -> { + assertTrue(result.getIsExecutionSuccess()); + assertNotNull(result.getRequest().getBody()); + final Iterator> fields = + ((ObjectNode) result.getRequest().getHeaders()).fields(); + fields.forEachRemaining(field -> { + if ("secret".equalsIgnoreCase(field.getKey())) { + assertEquals("****", field.getValue().get(0).asText()); + } + }); + }) + .verifyComplete(); + } }