Preserve exception messages for wrapped Django exceptions

[vanschelven]

I did not review your contributing guidelines, nor did I test anything using the automated tests. I also did only the most minimal of non-automated testing. Still, I thought I'd leave this here, as it may be useful to others. See it as an "issue description with PoC of a solution" if you will.

The problem: when raising standard Django exceptions Http404 or PermissionDenied, the error messages of these exceptions do not show up in the DRF's response. This is because these exceptions are recreated in the piece of code that this PR adapts.

In this PR this is solved by making sure the recreated exceptions are recreated with the same arguments as the original versions. Whether the signatures actually match up, I did not check properly.

[tomchristie]

It looks like there's a test case that'd need updating...

_ TestFilterBackendAppliedToViews.test_get_instance_view_filters_out_name_with_filter_backend _
tests/test_generics.py:522: in test_get_instance_view_filters_out_name_with_filter_backend
    assert response.data == {'detail': 'Not found.'}
E   AssertionError: assert {'detail': Er...='not_found')} == {'detail': 'Not found.'}
E     Differing items:
E     {'detail': ErrorDetail(string='No BasicModel matches the given query.', code='not_found')} != {'detail': 'Not found.'}
E     Use -v to get the full diff

[vanschelven]

It looks like there's a test case that'd need updating...

Indeed, but what would the desired behavior be? If I understand it correctly that test shows that the response data of a request contains some Python objects (as opposed to pure json)... perhaps because of mocking at some level?

[jeking3]

I just ran into this issue as well and had to add similar code to my own exception handler that runs and then calls the drf_exception_handler. However it is missing a unit test.

Without this code the resulting detail is "Not Found." but with this code it is whatever got put into the Http404 (usually from get_object_or_404, such as "No matches the given query." - therefore testing to make sure the resulting message for the exception is not "Not Found." would be an acceptable unit test.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[vanschelven]

Reading the discussion above it appears to me that the staleness is caused by a lack of response from @tomchristie on my question on how to proceed.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[vanschelven]

I've updated the failing test to be succeeding; this at least amounts to having an implicit test on the newly introduced behavior as more or less suggested by @jeking3

[vanschelven]

If I understand it correctly that test shows that the response data of a request contains some Python objects (as opposed to pure json)... perhaps because of mocking at some level?

As I understand now: no. ErrorDetail are picked up by another part of the DRF machinery before an actual HTTP response is sent over the wire. So having a test match on ErrorDetail is fine.

[vanschelven]

@tomchristie anything I could do to move this forward?

[vanschelven]

Stalebot go away

…

On Mon, Apr 18, 2022, 13:57 stale[bot] ***@***.***> wrote: This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. — Reply to this email directly, view it on GitHub <#8051 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABWUWOQAB7JV37M4CDRPBLVFVL3BANCNFSM47FHYKKQ> . You are receiving this because you authored the thread.[image: Web Bug from https://github.com/notifications/beacon/AABWUWKBJDCZGACBCAL63WDVFVL3BA5CNFSM47FHYKK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOIGS5MAY.gif]Message ID: ***@***.***> [ { ***@***.***": "http://schema.org", ***@***.***": "EmailMessage", "potentialAction": { ***@***.***": "ViewAction", "target": " #8051 (comment)", "url": " #8051 (comment)", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { ***@***.***": "Organization", "name": "GitHub", "url": " https://github.com" } } ]

[tomchristie]

Okay, yup this seems reasonable. 👍

[ferndot]

I am very concerned about the security implications of this change. It publicly exposes information, such as internal model names, which could potentially assist a malicious user. I see that it was intended to be helpful, but the Http404 and PermissionDenied exceptions both seem like places where a more generic error message is appropriate.

[liammonahan]

I have to agree with @ferndot about this change. I think it should be reverted since it makes it more difficult to obscure the underlying models involved in an endpoint.

The fact that the original author of this proposed change started off their PR with "I did not review your contributing guidelines" should have been a tip-off that they weren't going to be thinking very hard about the broader effects of this change on other users of DRF.

[ulgens]

+1 for @ferndot and @liammonahan

I can't think of a valid use case that I need error details from model layer, in a 404 response coming from an API handler.

[auvipy]

can any of you send a PR with improvement suggested here? specially the security concerns raised here

[vanschelven]

I can't think of a valid use case that I need error details [..]

(Ellipsis to make my point in an evil way). The fact that this information is originally put on the later-wrapped exceptions in the first place is strong evidence that there are some contexts in which this information is useful.

To give an actual example: In my original use-case, the API was internal, so exposure to the API endpoint was perfectly fine and helped in debugging.

I haven't the time to look into this right now (I'm not using the DRF on any of my projects currently)... but wouldn't it be possible to come up with a solution that meets both the original concern and the later-raised concerns about information exposure? E.g. by keeping the solution from the present PR, while later hiding this information (before it's exposed over the wire) by default (with options for customization).

IIRC this is what Django does too: exceptions with rich information (as evidenced by the fact that this information is present in the wrapped exceptions) and a good set of customizable handlers with no-exposure by default. I also recall DRF has something along these lines...

[ulgens]

(I destroyed my previous comment, no need to start any pointless discussion, please ignore it if you managed to catch it.)

@auvipy I can try to handle this tomorrow.

[auvipy]

(I destroyed my previous comment, no need to start any pointless discussion, please ignore it if you managed to catch it.)

@auvipy I can try to handle this tomorrow.

ping me when you have something to review

[llybin]

REST_FRAMEWORK = {
    "EXCEPTION_HANDLER": "drf.exceptions.custom_exception_handler",
}

if someone needs to keep the previous behavior, just create custom_exception_handler