Conversation
| if (*new_path) { | ||
| TSUrlPathSet(rri->requestBufp, rri->requestUrl, new_path, strlen(new_path)); | ||
| } | ||
| TSUrlHttpParamsSet(rri->requestBufp, rri->requestUrl, nullptr, 0); |
There was a problem hiding this comment.
Can we really remove this line? The original behavior may mean to remove "matrix parameters", which is everything that follows a semicolon in a URL.
There was a problem hiding this comment.
Im trying to create an autest for this first before merging. I believe what it was doing was packing it all into 'new_path', setting the Url path, and then removing the matrix params afterwards.
So with the old implementation would doing a ParamsSet end up re-evaluating the path to locate the params again before removing them? I was figuring that what it was doing was setting the Path with any included new path params and then remove ATS' stored path params so that it would not end up being duplicated
There was a problem hiding this comment.
I didn't follow the entire logic. It just doesn't look like equivalent even if we set nullptr. If new_path contains parameters it may be fine, but what if we don't set new_path here?
There was a problem hiding this comment.
alright let me think about it some more. Pinged some around here but no one really knows how this code was meant to work any more so its a bit of reverse engineering going on. I believe that if we dont set new_path so it doesnt clean up the path params, that it possibly would have been rejected anyway because the plugin is going to parse the path params as things that are expected to be signed (or elements of signing that it needs to interpret) and so it would error out on them
3 participants
This removes the ParamsSet call within url_sig which was just clearing out the ATS parsed params anyway