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

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

Closed
brideck opened this issue Nov 30, 2022 · 14 comments
Closed

Comments

@brideck
Copy link

brideck commented Nov 30, 2022

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
Copy link
Contributor

spericas commented Dec 1, 2022

@jansupol @alwin-joseph WDYT?

@jansupol
Copy link
Contributor

jansupol commented Dec 1, 2022

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
Copy link
Contributor

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

@spericas
Copy link
Contributor

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

@alwin-joseph ?

@alwin-joseph
Copy link
Contributor

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 dd0a8d2. 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
Copy link
Contributor

@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
Copy link
Contributor

Created #1139 to exclude the test.

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

@brideck
Copy link
Author

brideck commented Dec 15, 2022

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
Copy link
Contributor

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
Copy link
Author

brideck commented Dec 15, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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 #1126 (comment) )

@jim-krueger
Copy link
Contributor

@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
Copy link
Contributor

This issue has been resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants