-
Notifications
You must be signed in to change notification settings - Fork 46
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
Discovery token is filtered out in exception message [API-1738]. #1044
Conversation
Linux test PASSed. |
1 similar comment
Linux test PASSed. |
Windows test PASSed. |
FAIL(); | ||
} catch (const exception::illegal_state& e) { | ||
std::string message = e.what(); | ||
ASSERT_EQ(message.find(discovery_token), std::string::npos); |
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.
you should also check the nested exception message!!!
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.
Nested exception contains it.
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.
so, it still leaks?
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.
Changed the implementation, token
is filtered out in the class which is the source of the exception. So it is not leaked by neither inner nor outer exceptions.
There is a problem related with an unit test, I wrote the test to check inner exception message but std::rethrow_if_nested
doesn't throw the inner exception. Added it to comment.
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.
Let's open an issue for this.
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.
I will open after merge with the link of the test.
timeout_); | ||
auto& conn_stream = httpsConnection.connect_and_get_response(); | ||
return parse_json_response(conn_stream); | ||
} catch (std::exception& e) { | ||
std::string message{ e.what() }; | ||
boost::replace_all(message, discovery_token, "<DISCOVERY_TOKEN>"); | ||
std::throw_with_nested( |
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.
can it leak due to nested exception?
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.
On application crash it is not printed to stdout
. But the token is included nested_exception
that's why I added filter
handler to SyncHttpsRequest
earlier.
Windows test PASSed. |
Linux test PASSed. |
Windows test PASSed. |
1 similar comment
Windows test PASSed. |
Linux test PASSed. |
Linux test PASSed. |
Windows test PASSed. |
Linux test PASSed. |
Windows test PASSed. |
Windows test PASSed. |
Linux test PASSed. |
token_should_not_be_leaked
test is added for this case.