Skip to content

Commit a040b86

Browse files
peterniedpeternied
authored
Fix windows encoding issues (opensearch-project#2206)
Adds spotbugs [1] to detect internalization before they are added to the codebase, also fixed several encoding bugs that impact windows users. [1] https://spotbugs.readthedocs.io/en/stable/index.html Closes opensearch-project#2194 Signed-off-by: Peter Nied <petern@amazon.com>
1 parent a57fd0a commit a040b86

11 files changed

Lines changed: 44 additions & 11 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ jobs:
3434
-x spotlessCheck
3535
-x checkstyleMain
3636
-x checkstyleTest
37+
-x spotbugsMain
3738
3839
- name: Coverage
3940
uses: codecov/codecov-action@v1
@@ -80,6 +81,7 @@ jobs:
8081
-x spotlessCheck
8182
-x checkstyleMain
8283
-x checkstyleTest
84+
-x spotbugsMain
8385
8486
backward-compatibility:
8587
runs-on: ubuntu-latest

‎.github/workflows/code-hygiene.yml‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,3 +42,18 @@ jobs:
4242
- uses: gradle/gradle-build-action@v2
4343
with:
4444
arguments: checkstyleMain checkstyleTest
45+
46+
spotbugs:
47+
runs-on: ubuntu-latest
48+
name: Spotbugs scan
49+
steps:
50+
- uses: actions/checkout@v2
51+
52+
- uses: actions/setup-java@v2
53+
with:
54+
distribution: temurin # Temurin is a distribution of adoptium
55+
java-version: 11
56+
57+
- uses: gradle/gradle-build-action@v2
58+
with:
59+
arguments: spotbugsMain

‎build.gradle‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ plugins {
5757
id 'nebula.ospackage' version "8.3.0"
5858
id "org.gradle.test-retry" version "1.3.1"
5959
id 'eclipse'
60+
id "com.github.spotbugs" version "5.0.13"
6061
}
6162

6263
allprojects {
@@ -85,6 +86,14 @@ spotless {
8586
}
8687
}
8788

89+
spotbugs {
90+
includeFilter = file('spotbugs-include.xml')
91+
}
92+
93+
spotbugsTest {
94+
enabled = false
95+
}
96+
8897
java.sourceCompatibility = JavaVersion.VERSION_11
8998
java.targetCompatibility = JavaVersion.VERSION_11
9099

‎spotbugs-include.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<FindBugsFilter>
2+
<Match>
3+
<Bug category="I18N" />
4+
</Match>
5+
</FindBugsFilter>

‎src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import java.io.IOException;
1515
import java.net.URI;
1616
import java.net.URISyntaxException;
17+
import java.nio.charset.StandardCharsets;
1718
import java.security.AccessController;
1819
import java.security.PrivilegedActionException;
1920
import java.security.PrivilegedExceptionAction;
@@ -152,7 +153,7 @@ private AuthTokenProcessorAction.Response handleImpl(RestRequest restRequest, Re
152153
SettingsException {
153154
if (token_log.isDebugEnabled()) {
154155
try {
155-
token_log.debug("SAMLResponse for {}\n{}", samlRequestId, new String(Util.base64decoder(samlResponseBase64), "UTF-8"));
156+
token_log.debug("SAMLResponse for {}\n{}", samlRequestId, new String(Util.base64decoder(samlResponseBase64), StandardCharsets.UTF_8));
156157
} catch (Exception e) {
157158
token_log.warn(
158159
"SAMLResponse for {} cannot be decoded from base64\n{}",

‎src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,7 @@ public void logDocumentWritten(ShardId shardId, GetResult originalResult, Index
419419
try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, THROW_UNSUPPORTED_OPERATION, originalResult.internalSourceRef(), XContentType.JSON)) {
420420
Object base64 = parser.map().values().iterator().next();
421421
if(base64 instanceof String) {
422-
originalSource = (new String(BaseEncoding.base64().decode((String) base64)));
422+
originalSource = (new String(BaseEncoding.base64().decode((String) base64), StandardCharsets.UTF_8));
423423
} else {
424424
originalSource = XContentHelper.convertToJson(originalResult.internalSourceRef(), false, XContentType.JSON);
425425
}
@@ -430,7 +430,7 @@ public void logDocumentWritten(ShardId shardId, GetResult originalResult, Index
430430
try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, THROW_UNSUPPORTED_OPERATION, currentIndex.source(), XContentType.JSON)) {
431431
Object base64 = parser.map().values().iterator().next();
432432
if(base64 instanceof String) {
433-
currentSource = (new String(BaseEncoding.base64().decode((String) base64)));
433+
currentSource = new String(BaseEncoding.base64().decode((String) base64), StandardCharsets.UTF_8);
434434
} else {
435435
currentSource = XContentHelper.convertToJson(currentIndex.source(), false, XContentType.JSON);
436436
}
@@ -457,7 +457,7 @@ public void logDocumentWritten(ShardId shardId, GetResult originalResult, Index
457457
try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, THROW_UNSUPPORTED_OPERATION, currentIndex.source(), XContentType.JSON)) {
458458
Object base64 = parser.map().values().iterator().next();
459459
if(base64 instanceof String) {
460-
msg.addSecurityConfigContentToRequestBody(new String(BaseEncoding.base64().decode((String) base64)), id);
460+
msg.addSecurityConfigContentToRequestBody(new String(BaseEncoding.base64().decode((String) base64), StandardCharsets.UTF_8), id);
461461
} else {
462462
msg.addSecurityConfigTupleToRequestBody(new Tuple<XContentType, BytesReference>(XContentType.JSON, currentIndex.source()), id);
463463
}

‎src/main/java/org/opensearch/security/configuration/ConfigurationLoaderSecurity7.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
package org.opensearch.security.configuration;
2828

2929
import java.io.IOException;
30+
import java.nio.charset.StandardCharsets;
3031
import java.util.Arrays;
3132
import java.util.HashMap;
3233
import java.util.Map;
@@ -253,7 +254,7 @@ private SecurityDynamicConfiguration<?> toConfig(GetResponse singleGetResponse,
253254

254255
parser.nextToken();
255256

256-
final String jsonAsString = SecurityUtils.replaceEnvVars(new String(parser.binaryValue()), settings);
257+
final String jsonAsString = SecurityUtils.replaceEnvVars(new String(parser.binaryValue(), StandardCharsets.UTF_8), settings);
257258
final JsonNode jsonNode = DefaultObjectMapper.readTree(jsonAsString);
258259
int configVersion = 1;
259260

‎src/main/java/org/opensearch/security/ssl/DefaultSecurityKeyStore.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.io.ByteArrayInputStream;
2121
import java.io.File;
2222
import java.io.IOException;
23+
import java.nio.charset.StandardCharsets;
2324
import java.nio.file.Files;
2425
import java.nio.file.LinkOption;
2526
import java.nio.file.Path;
@@ -646,7 +647,7 @@ private boolean areSameCerts(final X509Certificate[] currentX509Certs, final X50
646647

647648
final Function<? super X509Certificate, String> certificateSignature = cert -> {
648649
final byte[] signature = cert !=null && cert.getSignature() != null ? cert.getSignature() : null;
649-
return new String(signature);
650+
return new String(signature, StandardCharsets.UTF_8);
650651
};
651652

652653
final Set<String> currentCertSignatureSet = Arrays.stream(currentX509Certs)

‎src/main/java/org/opensearch/security/support/ConfigHelper.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.io.IOException;
3232
import java.io.Reader;
3333
import java.io.StringReader;
34+
import java.nio.charset.StandardCharsets;
3435

3536
import org.apache.logging.log4j.LogManager;
3637
import org.apache.logging.log4j.Logger;
@@ -91,7 +92,7 @@ public static void uploadFile(Client tc, String filepath, String index, CType cT
9192
public static Reader createFileOrStringReader(CType cType, int configVersion, String filepath, boolean populateEmptyIfFileMissing) throws Exception {
9293
Reader reader;
9394
if (!populateEmptyIfFileMissing || new File(filepath).exists()) {
94-
reader = new FileReader(filepath);
95+
reader = new FileReader(filepath, StandardCharsets.UTF_8);
9596
} else {
9697
reader = new StringReader(createEmptySdcYaml(cType, configVersion));
9798
}
@@ -143,7 +144,7 @@ public static <T> SecurityDynamicConfiguration<T> fromYamlReader(Reader yamlRead
143144
}
144145

145146
public static <T> SecurityDynamicConfiguration<T> fromYamlFile(String filepath, CType ctype, int version, long seqNo, long primaryTerm) throws IOException {
146-
return fromYamlReader(new FileReader(filepath), ctype, version, seqNo, primaryTerm);
147+
return fromYamlReader(new FileReader(filepath, StandardCharsets.UTF_8), ctype, version, seqNo, primaryTerm);
147148
}
148149

149150
public static <T> SecurityDynamicConfiguration<T> fromYamlString(String yamlString, CType ctype, int version, long seqNo, long primaryTerm) throws IOException {

‎src/main/java/org/opensearch/security/tools/SecurityAdmin.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -896,7 +896,7 @@ private static boolean retrieveFile(final RestHighLevelClient restHighLevelClien
896896
}
897897

898898
System.out.println("Will retrieve '"+"/" +id+"' into "+filepath+" "+(legacy?"(legacy mode)":""));
899-
try (Writer writer = new FileWriter(filepath)) {
899+
try (Writer writer = new FileWriter(filepath, StandardCharsets.UTF_8)) {
900900

901901
final GetResponse response = restHighLevelClient.get(new GetRequest(index).id(id).refresh(true).realtime(false), RequestOptions.DEFAULT);
902902

0 commit comments

Comments
 (0)