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

Remove stack traces from web API calls. #5070

Merged
merged 13 commits into from
Feb 4, 2021

Conversation

lenko-d
Copy link

@lenko-d lenko-d commented Dec 7, 2020

This PR unwraps errors in order to remove stack traces from web API calls.

Testing done

Verification of the APIs

Verified that the following APIs don't return the stacktraces:

curl -k -H "Content-Type: application/json" -d '{"username": "example", "password":"invalid"}' https://192.168.1.102:32009/proxy/v1/webapi/sessions 
{
    "error": {
        "message": "access denied"
    }
}
curl -k -H "Content-Type: application/json" -d '{"username": "example", "password":"invalid"}' https://192.168.1.102:32009/proxy/v1/webapi/sessions/renew
{
    "error": {
        "message": "missing session cookie"
    }
}
curl -k -H "Content-Type: application/json" -d '{"username": "example", "password":"invalid"}' https://192.168.1.102:32009/proxy/v1/webapi/github/logi/console
{
    "error": {
        "message": "missing RedirectURL"
    }
}
curl -k -H "Content-Type: application/json" -d '{"username": "example", "password":"invalid"}' https://192.168.1.102:32009/proxy/v1/webapi/u2f/sessions
{
    "error": {
        "message": "bad auth credentials"
    }
}
curl -k -H "Content-Type: application/json" -d '{"username": "example", "password":"invalid"}' https://192.168.1.102:32009/portalapi/v1/sites/sites/domin/users/username/reset
{
    "error": {
        "message": "method not found"
    }
}

Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

In all places where you do unwrap now, I would consider logging the full error with the stack trace so this information is available for debugging in the logs.

lib/httplib/httplib.go Show resolved Hide resolved
@lenko-d
Copy link
Author

lenko-d commented Dec 8, 2020

@r0mant, I think that I'm not going to use the Unwrap approach at all. The problem is that Unwrap changes the structure of the response considerably. Before Unwrap:

{
    "error": {
        "message": "method not found"
    },
    "traces": [.......]
}

After Unwrap:

{
    "message": "method not found"
}

The change above will most likely require changes for the clients of the API. This doesn't sound good to me.

@r0mant
Copy link
Collaborator

r0mant commented Dec 8, 2020

@lenko-d Yeah, we definitely shouldn't change the response structure, you can just return the same structure but without traces included.

…ces disabled in web API responses due to security requirements.
@lenko-d lenko-d requested a review from r0mant December 8, 2020 23:19
lib/httplib/httplib.go Outdated Show resolved Hide resolved
integration/helpers.go Outdated Show resolved Hide resolved
integration/helpers.go Outdated Show resolved Hide resolved
lib/auth/middleware.go Outdated Show resolved Hide resolved
Copy link
Contributor

@knisbet knisbet left a comment

Choose a reason for hiding this comment

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

Looks fine to me once Dima's nitpick is addressed.

…it should not matter whether the strack traces are sent over the wire.
…it should not matter whether the strack traces are sent over the wire.
lib/httplib/httplib.go Outdated Show resolved Hide resolved
lib/httplib/httplib.go Outdated Show resolved Hide resolved
lib/httplib/httplib.go Outdated Show resolved Hide resolved
Copy link
Contributor

@a-palchikov a-palchikov left a comment

Choose a reason for hiding this comment

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

lgtm

@zmb3 zmb3 deleted the lenko/3.2-g/2269-no-stacktraces-in-web-api branch September 9, 2022 18:36
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.

4 participants