Problem/Motivation

Since #3621017 (7.7.0) Raven::getClient() passes the Sentry SDK's data_collection options:

  • cookies are limited to big_pipe_nojs;
  • request bodies follow "Send request body to Sentry";
  • user info follows "Capture end user IP addresses";
  • stack frame variables follow "Reflection tracing".

Cookies are covered. Two categories of the incoming request cannot be configured and keep the SDK defaults, which is deny-list mode with only the SDK's built-in sensitive terms (auth, token, secret, password, key, session, sso, csrf and a few more):

  • http_headers: every request header is sent, except Cookie/Set-Cookie, headers whose names match the deny list, and X-Forwarded-For/X-Real-IP. That includes User-Agent, Accept-Language, Referer (the full URL of the previous page with its query string) and any custom headers a proxy adds.
  • url_query_params: the query string is sent in request.url and request.query_string. Only the values of parameters whose names match the deny list become [Filtered]. Parameters such as the OAuth/OpenID Connect code and state, e-mail addresses in exposed filters or search terms are sent as they are.

Separately from the SDK, Raven::log() adds the Drupal log context's request_uri and referer to the event's extra data and to the structured log attributes. Both are full URLs with their query strings.

A site that must not send this data today needs its own OptionsAlter subscriber with a before_send callback that rewrites request and extra on every event.

Steps to reproduce

  1. Configure a PHP DSN and enable the "Error" log level.
  2. Request a page with a query string, e.g. /node/1?code=abc&state=xyz, on which an error is logged.
  3. In Sentry, the event shows the query string in request.url, request.query_string and extra.request_uri, and all request headers under request.headers.

Proposed resolution

Set data collection for URL query parameters, HTTP request headers and HTTP response headers to allow list, make the allow lists configurable and provide default allow lists. Apply to both the PHP and browser SDKs. Data collection options can still be overridden to change the mode (deny list, disable completely, etc.)

Remaining tasks

Review the MR.

User interface changes

Three new allow list configurations, shared by PHP and browser SDKs, at the bottom of the Raven settings form.

API changes

None.

Data model changes

Three new sequence configs in raven.settings (schema, install config, post-update function).

Text initially generated with AI, with human modifications.

Issue fork raven-3630202

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

daniel.pernold created an issue. See original summary.

daniel.pernold opened merge request !84

daniel.pernold’s picture

daniel.pernold commented
Status: Active » Needs review

Merge request !84 is ready for review.

mfb’s picture

mfb
they or he
commented

What do you think about providing default allow lists for headers and query strings like I did for cookies? Some headers, and query strings - as used by Drupal core - should be fairly innocuous.

mfb’s picture

mfb
they or he
commented

Decide whether the same settings should also apply to:

the browser SDK (dataCollection.httpHeaders and dataCollection.urlQueryParams in PageAttachments);

Yes, I do think URL query parameter collection policy should apply to both the PHP SDK and browser SDK. Otherwise, query params leak out as soon as someone enables the browser SDK. However, there is a problem: apparently the dataCollection.urlQueryParams policy doesn't actually work in the 10.x branch of the browser SDK, only in the new 11.x branch. See https://github.com/getsentry/sentry-javascript/pull/23061 Even the query params that are supposed to be filtered by default, such as jwt, leak out in the 10.x branch :(

So there'd have to be a disclaimer that this feature doesn't fully work yet, or wait until the next major version of Raven module, which will use the browser SDK 11.x branch.

daniel.pernold’s picture

daniel.pernold commented

Thank you very much for the thorough review and the ideas, mfb!

I've updated MR !84:

  • The Referer header and the URLs of the log context (extra data and log attributes) now go through HttpUrlCollector, so the configured query parameter policy applies to them as well.
  • Default allow lists for request headers and query parameters, along the lines of the cookie allow list. Both lists can be changed on the settings page. An empty list sends all values except those with sensitive names, and unchecking the setting turns it off. The defaults are:
    • Headers: accept, cache-control, content-length, content-type, host, if-modified-since, if-none-match, origin, pragma, referer, sec-ch-ua, sec-fetch, user-agent, x-requested-with.
    • Query parameters, as used by core: _drupal_ajax, _format, _wrapper_format, ajax_form, ajax_page_state, check_logged_in, destination, items_per_page, order, page, sort.
  • The post-update function also gives existing sites the default allow lists, so they send less from now on. If you'd rather keep the current behaviour for existing sites (empty lists), I'm happy to change that.

About the browser SDK: thank you for looking into this and for the link to getsentry/sentry-javascript#23061. Since dataCollection.urlQueryParams doesn't take effect in the 10.x branch, a setting there would promise more than it can deliver today. What would you think of leaving the browser SDK out of this issue and opening a follow-up for the next major version with the 11.x browser SDK? PageAttachments could then pass the same settings on to dataCollection.httpHeaders and dataCollection.urlQueryParams as they are. The new settings sit in the PHP section of the form, so until then they already read as PHP only. Of course, if you'd prefer to have it in this issue with a disclaimer, I'll gladly add that instead.

One small thing I noticed while testing: the SDK's sensitive deny list contains "key", so Drupal's search parameter keys is always sent as [Filtered]. That's probably fine, I just wanted to mention it.

mfb’s picture

mfb
they or he
commented

I reported the 10.x browser SDK leaking URL query params upstream to Sentry, and did not confirm but it looks like they already fixed it? https://github.com/getsentry/sentry-javascript/issues/25247

In that case, the new query param setting could apply to both PHP and JS - and live at the bottom of the settings form with the capture user IP address setting.

mfb’s picture

mfb
they or he
commented

I also suggested upstream that the PHP SDK should filter the Referer header: https://github.com/getsentry/sentry-php/issues/2254 (someone still needs to submit a PR)

If/when that happens, the relevant code could be removed here.

mfb’s picture

mfb
they or he
commented

Additional testing on browser SDK:

In the 10.x browser SDK, User-Agent and Referer are always sent to Sentry as "synthetic" HTTP headers (captured from JS window object) regardless of dataCollection.httpHeaders configuration. In 11.x, setting dataCollection.httpHeaders to FALSE results in these not being sent. Seems like a bug I could report upstream.

In both 10.x and 11.x browser SDK, the Referer is sent to Sentry unfiltered, rather than being processed as a URL, with sensitive query params filtered. Issue reported upstream: https://github.com/getsentry/sentry-javascript/issues/25303

mfb’s picture

mfb
they or he
commented

did not confirm but it looks like they already fixed it? https://github.com/getsentry/sentry-javascript/issues/25247

Well, the browser SDK 10.x branch fix (by a bot :) is still pending: https://github.com/getsentry/sentry-javascript/pull/25297

mfb’s picture

mfb
they or he
commented

At this point we should be looking pretty good - the issues I filed on the PHP and browser SDKs have been resolved and backported.

mfb’s picture

mfb
they or he
commented
Issue tags: +Needs issue summary update

For simplicity, maybe we can just configure HTTP headers and query parameters as an allow list? Disabling them completely doesn't seem particularly helpful, since it would hide the fact that the request had query parameters.

Now also applying the data collection options to the browser SDK, and the URL for request performance transactions.

mfb’s picture

mfb
they or he
commented
Issue summary: View changes
Issue tags: -Needs issue summary update
mfb’s picture

mfb
they or he
commented
Status: Needs review » Reviewed & tested by the community

If @daniel.pernold or anyone else wants to review this, please do

  • mfb committed 187657ca on 7.x authored by daniel.pernold 
    feat: #3630202 Add settings to stop sending request headers and URL...
mfb’s picture

mfb
they or he
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.