Skip to content

Deprecate the support for URL param segment #11519

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

Merged
maskit merged 4 commits into from
Jul 18, 2024
Merged

Deprecate the support for URL param segment #11519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83a0581
Make TSUrlHttpParamsGet include params segment
maskit Jul 6, 2024
fa80627
Add some unit tests
maskit Jul 8, 2024
e0129cd
Update docs
maskit Jul 8, 2024
1dcf4d1
Fix
maskit Jul 9, 2024
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
6 changes: 6 additions & 0 deletions include/ts/ts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,9 @@ TSReturnCode TSUrlFtpTypeSet(TSMBuffer bufp, TSMLoc offset, int type);
argument. Note: the returned string is not guaranteed to be
null-terminated.

This function is deprecated and returns empty string.
Plugins that need "params" can call TSUrlPathGet to get a whole path string to parse it.

@param bufp marshal buffer containing the URL.
@param offset location of the URL.
@param length of the returned string.
Expand All @@ -683,6 +686,9 @@ const char *TSUrlHttpParamsGet(TSMBuffer bufp, TSMLoc offset, int *length);
copies the string to within bufp, so you can modify or delete
value after calling TSUrlHttpParamsSet().

This function is deprecated. The value passed will be internally appended to the path portion.
Thus, TSUrlHttpParamsGet will not return the value set by this function.
Copy link
Copy Markdown
Contributor

@bryancall bryancall Jul 15, 2024

Choose a reason for hiding this comment

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

I would like to see us remove the TSUrlHttpParamsGet() instead of passing back an empty string. People should change the logic in their plugins based on the change instead of it continuing to build and possibly discovering an issue later in production.

Copy link
Copy Markdown
Member Author

@maskit maskit Jul 15, 2024

Choose a reason for hiding this comment

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

Although that is a valid option, that would make the migration to 10.0 hard (plugins have to be changed to upgrade ATS). And we'd have to change the Lua API, Cripts, and url_sig plugin.

Everybody has to change the logic in their plugins at some point, if they use Get/SetParams, but this deprecation gives grace period.

What issues would be found? The APIs are not working as expected in the first place. And this change fixes a common mistake under the hood without fixing plugins.


