stevejgordon
commented
4 years ago Hey @wazowsk1! Thanks for the suggestion. Originally, I'd seen the use case for this being mostly internal API-to-API situations so I wasn't too concerned with trusting the input. I didn't want to get too opinionated about what constitutes a valid ID. The approach I would take (today) is to add another custom middleware to inspect the ID before/after the correlation ID middleware and handle it accordingly.
That all said, this has been on the back of mind for a while, and this issue jogged me back into action. It feels reasonable to provide a built-in mechanism for validation of the header to meet security requirements. My original idea was to allow registration of an ICorrelationIdValidator and a default implementation to validate for a reasonable length. I'm undecided if that should be the default behaviour (with opt-out) or an opt-in. Since it's security-related I'm tempted to make it default but that could be a breaking change so would need a major version bump.
The sticking point for me was what to do in the invalid case. Here it gets more complex as there are a few possible actions one might take. Return a bad request to the caller immediately, ignore the header and generate a valid one, replace the bad value with "Invalid ID", log/send a metric, do something else, etc. I can also see cases where it could be a combination of things.
This becomes the concern of the consuming application and there are a few ways I can think to deal with this.
-
Provide an
IInvalidCorrelationIdHandler(named TBD) interface with aHandleInvalidIdmethod that gets invoked for all requests with an invalid correlation ID. The library could provide and register a default implementation(s) for common cases but support replacement of this for full customisation. -
Start using the HttpContext Features collection (something I'm considering for this library already) so that downstream middleware and user code can then identify requests with a bad correlation ID and deal with it according to their rules.
-
A combination of the above may also be valid and valuable.
So, long story short; yes I think this makes sense. Having written this down (thanks for prompting that) I'm inclined to look at this for a 3.1.0 version if it can be introduced without breaking changes. In that case, it would be opt-in behaviour. At the next major release, I can make a call on whether to provide some form of validation and failure behaviour by default, with opt-out/replacement an option.
I'd welcome thoughts from others about how this feature should work.
There should be some sanity checks for the header value, currently the header value is trusted input. But when using this for a client facing application the header could be tampered with and not be what is expected. For example the header value could be a very long value causing this to be logged by every log line, potentially causing problems down the line.