Copilot commented on code in PR #13789:
URL: https://github.com/apache/trafficserver/pull/13789#discussion_r4220624408
##########
plugins/experimental/ja4_fingerprint/ja4.h:
##########
@@ -166,9 +166,11 @@ make_JA4_fingerprint(TLSClientHelloSummary const
&TLS_summary, UnaryOp hasher)
std::string result;
result.append(make_JA4_a_raw(TLS_summary));
result.push_back(JA4::PORTION_DELIMITER);
- result.append(hasher(make_JA4_b_raw(TLS_summary)).substr(0, 12));
+ std::string const b_raw{make_JA4_b_raw(TLS_summary)};
+ result.append((b_raw.empty()) ? "000000000000" : hasher(b_raw).substr(0,
12));
result.push_back(JA4::PORTION_DELIMITER);
- result.append(hasher(make_JA4_c_raw(TLS_summary)).substr(0, 12));
+ std::string const c_raw{make_JA4_c_raw(TLS_summary)};
+ result.append((c_raw.empty()) ? "000000000000" : hasher(c_raw).substr(0,
12));
Review Comment:
The `\"000000000000\"` literal is duplicated. To reduce repetition and
prevent accidental inconsistencies, define a named constant (e.g., `constexpr
std::string_view EMPTY_HASH12 = \"000000000000\";`) and reuse it for both B and
C portions.
##########
plugins/experimental/ja4_fingerprint/plugin.cc:
##########
@@ -323,7 +323,7 @@ add_ciphers(JA4::TLSClientHelloSummary &summary,
TSClientHello ch)
if (buflen > 0) {
for (std::size_t i = 0; i + 1 < buflen; i += 2) {
- summary.add_cipher(make_word(buf[i], buf[i + 1]));
+ summary.add_cipher(make_word(buf[i + 1], buf[i]));
Review Comment:
`make_word` takes `(lowbyte, highbyte)`, so needing to pass `buf[i + 1]`
before `buf[i]` is easy to misread and re-break in future edits. Consider
making the intent explicit by introducing a helper for big-endian parsing
(e.g., `make_word_be(high, low)`), renaming parameters to `byte_lo/byte_hi`, or
assembling the value inline with shifts to match the on-wire order.
##########
plugins/experimental/ja4_fingerprint/plugin.cc:
##########
@@ -418,7 +418,7 @@ append_JA4_headers(TSCont /* cont ATS_UNUSED */, TSHttpTxn
txnp, std::string con
// traverse multiple proxies.
bool const ja4_header_exists = header_exists(bufp, hdr_loc, "ja4", 3) ||
header_exists(bufp, hdr_loc,
JA4_VIA_HEADER.data(), static_cast<int>(JA4_VIA_HEADER.length()));
- bool const skip_ja4_headers = global_preserve_enabled && ja4_header_exists;
+ bool const skip_ja4_headers = global_preserve_enabled && ja4_header_exists;
Review Comment:
There are two spaces before `=` here, which is inconsistent with typical
formatting and the surrounding lines. Replace with a single space to avoid
noisy diffs and keep formatting consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
