-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
ANCHOR-403 Refactor TOML Exception obfuscation for URL/Parsing to Toml Functions our of NetUtil #1051
ANCHOR-403 Refactor TOML Exception obfuscation for URL/Parsing to Toml Functions our of NetUtil #1051
Changes from all commits
d4f087b
2d74b54
c797ff5
30c7943
93acffc
05c167d
eb31590
4e9c934
1c2ed0d
e73c208
8da260c
19c84e5
3c6f9b0
807ff46
33369dd
574e8c7
90d9550
4bd7244
be182c3
f272904
ea1cd61
eaf3224
7288802
7a4cec4
d726f77
5ec4ad3
90c584b
10e6562
482057e
56ece29
a622baf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,39 @@ | ||
package org.stellar.anchor.util; | ||
|
||
import static org.stellar.anchor.util.Log.*; | ||
|
||
import com.moandjiezana.toml.Toml; | ||
import java.io.IOException; | ||
import java.net.URL; | ||
import org.stellar.anchor.api.exception.InvalidConfigException; | ||
|
||
public class Sep1Helper { | ||
public static TomlContent readToml(String url) throws IOException { | ||
return new TomlContent(new URL(url)); | ||
try { | ||
String tomlValue = NetUtil.fetch(url); | ||
return new TomlContent(tomlValue); | ||
} catch (IOException e) { | ||
String obfuscatedMessage = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exception message is swallowed, we should log it. Same for method below There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. whoops i think i confused myself. meant to log the exception. :) fixed that. thanks |
||
String.format("An error occurred while fetching the TOML from %s", url); | ||
Log.error(e.toString()); | ||
throw new IOException(obfuscatedMessage); // Preserve the original exception as the cause | ||
} | ||
} | ||
|
||
public static TomlContent parse(String tomlString) { | ||
return new TomlContent(tomlString); | ||
public static TomlContent parse(String tomlString) throws InvalidConfigException { | ||
try { | ||
return new TomlContent(tomlString); | ||
} catch (Exception e) { | ||
// Obfuscate the message and rethrow | ||
String obfuscatedMessage = "Failed to parse TOML content. Invalid Config."; | ||
Log.error(e.toString()); // Log the parsing exception | ||
throw new InvalidConfigException( | ||
obfuscatedMessage); // Preserve the original exception as the cause | ||
} | ||
} | ||
|
||
public static class TomlContent { | ||
private final Toml toml; | ||
|
||
TomlContent(URL url) throws IOException { | ||
String tomlValue = NetUtil.fetch(url.toString()); | ||
toml = new Toml().read(tomlValue); | ||
philipliu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
TomlContent(String tomlString) { | ||
toml = new Toml().read(tomlString); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
package org.stellar.anchor.util | ||
|
||
import java.io.IOException | ||
import okhttp3.mockwebserver.MockResponse | ||
import okhttp3.mockwebserver.MockWebServer | ||
import org.junit.jupiter.api.AfterEach | ||
import org.junit.jupiter.api.Assertions.* | ||
import org.junit.jupiter.api.BeforeEach | ||
import org.junit.jupiter.api.Test | ||
import org.stellar.anchor.api.exception.InvalidConfigException | ||
|
||
class Sep1HelperTest { | ||
val stellarToml = | ||
""" | ||
ACCOUNTS = [ "GCSGSR6KQQ5BP2FXVPWRL6SWPUSFWLVONLIBJZUKTVQB5FYJFVL6XOXE" ] | ||
VERSION = "0.1.0" | ||
SIGNING_KEY = "GBDYDBJKQBJK4GY4V7FAONSFF2IBJSKNTBYJ65F5KCGBY2BIGPGGLJOH" | ||
NETWORK_PASSPHRASE = "Test SDF Network ; September 2015" | ||
|
||
WEB_AUTH_ENDPOINT = "http://localhost:8080/auth" | ||
KYC_SERVER = "http://localhost:8080/sep12" | ||
TRANSFER_SERVER_SEP0024 = "http://localhost:8080/sep24" | ||
DIRECT_PAYMENT_SERVER = "http://localhost:8080/sep31" | ||
ANCHOR_QUOTE_SERVER = "http://localhost:8080/sep38" | ||
|
||
[[CURRENCIES]] | ||
code = "SRT" | ||
issuer = "GCDNJUBQSX7AJWLJACMJ7I4BC3Z47BQUTMHEICZLE6MU4KQBRYG5JY6B" | ||
status = "test" | ||
is_asset_anchored = false | ||
anchor_asset_type = "other" | ||
desc = "A fake anchored asset to use with this example anchor server." | ||
|
||
[DOCUMENTATION] | ||
ORG_NAME = "Stellar Development Foundation" | ||
ORG_URL = "https://stellar.org" | ||
ORG_DESCRIPTION = "SEP 24 reference server." | ||
ORG_KEYBASE = "stellar.public" | ||
ORG_TWITTER = "StellarOrg" | ||
ORG_GITHUB = "stellar" | ||
""" | ||
.trimIndent() | ||
|
||
private val mockWebServer = MockWebServer() | ||
|
||
@BeforeEach | ||
fun setup() { | ||
mockWebServer.start() | ||
} | ||
|
||
@AfterEach | ||
fun teardown() { | ||
mockWebServer.shutdown() | ||
} | ||
|
||
@Test | ||
fun `test Read Toml with IOException`() { | ||
// Enqueue a response with an HTTP error status code | ||
mockWebServer.enqueue(MockResponse().setResponseCode(500)) | ||
val exception = | ||
assertThrows(IOException::class.java) { | ||
Sep1Helper.readToml(mockWebServer.url("/").toString()) | ||
} | ||
// You may need to adjust the assertion based on the specific behavior of Sep1Helper | ||
assertTrue(exception.message!!.contains("An error occurred while fetching the TOML from")) | ||
} | ||
|
||
@Test | ||
fun `test Parse Invalid Toml String`() { | ||
val invalidTomlString = "key = value" // An invalid TOML string without quotes | ||
val exception = | ||
assertThrows(InvalidConfigException::class.java) { Sep1Helper.parse(invalidTomlString) } | ||
assertEquals("Failed to parse TOML content. Invalid Config.", exception.message) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,13 @@ | ||
package org.stellar.anchor | ||
|
||
import java.io.IOException | ||
import java.nio.file.Files | ||
import java.nio.file.Path | ||
import okhttp3.mockwebserver.MockResponse | ||
import okhttp3.mockwebserver.MockWebServer | ||
import org.junit.jupiter.api.Assertions.assertEquals | ||
import org.junit.jupiter.api.Assertions.assertThrows | ||
import org.junit.jupiter.api.Assertions.assertTrue | ||
import org.junit.jupiter.api.Test | ||
import org.stellar.anchor.config.Sep1Config.TomlType.* | ||
import org.stellar.anchor.platform.config.PropertySep1Config | ||
|
@@ -13,8 +16,9 @@ import org.stellar.anchor.platform.config.Sep1ConfigTest | |
import org.stellar.anchor.sep1.Sep1Service | ||
|
||
class Sep1ServiceTest { | ||
|
||
lateinit var sep1: Sep1Service | ||
val stellarToml = | ||
private val stellarToml = | ||
""" | ||
ACCOUNTS = [ "GCSGSR6KQQ5BP2FXVPWRL6SWPUSFWLVONLIBJZUKTVQB5FYJFVL6XOXE" ] | ||
VERSION = "0.1.0" | ||
|
@@ -47,7 +51,6 @@ class Sep1ServiceTest { | |
|
||
@Test | ||
fun `test Sep1Service reading toml from inline string`() { | ||
|
||
val config = PropertySep1Config(true, TomlConfig(STRING, stellarToml)) | ||
sep1 = Sep1Service(config) | ||
assertEquals(sep1.stellarToml, stellarToml) | ||
|
@@ -61,14 +64,62 @@ class Sep1ServiceTest { | |
} | ||
|
||
@Test | ||
fun `test Sep1Service reading toml from url`() { | ||
fun `getStellarToml fetches data during re-direct`() { | ||
val mockServer = MockWebServer() | ||
mockServer.start() | ||
val mockAnchorUrl = mockServer.url("").toString() | ||
mockServer.enqueue(MockResponse().setBody(stellarToml)) | ||
|
||
val config = PropertySep1Config(true, TomlConfig(URL, mockAnchorUrl)) | ||
sep1 = Sep1Service(config) | ||
assertEquals(sep1.stellarToml, stellarToml) | ||
} | ||
|
||
// this test is not expected to raise an exception. given the re-direct to a malicious | ||
// endpoint still returns a 200 the exception will be raised/obfuscated | ||
// when the toml is parsed. | ||
@Test | ||
fun `getStellarToml fetches invalid data during malicious re-direct`() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I think the intention is to make it obvious this can happen, but it seems a bit weird given that this is something we would want to prevent. Maybe we could change the test name to something like |
||
val mockServer = MockWebServer() | ||
mockServer.start() | ||
val mockAnchorUrl = mockServer.url("").toString() | ||
val metadata = | ||
"{\n" + | ||
" \"ami-id\": \"ami-12345678\",\n" + | ||
" \"instance-id\": \"i-1234567890abcdef\",\n" + | ||
" \"instance-type\": \"t2.micro\"\n" + | ||
" // ... other metadata ...\n" + | ||
"}" | ||
|
||
// Enqueue a response with a 302 status and a Location header to simulate a redirect. | ||
mockServer.enqueue( | ||
MockResponse() | ||
.setResponseCode(302) | ||
.setHeader("Location", mockServer.url("/new_location").toString()) | ||
) | ||
|
||
// Enqueue a response at the redirect location that simulates AWS metadata leak. | ||
mockServer.enqueue(MockResponse().setResponseCode(200).setBody(metadata)) | ||
|
||
val config = PropertySep1Config(true, TomlConfig(URL, mockAnchorUrl)) | ||
val sep1 = Sep1Service(config) | ||
assertEquals(sep1.getStellarToml(), metadata) | ||
mockServer.shutdown() | ||
} | ||
|
||
@Test | ||
fun `getStellarToml throws exception when redirected location results in error`() { | ||
val mockServer = MockWebServer() | ||
mockServer.start() | ||
val mockAnchorUrl = mockServer.url("/new_location").toString() | ||
|
||
// Enqueue a response that provides a server error. | ||
mockServer.enqueue(MockResponse().setResponseCode(500)) | ||
|
||
val config = PropertySep1Config(true, TomlConfig(URL, mockAnchorUrl)) | ||
val exception = assertThrows(IOException::class.java) { sep1 = Sep1Service(config) } | ||
assertTrue( | ||
exception.message?.contains("code=500, message=Server Error, url=http://localhost:") == true | ||
) | ||
mockServer.shutdown() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure don't want to return 4xx or 5xx response bodies?
If this function only has one reference at the moment, I would suggest changing the return value to include both the response body and status code, that way callers can decide for themselves if they're interested in non-success response bodies.