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

Improved error logging #3016

Merged
merged 2 commits into from May 15, 2023
Merged

Improved error logging #3016

merged 2 commits into from May 15, 2023

Conversation

gwynne
Copy link
Member

@gwynne gwynne commented May 15, 2023

Some kinds of errors provide additional "debug" information, which can give much more detail than the "plain" description of the error. In many cases this debug info can contain sensitive data, such as specifics about a database schema, so Vapor only uses the plain description when sending errors to clients (and in release environments, all details are suppressed).

To date, the plain description has also been used for logging errors. This can make it very difficult for developers to figure out what's going wrong with their code if the error in question only provides meaningful information in its debug data - for example, the PostgreSQL database driver implementation does this rather than relying on a higher-level layer like Vapor to obfuscate potentially sensitive information. This PR changes the logging of errors to include the debug information (and only the logging; the responses sent to clients are unchanged).

… of interpolating it directly (which is the same as using `String(describing:)`) so the logged errors show more information without displaying that information in error responses sent to clients.
@gwynne gwynne added enhancement New feature or request semver-patch Internal changes only labels May 15, 2023
@gwynne gwynne requested a review from 0xTim May 15, 2023 09:50
@gwynne gwynne self-assigned this May 15, 2023
@gwynne gwynne added this to Awaiting Review in Vapor 4 via automation May 15, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 15, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@d276e10). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3016   +/-   ##
=======================================
  Coverage        ?   76.92%           
=======================================
  Files           ?      211           
  Lines           ?     7770           
  Branches        ?        0           
=======================================
  Hits            ?     5977           
  Misses          ?     1793           
  Partials        ?        0           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d276e10...420ffc5. Read the comment docs.

@madsodgaard
Copy link
Contributor

@gwynne Couldn't sensitive information contained stored in log files still be a concern in production? Not sure what "sensitive" information we are talking about here. Is it database connection details or something less concerning such as schema information?

@0xTim
Copy link
Member

0xTim commented May 15, 2023

@madsodgaard this is only really converting the error to a better string - if the error contains sensitive information then yeah it will be logged, but in the same way that logging the whole request will leak sensitive info. We can't really police that as a framework and instead should provide best practices for logging and errors (i.e. errors should be fairly generic, log messages should be specific but either scrubbed from PII or using a custom backend combined with log metadata to ensure nothing is output

@gwynne
Copy link
Member Author

gwynne commented May 15, 2023

In this case I did the update pretty much specifically for the sake of PSQLError (as a companion to vapor/postgres-kit#244 and/or vapor/postgres-nio#360) - "potentially sensitive" data in that case means things like details of the database schema and entire queries, which can easily reveal exploitable info. The expectation is that sending the details to the request's logger will put the details somewhere that they can be seen by the server admin (which is potentially lifesaving in a production environment), while still keeping them from clients even in debug builds.

@gwynne gwynne merged commit 1017c37 into main May 15, 2023
13 checks passed
Vapor 4 automation moved this from Awaiting Review to Done May 15, 2023
@gwynne gwynne deleted the verbose-error-logging branch May 15, 2023 12:07
@VaporBot
Copy link
Contributor

These changes are now available in 4.75.2

keniwhat pushed a commit to keniwhat/vapor that referenced this pull request Aug 29, 2023
* main: (75 commits)
  Make Storage Sendable (vapor#3056)
  Add Sendable Conformances to undelying types (vapor#3054)
  Resolve issue vapor#2650 (vapor#2674)
  Fix for vapor#2574 Missing quote from value (vapor#2839)
  Allow specifying a timeout for client requests (vapor#3043)
  Update dependencies with known CVEs to the latest versions (vapor#3038)
  Create CODEOWNERS
  Improve error reporting for `EncodingError` and `DecodingError` (vapor#2981)
  Fix incorrect use of non-localhost connection in test
  Update README with new Sponsor (vapor#3025)
  Add `ContentContainer.decode(_:as:)` (vapor#3023)
  Fixed drain handler call order in case of asynchronous buffer handling (vapor#3009)
  Update README with new Sponsor (vapor#3024)
  Update README with new Sponsor (vapor#3020)
  Don't use UnsafeRawBufferPointer.withMemoryRebound(to:_:) before Swift 5.7.2 (vapor#3021)
  Avoid deadlocking websocket tests (vapor#3019)
  Update README with new Sponsor (vapor#3014)
  Fix `Range: bytes=0-0` header not working properly (vapor#3010)
  Remove use of HTTPBin (vapor#3017)
  Improved error logging (vapor#3016)
  ...

# Conflicts:
#	Sources/Vapor/HTTP/Headers/HTTPHeaders+ContentRange.swift
#	Sources/Vapor/Utilities/FileIO.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-patch Internal changes only
Projects
Vapor 4
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants