Skip to content

fix(logging): remove query strings from logged URLs - #16

Merged
knittingcode merged 11 commits into
mainfrom
fix/fcrm-ovn-log-query-redaction
Oct 24, 2026
Merged

knittingcode merged 11 commits into
mainfrom
fix/fcrm-ovn-log-query-redaction

Conversation

@knittingcode

@knittingcode knittingcode commented Oct 23, 2026 •

Copy link
Copy Markdown
Owner

Summary

Symfony's router logs every matched route at info on the request channel with the full request URI, query string included. That contradicts docs/architecture/observability.md ("query strings … are not logged"). Found by the overnight security review (OVN-104); reproduced on the dev stack, where a value sent as /api/v1/system/status?sprintcheck=… appeared in the api log.

Change

LogRedactor replaces the query string (and fragment) of values under URL keys (request_uri, uri, url, referer, referrer) with ?[redacted], as the edge's access log already does. The request channel stays enabled, because Symfony also logs uncaught exceptions there.

Tests

  • Two unit tests in JsonLineFormatterTest: the router record's query value (an e-mail and a page number) is removed; a URL without a query is kept.
  • Local, in the dev api image with the worktree mounted:
    • unit tests 114/114 and architecture tests 23/23;
    • PHPStan, PHP-CS-Fixer and both Deptrac configurations clean.
  • Integration and stack suites: in this PR's CI.

Limitations

  • Values under other keys are still only scrubbed by the existing credential patterns.
  • The raw exception text in the readiness and status logs (review item OVN-L2) is unchanged. It is already scrubbed and needed for diagnosis.

Updates after the audit (2026-10-24)

  • Merged main. Main, including fix(docker): upgrade brotli-libs for CVE-2026-111497 in the runtime images #18, was merged into the branch with a merge commit (no force push).
  • More URL keys. redirect_uri, location, target_path, href and return_to are now URL keys too (e5e0082). A unit test checks every URL key.
  • ST-43 can now catch the leak this PR fixes. It previously sent only ?token=, which was masked even before this PR. It now also sends ?search=…, a value the old code logged in request_uri (de5873b).
  • Referer leak (confirmed). Symfony writes the full Referer into the message of an unknown route ("No route found … (from "")"), at info level on the request channel. A ?search=marker in the Referer was confirmed in the log against real PHP-FPM.
    • LogRedactor::scrub() now removes the query and fragment of any scheme:// URL in free text (3e640ee).
    • Added LoggingTest::testTheQueryOfAReferrerIsNotLogged and unit cases (e967d86).
    • Added the negative control "URL queries kept in log text". Its tests failed 6 of 6 when it was applied.
    • Updated docs/architecture/observability.md.
  • Local results:
    • Unit tests: 130.
    • Full integration suite in a disposable project: 267 OK.
    • PHPStan, PHP-CS-Fixer and Deptrac clean.
  • Limitation: relative URLs in free text (/path?x) are not cleaned. Symfony's 404 message contains the absolute requested URL without its query, plus the Referer.
  • Merge authorized by the owner (task FCRM-SEC-023M-016M), after fix(security): share the login limits across every spelling of an account #23.

