Skip to content

Only send panic text if PrintStack true#245

Merged
jszwedko merged 2 commits into
urfave:masterfrom
trevrosen:bug/244/panic-text-off-wire
Jan 17, 2019
Merged

Only send panic text if PrintStack true#245
jszwedko merged 2 commits into
urfave:masterfrom
trevrosen:bug/244/panic-text-off-wire

Conversation

@trevrosen

@trevrosen trevrosen commented Jan 16, 2019

Copy link
Copy Markdown
Contributor

Fixes #244

What

Only print panic text if PrintStack is true.

Why

The previous implementation would make sure that the stack wasn't sent unless PrintStack was set to true, but would nonetheless send whatever text was passed to panic(). This is bad for two reasons:

  1. it creates a security risk by leaking information about the server's internal functioning
  2. it creates unexpected behavior for the developer who expects that setting PrintStack to false would suppress all stack trace information including the panic text.

@jszwedko jszwedko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @trevrosen !

I do think this is closer to the original intention (6939988) of only optionally showing runtime information for debugging purposes and it makes sense to me to default to not showing anything.

Ideally, I think we'd call it something other than PrintStack in the future if we ever break compatibility.

I think we may still want have a very basic output in the case that they are not printing the stack though. Something like what you get from https://httpstat.us/500 (with a content type of text/plain and body of "Internal Server Error"). What do you think?

@trevrosen

Copy link
Copy Markdown
Contributor Author

Honestly I'm OK with the status code and empty body, but I don't have a real problem w/ dumping that string to the wire. Is that blocking merge here?

@jszwedko

Copy link
Copy Markdown
Contributor

👍 if you aren't opposed to it. I just view it as a sensible default compared with what other webservers do and would prefer not to switch the default recovery output more times than we can avoid (i.e. if we deferred this to later).

@trevrosen

Copy link
Copy Markdown
Contributor Author

Done. I carried forward the header check/set bit even though it's not strictly necessary just in the interest of following the old code path as much as possible modulo this text change.

@jszwedko jszwedko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, thank @trevrosen !

@jszwedko
jszwedko merged commit 2829cbe into urfave:master Jan 17, 2019
wusphinx pushed a commit to wusphinx/negroni that referenced this pull request Nov 19, 2021
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.

2 participants