@param bufp marshal buffer containing the URL.
@param offset location of the URL.
@param value HTTP params string to set in the URL.
Expand Down
76 changes: 23 additions & 53 deletions src/proxy/hdrs/URL.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,8 +551,22 @@ URLImpl::set_path(HdrHeap *heap, const char *value, int length, bool copy_string
void
URLImpl::set_params(HdrHeap *heap, const char *value, int length, bool copy_string)
Copy link
Copy Markdown
Contributor

@bryancall bryancall Jul 15, 2024

Choose a reason for hiding this comment

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

Why do we still have set_params if we don't support params anymore?

Copy link
Copy Markdown
Member Author

@maskit maskit Jul 15, 2024

Choose a reason for hiding this comment

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

To keep compatibility as possible as we can, since we only deprecate them at this time.

{
url_called_set(this);
mime_str_u16_set(heap, value, length, &(this->m_ptr_params), &(this->m_len_params), copy_string);
int path_len = this->m_len_path;

// Truncate the current param segment if it exists
if (auto p = strchr(this->m_ptr_path, ';'); p != nullptr) {
auto params_len = path_len - (p - this->m_ptr_path);
path_len -= params_len;
}

int total_length = path_len + 1 + length;
char *path_and_params = static_cast<char *>(alloca(total_length));

memcpy(path_and_params, this->m_ptr_path, path_len);
memcpy(path_and_params + path_len, ";", 1);
memcpy(path_and_params + path_len + 1, value, length);

this->set_path(heap, path_and_params, total_length, true);
}

/*-------------------------------------------------------------------------
Expand Down Expand Up @@ -887,10 +901,6 @@ url_length_get(URLImpl *url, unsigned normalization_flags)
length += 1; // +1 for "/"
}

if (url->m_ptr_params && url->m_len_params > 0) {
length += url->m_len_params + 1; // +1 for ";"
}

if (url->m_ptr_query && url->m_len_query > 0) {
length += url->m_len_query + 1; // +1 for "?"
}
Expand Down Expand Up @@ -962,12 +972,6 @@ url_to_string(URLImpl *url, Arena *arena, int *length)
memcpy(&str[idx], url->m_ptr_path, url->m_len_path);
idx += url->m_len_path;

if (url->m_ptr_params && url->m_len_params > 0) {
str[idx++] = ';';
memcpy(&str[idx], url->m_ptr_params, url->m_len_params);
idx += url->m_len_params;
}

if (url->m_ptr_query && url->m_len_query > 0) {
str[idx++] = '?';
memcpy(&str[idx], url->m_ptr_query, url->m_len_query);
Expand Down Expand Up @@ -1433,8 +1437,6 @@ url_parse_http(HdrHeap *heap, URLImpl *url, const char **start, const char *end,
const char *cur;
const char *path_start = nullptr;
const char *path_end = nullptr;
const char *params_start = nullptr;
const char *params_end = nullptr;
const char *query_start = nullptr;
const char *query_end = nullptr;
const char *fragment_start = nullptr;
Expand All @@ -1456,13 +1458,9 @@ url_parse_http(HdrHeap *heap, URLImpl *url, const char **start, const char *end,
if (*cur == '/') {
path_start = cur;
}
mask = ';' & '?' & '#';
mask = '?' & '#';
parse_path2:
if ((*cur & mask) == mask) {
if (*cur == ';') {
path_end = cur;
goto parse_params1;
}
if (*cur == '?') {
path_end = cur;
goto parse_query1;
Expand All @@ -1472,26 +1470,11 @@ url_parse_http(HdrHeap *heap, URLImpl *url, const char **start, const char *end,
goto parse_fragment1;
}
} else {
ink_assert((*cur != ';') && (*cur != '?') && (*cur != '#'));
ink_assert((*cur != '?') && (*cur != '#'));
}
GETNEXT(done);
goto parse_path2;

parse_params1:
params_start = cur + 1;
GETNEXT(done);
parse_params2:
if (*cur == '?') {
params_end = cur;
goto parse_query1;
}
if (*cur == '#') {
params_end = cur;
goto parse_fragment1;
}
GETNEXT(done);
goto parse_params2;

parse_query1:
query_start = cur + 1;
GETNEXT(done);
Expand Down Expand Up @@ -1552,12 +1535,6 @@ url_parse_http(HdrHeap *heap, URLImpl *url, const char **start, const char *end,
// nothing_after_host check.
url->m_path_is_empty = true;
}
if (params_start) {
if (!params_end) {
params_end = cur;
}
url->set_params(heap, params_start, params_end - params_start, copy_strings);
}
if (query_start) {
// There was a query string marked by '?'.
if (!query_end) {
Expand Down Expand Up @@ -1715,11 +1692,6 @@ url_print(URLImpl *url, char *buf_start, int buf_length, int *buf_index_inout, i
TRY(mime_mem_print(url->m_ptr_path, url->m_len_path, buf_start, buf_length, buf_index_inout, buf_chars_to_skip_inout));
}

if (url->m_ptr_params && url->m_len_params > 0) {
TRY(mime_mem_print(";", 1, buf_start, buf_length, buf_index_inout, buf_chars_to_skip_inout));
TRY(mime_mem_print(url->m_ptr_params, url->m_len_params, buf_start, buf_length, buf_index_inout, buf_chars_to_skip_inout));
}

if (url->m_ptr_query && url->m_len_query > 0) {
TRY(mime_mem_print("?", 1, buf_start, buf_length, buf_index_inout, buf_chars_to_skip_inout));
TRY(mime_mem_print(url->m_ptr_query, url->m_len_query, buf_start, buf_length, buf_index_inout, buf_chars_to_skip_inout));
Expand Down Expand Up @@ -1753,8 +1725,6 @@ url_describe(HdrHeapObjImpl *raw, bool /* recurse ATS_UNUSED */)
obj->m_len_port, obj->m_port);
Dbg(dbg_ctl_http, "\tPATH: \"%.*s\", PATH_LEN: %d,", obj->m_len_path, (obj->m_ptr_path ? obj->m_ptr_path : "NULL"),
obj->m_len_path);
Dbg(dbg_ctl_http, "\tPARAMS: \"%.*s\", PARAMS_LEN: %d,", obj->m_len_params, (obj->m_ptr_params ? obj->m_ptr_params : "NULL"),
obj->m_len_params);
Dbg(dbg_ctl_http, "\tQUERY: \"%.*s\", QUERY_LEN: %d,", obj->m_len_query, (obj->m_ptr_query ? obj->m_ptr_query : "NULL"),
obj->m_len_query);
Dbg(dbg_ctl_http, "\tFRAGMENT: \"%.*s\", FRAGMENT_LEN: %d]", obj->m_len_fragment,
Expand Down Expand Up @@ -1855,8 +1825,8 @@ url_CryptoHash_get_general(const URLImpl *url, CryptoContext &ctx, CryptoHash &h
ends[7] = strs[7] + 1;
ends[8] = strs[8] + url->m_len_path;

strs[9] = ";";
strs[10] = url->m_ptr_params;
strs[9] = "";
strs[10] = nullptr;
strs[11] = "?";

// Special case for the query paramters, allowing us to ignore them if requested
Expand All @@ -1868,8 +1838,8 @@ url_CryptoHash_get_general(const URLImpl *url, CryptoContext &ctx, CryptoHash &h
ends[12] = nullptr;
}

ends[9] = strs[9] + 1;
ends[10] = strs[10] + url->m_len_params;
ends[9] = strs[9] + 0;
ends[10] = strs[10] + 0;
ends[11] = strs[11] + 1;

p = buffer;
Expand Down Expand Up @@ -1927,7 +1897,7 @@ url_CryptoHash_get(const URLImpl *url, CryptoHash *hash, bool ignore_query, cach
{
URLHashContext ctx;
if ((url_hash_method != 0) && (url->m_url_type == URL_TYPE_HTTP) &&
((url->m_len_user + url->m_len_password + url->m_len_params + (ignore_query ? 0 : url->m_len_query)) == 0) &&
((url->m_len_user + url->m_len_password + (ignore_query ? 0 : url->m_len_query)) == 0) &&
(3 + 1 + 1 + 1 + 1 + 1 + 2 + url->m_len_scheme + url->m_len_host + url->m_len_path < BUFSIZE) &&
(memchr(url->m_ptr_host, '%', url->m_len_host) == nullptr) && (memchr(url->m_ptr_path, '%', url->m_len_path) == nullptr)) {
url_CryptoHash_get_fast(url, ctx, hash, generation);
Expand Down
82 changes: 82 additions & 0 deletions src/proxy/hdrs/unit_tests/test_URL.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,3 +697,85 @@ TEST_CASE("UrlHashGet", "[url][hash_get]")
}
}
}

struct get_path_test_case {
const std::string description;
const std::string uri;
const std::string path;
};

// clang-format off
std::vector<get_path_test_case> get_path_test_cases = {
{
"Semicolon in paths 1",
"http://foo.test/abc/xyz;p1=1,p2=2",
"abc/xyz;p1=1,p2=2",
},
{
"Semicolon in paths 2",
"http://foo.test/abc;p1=1,p2=2/xyz",
"abc;p1=1,p2=2/xyz",
},
{
"Semicolon in paths 3",
"http://foo.test/abc/xyz;p1=1,p2=2?q1=1",
"abc/xyz;p1=1,p2=2",
},
{
"Semicolon in paths 4",
"http://foo.test/abc;p1=1,p2=2/xyz?q1=1",
"abc;p1=1,p2=2/xyz",
},
};

/** Return the hash related to a URI.
*
* @param[in] uri The URI to hash.
* @return The hash of the URI.
*/
TEST_CASE("UrlPathGet", "[url][path_get]")
{
for (auto const &test_case : get_path_test_cases) {
std::string description = test_case.description + ": " + test_case.uri + " -> " + test_case.path;
SECTION(description) {
URL url;
HdrHeap *heap = new_HdrHeap();
url.create(heap);
url.parse(test_case.uri);
const char *path;
int path_len;
path = url.path_get(&path_len);
CHECK(std::string_view(path, path_len) == test_case.path);
heap->destroy();
}
}
}


/**
* Tests for deprecated params_get/set
*/
TEST_CASE("UrlParamsGet", "[url][params_get]")
{
// Expected behavior
// - ParamsGet always return empty string
// - ParamsSet appends the value to path
// - PathGet returns params appended by ParamsSet

const char *value;
int value_len;

URL url;
HdrHeap *heap = new_HdrHeap();
url.create(heap);
url.parse("https://foo.test/path;p=1");
value = url.path_get(&value_len);
CHECK(std::string_view(value, value_len) == "path;p=1");
url.params_set("param=1", 7);
value = url.params_get(&value_len);
CHECK(value == nullptr);
CHECK(value_len == 0);
value = url.path_get(&value_len);
CHECK(std::string_view(value, value_len) == "path;param=1");
heap->destroy();
}
3 changes: 1 addition & 2 deletions src/proxy/http/HttpSM.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,8 +709,7 @@ HttpSM::state_read_client_request_header(int event, void *data)
call_transact_and_set_next_state(HttpTransact::TooEarly);
return 0;
} else if (!SSLConfigParams::server_allow_early_data_params &&
(t_state.hdr_info.client_request.m_http->u.req.m_url_impl->m_len_params > 0 ||
t_state.hdr_info.client_request.m_http->u.req.m_url_impl->m_len_query > 0)) {
t_state.hdr_info.client_request.m_http->u.req.m_url_impl->m_len_query > 0) {
SMDbg(dbg_ctl_http, "client request was from early data but HAS parameters");
call_transact_and_set_next_state(HttpTransact::TooEarly);
return 0;
Expand Down
8 changes: 3 additions & 5 deletions src/traffic_cache_tool/CacheScan.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,19 +388,17 @@ CacheScan::get_alternates(const char *buf, int length, bool search)
std::string str;

if (search) {
swoc::bwprint(str, "{}://{}:{}/{};{}?{}", std::string_view(url->m_ptr_scheme, url->m_len_scheme),
swoc::bwprint(str, "{}://{}:{}/{}?{}", std::string_view(url->m_ptr_scheme, url->m_len_scheme),
std::string_view(url->m_ptr_host, url->m_len_host), std::string_view(url->m_ptr_port, url->m_len_port),
std::string_view(url->m_ptr_path, url->m_len_path), std::string_view(url->m_ptr_params, url->m_len_params),
std::string_view(url->m_ptr_query, url->m_len_query));
std::string_view(url->m_ptr_path, url->m_len_path), std::string_view(url->m_ptr_query, url->m_len_query));
if (u_matcher->match(str.data())) {
str = this->stripe->hashText + " " + str;
std::cout << "match found " << str << std::endl;
}
} else {
swoc::bwprint(str, "stripe: {} : {}://{}:{}/{};{}?{}", std::string_view(this->stripe->hashText),
swoc::bwprint(str, "stripe: {} : {}://{}:{}/{}?{}", std::string_view(this->stripe->hashText),
std::string_view(url->m_ptr_scheme, url->m_len_scheme), std::string_view(url->m_ptr_host, url->m_len_host),
std::string_view(url->m_ptr_port, url->m_len_port), std::string_view(url->m_ptr_path, url->m_len_path),
std::string_view(url->m_ptr_params, url->m_len_params),
std::string_view(url->m_ptr_query, url->m_len_query));
std::cout << str << std::endl;
}
Expand Down