Skip to content

fix BZ69492, Conditional requests comply rfc9110#796

Closed
Chenjp wants to merge 3 commits intoapache:mainfrom
Chenjp:rfc9110_Section13_BZ69492
Closed

fix BZ69492, Conditional requests comply rfc9110#796
Chenjp wants to merge 3 commits intoapache:mainfrom
Chenjp:rfc9110_Section13_BZ69492

Conversation

@Chenjp
Copy link
Copy Markdown
Contributor

@Chenjp Chenjp commented Dec 10, 2024

  1. preconditions evaluation implementation comply spec, as rfc 9110 section 13.2.2. defined.
  2. fix BZ69492;

Comment thread java/org/apache/catalina/servlets/DefaultServlet.java Outdated
1) preconditions evaluation implementation comply  spec, as rfc 9110 section 13.2.2. defined.
2) fix BZ69492;
@Chenjp Chenjp force-pushed the rfc9110_Section13_BZ69492 branch from e3ac311 to 1127b87 Compare December 10, 2024 11:31
}

boolean conditionSatisfied;
int headerCount = Collections.list(request.getHeaders("If-Match")).size();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This check should be simplified with !headerValues.hasMoreElements()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is useful later:

        if (hasAsteriskValue && headerCount > 1) {

@rmaucher another replacement would be:

        List<String> headerValues = Collections.list(request.getHeaders("If-Match"));
        int headerCount = headerValues.size();
        if (headerCount == 0) {
            return true;
        }

        boolean conditionSatisfied = false;
        boolean hasAsteriskValue = false;// check existence of special header value '*'
        for (String headerValue:headerValues) {
            if(conditionSatisfied) {
                break;
            }

            if ("*".equals(headerValue)) {
                hasAsteriskValue = true;
                conditionSatisfied = true;
            } else {
                // RFC 7232 requires strong comparison for If-Match headers
                Boolean matched = EntityTag.compareEntityTag(new StringReader(headerValue), false, resourceETag);
                if (matched == null) {
                    if (debug > 10) {
                        log("DefaultServlet.checkIfMatch:  Invalid header value [" + headerValue + "]");
                    }
                    response.sendError(HttpServletResponse.SC_BAD_REQUEST);
                    return false;
                } else {
                    conditionSatisfied = matched.booleanValue();
                }
            }
        }
        if (hasAsteriskValue && headerCount > 1) {
            // Note that an If-Match header field with a list value containing "*" and other values (including other
            // instances of "*") is syntactically invalid (therefore not allowed to be generated) and furthermore is
            // unlikely to be interoperable.
            response.sendError(HttpServletResponse.SC_BAD_REQUEST);
            return false;
        }
        if (!conditionSatisfied) {
            response.sendError(HttpServletResponse.SC_PRECONDITION_FAILED);
            return false;
        }

String headerValue = request.getHeader("If-None-Match");
if (headerValue != null) {
String resourceETag = generateETag(resource);
int headerCount = Collections.list(request.getHeaders("If-None-Match")).size();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same.


// RFC9110 #13.3.2 defines preconditions evaluation order
int next = 1;
while (next < 6) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have the impression it works, but the whole thing looks weird.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it does follow rfc spec strictly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have the impression it works, but the whole thing looks weird.

Although it looks weird, it does follow rfc spec strictly and is translated directly from rfc9110 #13 - Precedence of Preconditions, which is very readable.

If adjust evaluation order without enough respect for the spec, bug might come knocking at door:

        // RFC 9110 Section 13.2.2 - If-Unmodified-Since should be evaluate before If-Modified-Since.
        testPreconditions(Task.HEAD_INDEX_HTML, null, IfPolicy.DATE_LT, null, IfPolicy.DATE_GT, null, 412);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, I see the origin of the weird code now. Thanks for the extra test, the old code was not accurate about the order.

@Chenjp
Copy link
Copy Markdown
Contributor Author

Chenjp commented Dec 11, 2024

@rmaucher commit 990f7e6 need aware of following invalid scenario:

        case ETAG_ALL:
            headerValues.add("\"1a2b3c4d\"");
            headerValues.add("*");
            break;

@rmaucher
Copy link
Copy Markdown
Contributor

I applied the PR with some modifications (and the edge case).

@rmaucher rmaucher closed this Dec 11, 2024
Chenjp referenced this pull request Dec 12, 2024
Expand tests to cover If-Modified-Since.
Expand tests for multiple values.
Fix bug by aligning code for If-Modified-Since with If-Unmodified-Since
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants