Broker does not accept url-encoded forward slash in instance_id

[weimel]

We expect all our service's instance_ids to have a forward slash among other non-standard characters e.g. a/81eb:59d6 The forward slash gets encoded to %2F but the broker still returns a 404 not found error when making a provision call. A normal string as the instance_id works perfectly fine.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/179388616

The labels on this github issue will be updated when the story is started.

[blgm]

This seems to be related to gorilla/mux#132

One way to fix it would be to change the route matchers from:

router.HandleFunc("/v2/service_instances/{instance_id}", apiHandler.Provision).Methods("PUT")

to

router.HandleFunc("/v2/service_instances/{instance_id:.*}", apiHandler.Provision).Methods("PUT")

However looking toward the bottom of gorilla/mux#132
it looks like gorilla/mux#184 and gorilla/mux#190 should have fixed it. There's a bit more investigation to be done here as there might be a better fix.

I don't think the requirement to have URL encoded characters in the instance_id is very common, and I've only come across a UUID/GUID being used. I'd be inclined to avoid URL encoded characters if you can as you'll be relying on a less tested code path. But sometimes these things are unavoidable.

@weimel is this something that you'd be interested in investigating further and submitting a PR?

[weimel]

Using router.UseEncodedPath() in this file https://github.com/pivotal-cf/brokerapi/blob/main/api.go#L40 would solve the problem. Adding it outside of the method did not work properly so right now we have resorted to our own implementations of these methods. Perhaps it would be useful to add an option to the New() and NewWithCustomAuth() to accept url encoding

[blgm]

Hi @weimel. I've created PR #178 as an idea of how to this option. Rather than adding new constructors it introduces the functional options pattern which allows for more flexibility. I'm waiting for feedback from the contributor team, and they might decide that this is a bad approach. But I'd also be interested to hear your feedback, and whether it solves your problem?

[weimel]

Yes, this implementation solves our issue. Thanks!

[pivotal-marcela-campo]

We published the fix for this in release 8.2.0