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

Possible fixes to address perf issue in 6.31 #2131

Merged
merged 6 commits into from
Jul 11, 2023
Merged

Possible fixes to address perf issue in 6.31 #2131

merged 6 commits into from
Jul 11, 2023

Conversation

jennyf19
Copy link
Collaborator

No description provided.

jennyf19 and others added 2 commits July 8, 2023 17:10
The bug:
- the constructor of JSonWebToken taking header and payload supposes that these are json, not encoded. They should not be assigned directly to the encoded members. This is likely to provoke plenty of exception.
- the potential exception: in ToString(), we don't verify that there is at least one dot. Again could provoke an exception on malformed tokens.
@jennyf19 jennyf19 requested review from jmprieur and henrik-me July 10, 2023 17:26
Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

Suggested one change for Henrik's optimization
(as _lastDot = EncodedToken.LastIndexOf('.'); will return -1
if there is no dot, so we would re-run it again next time.

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs Outdated Show resolved Hide resolved
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs Outdated Show resolved Hide resolved
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @jennyf19

@jennyf19 jennyf19 merged commit f7edc77 into dev Jul 11, 2023
@jennyf19 jennyf19 deleted the jennyf/perfcheck branch July 11, 2023 19:41
@jennyf19 jennyf19 added this to the 6.32.0 milestone Jul 11, 2023
{
if (arg == null)
return "null";

if (arg is ISafeLogSecurityArtifact && IdentityModelEventSource.LogCompleteSecurityArtifact)
if (IdentityModelEventSource.LogCompleteSecurityArtifact && arg is ISafeLogSecurityArtifact)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use in-line is operator:

        if (IdentityModelEventSource.LogCompleteSecurityArtifact && arg is ISafeLogSecurityArtifact safeLogSecurityArtifact)
            return safeLogSecurityArtifact.UnsafeToString();

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.

5 participants