Updates before the merge (2026-10-24)

  • Merged main with fix(security): share the login limits across every spelling of an account #23 (fa6b09d, merge commit, no conflicts, no force push).
  • Relative URLs under URL keys (c514fbc): testEveryUrlKeyLosesTheQueryOfARelativeUrl checks every URL key with /accounts?search=…#…. Only the key rule protects such values; before, removing it went unnoticed because absolute test URLs are also masked by the free-text rule. A new negative control, "URL keys keep their query", removes the key rule (11 of 11 tests fail under it).
  • Real-stack Referer check (e1aa80a): ST-43 now also requests an unknown route with Referer: https://crm.example.test/accounts?search=<marker>. The marker must be in neither the api log (real PHP-FPM, dev Monolog configuration on stderr) nor the edge access log, and both records of that request must quote the Referer as …/accounts?[redacted], so the check cannot pass when the record is missing.
  • Docs (531c132): the redaction table lists the URL key rule; the negative-control count is 38 (it said 35; main had 36 after fix(security): share the login limits across every spelling of an account #23).
  • Local results (head 531c132): unit 145, architecture 23, PHPStan, PHP-CS-Fixer, Deptrac and ShellCheck (-x) clean; in a disposable Compose project, LoggingTest, JsonLineFormatterTest and SecurityLoggingTest OK (66 tests), and both logging controls detected.
  • Still not covered: relative URLs in free text, a " inside a query (browsers encode it), and PHP errors raised before Symfony's handlers exist, which PHP-FPM writes to stderr unredacted (unchanged by this PR).

Symfony's router logs every matched route at info on the request channel
with the full request URI, query string included. So any query value,
such as an e-mail address in a search parameter, reached the logs. That
contradicts docs/architecture/observability.md ("query strings … are not
logged"). Reproduced on the dev stack: a value sent as
/api/v1/system/status?sprintcheck=… appeared in the api log.

The redactor now replaces the query string (and fragment) of values under
URL keys (request_uri, uri, url, referer, referrer) with "?[redacted]",
as the edge's access log already does. The request channel stays
enabled, since Symfony also logs uncaught exceptions there.
Extend the URL keys whose values lose their query string and fragment
with redirect_uri, location, target_path, href and return_to, and test
every URL key.
The query sent by ST-40 to ST-43 only had token=, which was masked
before query strings were removed, so ST-43 could not detect a logged
query string. Also send search=, whose key does not look sensitive, and
require its value to be absent from the api and edge logs.
Symfony's RouterListener quotes the full Referer in the message of an
unknown route (No route found ... (from "<referer>")), which is logged at
info, so the query of the page the user came from (search terms, other
personal data) reached the log. Only URL values under known context keys
lost their query. scrub() now replaces the query and fragment of every
scheme://… URL in messages and strings with ?[redacted].
An unknown route requested with a Referer that carries a search marker
must not log the marker. The negative control drops the free-text URL
query redaction.
Only the key rule removes the query of a relative URL such as a redirect
target; absolute URLs are also masked by the free-text rule, so no test
noticed when the key rule was removed. Every URL key is now checked with a
relative URL, and a negative control removes the key rule.
ST-43 also requests an unknown route with a Referer whose query holds a
marker. The marker must be in neither the api nor the edge log, and both
records of that request must quote the Referer as ?[redacted], so the
check fails if the request never reached the logs.
The redaction table lists the URL keys whose relative and absolute
values lose their query. The negative-control count is 38 with #23 and
this branch.
@knittingcode
knittingcode merged commit fe64f7c into main Oct 24, 2026
2 checks passed
@knittingcode
knittingcode deleted the fix/fcrm-ovn-log-query-redaction branch October 24, 2026 14:08
knittingcode added a commit that referenced this pull request Oct 24, 2026
Main has 38 controls with #23 and #16; this branch adds three, so the
documented count is 41.
knittingcode added a commit that referenced this pull request Oct 24, 2026
Conflict in docs/architecture/authentication.md: kept this branch's unit
test row; the control count is main's 38 plus this branch's 10.
knittingcode added a commit that referenced this pull request Oct 24, 2026
knittingcode added a commit that referenced this pull request Oct 25, 2026
Negative-control analyses found protections on main whose regressions no
test would catch, and protections that were tested but had no control.

RouteSecurityTest pins the exact permission of every tenant route, so a
weakened or new tenant route must be added on purpose. AuditTrailTest
checks that the operator's disable, enable and reset-password are audited
with the operator as actor and no secrets, and that enabling a user,
creating a user or an organization and adding a membership store nothing
when their audit event cannot be written. SecurityLoggingTest checks that
the spellings of an address that reach one account share one pseudonym
(#23), and JsonLineFormatterTest that a URL key loses its fragment even
without a query (#16).

Ten new negative controls (53 with main's 43): a tenant route weakened to
another permission, enabling a user without an audit event or outside its
transaction, URL fragments under URL keys, the referer key, values of
sensitive keys, database driver messages, the failed-login pseudonym
folding, and the member list order and total. Tests and documentation
only; no production code changes.

When the membership administration API (#19) is merged after this, its
three membership routes must be added to the pinned map (the test fails
otherwise) and the documented control count recounted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant