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.

Sign up for GitHub

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

Jump to bottom

Add rule to mark Bearer token issues as security issues #32709

Merged
merged 1 commit into from
Apr 18, 2023
Merged

Add rule to mark Bearer token issues as security issues #32709

merged 1 commit into from
Apr 18, 2023

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Apr 18, 2023

Done because #32701 was left as needs-triage

@geoand geoand requested a review from sberyozkin April 18, 2023 07:51
@quarkus-bot quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Apr 18, 2023
gsmet
gsmet previously requested changes Apr 18, 2023
View reviewed changes
.github/quarkus-github-bot.yml Outdated
@@ -226,6 +226,10 @@ triage:
- extensions/security/
- extensions/elytron
- integration-tests/elytron
- id: bearer-token
labels: [area/security]
title: "Bearer token"
Copy link
Member

Choose a reason for hiding this comment

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

I did a quick search and I think we should go with the following. It won't catch too many false positives AFAICS.

Suggested change
title: "Bearer token"
title: "bearer"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure it's correct, so I'll leave it to @sberyozkin to break the tie

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, here's what I did: https://github.com/quarkusio/quarkus/issues?q=is%3Aissue+bearer+in%3Atitle+

But I don't mind either way, feel free to merge if you have a strong opinion about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you are right, it does look good indeed. Let's get @sberyozkin blessing first and then I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this, do you agree @sberyozkin ?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @geoand for the PR and @gsmet for the suggestion, yeah, bearer alone should be sufficient, we have bearer authorization, bearer authenticaton in a few places, token is not always featuring and is implied in those cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, PR should be ready then

sberyozkin
sberyozkin approved these changes Apr 18, 2023
View reviewed changes
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks Georgios

@sberyozkin sberyozkin merged commit 89ca6e7 into quarkusio:main Apr 18, 2023
@quarkus-bot quarkus-bot bot added this to the 3.1 - main milestone Apr 18, 2023
@geoand geoand deleted the bearer-token-notify branch April 18, 2023 09:50
@geoand
Copy link
Contributor Author

geoand commented Apr 18, 2023

💪🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet left review comments

@sberyozkin sberyozkin sberyozkin approved these changes

Assignees
No one assigned
Labels
area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure
Projects
None yet
Milestone
3.1.0.CR1
Development

Successfully merging this pull request may close these issues.
3 participants
@geoand @sberyozkin @gsmet