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

Allow header expected in responses to GET requests when RFC7231 has no such requirement #27

Open
ivaylomitrev opened this issue Apr 28, 2023 · 4 comments

Comments

@ivaylomitrev
Copy link

It seems that tester tool expects the "Allow" header to be present in responses to GET requests when RFC 7231#7.4.1 states:

An origin server MUST generate an Allow field in a 405 (Method Not Allowed) response and MAY do so in any other response.

The particular test is encountered in runtest#L375 as of the current version of the tool:

self.verify(allow is not None, "GET header should include Allow for %s" % url)

Is there another reason for the test that is outside of the known requirements for the "Allow" header?

@petterreinholdtsen
Copy link
Owner

petterreinholdtsen commented Apr 28, 2023 via email

@ivaylomitrev
Copy link
Author

Thanks!

I did glance at the CORS specification and it has no reference to the Allow header from what I could see.

And, judging by RFC7231, the only place where Allow is required is 405 Method Not Allowed responses. It is only recommended to be provided in responses to OPTIONS. There is no mention of it with regard to GET requests.

@tsodring
Copy link
Collaborator

Looking at this again, I agree the ALLOWS header is a little strange. The ALLOWS header was likely introduced from a discussion on Access-Control-Allow-Headers from CORS.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Methods

The Access-Control-Allow-Methods response header specifies one or more methods allowed when accessing a resource in response to a preflight request.

An example of the header is shown below:

OPTIONS https://n5.example.com/api/arkivstruktur/mappe/7f000101-8987-1467-8189-878cf131018a
Access-Control-Request-Method: GET, PATCH, PUT, DELETE
Access-Control-Request-Headers: origin, x-requested-with

The intention is that the client knows what it can do with the object. My understanding is that the client could use this e.g., in a user interface to toggle various user interface elements. In the above example the client can retrieve, update and delete the object, and the user interface can automatically switch on buttons to show this to the user.

So in many ways it is about exposing authorization. Somewhere the actual meaning of this seems to have gotten lost. The point, I believe, was to use the ALLOWS header to explicitly expose what the client/user is authorized to do with the object, without baking it in / hiding it in CORS.

The question then is what we should do about it. I think it should be stated explicitly in the API specification. Currently, the publication of authorization details are left to inspecting the Access-Control-Allow-Methods header.

I agree that it does not make sense to have ALLOWS everywhere. I think we should bring the issue up in the next API spec meeting and decide if we explicitly use ALLOWS header for this or not. I don't think it is a good idea to "steal" the ALLOWS header for the API specification. If it is not clear that the client should know what it can do with an object from the CORS header, then we should consider adding a header.

petterreinholdtsen pushed a commit that referenced this issue Jul 26, 2023
It is not clear that we should be checking for the ALLOW header when testing. This was discussed in [27](#27). Allow is part of CORS and we are likely using it wrongly. The proposed way of dealing with this is to remove it until we know what we want to achieve with it.
petterreinholdtsen pushed a commit that referenced this issue Jun 18, 2024
…CORS.

It has been unclear if the CORS / Allows logic in runtest is correct. At some some stage
the understanding in the API spec with regards to CORS resulted in a misinterpreted
implementation. This has been partially discussed in

#27

Other links relevantfor this change:
https://www.baeldung.com/cs/cors-preflight-requests
https://www.baeldung.com/spring-security-cors-preflight
https://fetch.spec.whatwg.org/

A CORS response should include a 'Access-Control-Allow-Methods' and a
'Access-Control-Allow-Origin'. Others may be relevant headers may be relevant
but these two are a minimum

A preflight is discussed here:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/OPTIONS#preflighted_requests_in_cors

In this branch we have not dealt with 'Access-Control-Request-Headers'.
@petterreinholdtsen
Copy link
Owner

petterreinholdtsen commented Jun 18, 2024 via email

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

No branches or pull requests

3 participants