Feat/sdk 7250 capture wdio conf - #147
Merged
rahulpsq merged 15 commits intoAug 14, 2026
Merged
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>
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>
…, 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>
…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>
…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>
…sure notice Fourth review round. Five findings, all reproduced before fixing. 1. The capture manifest never reached the archive. The service log is snapshotted into the staging dir before the manifest and capture lines are written, so everything logged after that copy stayed only in the local file. Confirmed on a controlled single run: the archived log was 2 178 bytes shorter than the local one and contained neither line. Support downloading a bundle saw no manifest, no resolution strategy and no capture failures. The manifest is now its own archive entry (capture-manifest.txt) carrying the entry list, the strategy, the captured config names, the package.json location and any failures -- inside the tarball by construction rather than by ordering luck. Worth noting this also invalidated a regression test: the BStackAutomation coverage asserts the manifest by reading the LOCAL log, so it passed while the archive lacked it. 2. Acronym-prefixed camelCase keys leaked: APIToken, JWTSecret, SSHKey, AWSSecret, OTPKey. The camel core required a lowercase char immediately before the suffix, so an uppercase acronym failed it, and the whole-word pass rejected `Token` for the same preceding `I`. That guard was never load-bearing -- the alternation is already case-sensitive, so `hotkey` can never match it regardless of what precedes. Relaxed the core; the full over-redaction corpus (hotkey, HOTKEY, keyword, monkeypatch, tokenizer, secretary, donkey) still survives. 3. No runtime disclosure. The Node SDK prints AUTOLOGCAPTURE_NOTIFICATION when auto-capture is active; a wdio customer got only a changeset entry and a JSDoc comment for a strictly broader capture. Added the equivalent info line naming what is collected and how to disable. 4. "Auto-captured 1 config file(s) via undefined: package.json" -- package.json was appended to the config list before the count was taken, and strategy is undefined on every failure path, so the line claimed success in exactly the case where no config was captured. Config files and package.json are now logged separately, and the strategy clause is dropped when absent. 5. Imported configs were read from disk twice: once to seed the import frontier, discarded, then again to archive. Beyond the wasted read the two could disagree if a file changed in between. collectLocalImports now returns the content it already has. Also raised the unterminated-PEM per-line bound from 200 to 8192 chars. The bounded quantifiers fail OPEN, so a key written unwrapped on a single line exceeded the bound, matched nothing and shipped. The URL userinfo bound is deliberately left as-is -- see the review reply. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…harmful It looked like a free extra signal, but `config._` is derived from the same argv the scan above already reads, minus the scan's guard that skips a flag's value. So it can only ever differ by taking something the scan correctly rejected. Proven: with `wdio --spec ./a.js` and no config positional, that rung resolves the SPEC file as the config; without it the resolver correctly falls through to wdio.conf.js. Regression test added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…anifest paths relative
Review round 5. Both findings valid; the first also reverses my previous commit.
1. Config resolution now mirrors the CLI: one candidate, one stem, six extensions, no search.
Verified with real yargs and wdio's own option declarations (spec: {type:'array'},
watch: {type:'boolean'}) that my last commit was wrong. I removed the `config._` rung after
"proving" it would resolve a spec file, but I had hand-built `{_: ['./a.js']}` -- an input
yargs cannot produce, because a declared array option consumes its value and leaves `_`
EMPTY. The rung was never reachable that way.
The inverse is real: `wdio --watch ./configs/a.conf.ts` leaves the config in `_[0]`, and the
raw-argv scan skipped it by its own "previous token is a flag" rule. Confirmed against a
live wdio run -- that invocation now resolves as config_positional, where at head it fell
through to a directory guess. `config._` is the post-yargs value the CLI itself trusts;
scanning raw argv re-implements yargs with a heuristic that cannot know which flags are
boolean.
Also fixes a case none of the old rungs could reach: the CLI hands Launcher the probed path
but leaves `config-path` as the user's spelling, so a TS project legally carries
`configs/a.conf.js` there while `configs/a.conf.ts` is on disk. Confirmed live -- wdio starts
fine and reports the non-existent spelling. Stem-probing the CLI value resolves it
deterministically, which is what makes the directory-scan rung unnecessary rather than
load-bearing.
Removed: isReadableFile-based resolveCandidate, probeConfigBasename, scanForSingleConfig,
scanArgvForConfig and the ladder body. 347 -> 296 code lines. The `argv_positional` and
`single_conf_scan` strategies and the `config_ambiguous` reason are gone.
One test changed meaning rather than breaking: util.test.ts asserted no failure string on
upload, which only held because the old directory scan was matching vitest.config.ts in the
test cwd. It now correctly reports the soft `config_capture: config_not_found`, with success
still true.
2. The capture manifest emitted an absolute project directory, reversing the decision made
earlier in this PR to keep uploaded paths cwd-relative. relativeToCwd is now exported and
used for both the manifest field and the debug line.
Re-ran the live invocation matrix as asked: run form -> cli_config_path, bare form ->
config_positional, no-arg -> root_dir_default, --watch form -> config_positional.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ts folder name relativeToCwd is `path.relative(...) || path.basename(...)`, and `path.relative(cwd, cwd)` is ''. That fallback was written for FILE paths, where the result is never empty. The manifest passes a DIRECTORY, so the common case -- a package.json at the project root -- hit the fallback and printed the folder name: manifest at cwd -> "package.json: my-e2e-project" project checked out in $HOME -> "package.json: jane.doe" <- OS username, in the tarball manifest above cwd -> "package.json: ../.." <- correct Row 2 is the same exposure the relative-path handling exists to prevent, and row 1 silently reports a folder name where the reader expects ".". Added relativeDirToCwd, which renders empty as "." because for a directory empty MEANS cwd, and used it at both call sites. relativeToCwd keeps its file semantics, with a note not to pass a directory to it. Live-verified: the debug line now reads "Auto-captured package.json from ." with the cwd basename absent. Also rewrote the "leaves no staging directory behind" test, which was flaky for a reason unrelated to this review: it diffed a listing of os.tmpdir(), and vitest runs test FILES in parallel workers where util.test.ts also calls uploadLogs, so it raced against staging dirs another worker was creating and removing. It now spies on mkdtempSync and asserts the specific directories that this call created are gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The import follower resolved any relative specifier landing on a supported extension, with no
name filter. So a config doing `import { helper } from './helpers/utils.js'` had that module
captured and uploaded too -- ordinary application source, not configuration. Confirmed on a real
run before this change: the archive carried `utils.js`.
Capturing the split-config case is the point of following imports at all; shipping a customer's
application modules is not. Imports are now followed only when the resolved file is named like a
config (`*.conf.*` / `*.config.*`), which keeps `base.conf.js` and `wdio.shared.conf.ts` and
drops everything else. Verified on the same fixture, with the entry config still importing the
helper:
Auto-captured 2 config file(s) via cli_config_path: wdio.bstack.conf.js, base.conf.js
archive entries: bstack-wdio-service.log, sdk-cli-debug.log, wdio.bstack.conf.js,
base.conf.js, package.json, capture-manifest.txt
This also makes the user-facing wording true as written: the changeset, the JSDoc and the
runtime notice all say "the local config files it imports", which was inaccurate until now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the shipped log The manifest existed only because the capture lines never reached the archive: the service log was copyFileSync'd into the staging dir before those lines were written, so they lived only in the developer's local file. Rather than ship a second file, take the reviewer's first suggested option and fix the ordering. Ordering alone was not sufficient, which a downloaded bundle proved: BStackLogger writes through an async fs.WriteStream, so the summary lines were still buffered when the copy ran and the shipped log was silently truncated. Adds BStackLogger.flushLogFile() -- a zero-length write whose callback fires after every queued chunk reaches the fs layer, draining the buffer without ending the stream, and bounded by a timeout so a stuck stream can never hold up the upload. The trailing "archive entries" line stays local-only by design: it lists what actually landed, and the log file is itself one of those entries. Verified end-to-end on build xbpsytnxggzx6kp03mjt0j4zptx4hs9ece2kocv7 -- the downloaded bundle carries no capture-manifest.txt, and its bstack-wdio-service.log contains the strategy, the captured config names and the package.json origin. 0 credential hits across every file in the bundle. Tests: both fixes are independently mutation-checked (removing either the flush or the reorder fails the suite), plus direct tests for flushLogFile. 1149 tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rahulpsq
requested review from
Bhargavi-BS and
xxshubhamxx
and removed request for
a team
August 14, 2026 14:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this about?
Related Jira task/s
Release (mandatory for every PR — required for the
ready-for-reviewlabel)Version bump: (required — tick exactly one)
Release notes type: (optional)
Release notes (customer-facing): (optional but encouraged)
Release notes (internal): (required — engineer-facing; what actually changed / why)
Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.