~ knotless Search/
↑↓ move↵ open⇧↵ all results

chadtmiller.com/knotless

git that lives in your Atmosphere account

knotless.dev
1 collaborator

knotless 0.13.0 Latest 2 packages, 1 image

1
10

Request lines log their query, with what a secret could be in left out

Merged
Merged chadtmiller.com asked to merge into main from chadtmiller.com/knotless:serve/log-queries

What changes

Each request's line now logs its query string as url.query (knotless::telemetry::query). Values that could be secrets are replaced by _: the parameters code, state, iss, token, access_token, refresh_token, password, secret and key, matched without regard to case. The query is cut to 200 characters, and requests under /oauth log none. Calls the server makes still log no query.

The privacy policy's Logs paragraph now says query strings are logged with secrets removed, and that login requests log none. docs/observability.md and deploy/README.md say the same.

Why

A slow request's line named its path but not what it asked for. On prod, a crawler's loads of /chadtmiller.com/knotless/issues took 1.7–3s each, and nothing in the logs could say which filter made them slow. The likely cause, an author: filter resolving a handle each time, could only be inferred. Seeing what each slow page asked for is needed to work on performance.

How it was checked

queries_leave_secrets_out covers secret names in any case, a bare parameter, and the cut. served_and_summarized shows url.query on a request line. The rest is left to CI.

Shortcomings

A secret sent under a parameter name not on the list is logged. The list covers what the server's routes take today: OAuth's callback parameters, and tokens.

Shots-moved: privacy Changeset: none

🤖 Generated with Claude Code

Asked for changes

This logs each request's query as url.query. Values of listed secret parameter names are replaced by _, the query is cut to 200 characters, and /oauth requests log none. The privacy policy and docs are updated to match.

The redaction can be bypassed by percent-encoding the parameter name. query() compares the raw name, so ?%63ode=abc or ?to%6Ben=abc isn't matched and is logged in full. Axum's Query extractor and most servers decode names before matching, so the handler would still accept those requests. That undercuts the privacy policy's claim that secret values are removed. Decode the name (percent-decoding, and + to space) before comparing it to SECRETS, and add a test for an encoded name. Keep the original pair text for names that aren't secrets.

crates/knotless/src/telemetry.rsline 286Outdated
pub fn query(query: &str) -> String {
let pairs = query.split('&').map(|pair| {
let name = pair.split('=').next().unwrap_or_default();
let secret = SECRETS.iter().any(|s| name.eq_ignore_ascii_case(s));
review.knotless.dev

name is the raw text, so an encoded name such as %63ode or to%6Ben skips the list and its value is logged. Percent-decode the name before eq_ignore_ascii_case.

Asked for changes

The diff has no changes to query() since round 1. The redaction bypass I asked you to fix is still there. query() compares the raw parameter name to SECRETS. A request like ?%63ode=abc, ?to%6Ben=abc or ?pass%77ord=x is therefore logged with its value in full. The Query extractor decodes names before it matches them, so the handler still accepts those requests. That makes the privacy policy's "any value that could be a secret is removed" untrue.

To fix it:

  • Percent-decode the name, and turn + into a space, before comparing it to SECRETS.
  • Keep the original pair text for names that aren't secrets.
  • Add a case to queries_leave_secrets_out with an encoded name such as %63ode=abc or TOK%45N=x.

The rest is fine: the cut, the /oauth exclusion, the docs and the privacy text all match the code.

crates/knotless/src/telemetry.rsline 288Outdated
let query = query.filter(|_| !path.starts_with("/oauth"))?;
let pairs = query.split('&').map(|pair| {
let name = pair.split('=').next().unwrap_or_default();
let secret = SECRETS.iter().any(|s| name.eq_ignore_ascii_case(s));
review.knotless.dev

name is still the raw text, so encoded names like %63ode get through. Decode it first (percent-decoding, + to space), then compare.

Asked for changes

Carried forward: the change this pull makes at 7267820de0aca24da09b6ffa4f39c77a43cd2ef1 is the one reviewed at 84ae96ffc15249491709526bd958922103159eb3, so that review stands. What is new comes from its base.

Merged as 851c245