Skip to content

Commit

Permalink
fix: encode queryParam value and fix parsing logic for queryParams (#…
Browse files Browse the repository at this point in the history
…33720)

## Description

- Add encoding for Api key secret value before storing it as query param
to avoid incorrect parsing.
- Remove hardcoded "api_key" check to mask headers.
- Used standard URL parsing APIs instead of custom
- Added unit test to cover the case 

Fixes #33742
flaws in the implementation
[here](#33528)

## Automation

/test datasource

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!IMPORTANT]
> 🟣 🟣 🟣 Your tests are running.
> Tests running at:
<https://github.com/appsmithorg/appsmith/actions/runs/9256269257>
> Commit: 602b0f5
> Workflow: `PR Automation test suite`
> Tags: `@tag.Datasource`

<!-- end of auto-generated comment: Cypress test results  -->






## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No
  • Loading branch information
rishabhrathod01 committed May 27, 2024
1 parent 8788fd6 commit 43bd032
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -68,19 +68,18 @@ private String getHeaderValue() {
headerValue = this.headerPrefix.trim() + " ";
}
headerValue += this.value;

return headerValue.trim();
}

private URI appendApiKeyParamToUrl(URI oldUrl) {

return UriComponentsBuilder.newInstance()
.scheme(oldUrl.getScheme())
.host(oldUrl.getHost())
.port(oldUrl.getPort())
.path(oldUrl.getPath())
.query(oldUrl.getQuery())
.queryParam(label, value)
.encode()
.fragment(oldUrl.getFragment())
.build()
.toUri();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,15 @@
import org.springframework.web.reactive.function.client.ClientResponse;
import org.springframework.web.reactive.function.client.ExchangeFilterFunction;
import org.springframework.web.reactive.function.client.ExchangeFunction;
import org.springframework.web.util.UriComponentsBuilder;
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;

Expand All @@ -44,7 +43,7 @@ public class RequestCaptureFilter implements ExchangeFilterFunction {
private ClientRequest request;
private final ObjectMapper objectMapper;
private final BodyReceiver bodyReceiver = new BodyReceiver();
private final String MASKED_VALUE = "****";
private static final String MASKED_VALUE = "****";

public RequestCaptureFilter(ObjectMapper objectMapper) {
this.objectMapper = objectMapper;
Expand All @@ -56,38 +55,29 @@ public RequestCaptureFilter(ObjectMapper objectMapper) {
return next.exchange(request);
}

private URI createURIWithMaskedQueryParam(URI uriToMask, String queryParamKeyToMask) {
private static String maskQueryParamInURL(URI uriToMask, String queryParamKeyToMask) {
MultiValueMap<String, String> queryParams =
UriComponentsBuilder.fromUri(uriToMask).build().getQueryParams();
UriComponentsBuilder uriBuilder = UriComponentsBuilder.fromUri(uriToMask);
// Clear the query parameters from the builder
uriBuilder.replaceQuery(null);
// Loop through the query parameters
for (Map.Entry<String, List<String>> entry : queryParams.entrySet()) {
String key = entry.getKey();
List<String> values = entry.getValue();

// If the key matches the queryParamKeyToMask, mask its values
if (key.equals(queryParamKeyToMask)) {
values = values.stream().map(value -> MASKED_VALUE).toList();
}

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;
}
// Add the (possibly masked) parameters back to the builder
for (String value : values) {
uriBuilder.queryParam(key, 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;
}
return uriBuilder.build().toUriString();
}

public ActionExecutionRequest populateRequestFields(
Expand All @@ -105,7 +95,6 @@ public ActionExecutionRequest populateRequestFields(
AtomicBoolean isMultipart = new AtomicBoolean(false);

String headerToMask = "";

String URLString = request.url().toString();

AuthenticationDTO authentication = datasourceConfiguration.getAuthentication();
Expand All @@ -117,18 +106,15 @@ public ActionExecutionRequest populateRequestFields(
headerToMask = auth.getLabel();
}
if (auth.getAddTo().equals(ApiKeyAuth.Type.QUERY_PARAMS)) {
URI maskedURI = createURIWithMaskedQueryParam(request.url(), auth.getLabel());
URLString = maskedURI.toString();
URLString = maskQueryParamInURL(request.url(), auth.getLabel());
}
}
}
String finalHeaderToMask = headerToMask;
MultiValueMap<String, String> headers =
CollectionUtils.toMultiValueMap(new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH));
request.headers().forEach((header, value) -> {
if (HttpHeaders.AUTHORIZATION.equalsIgnoreCase(header)
|| "api_key".equalsIgnoreCase(header)
|| finalHeaderToMask.equals(header)) {
if (HttpHeaders.AUTHORIZATION.equalsIgnoreCase(header) || finalHeaderToMask.equals(header)) {
headers.add(header, MASKED_VALUE);
} else {
headers.addAll(header, value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2662,7 +2662,7 @@ public void testMaskingOfAPIKeyAddedToQueryParams() {
mockEndpoint.enqueue(new MockResponse().setBody("{}").addHeader("Content-Type", "application/json"));

AuthenticationDTO authenticationDTO =
new ApiKeyAuth(ApiKeyAuth.Type.QUERY_PARAMS, "secret", null, "secret_value");
new ApiKeyAuth(ApiKeyAuth.Type.QUERY_PARAMS, "secret", null, "secret&another=value");
dsConfig.setAuthentication(authenticationDTO);

ActionConfiguration actionConfig = new ActionConfiguration();
Expand Down

0 comments on commit 43bd032

Please sign in to comment.