Skip to content
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

CORE-81: strip info from error messages before responding #3213

Merged
merged 2 commits into from
Mar 6, 2025

Conversation

davidangb
Copy link
Contributor

@davidangb davidangb commented Mar 5, 2025

Ticket: CORE-81

Two changes in this PR to address info disclosure in error messages:

  1. In the main exception handler, strip stack traces from the error response.
  2. In HttpClientUtils, hide any private uris

Both of these fixes should cover many more cases than the three mentioned in the Jira ticket, though this PR is certainly not a comprehensive audit of info disclosure in error messages.


PR checklist

  • Include the JIRA issue number in the PR description and title
  • Make sure Swagger is updated if API changes
    • ...and Orchestration's Swagger too!
  • If you changed anything in model/, then you should publish a new official rawls-model and perform the corresponding dependency updates as specified in the README:
    • in the automation subdirectory
    • in workbench-libs
    • in firecloud-orchestration
  • Get two thumbsworth of PR review
  • Verify all tests go green, including CI tests
  • Squash commits and merge to develop (branches are automatically deleted after merging)
  • Inform other teams of any substantial changes via Slack and/or email

@@ -34,7 +34,7 @@ trait HttpClientUtils extends LazyLogging {
executeRequest(http, httpRequest) recover { case t: Throwable =>
throw new RawlsExceptionWithErrorReport(
ErrorReport(StatusCodes.InternalServerError,
s"HTTP call failed: ${httpRequest.uri}. Response: ${t.getMessage}",
s"HTTP call failed: ${filterPrivate(httpRequest.uri)}. Response: ${t.getMessage}",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It felt helpful to include the target uri in the error message, as long as that uri was not private; so I added a filter instead of removing the uri altogether

@davidangb davidangb changed the title CORE-81: strip stack traces from ErrorReports before responding CORE-81: strip info from error messages before responding Mar 5, 2025
@davidangb davidangb marked this pull request as ready for review March 5, 2025 21:45
@davidangb davidangb requested a review from a team as a code owner March 5, 2025 21:45
@davidangb davidangb requested review from calypsomatic and samanehsan and removed request for a team March 5, 2025 21:45
@davidangb davidangb merged commit 961e20d into develop Mar 6, 2025
29 checks passed
@davidangb davidangb deleted the da_CORE-81_errorDisclosure branch March 6, 2025 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants