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

GraphQL to terminate the request even if it was active #26934

Merged
merged 1 commit into from
Jul 26, 2022
Merged

GraphQL to terminate the request even if it was active #26934

merged 1 commit into from
Jul 26, 2022

Conversation

phillip-kruger
Copy link
Member

Fix #26748
(Again)

Thanks @cescoffier for the help :)

Signed-off-by: Phillip Kruger phillip.kruger@gmail.com

@phillip-kruger
GraphQL to terminate the request even if it was active
08e5c31 
Signed-off-by: Phillip Kruger <phillip.kruger@gmail.com>
@gsmet
Copy link
Member

gsmet commented Jul 26, 2022 •
edited
Loading

Hmmm, too bad we missed it when reviewing. I'm wondering what I should do:

  1. release a 2.10.4.Final? yes/no
  2. skip 2.11.0.Final and release a 2.11.1.Final tomorrow with the fix? yes/no

If we do 2/, we could skip 1/ but we might want to release a 2.10.4.Final with just this fix to have a safe upgrade target for users of 2.10?

Other option is to just wait for 2.11.1.Final in a week or so but I don't think it's acceptable.

/cc @geoand

@geoand
Copy link
Contributor

geoand commented Jul 26, 2022

If we do 2/, we could skip 1/ but we might want to release a 2.10.4.Final with just this fix to have a safe upgrade target for users of 2.10?

I prefer this solution.

@gsmet
Copy link
Member

gsmet commented Jul 26, 2022

@geoand so release both 2.10.4.Final and 2.11.1.Final?

@geoand
Copy link
Contributor

geoand commented Jul 26, 2022

Yeah, I think given the severity of this, it's the best thing to do.

@gsmet
Copy link
Member

gsmet commented Jul 26, 2022

Agreed even if I'm not terribly excited about it :).

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 26, 2022

Failing Jobs - Building 08e5c31

Status Name Step Failures Logs Raw logs
✔️ Gradle Tests - JDK 11
✖ Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.CompositeBuildWithDependenciesDevModeTest.main line 24 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@gsmet gsmet merged commit 872fda2 into quarkusio:main Jul 26, 2022
@quarkus-bot quarkus-bot bot added this to the 2.12 - main milestone Jul 26, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 26, 2022

Milestone is already set for some of the items:

We haven't automatically updated the milestones for these items.

This message is automatically generated by a bot.

@gsmet gsmet modified the milestones: 2.12 - main, 2.11.1.Final Jul 26, 2022
@gsmet gsmet modified the milestones: 2.11.1.Final, 2.10.4.Final Jul 26, 2022
gsmet
gsmet reviewed Jul 26, 2022
View reviewed changes
...ntime/src/main/java/io/quarkus/smallrye/graphql/runtime/SmallRyeGraphQLExecutionHandler.java
Comment on lines -199 to +200
if (ctx.getBody() != null) {
return ctx.getBodyAsString();
if (ctx.body() != null) {
return ctx.body().asString();
Copy link
Member

Choose a reason for hiding this comment

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

Next time, let's avoid to shoe in this type of change in a security fix as this is not backportable to 2.10.

Copy link
Member

Choose a reason for hiding this comment

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

(I fixed it, no need for additional work on your side)

@phillip-kruger phillip-kruger mentioned this pull request Aug 8, 2022
Merged
@phillip-kruger phillip-kruger deleted the graphql-terminate-issue branch June 13, 2023 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet approved these changes

@cescoffier cescoffier cescoffier approved these changes

Assignees
No one assigned
Labels
area/graphql area/smallrye kind/bugfix
Projects
None yet
Milestone
2.10.4.Final
Development

Successfully merging this pull request may close these issues.

CVE-2022-2466 - Request Context not terminated with GraphQL
4 participants
@phillip-kruger @gsmet @geoand @cescoffier