Sling
  1. Sling
  2. SLING-2443

Missing WWW-Authenticate header on OPTIONS request with trunk servlets.resolver bundle

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: JCR Webdav 2.1.2
    • Component/s: Servlets
    • Labels:
      None

      Description

      Running the launchpad/builder standalone jar from the trunk correctly returns an WWW-Authenticate header on an OPTIONS request with no credentials:

      $ curl -D - -X OPTIONS http://localhost:8080/
      HTTP/1.1 401 Unauthorized
      WWW-Authenticate: Basic realm="Jackrabbit Webdav Server"
      Content-Type: ...

      But if I replace the org.apache.sling.servlets.resolver 2.1.0 bundle that's in there with the latest snapshot (revision 1302994) that header is missing: it gets removed by the response.reset() call in DefaultErrorHandlerServlet.sendIntro(...), which makes it impossible to connect with WebDAV.

      That response.reset() call was not present in 2.1.0.

        Activity

        Hide
        Felix Meschberger added a comment - - edited

        Hmm, somehow it sounds wrong that 401 response to ask for authentication go through error handling scripts ...

        Rather 401 request for HTTP Basic authentication should do:

        setStatus
        setHeader(WWW-Authenticate)
        flush

        Show
        Felix Meschberger added a comment - - edited Hmm, somehow it sounds wrong that 401 response to ask for authentication go through error handling scripts ... Rather 401 request for HTTP Basic authentication should do: setStatus setHeader(WWW-Authenticate) flush
        Hide
        Bertrand Delacretaz added a comment -

        IIUC it's jackrabbit's AbstractWebdavServlet. sendUnauthorized(...) method that sends this:

        response.setHeader("WWW-Authenticate", getAuthenticateHeaderValue());
        if (error == null || error.getErrorCode() != HttpServletResponse.SC_UNAUTHORIZED)

        { response.sendError(HttpServletResponse.SC_UNAUTHORIZED); }

        else

        { response.sendError(error.getErrorCode(), error.getStatusPhrase()); }

        and that looks a bit tricky to override (just had a quick look)

        [1] http://svn.apache.org/repos/asf/jackrabbit/trunk/jackrabbit-webdav/src/main/java/org/apache/jackrabbit/webdav/server/AbstractWebdavServlet.java

        Show
        Bertrand Delacretaz added a comment - IIUC it's jackrabbit's AbstractWebdavServlet. sendUnauthorized(...) method that sends this: response.setHeader("WWW-Authenticate", getAuthenticateHeaderValue()); if (error == null || error.getErrorCode() != HttpServletResponse.SC_UNAUTHORIZED) { response.sendError(HttpServletResponse.SC_UNAUTHORIZED); } else { response.sendError(error.getErrorCode(), error.getStatusPhrase()); } and that looks a bit tricky to override (just had a quick look) [1] http://svn.apache.org/repos/asf/jackrabbit/trunk/jackrabbit-webdav/src/main/java/org/apache/jackrabbit/webdav/server/AbstractWebdavServlet.java
        Hide
        Felix Meschberger added a comment -

        In this case, this would rather be a bug for Jackrabbit.

        I am against removing the response.reset call from the DefaultErrorHandlerServlet because this sounds absolutely reasonable to do when handling errors in a generic (default) way.

        Show
        Felix Meschberger added a comment - In this case, this would rather be a bug for Jackrabbit. I am against removing the response.reset call from the DefaultErrorHandlerServlet because this sounds absolutely reasonable to do when handling errors in a generic (default) way.
        Hide
        Bertrand Delacretaz added a comment -

        I agree about the reset in general, but the following pattern seems reasonable to me:

        // Sling servlet is in trouble
        response.setHeader("X-something-specific", "some useful value")
        response.sendError(...)

        And if you do this in a Sling servlet now that won't work...maybe we should preserve the headers instead of resetting everything?

        Show
        Bertrand Delacretaz added a comment - I agree about the reset in general, but the following pattern seems reasonable to me: // Sling servlet is in trouble response.setHeader("X-something-specific", "some useful value") response.sendError(...) And if you do this in a Sling servlet now that won't work...maybe we should preserve the headers instead of resetting everything?
        Hide
        Reto Bachmann-Gmür added a comment -

        401 is an error code by http so it handling by error scripts seems reasonable by me. I think resetting the response is reasonable for server errors (5XX) as in this case something went wrong in producing the response but not for client errors (4XX) where there is something wrong with the request while the response can be assumed to be reasonable for that bogus request. Furthermore resetting headers but not the status line seems inconsistent to me.

        Show
        Reto Bachmann-Gmür added a comment - 401 is an error code by http so it handling by error scripts seems reasonable by me. I think resetting the response is reasonable for server errors (5XX) as in this case something went wrong in producing the response but not for client errors (4XX) where there is something wrong with the request while the response can be assumed to be reasonable for that bogus request. Furthermore resetting headers but not the status line seems inconsistent to me.
        Hide
        Felix Meschberger added a comment -

        Yes, if the sendError is used, 401 is just like any other error and is handled by the error handling script.

        The problem is, that 401 should not be sent to the client using sendError but using setStatus and committing the response to make sure the client gets the authentication requets.

        Show
        Felix Meschberger added a comment - Yes, if the sendError is used, 401 is just like any other error and is handled by the error handling script. The problem is, that 401 should not be sent to the client using sendError but using setStatus and committing the response to make sure the client gets the authentication requets.
        Hide
        Felix Meschberger added a comment -

        > That response.reset() call was not present in 2.1.0

        For completeness: This came in with SLING-1842

        Show
        Felix Meschberger added a comment - > That response.reset() call was not present in 2.1.0 For completeness: This came in with SLING-1842
        Hide
        Bertrand Delacretaz added a comment - - edited

        Fixed in revision 1308347 by overriding sendUnauthorized(...) to use setStatus instead of sendError

        Show
        Bertrand Delacretaz added a comment - - edited Fixed in revision 1308347 by overriding sendUnauthorized(...) to use setStatus instead of sendError

          People

          • Assignee:
            Bertrand Delacretaz
            Reporter:
            Bertrand Delacretaz
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development