Code-Sharp / WampSharp

A C# implementation of WAMP (The Web Application Messaging Protocol)
http://wampsharp.net
Other
385 stars 83 forks source link

Add inner exception in ConvertExceptionToRuntimeException #338

Open kartonka opened 2 years ago

kartonka commented 2 years ago

It would be important for us not to lose the exception information as it travels from our backend to our frontend. After analyzing the code our suggestion would be to adjust the ConvertExceptionToRuntimeException function to add the original exception as an inner exception to WampRpcCanceledException and to WampRpcRuntimeException.

darkl commented 2 years ago

This has been considered in the past and wasn't implemented as it is too vulnerable.

I am open to ideas how to implement this in a way that doesn't put the library users in risk.

Elad

On Fri, Sep 3, 2021, 08:41 Balázs Somos @.***> wrote:

It would be important for us not to lose the exception information as it travels from our backend to our frontend. After analyzing the code our suggestion would be to adjust the ConvertExceptionToRuntimeException function to add the original exception as an inner exception to WampRpcCanceledException and to WampRpcRuntimeException.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Code-Sharp/WampSharp/issues/338, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIS75ROLF23X3TZR5S622DUAC7A5ANCNFSM5DLVSDSA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

kartonka commented 2 years ago

I think this would remain backwards compatible:

        protected static WampException ConvertExceptionToRuntimeException(Exception exception)
        {
            if (exception is OperationCanceledException)
            {
                return new WampRpcCanceledException(null, exception.Message, null, exception.Message, exception);
            }

            return new WampRpcRuntimeException(null, exception.Message, null, exception.Message, exception);
        }
darkl commented 2 years ago

The issue is not backward compatibility, but having this behavior enabled by default. This might involuntary expose internal information of the code to an attacker, which might use it to attack the user's application.

I am open to ideas how to have this feature but also not have it enabled by default.

Elad

On Fri, Sep 3, 2021, 08:50 Balázs Somos @.***> wrote:

I's say this would remain backwards compatible:

    protected static WampException ConvertExceptionToRuntimeException(Exception exception)
    {
        if (exception is OperationCanceledException)
        {
            return new WampRpcCanceledException(null, exception.Message, null, exception.Message, exception);
        }

        return new WampRpcRuntimeException(null, exception.Message, null, exception.Message, exception);
    }

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Code-Sharp/WampSharp/issues/338#issuecomment-912514128, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIS75UJEH2SBY2GP7IXV6DUADABTANCNFSM5DLVSDSA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

kartonka commented 2 years ago

Hmm, good point, I see... As long as Wamp intends to provide security through such features, it is not possible to support applications that do not need such features (like ours). If Wamp did not want to provide this security but let the application decide to do it for themselves (e.g. through such a try-catch + throw), it could be changed. As this decision is not in my hands (maybe not even in yours), I guess we'll have to figure out a workaround but thanks for your time!:)

esso23 commented 2 years ago

Not sure what your use case is, but wouldn't it be enough to just have the exception logged on the Callee? (WampSharp supports that)

kartonka commented 2 years ago

We wanted to have the server layer between our frontend and backend the thinnest possible in our application and avoid implementing a function for every callee with an exception handling. Ideally, our backend functions would solely be called through attributed interfaces.

Example:

// Server
public interface IBackendClass
{
    [WampProcedure("com.myapp.method1")]
    public void Method1(); // throws many types of exceptions
}

public class BackendClassProxy: BackendClass, IBackendClass
{
    // empty!
}

// an instance of BackendClassProxy is registered via realm.Services.RegisterCallee

// Backend
public class BackendClass
{
    public void Method1()
    {
        if (endOfTheWorld())
        {
            throw new InvalidOperationException();
        }
        else
        {
            throw new MyVerySpecialExceptionType();
        }
        // ...
    }
}