jakartaee / servlet

Jakarta Servlet
https://eclipse.org/ee4j/servlet
Other
268 stars 87 forks source link

TCK: setMaxAgeZeroTest() expects different Max-Age between EE10 and EE11 #701

Closed pmd1nh closed 3 weeks ago

pmd1nh commented 2 months ago

Challenged Test: URLClient.setMaxAgeZeroTest()

TCK Version: Servlet TCK 6.0

Tested Implementation: Open Liberty

Description: Version 0 Set-Cookie header expects different values in TCK Servlet 6.0 and TCK Servlet 6.1

TCK Servlet 6.0 expects the Max-Age=0 presents in the Set-Cookie response header.

TCK Servlet 6.1 does not expect the Max-Age=0 in the Set-Cookie

The change in TCK Servlet 6.1 was because of 6.0 challenge which was addressed by #583. However, PR 583 was not pulled into TCK Servlet 6.0 but TCK Servlet 6.1.

Due to these TCK differences, implemented server also has two different behaviors.

Can TCK Servlet 6.0 allow either behavior to avoid inconsistent behaviors in server b/w 6.0 and 6.1?

pmd1nh commented 2 months ago

@markt-asf

markt-asf commented 2 months ago

493 was effectively accepted so I believe you can skip the affected test. That should allow 6.0 and 6.1 implementations to be consistent.

The 6.0 TCK is not under the (direct) control of the Servlet project. I suggest submitting a PR to the 10.0.x branch of the Platform TCK to exclude that test in https://github.com/jakartaee/platform-tck/blob/10.0.x/install/servlet/bin/ts.jtx

The platform TCK team are busy at the moment migrating to the new TCK structure. I don't know how long an updated Servlet 6.0 TCK release that includes the additional exclusion might take.

pnicolucci commented 2 months ago

@markt-asf any objection to marking this issue as TCK:accepted? Then we can work on a PR for the TCK.

markt-asf commented 2 months ago

I'd argue it is a duplicate of #493 - I'd rather any addition to the 6.0 exclusion list referenced the original issue.

pmd1nh commented 1 month ago

Hi @markt-asf ,

We'd like to challenge this TCK test in EE11.

The test should allow either Max-Age or Expires attribute in the Set-Cookie header.

  1. From the Servlet API Cookie, there is no [g|s]etExpires(). There is also no serlvet document states that the Expires attribute should replace the Max-Age attribute
  2. The Servlet API Cookie setMaxAge() does not state about replace/add Expires attribute when max-age is set to 0. (These contexts have been mentioned and acknowledged in the comments )

Servlet Container follows the Servlet Specification. If the Servlet Specification is changed to follow/reference a new RFC, those update/change behaviors need to be documented in the Servlet spec/API (for transparency and to avoid any misinterpretation).

(from https://www.rfc-editor.org/rfc/rfc6265#section-4.1.2.2

If a cookie has both the Max-Age and the Expires attribute, the Max-
   Age attribute has precedence and controls the expiration date of the
   cookie.

)

markt-asf commented 1 month ago

The Servlet 6.1 spec has required RFC 6265 compliance since Servlet 5.0.

RFC 6265 explicitly states (section 4.1.1)

 max-age-av        = "Max-Age=" non-zero-digit *DIGIT

It is neither reasonable nor practical for the Servlet specification to repeat the requirements of referenced specifications.

I agree the test can be excluded for the 6.0 TCK (under issue #493) but changes to the 6.0 TCK are outside the control of the Servlet project.

covener commented 1 month ago

FWIW, 6265 has unaccepted errata reported that points out that the ABNF contradicts the description of the semantics

https://www.rfc-editor.org/rfc/rfc6265#section-5.2.2

If delta-seconds is less than or equal to zero (0), let expiry-time be the earliest representable date and time. Otherwise, let the expiry-time be the current date and time plus delta-seconds seconds.

It seems that behavior is consistent with 5.2.2 rather than the ABNF.

markt-asf commented 1 month ago

@covener Those are the requirements for user agents (section 5) which are generally more relaxed than the requirements for servers (section 4).

scottmarlow commented 1 month ago

For reference, the challenge test link is https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/servlet/api/jakarta_servlet_http/cookie/URLClient.java#L360

scottmarlow commented 1 month ago

Note that the Specification pull request will reference https://github.com/jakartaee/servlet/issues/701+ https://github.com/jakartaee/servlet/issues/493 so that both issues are clearly listed. Any objections?

scottmarlow commented 1 month ago

pmd1nh asked:

@markt-asf any objection to marking this issue as TCK:accepted? Then we can work on a PR for the TCK.

markt-asf responded:

I'd argue it is a duplicate of #493 - I'd rather any addition to the 6.0 exclusion list referenced the original issue.

Should this TCK challenge issue be accepted or marked invalid?

If marked invalid, I think https://jakarta.ee/specifications/servlet/6.0/ will need to list TCK challenge #493 as the accepted TCK challenge that we excluded the setMaxAgeZeroTest for. If this TCK Challenge issue is marked accepted, issue#701 should be linked as the TCK challenge that we excluded the setMaxAgeZeroTest for.

No need to read but for more details:

The https://jakarta.ee/committees/specification/tckprocess/ does not describe how to resolve TCK challenges that are considered duplicate. The two described outcomes for TCK Challenges is either accepted or invalid.

scottmarlow commented 4 weeks ago

I'd argue it is a duplicate of https://github.com/jakartaee/servlet/issues/493 - I'd rather any addition to the 6.0 exclusion list referenced the original issue.

I suppose that we could update the Servlet Specification page to reference 493 and also the exclude lists which delays the persistence challenge that has been waiting since June ...

scottmarlow commented 3 weeks ago

I'd argue it is a duplicate of #493 - I'd rather any addition to the 6.0 exclusion list referenced the original issue.

I suppose that we could update the Servlet Specification page to reference 493 and also the exclude lists which delays the persistence challenge that has been waiting since June ...

@pmd1nh this issue seems stuck, I propose that we either take action to follow advice in https://github.com/jakartaee/servlet/issues/701#issuecomment-2315622529 (e.g. treat this TCK challenge as invalid) and instead use issue#493) or we remove this issue from the current round of changes to include in the EE 10 Platform TCK.

My preference is to reference issue#493 as per wishes of the Servlet Spec team as communicated by Servlet lead @markt-asf.

pmd1nh commented 3 weeks ago

@scottmarlow I think we have enough information log here to close this out as a dup of #493. Brian and you already merged the PR to exclude the test in EE10 platform TCK.

I close this out.