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

[Java] JWT without signature check. #5597

Merged
merged 15 commits into from Apr 29, 2021

Conversation

intrigus-lgtm
Copy link
Contributor

@intrigus-lgtm intrigus-lgtm commented Apr 3, 2021

This query detects cases where a signing key is for a JwtParser, but a parsing method is used that does not verify the signature using the signing key.

@github-actions github-actions bot added the Java label Apr 3, 2021
@intrigus-lgtm intrigus-lgtm changed the title [WIP] JWT without signature check. [Java] JWT without signature check. Apr 5, 2021
@intrigus-lgtm intrigus-lgtm marked this pull request as ready for review April 5, 2021 23:03
@intrigus-lgtm intrigus-lgtm requested a review from a team as a code owner April 5, 2021 23:03
Copy link
Contributor

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

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

That is a pretty neat query!
I have left some review comments regarding documentation and naming, but feel free to ignore those. The maintainers will probably give you more valuable feedback.

@JarLob
Copy link
Contributor

JarLob commented Apr 20, 2021

Hey @intrigus-lgtm,
It looks like the pattern is popular in tests. Could you please add a statement to exclude results from **/test/** path?

@intrigus-lgtm
Copy link
Contributor Author

Hey @intrigus-lgtm,
It looks like the pattern is popular in tests. Could you please add a statement to exclude results from **/test/** path?

I always thought that this should not be done in ql but in the "presentation layer"?
At least I have a faint memory of reading that somewhere.

@JarLob
Copy link
Contributor

JarLob commented Apr 20, 2021

I also had doubts and asked Java CodeQL team for advise and we decided to add the filtering for now, it can be removed later if the query is promoted from experimental.

@intrigus-lgtm
Copy link
Contributor Author

@JarLob done in 231b077.

@intrigus-lgtm
Copy link
Contributor Author

Hi @tamasvajk it looks like you have been assigned to review this.
Just a friendly ping in case this fell through the cracks.

I hope that the PR is in a state in which it needs few changes :)

@JarLob
Copy link
Contributor

JarLob commented Apr 21, 2021

@intrigus-lgtm The corresponding issue is in SecLab review stage, so I have asked for the changes. It will take time to rerun the LGTM query. Not sure about the PR.

Co-authored-by: Chris Smowton <smowton@github.com>
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

I note the stubs are not in fact stubs -- they largely have method bodies etc

@intrigus-lgtm
Copy link
Contributor Author

I note the stubs are not in fact stubs -- they largely have method bodies etc

You're right.
I will remove all method bodies.
Is there anything else I should remove?

@smowton
Copy link
Contributor

smowton commented Apr 27, 2021

Private fields, nontrivial field initialisers, any imports and classes that are no longer necessary after stubbing

@intrigus-lgtm
Copy link
Contributor Author

@smowton done in a8865e2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants