Fix: Prevent query parameter parsing from consuming request body in S…

[haresahani]

This pull request addresses an issue where the use of @QueryParameter in Stapler was causing the request body to be prematurely consumed in some servlet containers (see #416 ).

The fix involves modifying QueryParameter.java to manually parse the query string instead of using request.getParameter(). This change ensures that query parameters are obtained without triggering the consumption of the request body, which is especially important for POST requests that need to retain their body content.

Automated Tests: Ran the existing test suite, which verifies the functionality of @QueryParameter.
Manual Tests:
Tested GET and POST requests with query parameters to confirm they are parsed correctly.
Verified that POST requests with a body retain their content and are unaffected by query parameter processing.
Checked that requests without required query parameters trigger the appropriate exceptions.

Submitter Checklist
-Created the PR from a topic branch, not the main branch.
-Provided a descriptive title for the pull request.
-Linked to the relevant issue: #416 .
-Added new tests to cover the change, ensuring it works as expected.

[timja]

I don't think this is right, QueryParameter is used to get body parameters as well.

Since we moved from GET to POST for validation and are moving autocompletion away as well this would break it

[timja]

I see that @jtnord said it was a violation of the contract but Jenkins depends on this behaviour so this change can't be made without a lot of work I would say

[jtnord]

so this change can't be made without a lot of work I would say

Should be limited to to a couple of javascript libraries that handle this?

if we are never going to change the behaviour the least we can do is to fix the declared contract to march reality and tell consumers to not use this when they also want to process the request body.

I have zero recollection of what was the trigger for me to find and file #416

[timja]

Should be limited to to a couple of javascript libraries that handle this?

No its plugins and core, they use @QueryParameter in their doValidate / doCheck methods.

We send them as post parameters..

[timja]

As of https://www.jenkins.io/changelog/2.285/ (2021)

[jtnord]

Plugins (generally) do not construct the validation, it's part of form binding taglibs from core?

[timja]

On the server side they ask for parameters in their methods

[jtnord]

Sure, my point was if the taglibs sent a HTTP POST with query parameters (application/x-www-form-urlencoded) (in addition or replacement of the multipart/form-data rather then this could be fixed.

Now, having thought about this some more, some of the query parameters could be new secrets which would then be exposed by HTTP access logs which would be bad, so following that suggestion would not be a "good thing".™️

[haresahani]

Thank you for the detailed feedback and insights! I understand now that the behavior of @QueryParameter—accessing both query and body parameters—is a critical part of Jenkins core and plugin interactions. Based on your comments, I’d like to propose the following steps to address the concerns:

1. Preserving Current Behavior:
   Since Jenkins relies on this behavior, I agree that it cannot be altered without significant work and potential breakages. Instead, we can update the documentation or contract for @QueryParameter to reflect this dual behavior clearly. This would inform consumers of the API about the implications when using @QueryParameter with POST requests.

2. Alternative Approach for @QueryParameter trashes the request body #416:
   If the root issue of @QueryParameter trashes the request body #416 (request body consumption) remains valid, we could explore alternate solutions, such as:
   Introducing a new annotation (e.g., @QueryOnlyParameter) specifically designed to fetch query parameters without consuming the body.
   Providing guidance for consumers on handling scenarios where both query and body parameters are needed.

3. Validation and Secrets Handling:
   I acknowledge the potential security concerns raised by @jtnord regarding exposing secrets in query parameters through HTTP logs. This is a valid concern, and any proposed changes should carefully account for such risks.

4. Path Forward:
   Given the dependency on the existing behavior, would you agree to:
   Update the documentation for @QueryParameter to reflect its dual behavior?
   Open a new discussion or issue to explore safer and more robust alternatives to fetch query parameters without consuming the request body?

Please let me know your thoughts, and I’d be happy to adjust the PR or contribute further in line with your guidance. Thank you again for taking the time to review and discuss this in detail.

[timja]

From my point of view, option 4 updating the docs makes sense, 2 would be possible if there is a need for it, but I'm not sure there is a need.