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

UnhandledExceptionEvent.ToString() Throws NotSupportedException #1362

Closed
JustusGreiberORGADATA opened this issue Jun 16, 2023 · 6 comments · Fixed by #1363
Closed

UnhandledExceptionEvent.ToString() Throws NotSupportedException #1362

JustusGreiberORGADATA opened this issue Jun 16, 2023 · 6 comments · Fixed by #1363
Assignees
Labels
bug report Bug report from a user

Comments

@JustusGreiberORGADATA
Copy link

JustusGreiberORGADATA commented Jun 16, 2023

Which version of Duende IdentityServer are you using?
6.3.2
Which version of .NET are you using?
NET 6
Describe the bug

  1. System.Text.Json can't serialize exceptions. System.Text.Json - can't serialize exception dotnet/runtime#43026. It will refuse to serialize the contained IntPtr
  2. UnhandledExceptionEvent contains an exception https://github.com/DuendeSoftware/IdentityServer/blob/main/src/IdentityServer/Events/UnhandledExceptionEvent.cs#L44
  3. Event.ToString() calls LogSerializer.Serialize(this) which internally uses System.Text.Json https://github.com/DuendeSoftware/IdentityServer/blob/main/src/IdentityServer/Events/Infrastructure/Event.cs#L145

A clear and concise description of what the bug is.

To Reproduce

new UnhandledExceptionEvent(new Exception()).ToString();

should do it, although I haven't exactly tried, because in my case it happens during the OIDC flow (likely because I haven't migrated the database yet).

@josephdecock
Copy link
Member

Hi, thanks for this bug report. So far I've been unable to reproduce the problem. I would expect this unit test to show the error, but it passes:

    [Fact]
    public void UnhandledExceptionEventCanCallToString()
    {
        var e = new UnhandledExceptionEvent(new Exception("Boom?"));

        var s = e.ToString();

        s.Should().NotBeNullOrEmpty();
    }

Does that test pass for you?

@josephdecock josephdecock added the question Further information is requested label Jun 26, 2023
@JustusGreiberORGADATA
Copy link
Author

JustusGreiberORGADATA commented Jun 27, 2023

Hi,

my bad, the non serializable data is only added to the exception, if it is thrown. Try this:

 [TestMethod]
 public void Test2()
 {
     try
     {
         throw new Exception();
     }
     catch (Exception ex)
     {
         var exception = new Duende.IdentityServer.Events.UnhandledExceptionEvent(ex);

         var myString = exception.ToString();
     }
 }

It should throw the exception

Test method Ofcas.Odid.IdentityProvider.UITests.ErrorPageTest.Test2 threw exception: 
    System.NotSupportedException: Serialization and deserialization of 'System.Reflection.MethodBase' instances are not supported. Path: $.Exception.TargetSite. ---> System.NotSupportedException: Serialization and deserialization of 'System.Reflection.MethodBase' instances are not supported.

@josephdecock josephdecock self-assigned this Jul 6, 2023
@josephdecock josephdecock added bug report Bug report from a user and removed question Further information is requested labels Jul 7, 2023
@josephdecock josephdecock transferred this issue from DuendeSoftware/Support Jul 7, 2023
@josephdecock
Copy link
Member

Thanks @JustusGreiberORGADATA, I'm reproducing the error now.

@josephdecock
Copy link
Member

@JustusGreiberORGADATA are you requesting that a fix for this issue be patched into the 6.3.x branch, or can you wait for v7?

@JustusGreiberORGADATA
Copy link
Author

@josephdecock I know this is an issue now, so I don't care if it's only in v7. But just FYI: For other people upgrading to 6.3 it might be confusing, because the exception triggers if you are forgot to create the migrations.

@josephdecock
Copy link
Member

@JustusGreiberORGADATA , a fix for this is now merged and will be part of the next major release.

josephdecock added a commit that referenced this issue Sep 20, 2023
This fixes #1362

Thrown exceptions can't be serialized by System.Text.Json, because they
contain pointers that it won't allow.

This fix (suppressing serialization) is specific to the 6.3 release.
In 7.0, we will remove the property entirely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Bug report from a user
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants