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

Remove duplicated HTTP body size limit check #35513

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

snazy
Copy link
Contributor

@snazy snazy commented Aug 23, 2023

Also removes the duplicate addition of the body-size-limit enforcer.

@cescoffier
Copy link
Member

(copied from zulip)

That's the opposite. That would be the management one to remove, and only if it's mounted on a router having it already.
So, keep the one line 425 and remove the one line 553.

@snazy snazy force-pushed the repro-upload-unlimited branch from 02d6165 to 8b11209 Compare August 24, 2023 18:15
@snazy snazy changed the title Reproducer - unlimited upload route Test case demonstrating how to bypass HTTP body size limit Aug 24, 2023
@snazy snazy marked this pull request as ready for review August 24, 2023 18:17
@quarkus-bot

This comment has been minimized.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

I think it should work as expected now.
Does your case pass?

@snazy
Copy link
Contributor Author

snazy commented Aug 28, 2023

@cescoffier it does with the .order(-3)

@cescoffier
Copy link
Member

Yes, because your route must be the first.
I wonder which routes are at index -2 and -1...

@snazy
Copy link
Contributor Author

snazy commented Aug 28, 2023

-2 is the body-size-check.
Haven't seen any route for -1 though

@cescoffier
Copy link
Member

I think we should start having constants (and documentation) around these numbers.

@snazy
Copy link
Contributor Author

snazy commented Aug 28, 2023

I think we should start having constants (and documentation) around these numbers.

Makes sense. Want me to do that in this PR or better have a follow-up?

@cescoffier
Copy link
Member

Let's do it in another PR. That one is ready from my POV.

@cescoffier
Copy link
Member

Can you rebase? It should be good to go.

@cescoffier cescoffier changed the title Test case demonstrating how to bypass HTTP body size limit Remove duplicated HTTP body size limit check Sep 4, 2023
Also removes the duplicate addition of the body-size-limit enforcer.
@snazy snazy force-pushed the repro-upload-unlimited branch from 8b11209 to 5bee7fc Compare September 4, 2023 09:04
@snazy
Copy link
Contributor Author

snazy commented Sep 4, 2023

Can you rebase?

Sure, done.

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 4, 2023

Failing Jobs - Building 5bee7fc

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 20
Native Tests - Virtual Thread - Messaging Update Docker Client User Agent ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: integration-tests/smallrye-context-propagation 

📦 integration-tests/smallrye-context-propagation

io.quarkus.context.test.SimpleContextPropagationTest.testArcTCContextPropagationDisabled line 92 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <2> but was: <3>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)

@cescoffier cescoffier merged commit 4a62696 into quarkusio:main Sep 5, 2023
@cescoffier
Copy link
Member

Thanks!

@quarkus-bot quarkus-bot bot added this to the 3.4 - main milestone Sep 5, 2023
@snazy snazy deleted the repro-upload-unlimited branch September 5, 2023 09:38
snazy added a commit to snazy/quarkus that referenced this pull request Sep 13, 2023
As discussed in quarkusio#35513 it is beneficial to have the route order marks defined as constants.

This change introduces a class containing nearly all these constants, except the extension specific values.
snazy added a commit to snazy/quarkus that referenced this pull request Sep 18, 2023
As discussed in quarkusio#35513 it is beneficial to have the route order marks defined as constants.

This change introduces a class containing nearly all these constants, except the extension specific values.
snazy added a commit to snazy/quarkus that referenced this pull request Sep 18, 2023
As discussed in quarkusio#35513 it is beneficial to have the route order marks defined as constants.

This change introduces a class containing nearly all these constants, except the extension specific values.
snazy added a commit to snazy/quarkus that referenced this pull request Oct 2, 2023
As discussed in quarkusio#35513 it is beneficial to have the route order marks defined as constants.

This change introduces a class containing nearly all these constants, except the extension specific values.
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this pull request Feb 8, 2024
As discussed in quarkusio#35513 it is beneficial to have the route order marks defined as constants.

This change introduces a class containing nearly all these constants, except the extension specific values.
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.

2 participants