jakartaee / rest

Jakarta RESTful Web Services
Other
361 stars 117 forks source link

TCK Challenge: Rest TCK should not use "suspicious sequence" in URI testing #1138

Closed brideck closed 7 months ago

brideck commented 1 year ago

Challenged Tests: ee.jakarta.tck.ws.rs.ee.rs.core.uriinfo.JAXRSClientIT#getNormalizedUriTest

TCK Version: Jakarta RESTful Web Services 3.1.x

Tested Implementation: Open Liberty -- containing RestEasy 6.1.0 & Weld 5.0.1

Description: Subsection 10 of section 3.5.2 of the Servlet 6.0 specification reads in part "If suspicious sequences are discovered during the prior processing steps, the request must be rejected with a 400 bad request rather than dispatched to the target servlet." Included in the default list of suspicious sequences is "Any "." or ".." segment with any encoded characters (e.g. /path/%2e%2e/info)."

The challenged test uses just such a string of encoded characters -- %50%51%52%30%39%70%71%72/%7e%2d%2E%5f. Any Rest implementation that is utilizing Servlet (as Open Liberty currently does) should reject this with a 400 response, which results in the test failing as written. The test should be updated to remove "%2E" and "." from the respective test strings so as to not run afoul of this Servlet requirement.

spericas commented 1 year ago

@jansupol @alwin-joseph WDYT?

jansupol commented 1 year ago

The Section 3.5 is new in Servlet 6, the requirements were not in 5.0. I believe we need to exclude the test for EE 10 and fix (remove the percent-encoded dots) for EE11.

arjantijms commented 1 year ago

If anyone does a PR to exclude the test, I can merge it (after the required time has passed).

spericas commented 1 year ago

If anyone does a PR to exclude the test, I can merge it (after the required time has passed).

@alwin-joseph ?

alwin-joseph commented 1 year ago

If anyone does a PR to exclude the test, I can merge it (after the required time has passed).

@alwin-joseph ?

Sure.

I see the master branch is currently used for 4.0 changes. Is there a branch that I can raise pull request for tck maintenance release (3.1.3)?

TCK 3.1.1 : Tagged using https://github.com/jakartaee/rest/releases/tag/tck-3.1.1 TCK 3.1.2 : The latest commit was https://github.com/jakartaee/rest/commit/dd0a8d2d0c98ff39f3f0a8b8bbdbd772135312d2. Can be tagged too as tck-3.1.2. So I think a new branch with this commit should exist to make this changes. which will be eventually merged to 4.0(master).

Please correct me if otherwise.

spericas commented 1 year ago

@alwin-joseph Create tag tck-3.1.2 as you suggested. You should be able to create a branch using this tag and make the necessary changes.

alwin-joseph commented 1 year ago

Created https://github.com/jakartaee/rest/pull/1139 to exclude the test.

@brideck Is the next maintenance release of the TCK(3.1.3) expected soon ?

brideck commented 1 year ago

Open Liberty would like the maintenance release fairly soon in order to complete our certification effort. I don't think we're expecting to need anymore updates to the TCK for that, although I would have said the same thing after the 3.1.2 release was created.

arjantijms commented 1 year ago

Open Liberty would like the maintenance release fairly soon

Each PR that didn't use the magic keyword has to wait for at least two full weeks.

brideck commented 1 year ago

Each PR that didn't use the magic keyword has to wait for at least two full weeks.

I don't know what the magic keyword you speak of is, but that's fine with us.

arjantijms commented 1 year ago

I don't know what the magic keyword you speak of is, but that's fine with us.

It's "fasttrack".

Every PR has to start with that. I've already suggested once to put that into the PR template, as people either forget it or don't know about it. But if a minimum of two weeks is no problem we can leave the PR as-is.

alwin-joseph commented 1 year ago

New service release of Jakarta REST TCK 3.1.3 is generated at https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/jakarta-restful-ws-tck-3.1.3.zip .

@brideck Can you please run the tests using this bundle to verify.

Post verification, will promote the TCK and add the TCK link to https://github.com/jakartaee/specifications/tree/master/restful-ws/3.1/_index.md as it was done in previous release (referring https://github.com/jakartaee/rest/issues/1126#issuecomment-1252435134 )

jim-krueger commented 1 year ago

@alwin-joseph Brian is out this week so I ran with the 3.1.3 zip you placed in staging and all of the Jakarta Rest TCK test now pass. Please promote this TCK the official site. Thanks

jim-krueger commented 7 months ago

This issue has been resolved.