ngclient: consistently encode URLs

[jku]

Currently we do not explicitly URL-encode rolenames (or target paths) when we use them in URLs. Let's review this code and make a conscious decisions about this.

My original thinking on this was that it's a specification issue: we can't just start encoding e.g. rolenames if the spec does not say we should... but thinking on it further I've started to believe that

• spec should specify how URLS are formed and what must be URL encoded and when
• but we can be more explicit in ngclient before that: it should not break anything that currently works

The latter point is true because requests (the http library) already encodes everything that would be illegal in a URL behind our backs: so emojis, non-ascii characters, most symbols are already being encoded, just not explicitly by us. Only some specific symbols that may be valid in URLs are maybe not encoded (examples: /, #, ?, &).

My current thinking is:

1. we should not require rolenames to be encoded: they are just strings
2. we should encode rolenames before using them in URLs. This should not break any reasonable rolename and is going to be safer

I am not as sure about target paths: they are URL-paths and we could just require them to be URL-encoded already (and maybe even validate that they are encoded?). At the very least we can't encode "/" in target paths because it's valid in a path: this leads me to think we should just require the target path to be correctly encoded already? An additional wrinkle for target paths is that the spec requires us to actually modify the path in quite tricky ways for consistent targets...

[kairoaraujo]

I want to work on this issue.

[kairoaraujo]

Should we close this one? 🤔 or at least remove the 1.0.0 blocker tag?

[jku]

I think it's done:

• rolenames in URLs are now handled explicitly (practical results are 99.9% same as before but now the encoding is visible and testable)
• target urls do use hashes and targetpath (that both come from remote) but there isn't really anything we can do -- I suppose we could test that they are valid url parts or look for constructs like "../" in them... but I don't see major benefits from doing so.

Closing.