ANCHOR-403 Refactor TOML Exception obfuscation for URL/Parsing to Toml Functions our of NetUtil #1051
Conversation
…ation step before the assertions
* ANCHOR-403-reece-SSRF-metadata * use assertThrows * additional cleanup * remove custom exception * make sure that the log event is actually captured by adding a verification step before the assertions * temporary setting toml to url to test * just testing. ignore * try other file * patch toml parser as well * replace comment * revert * lint * debugF * revert config changes * revert defaults
reecexlm
changed the title
|
Reference Server Preview is available here: |
| } catch (IOException e) { | ||
| // Obfuscate the message and rethrow | ||
| String obfuscatedMessage = String.format("Unable to fetch data from %s", url); | ||
| debugF(obfuscatedMessage); // Log the obfuscated message using the debugF method |
There was a problem hiding this comment.
If response is not successful this will produce 3 debug logs with the same message
There was a problem hiding this comment.
ah yea, im logging at each layer. let me look at that. thanks!
|
Reference Server Preview is available here: |
|
Reference Server Preview is available here: |
|
Something went wrong with PR preview build please check |
…P001-aws-metadata' into ANCHOR-403-reece-SDP001-aws-metadata
|
Reference Server Preview is available here: |
|
Reference Server Preview is available here: |
|
Reference Server Preview is available here: |
|
Reference Server Preview is available here: |
| String tomlValue = NetUtil.fetch(url); | ||
| return new TomlContent(tomlValue); | ||
| } catch (IOException e) { | ||
| String obfuscatedMessage = |
There was a problem hiding this comment.
Exception message is swallowed, we should log it. Same for method below
There was a problem hiding this comment.
whoops i think i confused myself. meant to log the exception. :) fixed that. thanks
|
Reference Server Preview is available here: |
|
|
||
| // Check if response was unsuccessful (ie not status code 2xx) and throw IOException | ||
| if (!response.isSuccessful()) { | ||
| debugF(message); |
There was a problem hiding this comment.
Nit: I guess if we're going to throw the exception with the same message, there's no need to log it.
There was a problem hiding this comment.
i appreciate the nit! ah yea, that was left over from when i was obfuscating here. I also don't need to catch/re-throw anymore. i will add some documentation for the fn as well so that its clear that fetch expects a response and successful return code.
| // 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.
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 getStellarToml fetches data during re-direct. What do you think?
|
Reference Server Preview is available here: |
| // Check if response was unsuccessful (ie not status code 2xx) and throw IOException | ||
| if (!response.isSuccessful()) { | ||
| throw new IOException(response.toString()); | ||
| } |
There was a problem hiding this comment.
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.
|
Reference Server Preview is available here: |
|
Something went wrong with PR preview build please check |
|
Reference Server Preview is available here: |
This PR moves the exception obfuscation for the stellar toml file handling to the Toml module and out of NetUtil. The reason is that NetUtil fetch is a lower level function that is shared. Obfuscating the exception message may not be applicable for other usages. THis PR also logs the actual exception message so that it is available for troubleshooting. Added some additional test coverage for Sep1Helper as well.
PR Structure
otherwise).
paymentservice.stellar, orallordocif the changes are broad or impact manypackages.