Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions packages/core/src/utils/data-collection/filterUrlQuery.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import type { CollectBehavior } from '../../types/datacollection';
import { filterQueryParams } from './filterQueryParams';

/**
* Applies a `CollectBehavior` to the query string of a full URL, leaving every other URL component
* (scheme, host, path, fragment) untouched.
*
* The query is located by string offsets rather than by parsing, so the URL is returned byte-for-byte
* apart from the query itself. This keeps relative URLs, non-HTTP schemes and unusual encodings intact,
* none of which survive a `URL` round-trip.
*
* Returns the URL with its query filtered, or with the query removed entirely when collection is off.
*/
export function filterUrlQuery(url: string, behavior: CollectBehavior): string {
// The fragment is delimited first: a `?` after a `#` belongs to the fragment, not the query.
const fragmentStart = url.indexOf('#');
const queryEnd = fragmentStart === -1 ? url.length : fragmentStart;

const queryStart = url.indexOf('?');
if (queryStart === -1 || queryStart > queryEnd) {
return url;
}

const prefix = url.slice(0, queryStart);
const query = url.slice(queryStart + 1, queryEnd);
const suffix = url.slice(queryEnd);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe just call this fragment?


const filtered = filterQueryParams(query, behavior);

return filtered ? `${prefix}?${filtered}${suffix}` : `${prefix}${suffix}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: filterUrlQuery removes the trailing ? from URLs with empty query strings even when data collection is enabled, due to an ambiguous falsy check.
Severity: LOW

Suggested Fix

In filterUrlQuery, change the condition from filtered ? to a more explicit check like filtered !== undefined. This will differentiate between an empty query string (where filtered is undefined but the ? should be kept) and the case where collection is disabled (where filtered is also undefined and the ? should be removed).

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: packages/core/src/utils/data-collection/filterUrlQuery.ts#L30

Potential issue: The function `filterUrlQuery` incorrectly removes the trailing `?` from
a URL with an empty query string (e.g., `https://example.com/api?`) even when data
collection is enabled. This happens because `filterQueryParams` returns `undefined` for
an empty query string. The subsequent falsy check `filtered ?` in `filterUrlQuery`
cannot distinguish this `undefined` from the case where collection is explicitly
disabled, causing it to strip the `?`. This contradicts the documentation which states
the `?` is only removed when collection is off and undermines the PR's goal of
preserving URLs.

Did we get this right? 👍 / 👎 to inform future reviews.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import { describe, expect, it } from 'vitest';
import { filterUrlQuery } from '../../../../src/utils/data-collection/filterUrlQuery';

describe('filterUrlQuery', () => {
describe('no query string', () => {
it('returns the URL unchanged', () => {
expect(filterUrlQuery('https://example.com/api/users', true)).toBe('https://example.com/api/users');
});

it('returns a URL with only a fragment unchanged', () => {
expect(filterUrlQuery('https://example.com/docs#section', true)).toBe('https://example.com/docs#section');
});

it('leaves a trailing `?` with no params alone', () => {
expect(filterUrlQuery('https://example.com/api?', true)).toBe('https://example.com/api');
});
});

describe('denyList mode (true)', () => {
it('filters sensitive params and preserves the rest', () => {
const result = filterUrlQuery('https://example.com/api/users?token=abc123&q=a%20b%26c&page=5', true);

expect(result).toBe('https://example.com/api/users?token=[Filtered]&q=a%20b%26c&page=5');
});

it('preserves the fragment', () => {
const result = filterUrlQuery('https://example.com/api?token=abc&page=5#results', true);

expect(result).toBe('https://example.com/api?token=[Filtered]&page=5#results');
});

it('preserves userinfo, port and path', () => {
const result = filterUrlQuery('https://user:pw@example.com:8443/a/b?secret=x&ok=1', true);

expect(result).toBe('https://user:pw@example.com:8443/a/b?secret=[Filtered]&ok=1');
});
});

describe('off mode (false)', () => {
it('removes the query entirely', () => {
expect(filterUrlQuery('https://example.com/api/users?token=abc&page=5', false)).toBe(
'https://example.com/api/users',
);
});

it('removes the query but keeps the fragment', () => {
expect(filterUrlQuery('https://example.com/api?token=abc#results', false)).toBe(
'https://example.com/api#results',
);
});
});

describe('allow / deny behaviors', () => {
it('supports allowList mode', () => {
const result = filterUrlQuery('https://example.com/s?page=1&ref=x&sort=name', { allow: ['page', 'sort'] });

expect(result).toBe('https://example.com/s?page=1&ref=[Filtered]&sort=name');
});

it('supports extra deny terms', () => {
const result = filterUrlQuery('https://example.com/s?page=1&utm_source=email', { deny: ['utm'] });

expect(result).toBe('https://example.com/s?page=1&utm_source=[Filtered]');
});
});

describe('non-standard URLs', () => {
it('handles relative URLs', () => {
expect(filterUrlQuery('/api/users?token=abc&page=5', true)).toBe('/api/users?token=[Filtered]&page=5');
});

it('preserves duplicate params and their order', () => {
const result = filterUrlQuery('https://example.com/s?page=1&token=a&page=2', true);

expect(result).toBe('https://example.com/s?page=1&token=[Filtered]&page=2');
});

it('does not treat a `?` inside a fragment as a query', () => {
const result = filterUrlQuery('https://example.com/docs#/route?token=abc', true);

expect(result).toBe('https://example.com/docs#/route?token=abc');
});
});
});
Loading