diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index cab82ac0b92c..ea164a34179d 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -49,6 +49,8 @@ Unless explicitly noted (e.g. in the `Testing Conventions` section), only flag t - Flag direct `console.log` / `console.warn` / `console.error` / `console.info` / `console.debug` calls in SDK source. The accepted patterns are: - The SDK's `debug` logger (gated with `DEBUG_BUILD && debug.*`) for SDK-internal diagnostics. - `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. +- 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. +- 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()`. - 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. - 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. - What to do instead: Use an existing reference to the scope or client. For example, this is possible in most `Integration` hooks. diff --git a/.oxlintrc.base.json b/.oxlintrc.base.json index c9bc514a6548..930c4c72ece4 100644 --- a/.oxlintrc.base.json +++ b/.oxlintrc.base.json @@ -1,4 +1,10 @@ { + "jsPlugins": [ + { + "name": "sdk", + "specifier": "@sentry/eslint-plugin-sdk" + } + ], "$schema": "./node_modules/oxlint/configuration_schema.json", "plugins": ["typescript", "import", "jsdoc", "vitest"], "rules": { @@ -56,6 +62,12 @@ "typescript/no-deprecated": "error" }, "overrides": [ + { + "files": ["**/src/**/*.ts", "**/src/**/*.tsx"], + "rules": { + "sdk/no-unfiltered-url-attributes": "error" + } + }, { "files": ["**/*.ts", "**/*.tsx", "**/*.d.ts"], "rules": { diff --git a/.oxlintrc.json b/.oxlintrc.json index 1f50094313ff..1da665a17c26 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -19,7 +19,8 @@ "files": ["**/src/**"], "rules": { "sdk/no-class-field-initializers": "error", - "sdk/no-regexp-constructor": "error" + "sdk/no-regexp-constructor": "error", + "sdk/no-unfiltered-url-attributes": "error" } } ], diff --git a/packages/eslint-plugin-sdk/src/index.js b/packages/eslint-plugin-sdk/src/index.js index c23a1afcd373..ce0cd671c08d 100644 --- a/packages/eslint-plugin-sdk/src/index.js +++ b/packages/eslint-plugin-sdk/src/index.js @@ -16,5 +16,6 @@ module.exports = { 'no-focused-tests': require('./rules/no-focused-tests'), 'no-skipped-tests': require('./rules/no-skipped-tests'), 'no-unsafe-random-apis': require('./rules/no-unsafe-random-apis'), + 'no-unfiltered-url-attributes': require('./rules/no-unfiltered-url-attributes'), }, }; diff --git a/packages/eslint-plugin-sdk/src/rules/no-unfiltered-url-attributes.js b/packages/eslint-plugin-sdk/src/rules/no-unfiltered-url-attributes.js new file mode 100644 index 000000000000..d55b38f12d68 --- /dev/null +++ b/packages/eslint-plugin-sdk/src/rules/no-unfiltered-url-attributes.js @@ -0,0 +1,164 @@ +'use strict'; + +/** + * URL attributes carry the request query string, which `dataCollection.urlQueryParams` is supposed to + * gate. Filtering happens at the write site rather than centrally, so that a URL a user attaches + * themselves is left alone — which also means a site that forgets to filter leaks silently. + * + * This rule requires every SDK-set URL attribute to go through `filterCollectedUrl` / + * `filterCollectedUrlQuery`. Values that cannot contain a query string (a bare pathname, a route + * template, a string literal) are fine — disable the rule on that line and say why. + */ + +// Attribute keys that carry a query string, by constant name and by literal value. +const GUARDED_ATTRIBUTES = new Set(['URL_FULL', 'URL_QUERY', 'HTTP_TARGET', 'url.full', 'url.query', 'http.target']); + +// Helpers that apply `dataCollection.urlQueryParams`. +const FILTER_FUNCTIONS = new Set([ + 'filterCollectedUrl', + 'filterCollectedUrlQuery', + '_INTERNAL_filterCollectedUrl', + '_INTERNAL_filterCollectedUrlQuery', + 'filterUrlQuery', + 'filterQueryParams', + '_INTERNAL_filterQueryParams', + 'normalizeAndFilterQueryString', +]); + +// Helpers that remove the query string outright, so there is nothing left to filter. +const SANITIZING_FUNCTIONS = new Set([ + 'stripUrlQueryAndFragment', + 'getSanitizedUrlString', + 'getSanitizedUrlStringFromUrlObject', + 'stripDataUrlContent', + // react-router helper; returns `new URL(...).pathname` + 'getPathFromRequest', +]); + +/** Resolves the attribute name a key node refers to, whether it is `[URL_FULL]` or `'url.full'`. */ +function getAttributeName(keyNode, computed) { + if (computed && keyNode.type === 'Identifier') { + return keyNode.name; + } + if (keyNode.type === 'Literal' && typeof keyNode.value === 'string') { + return keyNode.value; + } + return undefined; +} + +/** + * Whether a value expression routes through one of the filter helpers. Walks conditionals and + * logical expressions so that `a ?? filterCollectedUrl(b)` and `cond ? filterCollectedUrl(a) : b` + * count as filtered on the branches that matter. + */ +function isFiltered(node, safeNames) { + if (!node) { + return false; + } + + switch (node.type) { + case 'CallExpression': { + const callee = node.callee; + const name = + callee.type === 'Identifier' + ? callee.name + : // e.g. `Sentry.filterCollectedUrl(...)` + callee.type === 'MemberExpression' && callee.property.type === 'Identifier' + ? callee.property.name + : undefined; + return !!name && (FILTER_FUNCTIONS.has(name) || SANITIZING_FUNCTIONS.has(name)); + } + // A local holding an already-filtered value, e.g. `const q = filterCollectedUrlQuery(...)`. + case 'Identifier': + return safeNames.has(node.name); + case 'ConditionalExpression': + return isFiltered(node.consequent, safeNames) || isFiltered(node.alternate, safeNames); + case 'LogicalExpression': + return isFiltered(node.left, safeNames) || isFiltered(node.right, safeNames); + case 'TSAsExpression': + case 'TSNonNullExpression': + case 'AwaitExpression': + return isFiltered(node.expression, safeNames); + default: + return false; + } +} + +/** Values that can never carry a query string do not need filtering. */ +function isTriviallySafe(node) { + if (!node) { + return true; + } + // String and regex literals are fixed values written by us. A regex means the object is a matcher + // (e.g. an ignore-list entry), not a span attribute being set. + if (node.type === 'Literal') { + return true; + } + if (node.type === 'NewExpression' && node.callee.type === 'Identifier' && node.callee.name === 'RegExp') { + return true; + } + if (node.type === 'TemplateLiteral' && node.expressions.length === 0) { + return true; + } + if (node.type === 'Identifier' && node.name === 'undefined') { + return true; + } + return false; +} + +/** Collects locals initialised from a filtering or sanitizing helper, so `const x = filter(...)` counts. */ +function collectSafeLocals(node, safeNames) { + if (node.id.type === 'Identifier' && isFiltered(node.init, safeNames)) { + safeNames.add(node.id.name); + } +} + +module.exports = { + meta: { + docs: { + description: + 'Require URL span attributes to be filtered with `filterCollectedUrl` so that `dataCollection.urlQueryParams` is respected.', + }, + schema: [], + }, + create: function (context) { + // Names of locals known to hold an already-filtered value. Declarations are visited before the + // attribute writes that use them in every real-world ordering, so a single pass is enough. + const safeNames = new Set(); + + function check(node, keyNode, computed, valueNode) { + const name = getAttributeName(keyNode, computed); + if (!name || !GUARDED_ATTRIBUTES.has(name)) { + return; + } + if (isFiltered(valueNode, safeNames) || isTriviallySafe(valueNode)) { + return; + } + + context.report({ + node, + message: + `Wrap the value of \`${name}\` in \`filterCollectedUrl()\` (or \`filterCollectedUrlQuery()\` for ` + + 'query strings) so `dataCollection.urlQueryParams` is applied. If this value can never contain a ' + + 'query string, disable this rule on the line and explain why.', + }); + } + + return { + VariableDeclarator(node) { + collectSafeLocals(node, safeNames); + }, + // `{ [URL_FULL]: value }` and `{ 'url.full': value }` + Property(node) { + check(node, node.key, node.computed, node.value); + }, + // `attributes[URL_FULL] = value` + AssignmentExpression(node) { + if (node.left.type !== 'MemberExpression') { + return; + } + check(node, node.left.property, node.left.computed, node.right); + }, + }; + }, +}; diff --git a/packages/server-utils/src/integrations/tracing-channel/amqplib.ts b/packages/server-utils/src/integrations/tracing-channel/amqplib.ts index 27066a9c5ac7..4ca0c8342ee2 100644 --- a/packages/server-utils/src/integrations/tracing-channel/amqplib.ts +++ b/packages/server-utils/src/integrations/tracing-channel/amqplib.ts @@ -576,6 +576,7 @@ function getConnectionAttributesFromUrl(url: unknown): SpanAttributes { } else if (typeof resolvedUrl === 'string') { const censoredUrl = censorPassword(resolvedUrl); attributes[ATTR_MESSAGING_URL] = censoredUrl; // todo(v11) remove this attribute + // oxlint-disable-next-line sdk/no-unfiltered-url-attributes -- AMQP connection URL, not an HTTP request URL attributes[URL_FULL] = censoredUrl; try { diff --git a/packages/server-utils/src/integrations/tracing-channel/aws-sdk/services/sqs.ts b/packages/server-utils/src/integrations/tracing-channel/aws-sdk/services/sqs.ts index 06c5a8543db2..6b4ae2c0e474 100644 --- a/packages/server-utils/src/integrations/tracing-channel/aws-sdk/services/sqs.ts +++ b/packages/server-utils/src/integrations/tracing-channel/aws-sdk/services/sqs.ts @@ -27,6 +27,7 @@ export class SqsServiceExtension implements ServiceExtension { const spanAttributes: Record = { [MESSAGING_SYSTEM]: 'aws_sqs', [MESSAGING_DESTINATION_NAME]: queueName, + // oxlint-disable-next-line sdk/no-unfiltered-url-attributes -- SQS queue identifier, not an HTTP request URL [URL_FULL]: queueUrl, [SENTRY_KIND]: 'client', };