Correctly filter URL with an email address in it

[krystofwoldrich]

Description

A related issue in Dart.

• Correctly filter URL with an email address in it sentry-dart#1418

This came up as an issue in sentry-dart but as the logic is the same it's likely an issue in sentry-java too.

We should check it and fix it if needed.

[adinauer]

Java SDK is affected as well, here's a test:

    @Test
    fun `keeps email address`() {
        val urlDetails = UrlUtils.parseNullable(
            "https://staging.server.com/api/v4/auth/password/reset/[email protected]"
        )!!
        assertEquals("https://staging.server.com/api/v4/auth/password/reset/[email protected]", urlDetails.url)
        assertNull(urlDetails.query)
        assertNull(urlDetails.fragment)
    }

with result:

Expected :https://staging.server.com/api/v4/auth/password/reset/[email protected]
Actual   :https://[Filtered]@example.com

[marandaneto]

@adinauer will you fix it right away? people that rely on transaction name and span desc from this parser, this will be broken, if you figure out how, I can just replicate on Dart.

[adinauer]

Wasn't planning to do this soon. We can bump prio if you want.

[marandaneto]

Wasn't planning to do this soon. We can bump prio if you want.

I believe the priority is rather important, otherwise, every span and transaction name will be wrong and not identified and they all be grouped together because the filtered URL is always the same every time the parsing breaks.

[adinauer]

Yeah, erring on the other side would mean leaking authority of URLs tho, so we have to be careful.

[marandaneto]

Well the logic to strip out the authority is there, it's just a matter of fixing the parsing to keep the domain and its sub/folders, it's a bug in the parsing, not a matter of PII anymore.

[adinauer]

Sure it's a bug but fixing it may break the filtering. It's not quite easy to determine whether something is part of an email or authority of an URL as there doesn't seem to be a clear set of characters we can use.

Take e.g. this URL https://[email protected] there's no clear way to tell whether staging.server.com?email=someone is the username of the authority section and example.com is the host or whether staging.server.com is the host and [email protected] is the query string. There's https://www.rfc-editor.org/rfc/rfc1738 pages 18-20:

user           = *[ uchar | ";" | "?" | "&" | "=" ]
password       = *[ uchar | ";" | "?" | "&" | "=" ]
uchar          = unreserved | escape
unreserved     = alpha | digit | safe | extra
alpha          = lowalpha | hialpha
lowalpha       = "a" | "b" | "c" | "d" | "e" | "f" | "g" | "h" |
                 "i" | "j" | "k" | "l" | "m" | "n" | "o" | "p" |
                 "q" | "r" | "s" | "t" | "u" | "v" | "w" | "x" |
                 "y" | "z"
hialpha        = "A" | "B" | "C" | "D" | "E" | "F" | "G" | "H" | "I" |
                 "J" | "K" | "L" | "M" | "N" | "O" | "P" | "Q" | "R" |
                 "S" | "T" | "U" | "V" | "W" | "X" | "Y" | "Z"
digit          = "0" | "1" | "2" | "3" | "4" | "5" | "6" | "7" |
                 "8" | "9"
safe           = "$" | "-" | "_" | "." | "+"
extra          = "!" | "*" | "'" | "(" | ")" | ","
escape         = "%" hex hex

So we could use / and # to help the regex but that doesn't solve https://[email protected]. Best appraoch will be to check if we can rewrite the code to use java.net.URL as it seems to be able to figure this out somehow. Haven't looked at the code in detail but seems a bit complicated at first glance.

[marandaneto]

@adinauer yes, I'd suggest using the builtin Uri or URI class depending on what we need, on Dart, the Uri works pretty well:

  test('test', () {
    final uri =
        Uri.parse('https://staging.server.com/[email protected]');
    uri.queryParameters;

    uri.queryParameters.forEach((key, value) {
      expect(key, 'email');
      expect(value, '[email protected]');
    });
  });

So to be honest, this is there already, maybe we don't even need our own regexes.

[stefanosiano]

@adinauer What places should we scrubble the email for? Do you have any more insight on this?

[adinauer]

Should be a change in UrlUtils by using e.g. java.net.URL and letting it take care of parsing the URL.

[stefanosiano]

@adinauer And is it only for OkHttp integration? Or other integrations like apollo need it?

[adinauer]

This should be the same for all integrations using UrlUtils