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

Presentation: Fix Presentation RPC returning details contained within thrown errors #7560

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

grigasp
Copy link
Member

@grigasp grigasp commented Jan 16, 2025

For security reasons we don't want the backend's error response to contain any details about the error. The RPC implementation was converting all errors to a 200 response containing the error message. Now we only do that for PresentationError errors, which contain errors that we want to return (e.g. "invalid element id", or "unknown presentation rule", etc.). The generic Error errors are just re-thrown to the RPC system, where they can be handled in a generic way.

@grigasp grigasp requested a review from a team as a code owner January 16, 2025 09:42
@grigasp grigasp enabled auto-merge (squash) January 16, 2025 09:42
@paulius-valiunas
Copy link
Contributor

Now we only do that for PresentationError errors, which contain errors that we want to return (e.g. "invalid element id", or "unknown presentation rule", etc.)

I still don't think those should be 200. For example, invalid element id definitely sounds like 400 (bad request)

@grigasp
Copy link
Member Author

grigasp commented Jan 16, 2025

Now we only do that for PresentationError errors, which contain errors that we want to return (e.g. "invalid element id", or "unknown presentation rule", etc.)

I still don't think those should be 200. For example, invalid element id definitely sounds like 400 (bad request)

We'll have to rework all that with the no-rpc changes, which are scheduled for 5.0 timeframe, won't we? If that's the case, I see no point in fixing this in this PR just to throw it away afterwards.

@paulius-valiunas
Copy link
Contributor

Now we only do that for PresentationError errors, which contain errors that we want to return (e.g. "invalid element id", or "unknown presentation rule", etc.)

I still don't think those should be 200. For example, invalid element id definitely sounds like 400 (bad request)

We'll have to rework all that with the no-rpc changes, which are scheduled for 5.0 timeframe, won't we? If that's the case, I see no point in fixing this in this PR just to throw it await afterwards.

RPC will co-exist with no-rpc for a while. We're not targeting to cover the full API surface with 5.0, at least not with the current manpower.

@grigasp
Copy link
Member Author

grigasp commented Jan 16, 2025

Now we only do that for PresentationError errors, which contain errors that we want to return (e.g. "invalid element id", or "unknown presentation rule", etc.)

I still don't think those should be 200. For example, invalid element id definitely sounds like 400 (bad request)

We'll have to rework all that with the no-rpc changes, which are scheduled for 5.0 timeframe, won't we? If that's the case, I see no point in fixing this in this PR just to throw it await afterwards.

RPC will co-exist with no-rpc for a while. We're not targeting to cover the full API surface with 5.0, at least not with the current manpower.

Okay, scheduled further improvements for 5.0: #7562

@grigasp grigasp merged commit 1589257 into master Jan 16, 2025
16 checks passed
@grigasp grigasp deleted the presentation/fix-backend-returning-error-details branch January 16, 2025 11:34
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.

3 participants