-
Notifications
You must be signed in to change notification settings - Fork 44
Disable accessibility when browserstackAccessibility plugin not loaded #1132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,115 @@ | |
| return CYPRESS_V10_AND_ABOVE_CONFIG_FILE_EXTENSIONS.includes(extension); | ||
| } | ||
|
|
||
| // Strip JS/TS comments so that commented-out plugin imports/calls are ignored | ||
| // by the static scans below. Best-effort: handles block and line comments while | ||
| // avoiding `://` in URLs. | ||
| const stripComments = (src) => { | ||
| return src | ||
| .replace(/\/\*[\s\S]*?\*\//g, ' ') // block comments | ||
| .replace(/(^|[^:])\/\/[^\n]*/g, '$1'); // line comments (skip URLs like http://) | ||
| }; | ||
|
|
||
| // Reads the cypress config source (comments stripped). Returns null if it cannot | ||
| // be read. | ||
| const readConfigSource = (user_config) => { | ||
| const configPath = user_config.run_settings && user_config.run_settings.cypressConfigFilePath; | ||
| if (!configPath || !fs.existsSync(configPath)) return null; | ||
| return stripComments(fs.readFileSync(configPath, { encoding: 'utf-8' })); | ||
| }; | ||
|
|
||
| // Finds the symbol the accessibility plugin is imported as, via require() or | ||
| // import, regardless of path style. Returns the binding name or null. | ||
| const getAccessibilityPluginBinding = (content) => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These regexes only match On the require-succeeded path this is benign (the invocation scan is lenient). But on the inconclusive path (TS config before bstack packages are installed), Suggest widening the regex to cover |
||
| const requireMatch = content.match(/(?:const|let|var)\s+([A-Za-z0-9_$]+)\s*=\s*require\(\s*['"][^'"]*accessibility-automation\/plugin['"]\s*\)/); | ||
| const importMatch = content.match(/import\s+([A-Za-z0-9_$]+)\s+from\s+['"][^'"]*accessibility-automation\/plugin['"]/); | ||
| return (requireMatch && requireMatch[1]) || (importMatch && importMatch[1]) || null; | ||
| }; | ||
|
|
||
| const isBindingCalled = (content, binding) => { | ||
| const callRegex = new RegExp('\\b' + binding.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '\\s*\\('); | ||
Check warningCode scanning / Semgrep OSS Semgrep Finding: javascript.lang.security.audit.detect-non-literal-regexp.detect-non-literal-regexp Warning
RegExp() called with a binding function argument, this might allow an attacker to cause a Regular Expression Denial-of-Service (ReDoS) within your application as RegExP blocks the main thread. For this reason, it is recommended to use hardcoded regexes instead. If your regex is run on user-controlled input, consider performing input validation or use a regex checking/sanitization library such as https://www.npmjs.com/package/recheck to verify that the regex does not appear vulnerable to ReDoS.
|
||
|
|
||
| return callRegex.test(content); | ||
| }; | ||
|
|
||
| // Static check: confirm the (already-imported) accessibility plugin is actually | ||
| // invoked in the config source. Lenient — if the import binding cannot be located | ||
| // via static parsing (unusual syntax) or the source cannot be read, we do NOT | ||
| // veto the require-based detection (return true), to avoid wrongly disabling | ||
| // valid configs. | ||
| const isAccessibilityPluginInvokedInSource = (user_config) => { | ||
| try { | ||
| const content = readConfigSource(user_config); | ||
| if (content === null) return true; | ||
| const binding = getAccessibilityPluginBinding(content); | ||
| if (!binding) return true; | ||
| return isBindingCalled(content, binding); | ||
| } catch (error) { | ||
| logger.debug(`Unable to verify accessibility plugin invocation: ${error.message || error}`); | ||
| return true; | ||
| } | ||
| }; | ||
|
|
||
| // Pure static fallback: confirm the plugin is BOTH imported AND invoked. Used | ||
| // only when the config could not be required (e.g. a TypeScript config before | ||
| // BrowserStack packages are installed), so such users are still evaluated. | ||
| const isAccessibilityPluginImportedAndCalledInSource = (user_config) => { | ||
| try { | ||
| const content = readConfigSource(user_config); | ||
| if (content === null) return false; | ||
| const binding = getAccessibilityPluginBinding(content); | ||
| if (!binding) return false; | ||
| return isBindingCalled(content, binding); | ||
| } catch (error) { | ||
| logger.debug(`Unable to scan cypress config for accessibility plugin: ${error.message || error}`); | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * Determines whether the BrowserStack accessibility plugin is genuinely wired | ||
| * into the user's cypress config, i.e. both imported AND invoked. | ||
| * | ||
| * Detection combines two signals: | ||
| * 1) Require-load: reading the cypress config executes its top-level requires; | ||
| * the plugin sets BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED on load, which | ||
| * readCypressConfigFile propagates back as a definitive 'true'/'false'. This | ||
| * tells us whether the plugin is imported (and does not false-positive on a | ||
| * commented-out require, since commented code never executes). | ||
| * 2) Static source scan: confirms the imported plugin binding is actually called | ||
| * in the config — so "imported but never called" is treated as not loaded. | ||
| * | ||
| * If the config could not be required (env var stays undefined, e.g. a TS config | ||
| * before packages are installed), we fall back to a pure static scan that checks | ||
| * for both import and invocation. | ||
| */ | ||
| exports.isAccessibilityPluginLoaded = (user_config) => { | ||
| try { | ||
| // Reset before reading so a stale value from a previous run cannot leak in. | ||
| delete process.env.BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED; | ||
| const { readCypressConfigFile } = require('../helpers/readCypressConfigUtil'); | ||
| readCypressConfigFile(user_config); | ||
|
|
||
| const detection = process.env.BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED; | ||
| if (detection === 'true') { | ||
| // Imported via require — additionally require that it is actually invoked. | ||
| const called = isAccessibilityPluginInvokedInSource(user_config); | ||
| if (!called) { | ||
| logger.debug('Accessibility plugin is imported but not invoked in the cypress config; treating as not loaded.'); | ||
| } | ||
| return called; | ||
| } | ||
| if (detection === 'false') return false; | ||
|
|
||
| // Inconclusive (config could not be required) — fall back to a static scan | ||
| // that checks for both import and invocation. | ||
| logger.debug('Accessibility plugin detection inconclusive from config require; falling back to source scan.'); | ||
| return isAccessibilityPluginImportedAndCalledInSource(user_config); | ||
| } catch (error) { | ||
| logger.debug(`Unable to determine if accessibility plugin is loaded: ${error.message || error}`); | ||
| return isAccessibilityPluginImportedAndCalledInSource(user_config); | ||
| } | ||
| } | ||
|
|
||
| exports.createAccessibilityTestRun = async (user_config, framework) => { | ||
|
|
||
| try { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,18 @@ const { decodeJWTToken } = require("../../helpers/utils"); | |
| const utils = require('../../helpers/utils'); | ||
| const http = require('http'); | ||
|
|
||
| // Marker set as soon as this plugin module is loaded by the user's cypress | ||
| // config (via `require('browserstack-cypress-cli/bin/accessibility-automation/plugin')`). | ||
| // The CLI reads the cypress config (which executes its top-level requires) before | ||
| // sending the build start event, and uses this marker to determine whether the | ||
| // accessibility plugin is actually wired in. Unlike a static text scan of the | ||
| // config file, this does NOT false-positive on commented-out requires. | ||
| process.env.BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED = 'true'; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Suggestion — load-bearing Setting |
||
|
|
||
| const browserstackAccessibility = (on, config) => { | ||
| // Also set on invocation, so that a runtime read of the plugin reflects that | ||
| // it was actually called within setupNodeEvents. | ||
| process.env.BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED = 'true'; | ||
| let browser_validation = true; | ||
| if (process.env.BROWSERSTACK_ACCESSIBILITY_DEBUG === 'true') { | ||
| config.env.BROWSERSTACK_LOGS = 'true'; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,10 +30,11 @@ const { | |
| printBuildLink | ||
| } = require('../testObservability/helper/helper'); | ||
|
|
||
| const { | ||
| const { | ||
| createAccessibilityTestRun, | ||
| setAccessibilityEventListeners, | ||
| checkAccessibilityPlatform, | ||
| isAccessibilityPluginLoaded, | ||
| supportFileCleanup | ||
| } = require('../accessibility-automation/helper'); | ||
| const { isTurboScaleSession, getTurboScaleGridDetails, patchCypressConfigFileContent, atsFileCleanup } = require('../helpers/atsHelper'); | ||
|
|
@@ -42,6 +43,10 @@ const TestHubHandler = require('../testhub/testhubHandler'); | |
|
|
||
| module.exports = function run(args, rawArgs) { | ||
| utils.normalizeTestReportingEnvVars(); | ||
| // Tracks the case where accessibility was requested but the plugin is not | ||
| // wired into the cypress config; surfaced in the end-of-session EDS event so | ||
| // such builds can be excluded from accessibility stability queries. | ||
| let accessibilityPluginNotLoaded = false; | ||
| markBlockStart('preBuild'); | ||
| // set debug mode (--cli-debug) | ||
| utils.setDebugMode(args); | ||
|
|
@@ -69,7 +74,7 @@ module.exports = function run(args, rawArgs) { | |
| /* Set testObservability & browserstackAutomation flags */ | ||
| const [isTestObservabilitySession, isBrowserstackInfra] = setTestObservabilityFlags(bsConfig); | ||
| const checkAccessibility = checkAccessibilityPlatform(bsConfig); | ||
| const isAccessibilitySession = bsConfig.run_settings.accessibility || checkAccessibility; | ||
| let isAccessibilitySession = bsConfig.run_settings.accessibility || checkAccessibility; | ||
| const turboScaleSession = isTurboScaleSession(bsConfig); | ||
| Constants.turboScaleObj.enabled = turboScaleSession; | ||
|
|
||
|
|
@@ -113,6 +118,15 @@ module.exports = function run(args, rawArgs) { | |
| // set build tag caps | ||
| utils.setBuildTags(bsConfig, args); | ||
|
|
||
| // If accessibility is requested but the BrowserStack accessibility plugin is | ||
| // not loaded in the cypress config, explicitly disable accessibility before | ||
| // the build start event so the build is not treated as an accessibility build. | ||
| if (isAccessibilitySession && isBrowserstackInfra && !isAccessibilityPluginLoaded(bsConfig)) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Consider reading the config once and threading the detection signal through, or memoizing on |
||
| logger.warn(Constants.userMessages.ACCESSIBILITY_PLUGIN_NOT_LOADED); | ||
| accessibilityPluginNotLoaded = true; | ||
| isAccessibilitySession = false; | ||
| } | ||
|
|
||
| checkAndSetAccessibility(bsConfig, isAccessibilitySession); | ||
|
|
||
| const preferredPort = 5348; | ||
|
|
@@ -422,6 +436,7 @@ module.exports = function run(args, rawArgs) { | |
| unique_id: utils.generateUniqueHash(), | ||
| package_error: utils.checkError(packageData), | ||
| checkmd5_error: utils.checkError(md5data), | ||
| accessibility_plugin_not_loaded: accessibilityPluginNotLoaded, | ||
| build_id: data.build_id, | ||
| test_zip_size: test_zip_size, | ||
| npm_zip_size: npm_zip_size, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,3 +12,17 @@ if (fs.existsSync(config.configJsonFileName)) { | |
|
|
||
| // write module in temporary json file | ||
| fs.writeFileSync(config.configJsonFileName, JSON.stringify(mod)) | ||
|
|
||
| // Requiring the cypress config above executes its top-level requires, which | ||
| // includes the BrowserStack accessibility plugin when the user has wired it in. | ||
| // The plugin sets BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED on load; surface that | ||
| // back to the parent CLI process via a temp flag file. | ||
| try { | ||
| const accessibilityPluginLoaded = process.env.BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED === 'true'; | ||
| fs.writeFileSync( | ||
| config.accessibilityPluginFlagFileName, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Suggestion — temp flag file can be left behind.
|
||
| JSON.stringify({ accessibilityPluginLoaded }) | ||
| ); | ||
| } catch (err) { | ||
| // best-effort: detection falls back to "not loaded" if this fails | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Suggestion —
stripCommentscan corrupt string literals.The
://guard protects URLs, but a//inside a non-URL string (e.g.const re = "a//b") is still stripped, and the block-comment pass strips/* */inside strings too.^isn't multiline-anchored (a preceding\nsatisfies[^:], so line-start comments are still handled). Impact is low since the require-load marker is the primary signal — worth a comment noting the scan is intentionally lossy.