Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[ASM] Stack trace leak vulnerability detection #5067
[ASM] Stack trace leak vulnerability detection #5067
Changes from 22 commits
5aba45c
16a25b7
b657997
0d3a32b
1eb1a9b
655c7a0
45a8308
d6a872f
a925eab
65c4a27
739663c
98749f9
211611a
3a83dc9
cc075c0
eb7881f
06ca5b3
d9d20a6
2359117
910f72a
2bfb51b
d72ca50
e53d220
14f1400
761f943
eafb06b
ede7ea7
4f9e3b9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a little strange to split this across two classes? As
OnMethodBegin
has a different signature in each case, I think they could go in the same class. Not that it really matters, can be left as is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I prefer to split like this, as it's clear which of the instrumentations are calling which method 🙂
What I'm not a fan of is the
Bis
suffix 😄 I have no idea what that means 😅 Personally I would call this oneDeveloperExceptionPageMiddlewareIntegration
and the earlier oneDeveloperExceptionPageMiddlewareIntegration_Pre_3_0_0
to make it explicit, but it's just a nit 🙂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that using another nomenclature would be more clear. Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't normally nest the duck types directly in the integrations 🤔 I think I like it 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether
UsingIntegrationPipeline
is enough here 🤔 InHttpContextHelper.AddHeaderTagsFromHttpResponse()
(and other places) we have to add an additionaltry-catch
, because we still run into issues accessing theHttpContext
. My gut feeling is that we don't need to do the following, because I don't think you're trying to access the response headers anyway (insideIastModule
for example).Just dropping it here for context, just in case you think it could be an issue 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we be checking that IAST is enabled too? 🤔 Or do we check that later on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's checked later in IASTModule. Still, this piece of code should not be reached if IAST is disabled because of the instrumentation category.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I had forgotten that we don't even rejit these if IAST isn't enabled 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we not have this already 😂