fix: Return 404 when environment key is not found

[rolodato]

The /flags and /identities endpoints all return 404 if the environment key does not exist, with an empty response. The Edge Proxy should have the same behaviour.

[matthewelwell]

The /flags and /identities endpoints all return 404 if the environment key does not exist, with an empty response.

Unfortunately, this isn't true. In fact, the core API returns a 403 with a JSON response. The Edge API returns an empty 404 as you mentioned. It's definitely frustrating to have an inconsistency here but I'd actually probably say that, of all of the different responses we send, 401 is probably the most correct?

[rolodato]

Mostly opinions and bikeshedding below - I'm also happy to close this PR and revisit all this later or not at all:

I would say 404 is actually correct, because the client-side SDK key is public by design. The SDK key is not a credential, it is only an ID. The behaviour should be the same as e.g. trying to fetch a non-existing key from an S3 bucket, which is a 404. The environment-document endpoint does return 401 if a key is missing/invalid, because we are sending credentials in this case.

There is already a decent amount of confusion on whether client-side SDK keys are public or not. Returning a 401 when it's wrong or missing only adds to this confusion, giving the impression that authorisation is required to fetch client-side flags. In the future we might want to introduce additional methods for fetching client-side flags that do require authorisation, such as requiring a signed context or secret, which would add to this confusion.

Since the default API endpoint for the Edge Proxy is the SaaS Edge API, I would also say that copying the behaviour of the Edge API and not Core is the correct default.

Also, the Core API's /flags and /identities endpoints return a 401 without a WWW-Authenticate header, but this is actually required by HTTP: https://www.rfc-editor.org/rfc/rfc9110.html#name-www-authenticate

IMO, we should merge this PR as-is, and modify the Core API responses to align with the Edge API responses. All SDKs already presumably handle both 404 and 401 responses correctly, so I would not consider this a breaking change on neither the Core API nor Edge Proxy.

[rolodato]

One practical reason to return 404 instead of 401 is because 404 responses can be cached by default: https://www.rfc-editor.org/rfc/rfc7231.html#section-6.1

[matthewelwell]

The environment-document endpoint does return 401 if a key is missing/invalid, because we are sending credentials in this case.

But we're using the same header to do the authentication as when we use a client-side key.

I do tend to agree with most of your opinions, but I think it's hard to draw the lines on how to behave in the different situations as they're not very clearly defined in the interface as it stands. Maybe we should actually build out a table of what we want to do.

endpoint	scenario	status code
/flags	missing x-environment-key	404?
/flags	invalid x-environment-key	404?
/identities	missing x-environment-key	404?
/identities	invalid x-environment-key	404?
/environment-document	missing x-environment-key	401?
/environment-document	invalid x-environment-key	401?

The issue here is then that one can send the server key when calling the flags or identities endpoint, right, so why is that different to the environment-document endpoint?

Sorry that this has become a bigger conversation, but I think it's important that we clear this up before making any changes anywhere.

[rolodato]

Out of all the SDK endpoints, environment-document is the only one that actually requires authorisation. This should be the only one to ever return 401 IMO. All other endpoints should never return 404.

[matthewelwell]

Out of all the SDK endpoints, environment-document is the only one that actually requires authorisation.

Ok, but does that imply that we should only send 401 if the key doesn't match the format for a server-side key, but still return a 404 if the key matches the expected format but there's no matching environment found?

This should be the only one to ever return 401 IMO. All other endpoints should never return 404.

I think this was a typo? Do you mean 'All other endpoints should return 404.' ?

[rolodato]

I actually think the fact that we can't obviously determine what the status codes should be is because the flags routes are not designed well. We are mixing identifiers (environment IDs) and credentials (server-side keys) which are entirely separate concerns.

The environment key ideally should be part of the URL itself, i.e. the resource that we are trying to fetch. For example, curl /flags/<environment-id> -H "Authorization: ..." should return 404 if the environment doesn't exist, but 401 if the authorization is bad. If no Authorization header is provided, it should only ever return 200 or 404.

I'll close this PR because there's no right answer, especially with the current API implementation. We can revisit if we ever decide to change the routes implementation.