Skip to content

Commit d59b1d1

Browse files
chargomeclaude
andauthored
chore: Add no-unfiltered-url-attributes rule (#23144)
#23061 moved URL filtering to each write site, so a URL the user attaches themselves is left alone. The tradeoff is that new write sites need to filter urls. The rule requires `url.full`, `url.query` and `http.target` to be wrapped in `filterCollectedUrl`, and recognises values that can't carry a query. Also adds a Bugbot rule for the shapes a lint rule can't see, like a URL passed through a helper first or attached to a breadcrumb instead of a span. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 05a0fe4 commit d59b1d1

7 files changed

Lines changed: 183 additions & 1 deletion

File tree

.cursor/BUGBOT.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ Unless explicitly noted (e.g. in the `Testing Conventions` section), only flag t
4949
- Flag direct `console.log` / `console.warn` / `console.error` / `console.info` / `console.debug` calls in SDK source. The accepted patterns are:
5050
- The SDK's `debug` logger (gated with `DEBUG_BUILD && debug.*`) for SDK-internal diagnostics.
5151
- `consoleSandbox(() => { console.warn(...) })` for intentional user-facing warnings (e.g. init-time misconfiguration messages). The `consoleSandbox` wrapper prevents the SDK's own console instrumentation from intercepting the call. Bare `console.*` calls outside very early init paths (e.g. before the logger is available) should be flagged.
52+
- Flag `url.full`, `url.query`, `http.target` or `request.query_string` being set from a URL that isn't filtered. Wrap the value in `filterCollectedUrl()` (or `filterCollectedUrlQuery()` for a bare query string), passing the `client` if one is in scope, so `dataCollection.urlQueryParams` applies. Values that can't contain a query (a bare pathname, a queue URL) are fine. The `sdk/no-unfiltered-url-attributes` lint rule catches direct attribute writes, so look for what it can't: URLs passed through a helper or variable first, deprecated aliases set next to a filtered attribute, and URLs on breadcrumbs or events instead of spans.
53+
- Flag span names built from a raw URL. Names follow `METHOD scheme://host/path` and must never contain a query string, so they need `stripUrlQueryAndFragment()`, not `filterCollectedUrl()`.
5254
- Flag usage of the following APIs: `getCurrentScope()`, `getIsolationScope()`, `getClient()` if they are avoidable. Flag it with severity Low and acknowledge from the start that this is more a "is this necessary" check, rather than a rule violation.
5355
- Reason for flagging: Usage of these APIs is problematic for multi-client setups where either there is no "current" client/scope, or the wrong client might be used. Calling these APIs would create a current scope, thereby misleading any future calls to these APIs.
5456
- What to do instead: Use an existing reference to the scope or client. For example, this is possible in most `Integration` hooks.

.oxlintrc.base.json

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
11
{
2+
"jsPlugins": [
3+
{
4+
"name": "sdk",
5+
"specifier": "@sentry/eslint-plugin-sdk"
6+
}
7+
],
28
"$schema": "./node_modules/oxlint/configuration_schema.json",
39
"plugins": ["typescript", "import", "jsdoc", "vitest"],
410
"rules": {
@@ -56,6 +62,12 @@
5662
"typescript/no-deprecated": "error"
5763
},
5864
"overrides": [
65+
{
66+
"files": ["**/src/**/*.ts", "**/src/**/*.tsx"],
67+
"rules": {
68+
"sdk/no-unfiltered-url-attributes": "error"
69+
}
70+
},
5971
{
6072
"files": ["**/*.ts", "**/*.tsx", "**/*.d.ts"],
6173
"rules": {

.oxlintrc.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@
1919
"files": ["**/src/**"],
2020
"rules": {
2121
"sdk/no-class-field-initializers": "error",
22-
"sdk/no-regexp-constructor": "error"
22+
"sdk/no-regexp-constructor": "error",
23+
"sdk/no-unfiltered-url-attributes": "error"
2324
}
2425
}
2526
],

packages/eslint-plugin-sdk/src/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,5 +16,6 @@ module.exports = {
1616
'no-focused-tests': require('./rules/no-focused-tests'),
1717
'no-skipped-tests': require('./rules/no-skipped-tests'),
1818
'no-unsafe-random-apis': require('./rules/no-unsafe-random-apis'),
19+
'no-unfiltered-url-attributes': require('./rules/no-unfiltered-url-attributes'),
1920
},
2021
};
Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
'use strict';
2+
3+
/**
4+
* URL attributes carry the request query string, which `dataCollection.urlQueryParams` is supposed to
5+
* gate. Filtering happens at the write site rather than centrally, so that a URL a user attaches
6+
* themselves is left alone — which also means a site that forgets to filter leaks silently.
7+
*
8+
* This rule requires every SDK-set URL attribute to go through `filterCollectedUrl` /
9+
* `filterCollectedUrlQuery`. Values that cannot contain a query string (a bare pathname, a route
10+
* template, a string literal) are fine — disable the rule on that line and say why.
11+
*/
12+
13+
// Attribute keys that carry a query string, by constant name and by literal value.
14+
const GUARDED_ATTRIBUTES = new Set(['URL_FULL', 'URL_QUERY', 'HTTP_TARGET', 'url.full', 'url.query', 'http.target']);
15+
16+
// Helpers that apply `dataCollection.urlQueryParams`.
17+
const FILTER_FUNCTIONS = new Set([
18+
'filterCollectedUrl',
19+
'filterCollectedUrlQuery',
20+
'_INTERNAL_filterCollectedUrl',
21+
'_INTERNAL_filterCollectedUrlQuery',
22+
'filterUrlQuery',
23+
'filterQueryParams',
24+
'_INTERNAL_filterQueryParams',
25+
'normalizeAndFilterQueryString',
26+
]);
27+
28+
// Helpers that remove the query string outright, so there is nothing left to filter.
29+
const SANITIZING_FUNCTIONS = new Set([
30+
'stripUrlQueryAndFragment',
31+
'getSanitizedUrlString',
32+
'getSanitizedUrlStringFromUrlObject',
33+
'stripDataUrlContent',
34+
// react-router helper; returns `new URL(...).pathname`
35+
'getPathFromRequest',
36+
]);
37+
38+
/** Resolves the attribute name a key node refers to, whether it is `[URL_FULL]` or `'url.full'`. */
39+
function getAttributeName(keyNode, computed) {
40+
if (computed && keyNode.type === 'Identifier') {
41+
return keyNode.name;
42+
}
43+
if (keyNode.type === 'Literal' && typeof keyNode.value === 'string') {
44+
return keyNode.value;
45+
}
46+
return undefined;
47+
}
48+
49+
/**
50+
* Whether a value expression routes through one of the filter helpers. Walks conditionals and
51+
* logical expressions so that `a ?? filterCollectedUrl(b)` and `cond ? filterCollectedUrl(a) : b`
52+
* count as filtered on the branches that matter.
53+
*/
54+
function isFiltered(node, safeNames) {
55+
if (!node) {
56+
return false;
57+
}
58+
59+
switch (node.type) {
60+
case 'CallExpression': {
61+
const callee = node.callee;
62+
const name =
63+
callee.type === 'Identifier'
64+
? callee.name
65+
: // e.g. `Sentry.filterCollectedUrl(...)`
66+
callee.type === 'MemberExpression' && callee.property.type === 'Identifier'
67+
? callee.property.name
68+
: undefined;
69+
return !!name && (FILTER_FUNCTIONS.has(name) || SANITIZING_FUNCTIONS.has(name));
70+
}
71+
// A local holding an already-filtered value, e.g. `const q = filterCollectedUrlQuery(...)`.
72+
case 'Identifier':
73+
return safeNames.has(node.name);
74+
case 'ConditionalExpression':
75+
return isFiltered(node.consequent, safeNames) || isFiltered(node.alternate, safeNames);
76+
case 'LogicalExpression':
77+
return isFiltered(node.left, safeNames) || isFiltered(node.right, safeNames);
78+
case 'TSAsExpression':
79+
case 'TSNonNullExpression':
80+
case 'AwaitExpression':
81+
return isFiltered(node.expression, safeNames);
82+
default:
83+
return false;
84+
}
85+
}
86+
87+
/** Values that can never carry a query string do not need filtering. */
88+
function isTriviallySafe(node) {
89+
if (!node) {
90+
return true;
91+
}
92+
// String and regex literals are fixed values written by us. A regex means the object is a matcher
93+
// (e.g. an ignore-list entry), not a span attribute being set.
94+
if (node.type === 'Literal') {
95+
return true;
96+
}
97+
if (node.type === 'NewExpression' && node.callee.type === 'Identifier' && node.callee.name === 'RegExp') {
98+
return true;
99+
}
100+
if (node.type === 'TemplateLiteral' && node.expressions.length === 0) {
101+
return true;
102+
}
103+
if (node.type === 'Identifier' && node.name === 'undefined') {
104+
return true;
105+
}
106+
return false;
107+
}
108+
109+
/** Collects locals initialised from a filtering or sanitizing helper, so `const x = filter(...)` counts. */
110+
function collectSafeLocals(node, safeNames) {
111+
if (node.id.type === 'Identifier' && isFiltered(node.init, safeNames)) {
112+
safeNames.add(node.id.name);
113+
}
114+
}
115+
116+
module.exports = {
117+
meta: {
118+
docs: {
119+
description:
120+
'Require URL span attributes to be filtered with `filterCollectedUrl` so that `dataCollection.urlQueryParams` is respected.',
121+
},
122+
schema: [],
123+
},
124+
create: function (context) {
125+
// Names of locals known to hold an already-filtered value. Declarations are visited before the
126+
// attribute writes that use them in every real-world ordering, so a single pass is enough.
127+
const safeNames = new Set();
128+
129+
function check(node, keyNode, computed, valueNode) {
130+
const name = getAttributeName(keyNode, computed);
131+
if (!name || !GUARDED_ATTRIBUTES.has(name)) {
132+
return;
133+
}
134+
if (isFiltered(valueNode, safeNames) || isTriviallySafe(valueNode)) {
135+
return;
136+
}
137+
138+
context.report({
139+
node,
140+
message:
141+
`Wrap the value of \`${name}\` in \`filterCollectedUrl()\` (or \`filterCollectedUrlQuery()\` for ` +
142+
'query strings) so `dataCollection.urlQueryParams` is applied. If this value can never contain a ' +
143+
'query string, disable this rule on the line and explain why.',
144+
});
145+
}
146+
147+
return {
148+
VariableDeclarator(node) {
149+
collectSafeLocals(node, safeNames);
150+
},
151+
// `{ [URL_FULL]: value }` and `{ 'url.full': value }`
152+
Property(node) {
153+
check(node, node.key, node.computed, node.value);
154+
},
155+
// `attributes[URL_FULL] = value`
156+
AssignmentExpression(node) {
157+
if (node.left.type !== 'MemberExpression') {
158+
return;
159+
}
160+
check(node, node.left.property, node.left.computed, node.right);
161+
},
162+
};
163+
},
164+
};

packages/server-utils/src/integrations/tracing-channel/amqplib.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,7 @@ function getConnectionAttributesFromUrl(url: unknown): SpanAttributes {
576576
} else if (typeof resolvedUrl === 'string') {
577577
const censoredUrl = censorPassword(resolvedUrl);
578578
attributes[ATTR_MESSAGING_URL] = censoredUrl; // todo(v11) remove this attribute
579+
// oxlint-disable-next-line sdk/no-unfiltered-url-attributes -- AMQP connection URL, not an HTTP request URL
579580
attributes[URL_FULL] = censoredUrl;
580581

581582
try {

packages/server-utils/src/integrations/tracing-channel/aws-sdk/services/sqs.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ export class SqsServiceExtension implements ServiceExtension {
2727
const spanAttributes: Record<string, unknown> = {
2828
[MESSAGING_SYSTEM]: 'aws_sqs',
2929
[MESSAGING_DESTINATION_NAME]: queueName,
30+
// oxlint-disable-next-line sdk/no-unfiltered-url-attributes -- SQS queue identifier, not an HTTP request URL
3031
[URL_FULL]: queueUrl,
3132
[SENTRY_KIND]: 'client',
3233
};

0 commit comments

Comments
 (0)