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

Exclude stacktrace from error response of Timelion backend #12973

Merged
merged 2 commits into from Aug 1, 2017

Conversation

Projects
None yet
5 participants
@thomasneirynck
Contributor

thomasneirynck commented Jul 19, 2017

Release Note: the Timelion backend no longer includes the stacktrace as part of the server response. This stacktrace is now logged to the server console.


Closes #12963.

Removing the stack from the error message prevents clients to see install path.

@thomasneirynck thomasneirynck requested a review from ppisljar Jul 19, 2017

@thomasneirynck thomasneirynck changed the title from Exclude stack from error response of Timelion backend to Exclude stacktrace from error response of Timelion backend Jul 19, 2017

@ppisljar

LGTM

@iam-xtreme

This comment has been minimized.

Show comment
Hide comment
@iam-xtreme

iam-xtreme Jul 20, 2017

@thomasneirynck thanks for raising the issue and fixing it. Where can i get the build with code-fix, so that I can verify this and close the issue?

iam-xtreme commented Jul 20, 2017

@thomasneirynck thanks for raising the issue and fixing it. Where can i get the build with code-fix, so that I can verify this and close the issue?

@thomasneirynck

This comment has been minimized.

Show comment
Hide comment
@thomasneirynck

thomasneirynck Jul 20, 2017

Contributor

@iam-xtreme I'm going to merge this and backport it to the latest 5 releases. You can then pick up the next patch (these usually go out every couple of weeks or so)

Contributor

thomasneirynck commented Jul 20, 2017

@iam-xtreme I'm going to merge this and backport it to the latest 5 releases. You can then pick up the next patch (these usually go out every couple of weeks or so)

@@ -4,7 +4,10 @@ import chainRunnerFn from '../handlers/chain_runner.js';
const timelionDefaults = require('../lib/get_namespaced_settings')();
function replyWithError(e, reply) {
reply({ title: e.toString(), message: e.toString(), stack: e.stack }).code(400);
reply({

This comment has been minimized.

@jbudz

jbudz Jul 20, 2017

Contributor

I think this should throw a boom.badImplementation instead (500) instead. If we want the stack trace still I think it makes sense to log it with the server logger.

The response still has some info, and it can be a little strange to understand:

{
    "title": "TypeError: Cannot read property 'to' of undefined",
    "message": "TypeError: Cannot read property 'to' of undefined"
}
@jbudz

jbudz Jul 20, 2017

Contributor

I think this should throw a boom.badImplementation instead (500) instead. If we want the stack trace still I think it makes sense to log it with the server logger.

The response still has some info, and it can be a little strange to understand:

{
    "title": "TypeError: Cannot read property 'to' of undefined",
    "message": "TypeError: Cannot read property 'to' of undefined"
}

@epixa epixa added v5.5.2 and removed v5.5.1 labels Jul 21, 2017

@iam-xtreme

This comment has been minimized.

Show comment
Hide comment
@iam-xtreme

iam-xtreme Jul 28, 2017

@thomasneirynck is this available in v5.5.1 or will it be released as part of v5.5.2?

iam-xtreme commented Jul 28, 2017

@thomasneirynck is this available in v5.5.1 or will it be released as part of v5.5.2?

@jbudz

This comment has been minimized.

Show comment
Hide comment
@jbudz

jbudz Jul 28, 2017

Contributor

@iam-xtreme It didn't make 5.5.1.

Contributor

jbudz commented Jul 28, 2017

@iam-xtreme It didn't make 5.5.1.

@thomasneirynck

This comment has been minimized.

Show comment
Hide comment
@thomasneirynck

thomasneirynck Jul 31, 2017

Contributor

jenkins, test this

Contributor

thomasneirynck commented Jul 31, 2017

jenkins, test this

1 similar comment
@thomasneirynck

This comment has been minimized.

Show comment
Hide comment
@thomasneirynck

thomasneirynck Jul 31, 2017

Contributor

jenkins, test this

Contributor

thomasneirynck commented Jul 31, 2017

jenkins, test this

@thomasneirynck

This comment has been minimized.

Show comment
Hide comment
@thomasneirynck

thomasneirynck Jul 31, 2017

Contributor

test failures probably requires merging in latest master

Contributor

thomasneirynck commented Jul 31, 2017

test failures probably requires merging in latest master

@thomasneirynck thomasneirynck merged commit 0353735 into elastic:master Aug 1, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details
@thomasneirynck

This comment has been minimized.

Show comment
Hide comment
@thomasneirynck

thomasneirynck Aug 1, 2017

Contributor

Backports:
6.x: 1e4d4c7
6.0: 556a75a
5.6: 723a8af
5.5: 59ad679

Contributor

thomasneirynck commented Aug 1, 2017

Backports:
6.x: 1e4d4c7
6.0: 556a75a
5.6: 723a8af
5.5: 59ad679

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment