feat(SDK-7250): capture the wdio config file in auto-captured logs - #128
feat(SDK-7250): capture the wdio config file in auto-captured logs#128AakashHotchandani wants to merge 8 commits into
Conversation
The archive uploaded at onComplete carried only our own two debug logs, so triaging an App-A11y no-scan report meant asking the customer how they had configured the service. It now also carries a credential-redacted copy of their wdio config, the local config files it imports, and package.json. WebdriverIO keeps the config path in ConfigParser's private #configFilePath (v8 and v9 alike) and no service can reach it, so configCapture.ts resolves it through a ladder of fallbacks: the `config-path` key yargs leaves behind from `run <configPath>`, the raw argv positional, rootDir, cwd, and finally a single unambiguous *.conf.* in either directory. Resolved once in onPrepare and published on the environment so the upload path never re-derives it from cwd -- that re-derivation is the bug SDK-5993 fixed in the Node SDK. Opt out with `disableAutoCaptureLogs: true` or BROWSERSTACK_DISABLE_AUTO_CAPTURE_LOGS=true. The flag is mirrored onto the environment because the detached cleanup rescue calls uploadLogs with no options -- and since opting out leaves logsUploaded false, that rescue is armed on exactly the runs that opted out. Also fixes two latent archive bugs this made reachable: the staging directory is now per-run (the fixed tmpdir()/logs.tar names let concurrent runs clobber and unlink each other's archives) and archive entry names are de-duplicated (two captured files sharing a basename silently overwrote each other). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Review
Verdict: clientSecret, refreshToken, privateKey, …) before customer config is uploaded. Everything else is solid; the suggestions are optional.
Summary: 0 critical · 1 warning · 4 suggestions across 9 files reviewed.
This PR uploads a credential-redacted copy of the customer's wdio.conf (plus its local imports and package.json) into the auto-captured Observability log bundle. The security posture is largely sound: BrowserStack creds are redacted, env-interpolated values are captured as literal source (not resolved), the opt-out is enforced across all three upload paths, and everything is best-effort/graceful. The one residual leak surface is the line/key-anchored redaction, which by construction misses compound camelCase secret keys and multi-line values — grounded in rules/security.md's documented redaction limitations.
Standalone npm package — no paired Binary PR and no gRPC/proto changes, so SDK↔Binary integration gates are N/A.
See inline comments below for full Problem and Suggested Fix detail on each finding.
Generated by Automated SDK PR review.
| .sort((a, b) => b.length - a.length) | ||
| .map(escapeRegex) | ||
| .join('|') | ||
| const redactRegex = new RegExp(`^.*?(?<![A-Za-z0-9_$])(${keys})(?![A-Za-z0-9_$]).*$`, 'gmi') |
There was a problem hiding this comment.
⚠️ Warning — [SECURITY] Line/key-anchored redaction misses compound camelCase secret keys (and multi-line values)
Problem
redactSensitiveContent scrubs a line only when it contains one of REDACTED_KEYS as a standalone token: the regex is ^.*?(?<![A-Za-z0-9_$])(<key>)(?![A-Za-z0-9_$]).*$. The lookbehind (?<![A-Za-z0-9_$]) rejects any preceding letter/digit/_/$, so a compound camelCase key whose sensitive word is a suffix is not matched:
clientSecret: 'abc123' // 'secret' preceded by 't' -> NOT redacted -> LEAK
refreshToken: 'ya29...' // 'token' preceded by 'h' -> NOT redacted -> LEAK
privateKey: '-----BEGIN' // 'key' preceded by 'e' -> NOT redacted -> LEAKThe enumerated camelCase names (accessKey, apiKey, accessToken, authToken, userName) survive only because they are literal list entries — arbitrary <prefix>Key/Token/Secret/Password are not. The primary BrowserStack creds (user/key/accessKey/browserstack.*) are covered, and env-interpolated values are captured as literal source (safe), so this is a residual third-party-secret surface, not a BrowserStack-cred leak.
Two further gaps survive by the same line-anchoring, both documented in rules/security.md: a value on a different line from its key (key:\n 'literal-secret') and basic-auth embedded in a non-proxyUrl URL (baseUrl: 'https://u:p@host'). The stated contract is "fail closed — over-redaction is acceptable, a leak is not"; for these compound keys the code actually chooses under-redaction (to keep hotkey/keyword intact), which is the opposite trade-off.
Suggested Fix
Add a suffix-anchored pass for the sensitive-word families in addition to the current whole-word pass, accepting the hotkey/monkeypatch false positives (over-redaction is the stated contract):
// after the existing whole-word redactRegex pass
const suffixRegex = /^.*[A-Za-z0-9_$]*(?:key|token|secret|password|passwd|credential)\s*[:=].*$/gim
text = text.replace(suffixRegex, '[REDACTED]')Or, more conservatively, enumerate the common compounds (clientSecret, refreshToken, idToken, bearerToken, privateKey, apiSecret, sessionSecret) into REDACTED_KEYS. Either way, add a unit test asserting clientSecret/refreshToken are scrubbed to lock the contract, and note the accepted residual gaps (multi-line value, basic-auth URL) explicitly in the disableAutoCaptureLogs JSDoc / release notes.
Confidence: 🟢 Objectively verifiable from the regex word boundary, and matches the documented redaction limitations in rules/security.md (line-anchored, key-name-anchored; misses multi-line values, basic-auth URLs, tokens without a recognized key-name prefix).
There was a problem hiding this comment.
Valid — fixed in 97e2f83, and the gap was slightly wider than reported.
Reproduced first: clientSecret, refreshToken, privateKey all survived the whole-word pass. Also client_secret — snake_case has the same problem, because the lookbehind rejects the preceding _ just as it rejects a preceding letter. That was not in the report.
I did not take the suggested regex, for two reasons: ^.*[A-Za-z0-9_$]*(?:key|token|...) replaces the entire line with [REDACTED], losing the key name that makes the artifact readable, and being case-insensitive it also takes hotkey, keyword and tokenizer with it. Over-redaction is acceptable as a tiebreaker, but it is not free here — the whole point of shipping the config is that a support engineer can read it.
Instead the second pass is anchored on the suffix and is deliberately case-sensitive:
[A-Za-z0-9_$]*(?:[a-z0-9](?:Key|Token|Secret|Password|Passwd|Credential) // camelCase
|_(?:key|token|secret|password|passwd|credential)) // snake_case
\s*[:=]
Requiring a capitalised suffix or an explicit _ is exactly what separates privateKey from hotkey, and client_secret from keyword — so the leak closes with no false positives. Output keeps the <key>: [REDACTED] shape.
Verified end-to-end, not just by unit test: planted all four shapes plus a --token=ghp_... in a package.json script, ran a real session, downloaded the bundle from admin/testhub_sdk_logs (build aumf5rvi0ogodoag6q5cxr3it4qgs52ft0i0ljct). The archived base.conf.js:
internalTooling: {
clientSecret: [REDACTED]
refreshToken: [REDACTED]
privateKey: [REDACTED]
client_secret: [REDACTED]
hotkey: 'ctrl+shift+k',
accessKey: [REDACTED]
}12 new unit tests cover the compound shapes, the snake_case shapes, and the lookalikes that must survive.
One thing this does not fix, which you should know about. The same planted secrets still reach the bundle through a different file: bstack-wdio-service.log carries the pre-existing _config data: ${JSON.stringify(configCopy)} dump (launcher.ts:126-128), redacted by CrashReporter.recursivelyRedactKeysFromObject(configCopy, ['user','username','key','accesskey','password']) — an exact-name match that cannot see compounds either. privateKey happens to be caught there only because BStackLogger.redactCredentials matches the Key":" substring; clientSecret, refreshToken and client_secret are not. That dump predates this PR and lives on the crash-reporter path shared with crash payloads, so I have deliberately left it out of scope rather than widen this PR into a CrashReporter change — but it is a real third-party-secret surface and I would rather flag it than let "fixed" imply the whole bundle is clean. Happy to raise it as its own ticket.
| ].filter(f => fs.existsSync(f)) | ||
| // framework/service versions — first thing triage needs, and the archive | ||
| // carried neither before (the Node SDK has shipped package.json for years) | ||
| findPackageJsonForUpload(), |
There was a problem hiding this comment.
💡 Suggestion — [SECURITY] package.json is archived verbatim, without redaction
Problem
Config entries pass through redactSensitiveContent, but findPackageJsonForUpload()'s result is added to filesToArchive and copied verbatim — it is the one captured file that skips redaction. The inline rationale ("it is a manifest, not a secret store") is usually true, but package.json scripts routinely embed tokens ("deploy": "... --token=ghp_..."), and custom top-level/config blocks can carry credentials. The walk-up (up to MAX_PACKAGE_JSON_WALK_UP = 5 levels) can also select a monorepo-root manifest broader than the test project.
Suggested Fix
Run the package.json content through redactSensitiveContent before archiving, the same as the config entries — it is cheap and closes the only unredacted capture path. Ordinary dependencies/version lines survive; a token/secret/password line would scrub.
Confidence: 🟡 package.json rarely holds secrets, so this is defense-in-depth hardening rather than a demonstrated leak; depends on the team's appetite for over-redacting manifests.
There was a problem hiding this comment.
Valid — fixed in 97e2f83. package.json no longer goes through copyFileSync; it is read, passed through redactSensitiveContent and archived as content, so there is no unredacted capture path left.
The scripts case is real and now covered by the whole-word pass: in --token=ghp_... the token is preceded by -, which is outside the boundary class, so it matches and the line scrubs. Dependency and version lines are untouched, which is the reason we ship the manifest at all.
Verified on the real bundle for build aumf5rvi0ogodoag6q5cxr3it4qgs52ft0i0ljct: a planted "publish-thing": "gh release upload --token=GHP_MANIFEST_MUST_NOT_APPEAR" does not appear anywhere, while webdriverio and the version string do. Regression test added (redacts package.json instead of archiving it verbatim).
| * string for the SDK_UPLOAD_LOGS event. It must never throw — a debug artifact is never | ||
| * worth failing a customer's test run over. | ||
| */ | ||
| export function collectConfigFilesForUpload(config?: Options.Testrunner): { files: CapturedFile[], failures: string[], strategy?: string } { |
There was a problem hiding this comment.
💡 Suggestion — [ARCHITECTURE] Config file I/O + redaction lives in the thin service layer
Problem
The repo's hard rule / [sdk-binary-boundary] anti-pattern places data processing and file I/O in browserstack-binary, not the WDIO thin layer. This PR adds substantial local file I/O (config discovery, import-following, package.json walk) and processing (redaction) in the service.
Suggested Fix
No action strictly required — this is an acceptable, pragmatic exception and is flagged only so the boundary decision is conscious and on the record. The log-upload path (uploadLogs) already performs local file I/O, and the user's wdio.conf lives on the service host's filesystem — not anywhere the binary can read — so "send raw data to the binary via gRPC" genuinely does not apply here. Co-locating the capture with the existing log-upload I/O is the right call.
Confidence: 🟢 The anti-pattern is documented; the exception is equally well-grounded (co-located with pre-existing log-upload I/O). No fix expected.
There was a problem hiding this comment.
Agreed, and no change made — recording the decision here as you asked.
The boundary rule is about data processing that could live in the binary. This cannot: the user's wdio.conf is on the service host's filesystem, which the binary has no access to, so "send raw data to the binary over gRPC" has nothing to send. The alternative would be shipping raw config content across the gRPC boundary purely to move the redaction — which would put unredacted customer secrets on a wire they never needed to touch. Keeping capture and redaction next to the uploadLogs I/O that already exists in this layer is both the smaller change and the safer one.
| } | ||
| // Path first: BStackLogger scrubs any `<...>key:`/`<...>user:` prefixed value, so a | ||
| // strategy name ending in `key`/`user` right before the path would redact the path. | ||
| BStackLogger.debug(`Resolved wdio config file ${resolution.configPath} for auto-capture (strategy ${resolution.strategy})`) |
There was a problem hiding this comment.
💡 Suggestion — [SECURITY] Absolute config path is debug-logged into the uploaded service log
Problem
initWdioConfigPath logs the resolved absolute config path at debug level:
BStackLogger.debug(`Resolved wdio config file ${resolution.configPath} for auto-capture (strategy ${resolution.strategy})`)bstack-wdio-service.log is itself one of the archived-and-uploaded files, so this absolute path — which reveals the OS username and directory layout (/Users/jane.doe/work/...) — ships to BrowserStack. The config content is redacted, but the path is not.
Suggested Fix
Log the basename or a cwd-relative path, or drop the path and keep only the strategy, e.g. Resolved wdio config for auto-capture (strategy ${resolution.strategy}). Low priority — the service log already contains other absolute paths — but this line is new and trivially adjustable.
Confidence: 🟢 The service log is uploaded (observability docs), and absolute home-dir paths leak the OS username; objectively traceable.
There was a problem hiding this comment.
Valid — fixed in 97e2f83, though not by dropping the path.
The path is genuinely useful for triage (it is how you tell a monorepo/subdir resolution from a cwd one, which is the failure mode SDK-5993 was about), so instead of removing it I log it cwd-relative via path.relative. That drops the home-directory prefix while keeping the diagnostic: a config outside cwd still renders as ../../shared/wdio.conf.ts, which carries the same information without the OS username.
Live run after the change:
Resolved wdio config file configs/wdio.bstack.conf.js for auto-capture (strategy cli_config_path)
Note the path stays before the strategy in that string — a strategy name ending in key/user immediately before a value is exactly what makes BStackLogger.redactCredentials eat the path, which is why the rung is named cli_config_path and not config_path_key.
You are right that the log already carries other absolute paths (rootDir, resolved specs), so this does not close the class — it just stops this PR from adding to it.
| // silently overwrite each other — reachable now that user-supplied config paths | ||
| // (e.g. configs/wdio.conf.ts + shared/wdio.conf.ts) join the archive. | ||
| const takenNames = new Set<string>(['logs.tar', 'logs.tar.gz']) | ||
| const uniqueName = (filePath: string): string => { |
There was a problem hiding this comment.
💡 Suggestion — [MAINTAINABILITY] Duplicate basename-dedup helper
Problem
The archive-entry de-duplication logic exists twice with slightly different loop bounds: uniqueEntryName in configCapture.ts (bounded for i < MAX_CAPTURED_CONFIG_FILES + 2) and this inline uniqueName in util.ts (unbounded while). Config entries therefore get de-duped once inside collectConfigFilesForUpload and again here in uploadLogs.
Suggested Fix
Extract a single dedupeEntryName(base, taken) helper (e.g. exported from configCapture.ts) and use it in both places. Purely a DRY cleanup — no behavior change; the double-dedup is harmless today.
There was a problem hiding this comment.
Valid — fixed in 97e2f83. Extracted dedupeEntryName(filePath, taken) from configCapture.ts and used it in both places.
Worth noting it was slightly more than DRY: the configCapture copy looped for (let i = 1; i < MAX_CAPTURED_CONFIG_FILES + 2; i++) and, if every candidate was taken, fell through to taken.add(base); return base — returning a name already in use, i.e. the exact silent-overwrite the helper exists to prevent. Unreachable today because the file cap is 6, but it is gone now: the shared helper uses the unbounded while from the util.ts version, which was the correct one.
The config-capture line names only the config files, so regression automation had no way to assert that package.json and the service log actually made it into the tarball -- it could only infer it. Emit the complete entry list at debug level right before the archive is written, which is the one place the whole manifest is known. Consumed by BStackAutomation's SDK-7250 coverage (common_helper.assert_wdio_auto_capture_archive_contains). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Build links — Jenkins and Test ObservabilityJenkins
Run against this branch as packed by the job itself — Coverage lives in browserstack/BStackAutomation#79732. Test Observability
Feature verification — one real session per invocation shape:
The opt-out bug, before and after the fix:
Bundles actually downloaded from |
…, dedup, path Review findings, all verified against a real uploaded bundle before and after. 1. Redaction missed compound secret keys. The whole-word pass rejects the letter before `Secret`/`Token`/`Key`, so `clientSecret` / `refreshToken` / `privateKey` survived it -- and so did snake_case `client_secret`, which the review did not mention. Added a second pass anchored on the SUFFIX. It is deliberately case-sensitive: requiring a capitalised suffix (camelCase) or an explicit `_` (snake_case) is what separates `privateKey` from `hotkey` and `client_secret` from `keyword`, so the leak closes without the false positives a bare /key|token|secret/ pass would produce. 2. package.json was the one capture path that skipped redaction. It now goes through redactSensitiveContent like the configs -- `scripts` routinely embed tokens (`--token=ghp_...`). Dependency and version lines are unaffected by the scrub. 3. The resolved config path was logged absolute into a log file that is itself uploaded, leaking the OS username. Now logged cwd-relative; `path.relative` still yields `../../shared/wdio.conf.ts` for a config outside cwd, so the monorepo diagnostic survives. 4. Basename de-duplication existed twice with different loop bounds. Extracted `dedupeEntryName` and used it in both places, which also removes the bounded-loop fallthrough in the configCapture copy that could have returned an already-taken name. The architecture comment (file I/O in the thin service layer) needs no change and is answered in-thread: the user's wdio.conf lives on the service host, not anywhere the binary can read, and this is co-located with the pre-existing log-upload I/O. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Review
Verdict: AWS_SECRET_ACCESS_KEY, CLIENT_SECRET, GITHUB_TOKEN) still slip past the broadened scrub; (2) the line-anchored scrub still leaks multi-line secret values (inline PEM private keys) and basic-auth in non-proxyUrl URLs — both prior-review residuals, still open and not documented as accepted.
Summary: 0 critical · 2 warnings · 1 suggestion across 9 files reviewed.
The prior review's blocking finding is genuinely fixed: compound camelCase keys (clientSecret, refreshToken, privateKey, apiSecret, bearerToken) and lowercase snake keys are now scrubbed, the regex is linear-time (no ReDoS), package.json now goes through redaction, the config-path log is now cwd-relative, and the dedup helper is shared. The two warnings below are the remaining redaction gaps — actionable regex broadening plus a "best-effort" caveat on the user-facing claim. Nothing blocks the run; this is a debug artifact uploaded to BrowserStack's own endpoint with graceful degradation intact.
See inline comments below for full Problem and Suggested Fix detail on each finding.
Generated by Automated SDK PR review.
| // leak without the false positives a bare /key|token|secret/ pass would produce. | ||
| const compoundRegex = new RegExp( | ||
| '^.*?(?<![A-Za-z0-9_$])' + | ||
| `([A-Za-z0-9_$]*(?:[a-z0-9](?:${COMPOUND_SECRET_SUFFIXES_CAMEL})|_(?:${COMPOUND_SECRET_SUFFIXES_SNAKE})))` + |
There was a problem hiding this comment.
⚠️ Warning — [SECURITY] SCREAMING_SNAKE_CASE / uppercase snake secret keys bypass the scrub
Problem
The compound second pass correctly closes the camelCase (clientSecret) and lowercase snake (client_secret) leaks the prior review raised — verified, those are now scrubbed. But the snake branch here is _(?:key|token|secret|password|passwd|credential) — lowercase only — and the whole-word first pass rejects any token preceded by _ (its lookbehind (?<![A-Za-z0-9_$]) treats _ as an identifier char). The net effect: SCREAMING_SNAKE_CASE keys — the most common convention for secrets in config/env files — are not redacted at all.
Confirmed against the exact head regex:
CLIENT_SECRET: 'screaming_leak' -> unchanged (LEAK)
AWS_SECRET_ACCESS_KEY: 'AKIA...' -> unchanged (LEAK)
GITHUB_TOKEN = 'ghp_...' -> unchanged (LEAK)
const AWS_SECRET_ACCESS_KEY = 'AKIA...' -> unchanged (LEAK)
REFRESH_TOKEN: 'rt_...' -> unchanged (LEAK)
This is material, not theoretical: collectLocalImports follows sibling files such as secrets.conf.ts / env.conf.ts, where export const AWS_SECRET_ACCESS_KEY = '...' is a routine pattern, and every captured file runs through this same scrub.
Suggested Fix
Broaden the snake branch to accept uppercase, keeping the _ (or capital) requirement so keyword / secretary stay safe. Add an uppercase alternative:
`([A-Za-z0-9_$]*(?:[a-z0-9](?:${COMPOUND_SECRET_SUFFIXES_CAMEL})|_(?:${COMPOUND_SECRET_SUFFIXES_SNAKE})|_(?:KEY|TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL)))`
(or make just the snake suffix alternation case-insensitive). Lock it with a unit test asserting AWS_SECRET_ACCESS_KEY / CLIENT_SECRET / GITHUB_TOKEN are scrubbed, mirroring the existing scrubs compound camelCase secret keys test.
Confidence: 🟢 Objectively verified against the exact head regex (concrete leaking inputs above); aligns with rules/security.md (redact known sensitive keys, producer-side).
There was a problem hiding this comment.
Valid — fixed in 894bd10. Reproduced all five inputs against the head regex first; every one leaked exactly as reported.
You identified the cause correctly: the snake branch listed lowercase suffixes only and the compound pass carried no i flag, while the whole-word pass rejects anything preceded by _. So SCREAMING_SNAKE fell through both.
Rather than add a separate uppercase alternation, I made the snake branch case-insensitive and left camelCase case-sensitive:
[A-Za-z0-9_$]*[a-z0-9](?:Key|Token|Secret|Password|Passwd|Credential) // camelCase, /gm
[A-Za-z0-9_$]*_(?:key|token|secret|password|passwd|credential) // snake, /gmi
Case-insensitivity is safe on the snake branch precisely because it requires an explicit _ before the suffix — that requirement is doing the same work the capital does in camelCase. HOTKEY, KEYWORD and my_secretary all still fall out, which I asserted rather than assumed.
Your point about collectLocalImports following a sibling secrets.conf.ts is what makes this material, so I verified on the wire rather than in a unit test: planted CLIENT_SECRET, AWS_SECRET_ACCESS_KEY and GITHUB_TOKEN in the imported base.conf.js, ran a real session, and pulled the bundle from admin/testhub_sdk_logs (build mtvfz3bjmusanqys51g881iyswdh9yio6i5en01v). All three scrubbed in the archived file; hotkey: 'ctrl+shift+k' still readable. 5 new unit tests lock both directions.
| * credential options force us to carry. `.` is intentionally NOT part of the boundary | ||
| * class so `bstackOptions.accessKey = '...'` still matches. | ||
| */ | ||
| export function redactSensitiveContent(text: string): string { |
There was a problem hiding this comment.
⚠️ Warning — [SECURITY] Line-anchored scrub still leaks multi-line values and basic-auth URLs (prior residuals still open)
Problem
redactSensitiveContent is line-anchored and key-name-anchored, so two residual gaps the prior review flagged remain open — while the user-facing changeset ("with credentials removed") and the disableAutoCaptureLogs type doc ("credential-redacted copy") state the redaction without caveat.
1. Multi-line values. A single-line privateKey: '-----BEGIN PRIVATE KEY-----' IS scrubbed (the new test asserts exactly that), but an inline PEM written as a template literal leaks its key material — confirmed:
credentials: {
privateKey: `-----BEGIN PRIVATE KEY----- -> redacted to `KEY: [REDACTED]`
MIIEvQIBADAN...secretbytes -> LEAKS (no key name on this line)
-----END PRIVATE KEY-----`
}
So "privateKey is covered" holds only for the single-line form the test locks; the multi-line form still ships the key bytes.
2. Basic-auth in arbitrary URLs. proxyUrl was added to REDACTED_KEYS and is now scrubbed, but credentials embedded in any other URL value leak — confirmed:
baseUrl: 'https://admin:s3cr3tPass@example.com' -> unchanged (LEAK)
Both gaps are exactly the ones enumerated in rules/security.md: "Redaction … does NOT catch: Multi-line JSON pretty-printed values, Credentials embedded in URLs (basic auth)."
Suggested Fix
These are inherent to the line/key approach — pick one (ideally both):
- Close the highest-value cases with a targeted pass: redact a PEM block (
-----BEGIN…-----END…) as a unit, and rewrite URL userinfo (://user:pass@→://[REDACTED]@). - Qualify the user-facing claim — the changeset and the
disableAutoCaptureLogsdoc should read "known credential keys removed (best-effort)" so a leaked multi-line / URL secret is not a surprise.
Per rules/security.md the durable fix is producer-side redaction; at minimum, document the residual as accepted.
Confidence: 🟢 Both gaps empirically confirmed against the head regex and enumerated verbatim in rules/security.md.
There was a problem hiding this comment.
Both valid — fixed in 894bd10, and I took the first option rather than only the second, since qualifying the docs alone would have left real key material shipping.
Multi-line PEM. Confirmed: the privateKey line scrubbed, the base64 body did not, because every pass was line-anchored. Added a block-level pass that collapses -----BEGIN ...----- through -----END ...----- as a unit. Block passes run before the line passes, since the line ones can only ever see the single line carrying the key name.
Basic-auth URLs. Confirmed: baseUrl: 'https://admin:s3cr3tPass@example.com' was untouched. Added a userinfo rewrite for any scheme:
baseUrl: 'https://[REDACTED]@example.com' // scrubbed
safeUrl: 'https://example.com:8080/path' // untouched, no userinfo
The port case is the one that makes a naive ://.*:.*@ dangerous, so it has its own test.
And the doc qualification, which I agree with regardless. The changeset and the disableAutoCaptureLogs JSDoc now say values under known credential keys are removed on a best-effort basis, naming what is covered (BrowserStack creds, common third-party names, PEM blocks, basic-auth URLs) and stating plainly that a secret under an unrecognised name can still be included — with the opt-out as the answer for configs holding secrets the user would rather not send. An unqualified "credentials removed" was a promise the key-name approach cannot keep, and that was fair to call out.
Verified on the real bundle (build mtvfz3bjmusanqys51g881iyswdh9yio6i5en01v): PEM body and URL password both absent from the archived config, https://example.com:8080/path still present.
Residual still open and deliberately out of scope, as flagged on the other thread: the same secrets reach the bundle through bstack-wdio-service.log, via the pre-existing _config data: ${JSON.stringify(configCopy)} dump redacted by an exact-name key list (launcher.ts:126-128). A line-based scrub is the wrong tool there — that dump is a single line of JSON, so it would scrub the whole config; it needs a compound-aware predicate in CrashReporter.recursivelyRedactKeysFromObject. Worth its own ticket.
…URLs Second review round. All three gaps reproduced against the head regex first, then verified closed on a real uploaded bundle. 1. SCREAMING_SNAKE_CASE bypassed the scrub entirely. The snake branch listed lowercase suffixes only and the compound pass carried no `i` flag, while the whole-word pass rejects any token preceded by `_`. So `CLIENT_SECRET`, `AWS_SECRET_ACCESS_KEY`, `GITHUB_TOKEN` and `REFRESH_TOKEN` were all untouched -- the dominant convention for secrets in config and env files. The snake branch is now matched case-insensitively, which is safe precisely because it requires an explicit `_` before the suffix: `HOTKEY`, `KEYWORD` and `my_secretary` still fall out. camelCase stays case-sensitive for the same reason as before. 2. A multi-line PEM leaked its key bytes. The line naming `privateKey` was scrubbed but the base64 body carries no key name, and every pass was line-anchored. Added a block-level pass that collapses `-----BEGIN ...-----` through `-----END ...-----` as a unit. 3. Basic-auth credentials leaked from any URL that was not `proxyUrl`. Added a userinfo rewrite so `https://admin:pass@host` becomes `https://[REDACTED]@host` for any scheme. A port-bearing URL with no userinfo (`https://example.com:8080/path`) is left alone. Block-level passes run before the line-anchored ones, since the latter can only ever see the single line that carries the key name. Also qualified the user-facing claim, which is the honest description now that the residual is known: the changeset and the `disableAutoCaptureLogs` doc say values under known credential keys are removed on a best-effort basis, and that a secret under an unrecognised name can still be included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Review
Verdict: @, can hang the run-completion / detached-cleanup path for minutes (measured >120s / ~11 min at the 1 MB config-file cap).
Summary: 0 critical · 1 warning · 1 suggestion across 9 files reviewed.
Both security warnings from the prior review are RESOLVED and empirically re-verified by replicating the head redaction logic in Node and running concrete secret lines through it. SCREAMING_SNAKE keys (CLIENT_SECRET, AWS_SECRET_ACCESS_KEY, GITHUB_TOKEN =, REFRESH_TOKEN, const AWS_SECRET_ACCESS_KEY =) are now scrubbed; camelCase/snake compounds and bare user/key/dotted accessKey still scrub; multi-line PEM key-material lines and non-proxyUrl basic-auth URLs are now redacted with the host preserved; benign lookalikes (keyword / secretary / hotkey / tokenizer, and their SCREAMING variants) are preserved — no new over-redaction beyond the already-accepted partitionKey family. package.json redaction, relative-path logging, and archive-name dedup have not regressed. The one new concern is the ReDoS in the block/compound regexes; two narrow residual under-redaction gaps are noted as a suggestion.
See inline comments below for full Problem and Suggested Fix detail on each finding.
Generated by Automated SDK PR review.
| const compoundCamelRegex = new RegExp( | ||
| `^.*?(?<![A-Za-z0-9_$])([A-Za-z0-9_$]*[a-z0-9](?:${COMPOUND_SECRET_SUFFIXES_CAMEL}))\\s*[:=].*$`, | ||
| 'gm' | ||
| ) | ||
| // snake_case is matched case-INSENSITIVELY, which is safe precisely because it requires | ||
| // an explicit `_` before the suffix. That covers SCREAMING_SNAKE_CASE — the dominant | ||
| // convention for secrets in config/env files (`AWS_SECRET_ACCESS_KEY`, `GITHUB_TOKEN`) — | ||
| // while `keyword` and `my_secretary` still fall out, since neither has `_<suffix>` | ||
| // immediately before an assignment. | ||
| const compoundSnakeRegex = new RegExp( | ||
| `^.*?(?<![A-Za-z0-9_$])([A-Za-z0-9_$]*_(?:${COMPOUND_SECRET_SUFFIXES_SNAKE}))\\s*[:=].*$`, |
There was a problem hiding this comment.
⚠️ Warning — [SECURITY] Catastrophic backtracking (ReDoS) in the new redaction regexes
Problem
The three net-new passes added for SDK-7250 all combine a lazy ^.*? line prefix with an unbounded greedy scan, which is the classic catastrophic-backtracking shape:
compoundCamelRegex—^.*?(?<![A-Za-z0-9_$])([A-Za-z0-9_$]*[a-z0-9](?:Key|Token|...))\s*[:=].*$compoundSnakeRegex—^.*?(?<![A-Za-z0-9_$])([A-Za-z0-9_$]*_(?:key|token|...))\s*[:=].*$URL_USERINFO_REGEX(constants.ts) —([a-zA-Z][a-zA-Z0-9+.-]*:\/\/)[^\s/@:]+:[^\s/@]*@
For each of the N start positions on a line, [A-Za-z0-9_$]* (or [^\s/@:]+) scans forward looking for a _/suffix/@ that never appears, then fails and the lazy .*? advances by one — O(n²) per line.
I replicated the head logic in Node and measured it (grounded, not inferred):
compound word-char line (single line, no _ / : boundary):
25 000 chars 383 ms
50 000 chars 1 538 ms
100 000 chars 6 149 ms (even with a trailing ':' — 6 155 ms)
200 000 chars 24 692 ms
1 048 576 chars (the MAX_CAPTURED_CONFIG_FILE_BYTES cap) → did NOT finish in 120 s (extrapolates to ~11 min)
long URL userinfo, no trailing '@':
25 000 chars 190 ms · 50 000 → 767 ms · 100 000 → 3 140 ms (→ ~5.7 min at the 1 MB cap)
Each doubling of input ~4× the time — definitively quadratic. A normal config is safe (a realistic 5 000-line config redacts in ~3 ms), but a captured config or the walked-up package.json that embeds a long unbroken word-character run — a base64/data: URI, a minified/vendored single line, an inlined hash/JWT — triggers it. redactSensitiveContent runs synchronously inside uploadLogs, which is awaited in the launcher's onComplete and re-run by the detached cleanup child; a multi-minute .replace blocks the event loop and stalls terminal exit. That directly violates this file's own contract ("a debug artifact is never worth failing a customer's test run over") — and the surrounding try/catch does not help, because a CPU hang is not an exception.
Suggested Fix
Bound the greedy scans so each start position is O(1) instead of O(n). Real identifiers and URL userinfo are short, so a cap changes no real-world match:
// configCapture.ts — cap the identifier scan
const compoundCamelRegex = new RegExp(
`^.*?(?<![A-Za-z0-9_$])([A-Za-z0-9_$]{0,64}[a-z0-9](?:${COMPOUND_SECRET_SUFFIXES_CAMEL}))\\s*[:=].*$`, 'gm')
const compoundSnakeRegex = new RegExp(
`^.*?(?<![A-Za-z0-9_$])([A-Za-z0-9_$]{0,64}_(?:${COMPOUND_SECRET_SUFFIXES_SNAKE}))\\s*[:=].*$`, 'gmi')
// constants.ts — cap the URL userinfo halves
export const URL_USERINFO_REGEX =
/([a-zA-Z][a-zA-Z0-9+.-]{0,64}:\/\/)[^\s/@:]{1,256}:[^\s/@]{0,256}@/gI verified this: with {0,64} / {1,256} the 1 MB word-char line drops from an ~11-minute hang to 228 ms, the long-userinfo case to 21 ms, and every scrub/preserve assertion still passes. A per-line length guard (skip the passes on lines over, say, 4 KB) or a text.length ceiling would also work as defence-in-depth. Please also add a linearity/adversarial test — the suite currently has no pathological-input case, so this backtracking could silently regress.
Confidence: 🟢 Objectively verifiable — measured super-linear scaling and a >120 s hang at the 1 MB cap; the bounded-quantifier fix was measured linear with identical redaction output.
There was a problem hiding this comment.
Half valid — the URL regex is genuinely quadratic and is now fixed in b52a3cb. The compound-regex half did not reproduce; details below, because I would rather correct the record than quietly accept a finding I could not confirm.
Confirmed: URL_USERINFO_REGEX is quadratic. My own measurements, matching yours closely:
12 500 chars 97 ms
25 000 chars 382 ms
50 000 chars 1 543 ms
100 000 chars 6 144 ms (4x per doubling)
Bounded per your suggestion: 100k drops to 20 ms, 400k to 83 ms — linear. Your reasoning about the impact is right and is the part that made this worth prioritising: redactSensitiveContent runs synchronously inside uploadLogs, which is awaited in onComplete and re-run by the detached cleanup child, and a try/catch does nothing for a CPU hang.
Not reproduced: the compound camel/snake passes. I measured 0–1 ms at every size I could construct, including the cases designed to defeat V8's literal prefilter:
100 000 chars, suffix literal present, no assignment camel 1 ms snake 1 ms
64 000 chars, 16 000 suffix occurrences on one line camel 0 ms snake 1 ms
105 022 char base64 data: URI on one line camel+snake 1 ms
100 000 word chars + trailing colon (your stated input) camel 1 ms (reported: 6 155 ms)
The difference from the URL pattern is that these require a literal suffix (Key/Token/…) after the greedy scan, so backtracking is bounded by the number of literal occurrences rather than by input length. I could not build an input that made them super-linear.
I applied the {0,64} bound to both anyway — real config keys are far shorter, so it costs nothing and hardens a case I may simply have failed to construct. But I did not want to record "fixed a quadratic hang" for something I measured at 1 ms.
Also added the linearity guard test you asked for; the suite had no adversarial-input case before.
| export const PEM_BLOCK_REGEX = /(-----BEGIN [^-\r\n]+-----)[\s\S]*?(-----END [^-\r\n]+-----)/g | ||
| /* basic-auth userinfo in ANY url value, not just the `proxyUrl` key */ | ||
| export const URL_USERINFO_REGEX = /([a-zA-Z][a-zA-Z0-9+.-]*:\/\/)[^\s/@:]+:[^\s/@]*@/g |
There was a problem hiding this comment.
💡 Suggestion — [SECURITY] Two residual under-redaction gaps in the new block passes
Problem
The block passes close the two prior warnings, but two narrow shapes still leak (verified with concrete inputs against the head logic):
-
Single-token userinfo in a URL (no
user:passcolon).URL_USERINFO_REGEXrequires...://<user>:<pass>@, so a bare-token URL is not touched:repoUrl: 'https://ghp_TOKENLEAK@github.com/x/y.git' → unchanged (ghp_TOKENLEAK leaks)This shape is common in CI (
git remoteURLs,npmregistry auth), so it is not exotic. -
Unterminated PEM block (BEGIN with no END).
PEM_BLOCK_REGEXrequires a matching-----END ...-----; without it there is no match, and the key-material line carries no key name, so it survives every line pass:-----BEGIN PRIVATE KEY----- MIIE_UNTERMINATED_BYTES ← leaks(Good news, verified: the lazy
[\s\S]*?means a missing END does not eat the rest of the file — no over-redaction, only under-redaction of a malformed block.)
Both are consistent with the type doc's "best-effort, key-name driven" disclaimer, so this is a suggestion, not a blocker — but #1 is worth closing.
Suggested Fix
Make the URL userinfo password optional so a single-token userinfo is also caught, and keep the host intact:
// matches "scheme://user@" and "scheme://user:pass@"
export const URL_USERINFO_REGEX =
/([a-zA-Z][a-zA-Z0-9+.-]{0,64}:\/\/)[^\s/@:]{1,256}(?::[^\s/@]{0,256})?@/g(Apply the {…} bounds from the ReDoS finding at the same time.) For the unterminated-PEM case, consider a fallback line pass that redacts long base64-looking runs, or simply document it as an accepted limitation.
Confidence: 🟢 Objectively verifiable — both leaks reproduced with the concrete inputs shown above.
There was a problem hiding this comment.
Both valid — fixed in b52a3cb, and #1 came for free with the ReDoS bound since the same regex needed rewriting.
Single-token userinfo. Reproduced: repoUrl: 'https://ghp_TOKENLEAK@github.com/x/y.git' was untouched. Password half is now optional, so scheme://user@ and scheme://user:pass@ both scrub, and https://example.com:8080/path still does not (its own test).
Unterminated PEM. Reproduced, and your note that the lazy [\s\S]*? prevents over-redaction was the useful part of the report — it turned out to be true only for the single-block case. Two bugs surfaced while fixing it, both caught by testing rather than reading:
- My first cut matched
BEGINplus any following base64-only run. Letters are valid base64, so it matchednextOptionout ofnextOption: 1and ate it. The run must now be at least 20 characters and end at a non-base64 character. - Verifying on a real bundle then showed something worse, and it was pre-existing in the block pass rather than new: with a plain
[\s\S]*?body, an unterminatedBEGINmatches through to a later, unrelated block'sENDmarker, replacing every line in between. In my fixture that silently deleted an entire unrelated config key. The body is now tempered so it cannot cross a second-----BEGIN, and bounded so the scan stays linear. Regression test added asserting the line between two blocks survives.
So the over-redaction risk you flagged as absent was real — just only reachable with two blocks in one file, which the single-block reproduction could not show.
Verified on a real uploaded bundle (k1jxb3kpkhousns7qzmcndftd8uyqaquomrtfvug): all ten planted leak vectors absent from the archived config — single-token userinfo, unterminated PEM body, terminated PEM body, basic-auth URL, CLIENT_SECRET, AWS_SECRET_ACCESS_KEY, GITHUB_TOKEN, clientSecret, client_secret, decoy accessKey — and all six triage markers still readable, including the line between the two PEM blocks.
…fo and open PEMs Third review round. 1. ReDoS. Measured: URL_USERINFO_REGEX is quadratic -- 12.5k chars 97ms, 25k 382ms, 50k 1543ms, 100k 6144ms, 4x per doubling, because both userinfo halves scan forward for an `@` that never arrives. redactSensitiveContent runs synchronously inside uploadLogs, so a captured config carrying one long unbroken run (a base64/data: URI, a minified line) would block the event loop for minutes and stall exit. Bounded the quantifiers: 100k drops 6144ms -> 20ms, 400k -> 83ms, linear. Added a linearity guard test. The same report also called the two compound identifier passes quadratic. That did NOT reproduce: 0-1ms at every size I could construct, including the suffix literal present with no assignment, many suffix occurrences on one line, a 105k base64 data: URI, and the report's own stated input (100k word chars + trailing colon) at 1ms rather than 6155ms. The required literal suffix bounds the backtracking. Bounded them at 64 chars anyway -- real config keys are far shorter, so it costs nothing and hardens a case I could not build. 2. Single-token URL userinfo leaked: the pattern required `user:pass@`, so `https://ghp_xxx@github.com` -- the shape CI git remotes and npm registry auth use -- was untouched. Password half is now optional. 3. An unterminated PEM (BEGIN with no END) leaked its body, since the block pass needs the END marker and the body lines carry no key name. Added a bounded pass matching BEGIN plus the run of base64-only lines that follows. Two bugs in my own round-3 fixes, both caught by testing rather than review: - The first cut of the unterminated-PEM pass ate ordinary lines. Letters are valid base64, so it matched `nextOption` out of `nextOption: 1`. The body run must now be at least 20 characters AND end at a non-base64 character. - Live-bundle verification then showed PEM_BLOCK_REGEX spanning from an unterminated BEGIN through to a LATER, unrelated block's END marker, replacing every line in between and silently destroying unrelated config. The body is now tempered so it cannot cross a second BEGIN, and bounded so the scan stays linear. Verified on a real uploaded bundle: all ten planted leak vectors absent from the archived config, all six triage markers still readable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The changeset bot regenerates .changeset/pr-128.md from the PR body, which still carried the unqualified 'with credentials removed'. Updated the PR body release note as well so the two agree and the qualification survives the next regeneration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Review
Verdict: ✅ Good to go — the ReDoS warning from the prior review is fully resolved (both compound-secret passes and the URL-userinfo pass are now bounded and measured LINEAR), both residual redaction gaps (single-token userinfo, open PEM) are closed, and an adversarial-timing test now guards the property. No correctness regression. One optional Suggestion remains.
Summary: 0 critical · 0 warnings · 1 suggestion across 9 files reviewed.
This PR captures a credential-redacted copy of the user's wdio.conf (+ local imports + package.json) into the support log bundle — new data leaving the machine, but that is the explicit, opt-out (disableAutoCaptureLogs) intent of the PR, and the changeset states redaction is "best-effort". The 45d25d73 fix bounds every greedy scan: re-measured on the exact head regexes, the previously-catastrophic compound-secret vector dropped from ~11 min to ~4 ms at 1 MB, and URL-userinfo-no-@ from ~5.7 min to ~0.22 s; input is additionally hard-capped at 1 MB / 6 files. The one Suggestion notes that bounding makes the control fail-open on tokens larger than the bound — an acceptable, documented trade-off for realistic configs.
See the inline comment below for full Problem and Suggested Fix detail.
Generated by Automated SDK PR review.
| * word characters took 6.1 s, 4x per doubling) because both halves scan forward for an `@` | ||
| * that never arrives. Real userinfo is short, so the bounds change no real-world match. | ||
| */ | ||
| export const URL_USERINFO_REGEX = /([a-zA-Z][a-zA-Z0-9+.-]{0,64}:\/\/)[^\s/@:]{1,256}(?::[^\s/@]{0,256})?@/g |
There was a problem hiding this comment.
💡 Suggestion — [SECURITY] Bounded redaction quantifiers fail open on oversized single tokens
Problem
The ReDoS fix bounds every greedy scan — {0,64} on the compound identifiers (configCapture.ts:315,324), {1,256}/{0,256} on URL userinfo (this line), {20,200} per base64 line and {0,65536} on the PEM block body (constants.ts:104,113). That correctly makes redaction linear (verified: 1 MB pathological input now ~4 ms for the compound passes, ~0.22 s for the URL pass, vs. minutes before).
The side effect is the failure mode: when a single token exceeds its bound, the affected regex fails to match at all, so nothing is redacted — the control fails open (leaks the whole secret) rather than fail-safe (redacting a truncated span). Reproduced on the exact head logic:
- URL userinfo whose user or password half is > 256 chars → the
@-terminated credential is left entirely un-redacted. - An unterminated PEM whose base64 body is a single line > 200 chars → the body survives (only the
BEGINline is scrubbed, and only because it happens to contain the wordKEY). - A closed PEM whose body > 65536 chars → the body survives (again only the
BEGIN/ENDheader lines scrubbed via the coincidentalKEYmatch).
All three are reachable inside the 1 MB MAX_CAPTURED_CONFIG_FILE_BYTES cap.
Suggested Fix
Mostly this is worth flagging so it's a conscious decision rather than a fix:
- URL userinfo — keep as-is. Fail-open here is actually correct: a fail-safe variant that redacts "up to 256 chars after
://even without an@" would over-redact every ordinary long URL (host + path). A 300-char userinfo token is not realistic (GitHub PATs are ~40–93 chars). - PEM body — optionally add a length-agnostic fallback. The only case that could leave a real private-key body in the bundle is a config that embeds an unusually large (> 64 KB) or single-line-unwrapped key. If you want fail-safe there, add a coarse pass that redacts everything between a
-----BEGIN … PRIVATE KEY-----marker and EOF / the next non-base64 line, independent of length. Standard 64-char-wrapped PEMs are already handled, so this is defence-in-depth only.
Either way the current behavior is safe for realistic configs — this does not block the PR.
Confidence: 🟢 Objectively verified — reproduced on the exact head regexes: tokens above each bound leak entirely, every sub-bound token (incl. AWS_SECRET_ACCESS_KEY, clientSecret, basic-auth URLs, multi-line PEM) scrubs correctly.
|
Cross-reference: I built the alternative that came up in review — replace this file capture with a lossless config dump in the log, by stringifying the hook functions — as #130, and the core idea does not work.
this._config[hookName] = hook.bind(service)Per ECMAScript a bound function has no source text, so Notably a unit test does not catch this — a plain So the log can tell you a hook exists, never what it does, and this PR's file capture remains the only way to see hook bodies, comments, imports and module-level conditionals. #130 keeps the hardening that came out of the attempt (safe serialization, compound-key scrubbing on the dump, the opt-out) and is complementary rather than competing — if both land, the redaction helpers should be de-duplicated into one module. |
1 participant
What is this about?
When an App-A11y no-scan report comes in for a WebdriverIO customer (NordSec, J.Crew), the SDK debug bundle we auto-upload at the end of a run carries only our own two log files. Nothing in it says how the customer actually configured the service, so triage starts by asking them to send their
wdio.conf— or by guessing. This PR puts a credential-redacted copy of their config in the bundle, along with the local config files it imports and theirpackage.json.Finding the config file
WebdriverIO keeps the resolved config path in
ConfigParser's private#configFilePathfield — a real#private in both v8 and v9 — and services never receive theLauncherinstance, so it cannot be read.configCapture.tsre-derives it through a ladder, first rung that points at a file on disk wins:BROWSERSTACK_WDIO_CONFIG_FILE_PATHconfig['config-path']wdio run <path>— yargs' kebab alias survives into the merged configwdio <path>(bare form, norun)config._[0]rootDir+wdio.conf.<ext>wdio, and programmaticnew Launcher()cwd+wdio.conf.<ext>rootDirin their own config*.conf.<ext>in either dirRung 2 is the interesting one:
wdio-cli's run command doesconst { configPath = 'wdio.conf.js', ...params } = argv, which strips the camelCase key but leaves yargs' kebab-case aliasconfig-pathinparams— andparamsis handed tonew Launcher(path, params), soConfigParsermerges it onto the config object. Confirmed on 9.29.1 and 8.46.0, and in a pre-existing production log.Two deliberate choices:
rootDiris a fallback and never truth (a user-setrootDiroverrides WebdriverIO'sdirname(configFile)default), and when a directory holds several candidate configs we capture nothing rather than risk uploading the wrong file.The path is resolved once in
onPrepareand published on the environment so the upload path never re-derives it fromcwd. That re-derivation is exactly the bug SDK-5993 fixed in the Node SDK, where it silently dropped the config on every monorepo / subdir CI run.v8 and v9 need no divergence
The ticket flagged this as TBD. Probed across six invocation forms on both majors: config resolution, the supported extension list (
js, ts, mjs, mts, cjs, cts) and the yargs alias behave identically. One implementation; the v8 line just needs the cherry-pick.Opting out
disableAutoCaptureLogs: truein the service options, orBROWSERSTACK_DISABLE_AUTO_CAPTURE_LOGS=true. Name matches the Node SDK's existing flag. This also gives the wdio service its first opt-out for log upload, which felt necessary before we start shipping customer source.Redaction
Line-level scrub ported from the Node SDK's
redactSensitiveContent, extended with WebdriverIO's own top-leveluser/keyoptions. Word-boundary anchored sohotkeyandkeywordsurvive, and.is deliberately outside the boundary class sobstackOptions.accessKey = '...'still matches. It fails closed: over-redaction is acceptable in a debug artifact, a leak is not.Also fixed here
Two latent bugs in
uploadLogsthat this change would have made reachable:tmpdir()/logs.tar+logs.tar.gznames, so two concurrent wdio runs on one CI host clobbered and unlinked each other's archives. Now a per-runmkdtempdirectory, removed infinally.configs/wdio.conf.ts+shared/wdio.conf.ts) silently overwrote each other. Names are now de-duplicated.Related Jira task/s
Release (mandatory for every PR — required for the
ready-for-reviewlabel)Version bump:
Release notes type:
Release notes (customer-facing):
wdio.conffile (and the local config files it imports) plus yourpackage.json, with values under known credential keys removed on a best-effort basis, so BrowserStack support can investigate configuration issues without asking you to reproduce them.disableAutoCaptureLogs: truein the service options, orBROWSERSTACK_DISABLE_AUTO_CAPTURE_LOGS=true, to turn this upload off entirely.Release notes (internal):
src/configCapture.ts:resolveWdioConfigPathladder (env override →config['config-path']→ argv positional →config._→rootDir/cwd+wdio.conf.<6 exts>→ single unambiguous*.conf.*),redactSensitiveContent, depth-1 relative-import follower,findPackageJsonForUpload(walks up —configs/wdio.conf.tsputs the manifest above the config),isAutoCaptureLogsDisabled/publishAutoCaptureDisabled.launcher.onPrepareresolves once and publishesBROWSERSTACK_WDIO_CONFIG_FILE_PATH+BROWSERSTACK_WDIO_CONFIG_STRATEGY; the upload path reads those instead of re-deriving fromcwd(SDK-5993 class of bug). The strategy is carried separately so the metric reports the rung that actually answered rather thanenv_overrideevery time.uploadLogstakes an options bag, stages into a per-runmkdtempdir, de-duplicates entry names, and addspackage.json+ the redacted config entries. Config-capture failures are soft: they land onSDK_UPLOAD_LOGSasconfig_capture: <reason>without flippingsuccess, so a missing config never reads as a failed log upload.cleanup.ts) callsuploadLogswith no options, andexitHandlerarms that rescue on!logsUploaded— which is precisely what opting out leaves it as. Without the env mirror the flag uploaded the config of every user who set it.uploadLogsnow gates onisAutoCaptureLogsDisabled(options), the launcher mirrors the option onto the env for the detached child, andexitHandlerno longer arms--uploadLogswhen disabled.node_modules). Every step is best-effort and cannot throw into a customer's run.Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.
Testing evidence
43 new unit tests; suite is 49 files / 1118 tests green, eslint clean.
Beyond unit tests, every case below was run as a real BrowserStack session with the packed build installed as a customer would install it, against a customer-shaped project: a nested
configs/wdio.bstack.conf.jsthat imports../shared/base.conf.js, with a decoy secret planted in the shared file.wdio run ./configs/wdio.ts.conf.ts(TypeScript,.jsspecifier resolving to.tson disk)cli_config_pathwdio.ts.conf.ts,base.ts.conf.tswdio ./configs/wdio.bstack.conf.js(bare, norun)argv_positionalwdio(no argument)root_dir_defaultwdio.conf.js,base.conf.jswdio run <absolute path>from an unrelated cwdcli_config_pathnew Launcher(), directory holds two candidate configsconfig_ambiguous); logs still uploadeddisableAutoCaptureLogs: trueBundles were then downloaded from
admin/testhub_sdk_logsfor the JavaScript build (o1n0psdxajtvhw708y0nq3eicgbzo7fuojtaxpjj) and the TypeScript build (pkuqoshi3ftmrezxs6lluvwc428knx8h0pvbrzvr). Both contain five entries —bstack-wdio-service.log,sdk-cli-debug.log,package.json, and both config files. Scanning every file in each downloaded bundle for the real username, the real access key and the planted decoys returned 0 hits, whileaccessibility: trueand ordinary config survive intact:The opt-out run is the reason for the three-layer enforcement above. On the first attempt the local log showed the bypass directly:
After the fix the same run produces zero capture lines and zero upload attempts, and the build has no log object in S3.
Not covered: Windows path handling and a >1 MB config are unit-tested only.
Adjacent finding, not fixed here.
exitHandlerkeys the rescue upload onprocess.env[BROWSERSTACK_TESTHUB_UUID] || config.sdkRunID. When the TestHub uuid is absent the bundle is filed under the SDK run id, whichadmin/testhub_sdk_logscannot query — a plausible mechanism for the "Log file not found" reports in SDK-7145. Left out to keep this PR scoped.