Skip to content

Remove paramset in url_sig #11590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ezelkow1 wants to merge 1 commit into from
Closed

Remove paramset in url_sig #11590

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion plugins/experimental/url_sig/url_sig.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,7 +890,6 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri)
if (*new_path) {
TSUrlPathSet(rri->requestBufp, rri->requestUrl, new_path, strlen(new_path));
}
TSUrlHttpParamsSet(rri->requestBufp, rri->requestUrl, nullptr, 0);
Copy link
Copy Markdown
Member

@maskit maskit Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@ezelkow1 ezelkow1 Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@maskit maskit Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

@ezelkow1 ezelkow1 Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

TSfree((void *)current_url);
Expand Down