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

webserver.cc: try to pass exception text with 500 errors #14670

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

Habbie
Copy link
Member

@Habbie Habbie commented Sep 13, 2024

Short description

fixes #14637 -if- the client sends Accept: application/json

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

fixes PowerDNS#14637 -if- the client sends `Accept: application/json`
@coveralls
Copy link

Pull Request Test Coverage Report for Build 10850324716

Details

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • 42 unchanged lines in 12 files lost coverage.
  • Overall coverage increased (+0.03%) to 64.7%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/webserver.cc 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
pdns/pollmplexer.cc 1 83.66%
pdns/recursordist/sortlist.cc 2 74.12%
pdns/recursordist/aggressive_nsec.cc 2 66.1%
modules/lmdbbackend/lmdbbackend.cc 2 73.54%
pdns/iputils.cc 3 55.45%
pdns/misc.hh 3 87.62%
pdns/tsigverifier.cc 3 77.22%
pdns/packethandler.cc 4 72.69%
pdns/recursordist/syncres.cc 4 79.62%
pdns/signingpipe.cc 5 86.86%
Totals Coverage Status
Change from base Build 10850027583: 0.03%
Covered Lines: 124861
Relevant Lines: 162277

💛 - Coveralls

@omoerbeek
Copy link
Member

A potential issue here is that the exception message might reveal info we do not want to share.

@Habbie
Copy link
Member Author

Habbie commented Sep 13, 2024

A potential issue here is that the exception message might reveal info we do not want to share.

to somebody who has an API token, right?

I'll give it a good ponder, it's a good question (yours, not mine :) )

@zeha
Copy link
Collaborator

zeha commented Sep 13, 2024

The thing I'm vaguely concerned about is clients starting to parse the error messages. But I hope its clear that they are not contractual.

@Habbie
Copy link
Member Author

Habbie commented Sep 13, 2024

The thing I'm vaguely concerned about is clients starting to parse the error messages. But I hope its clear that they are not contractual.

I actually intended to not have JSON, or perhaps Internal Server Error\n\n{json goes here} but it turns out we had the JSON so ready to go, so I took the easy way out. I agree we should decide that these texts are not contractual (while I bet some of the constraints, content checks etc. are treated this way now by clients)

@zeha
Copy link
Collaborator

zeha commented Sep 13, 2024

For content checks etc I'm less concerned; IOW I hope its easier to preserve the messages there.
For "internal server error" it's a different story.

@Habbie
Copy link
Member Author

Habbie commented Sep 16, 2024

A potential issue here is that the exception message might reveal info we do not want to share.

after pondering over the weekend, this does not worry me

@omoerbeek
Copy link
Member

A potential issue here is that the exception message might reveal info we do not want to share.

after pondering over the weekend, this does not worry me

Agreed, it's build into my system to be weary about these things, but in this case it's ok.

@Habbie Habbie merged commit 039b2c6 into PowerDNS:master Sep 25, 2024
79 checks passed
@Habbie Habbie deleted the webserver-ise-reason branch September 25, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

auth API: Include cause of Internal Server Error in response body
4 participants