Return HTTP 308 instead of 301 when URL has trailing slash

[irodzik]

EDIT: The original issue requested OPA ignore trailing slashes (which is a bit controversial). Instead we can change the server to return HTTP 308 instead of HTTP 301.

Expected Behavior

The same result when posting on url with /v1/data/path/to/document and /v1/data/path/to/document/.

Actual Behavior

The result is different.

Steps to Reproduce the Problem

1. Load following policy:

package somePolicy
default allow = false
allow = input.allow

2. POST /v1/data/somePolicy

{
	"input": {
		"allow": true
	}
}

Result:

{
  "result": {
    "allow": true
  }
}

POST /v1/data/somePolicy/
Result:

{
  "result": {
    "allow": false
  }
}

[tsandall]

When you execute POST v1/data/somePolicy/ (with the trailing slash) the OPA HTTP server redirects to the URL without the trailing slash:

$ curl -i -L localhost:8181/v1/data/somePolicy/ -X POST -d '{"input": {"allow": "true"}}'
HTTP/1.1 301 Moved Permanently
Location: /v1/data/somePolicy
Date: Mon, 24 Feb 2020 14:53:57 GMT
Content-Length: 0

HTTP/1.1 200 OK
Content-Type: application/json
Date: Mon, 24 Feb 2020 14:53:57 GMT
Content-Length: 79

{"decision_id":"3a2b8f68-e9b3-4153-bebe-81644e78b76a","result":{"allow":false}}

Redirecting POST requests is interesting because the HTTP RFC says the user may need to be involved. In case of curl it doesn't retransmit the message body.

I'm off two minds about this. Users who don't realize the difference between trailing and no-trailing slash will want the same behaviour. OTOH, there's no definitive answer here and some will argue that the URLs are not the same. I think the current behaviour is acceptable in that the client could retransmit the message the body.

cc @patrick-east

[anderseknert]

Most clients (including curl) will change the subsequent request following a 301/302 response to a GET, and that's why the body is dropped. The use of -X POST in the above example forces POST for the second request but that's done after the request has been converted to a GET request and with that dropping the body. This can be seen with curl -v:

...
< HTTP/1.1 301 Moved Permanently
< Location: /v1/data
< Date: Mon, 24 Feb 2020 18:32:05 GMT
< Content-Length: 0
<
* Connection #0 to host localhost left intact
* Issue another request to this URL: 'http://localhost:8181/v1/data'
* Switch from POST to GET
* Found bundle for host localhost: 0x7fea40504ca0 [serially]
* Re-using existing connection! (#0) with host localhost
* Connected to localhost (::1) port 8181 (#0)
> POST /v1/data HTTP/1.1
...

I guess the original purpose of this was something like "you can't POST here any longer, so here's a web page explaining what to do instead" but in the context of serving API clients it almost always makes more sense to serve a 307/308 on a POST redirect.

[irodzik]

I understand the tech behind it, but i think important thing is that, as a user of the API, you will get 200 and completely different document queried (or different decision made - which is super important when doing authorization). Idea posted by @anderseknert seems reasonable. If it's not suitable for you, I'd at least add information about this somewhere in documentation.

[patrick-east]

IMO we should probably update to support returning a 308 (307 might be appropriate.. but saying its permanent makes more sense IMO, clients don't need to like re-check.. they should always use the new location).

The only part I'm not sure on is whether or not we can safely change the return codes without breaking existing clients. This might require a major version bump for the API if there is concern about backwards compatibility (and from my point of view there is a little bit of concern).

[anderseknert]

I hear you, @patrick-east - I used to work on an identity server some years back and we had both the some problem and the same concerns about changing redirect status codes - what we did eventually was that we made the change but made it possible to revert to the old status code using some half-documented feature toggle should anyone need to. We kept this in for a couple of releases, but since nobody seemed to notice or ask about it we eventually had it removed. Might be a way forward.

[patrick-east]

We discussed this a bit at the OPA weekly meeting (notes https://docs.google.com/document/d/1v6l2gmkRKAn5UIg3V2QdeeCcXMElxsNzEzDkVlWDVg8/edit#heading=h.2ymp0280bora) The conclusion was that we will not change the return code for the v1 API's. When we make another major version bump its fair game.

In the meantime we have a few concrete action items we can take to improve the situation, especially for new users of OPA:

1. Update the docs to make more explicit -- There is a mention of this behavior in the HTTP API docs, but we should call it out very explicitly and explain what happens with popular clients (ie curl and postman). Maybe even add an example in https://www.openpolicyagent.org/docs/latest/#4-try-opa-run-server
2. Update the message in the api response -- As-is there is an empty body on the 301, if we returned a message saying that it was moved it would likely make the experience much better for curl users who currently just see no output.
3. Log more debugging info message -- The OPA server could dump a more explicit warning when these URL's are hit so that users could have a better clue as to what happened in the server logs.

[anderseknert]

@patrick-east is there a v2 API in the plans or is that hypothetical? :)

We had another developer bitten by this yesterday. With default allow = false kind of rules this is annoying but pretty much harmless (security wise). But once you flip the expression to something like..

deny[message] {
    input.client_id == data.clients_blacklist[_]
    message = "Client ID in blacklist"
}

..the 😱 factor goes up by an order of magnitude.

Of course, given proper testing and solid client implementations this might be unlikely to happen in a production setup. It does however give a real shaky impression when this happen in demos and pitches, which is of course the last thing you'd want from an authorization system.

[patrick-east]

@anderseknert sorry for the delay!

Its mostly hypothetical.. at some point we'll nudge OPA to v1.x.x and we'll have some flexibility to make backwards incompatible changes (there's an issue label for just that.. will add to this issue too).

Of course, given proper testing and solid client implementations this might be unlikely to happen in a production setup. It does however give a real shaky impression when this happen in demos and pitches, which is of course the last thing you'd want from an authorization system.

+1, but I think the key is proper testing and if we can make it more visible when API requests are not doing what is expected. For the trailing slash if we implement the handful of things that came out of the meeting that should go a long ways, and for the missing input value there is discussion around making things like the "console" decision log on by default with the server (and maybe the apache style logs off by default) so that it is easier to see that input was missing.

[anderseknert]

Thanks @patrick-east 👍 Agreed completely on making it more visible and obvious in docs and logs should the current behavior be kept as is.

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[phi1010]

On a short note: This also occurs when an URL has a double slash at some other position, e.g. http://opa.example.com//v1/data/