feat: client side change id validation

[IronCore864]

Add client-side validation for change ID used in URLs.

Closes #318.

[benhoyt]

Couple of comments. Also, per the ticket we should do this for all endpoints that include an ID or user input of some kind in the URL -- anything in daemon/api.go that has a {foo} in the Path. In addition to the changes one, that's these:

	Path:        "/v1/services/{name}",
	Path:       "/v1/tasks/{task-id}/websocket/{websocket-id}",  // probably do this one in Client.getTaskWebsocket?

The service name is more than an ID, it can be any service name, so we should be relatively permissive with that. Perhaps:

var serviceNameRegexp = regexp.MustCompile(`^[-_A-Za-z0-9]+$`)

[IronCore864]

Couple of comments. Also, per the ticket we should do this for all endpoints that include an ID or user input of some kind in the URL -- anything in daemon/api.go that has a {foo} in the Path. In addition to the changes one, that's these:

	Path:        "/v1/services/{name}",
	Path:       "/v1/tasks/{task-id}/websocket/{websocket-id}",  // probably do this one in Client.getTaskWebsocket?

The service name is more than an ID, it can be any service name, so we should be relatively permissive with that. Perhaps:

var serviceNameRegexp = regexp.MustCompile(`^[-_A-Za-z0-9]+$`)

I have added a check for service name.

For taskID/websocketID, it seems:

• taskID seems to be returned by the server side, not a user input
• websocketID is hard-coded to be one of the three values: control, stdio, stderr, also not a direct user input.

So I think we won't need to validate task ID and websocket ID?

[benhoyt]

Ah, good point about the websocket IDs not being user input. Looks good to me now, thanks.

[benhoyt]

I'm going to ask @flotter for a second opinion on the service name regex.

[flotter]

Thanks Tiexin! I left a question for you about service names. Let me know what you think ?

On a different note, I think I am not clear what we are protecting against on the client side here with this general strategy (I do not have a problem restricting the client input options, of course) ? We are not preventing anyone from directly accessing the server API without these checks, right ? I apologise if I am missing an important point here.

[IronCore864]

Thanks Tiexin! I left a question for you about service names. Let me know what you think ?

Hi @flotter, thanks for the review. I agree that we should not have a strong filter on the client-side so that we don't block users getting their services whose names are allowed by the server side. I have reverted the change for service name validation, keeping only the check for clientID.

And yes, you are right, the server can be accessed directly anyway, we aren't actually protecting much here, only an added layer of validation on the client side. The server side should be fine because if the change ID is invalid, it won't find it anyway.

Some more context: this change is because of a code review comment here and I agree it has its own merit, because user-generated content should be validated before processed. We already have a check on the noticeID, might as well keep the clientID check for consistency.

[benhoyt]

I somewhat disagree with that comment and probably should have discussed further with Gustavo at the time to come to consensus. Related: https://benhoyt.com/writings/dont-sanitize-do-escape/ ... I think this is somewhat trying to sanitize, rather than escape (which Go's net/http and net/url packages already do properly).

In the case of change IDs, let's keep the regex, because those are a well-known format.

For service names, they are relatively permissive when you set up the Pebble plan, even allowing a slash in them like "foo/bar". However, I just noticed we don't actually use the /v1/services/{name} route. Both v1GetService and v1PostService return BadRequest("not implemented"). I noticed this because the router we're using (gorilla/mux) actually disallows "/" in a {name} path segment, so those routes wouldn't have worked with "foo/bar" anyway.

I think we should probably remove that entire /v1/services/{name} route as it's not used and just confusing, and if we ever add such routes, we probably won't put the arbitrary service name in the path. (If we did, we should disallow "/" in the service name, but I don't think we should make that change now.)

So I think this PR is fine as it stands with just the change ID fix. I'll merge shortly.