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 15 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
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
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
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
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.
There was a problem hiding this comment.
Valid, and thank you for framing it as a conscious-decision item — I have taken your recommendation on both halves, one as a fix and one as a deliberate no-change.
URL userinfo — kept as-is, deliberately. Reproduced: a userinfo half over 256 chars leaks entirely. But your reasoning for leaving it is right and I would not want to "fix" it: a fail-safe variant that redacts up to N characters after :// without requiring the @ would over-redact every ordinary long URL. Real credentials are far below the bound (GitHub PATs ~40–93 chars), so the bound only fails open on inputs that are not credentials in practice.
Unterminated PEM — fixed in 5ebc318. This one I did not want to leave. Reproduced: an unterminated block whose base64 body is a single unwrapped line over 200 chars survives, and a real private key written unwrapped is entirely plausible. Raised the per-line bound to 8 KB, which covers any realistic key while staying bounded and linear. Test added with a 3 000-char single-line body.
Closed PEM over 65 536 chars — could not reproduce. A closed block with a ~78 KB body still redacts correctly on the head regexes, because the lazy tempered body matches to the first -----END. If you have an input where it leaks I will happily take it, but I could not construct one.
Net effect: the only remaining fail-open case is URL userinfo above 256 chars, which is a deliberate trade rather than an oversight.
|
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. |
| // changes no match; it is defence-in-depth against backtracking on a pathological line | ||
| // (a minified/base64 run), keeping every start position O(1) instead of O(n). | ||
| const compoundCamelRegex = new RegExp( | ||
| `^.*?(?<![A-Za-z0-9_$])([A-Za-z0-9_$]{0,64}[a-z0-9](?:${COMPOUND_SECRET_SUFFIXES_CAMEL}))\\s*[:=].*$`, |
There was a problem hiding this comment.
Acronym-prefixed camelCase secret keys are still archived unredacted — the same family as the SCREAMING_SNAKE gap fixed in 894bd10, and a common shape in real configs.
Evidence — measured against the head regexes (whole-word pass + both compound passes, in order):
LEAKS | APIToken: 'LEAK_1' -> APIToken: 'LEAK_1'
LEAKS | JWTSecret: 'LEAK_2' -> JWTSecret: 'LEAK_2'
LEAKS | SSHKey: 'LEAK_3' -> SSHKey: 'LEAK_3'
LEAKS | AWSSecret: 'LEAK_4' -> AWSSecret: 'LEAK_4'
LEAKS | OTPKey = 'LEAK_5' -> OTPKey = 'LEAK_5'
Both passes miss them for the same reason from opposite sides: the camel core requires [a-z0-9] immediately before the suffix, so the uppercase I in APIToken fails it — and the whole-word pass separately rejects Token because its lookbehind sees the preceding I. dbPassword / clientSecret match only because their pre-suffix char happens to be lowercase.
Fix — the [a-z0-9] guard isn't load-bearing. It exists to separate privateKey from hotkey, but the camel alternation is already case-sensitive (Key, not key), so hotkey can never match this pass regardless of what precedes the suffix. Relaxing the core closes all five:
const compoundCamelRegex = new RegExp(
`^.*?(?<![A-Za-z0-9_$])([A-Za-z0-9_$]{1,64}(?:${COMPOUND_SECRET_SUFFIXES_CAMEL}))\\s*[:=].*$`,
'gm'
)Verified against the over-redaction corpus this file already tests — hotkey, keyword, monkeypatch, tokenizer, secretary, HOTKEY (plus donkey) are all still kept with the relaxed core, so it closes the leak without new false positives.
There was a problem hiding this comment.
Valid — fixed in 5ebc318. Reproduced all five against the head regexes first; every one leaked.
Your analysis of the cause is exactly right, including the part I had wrong: the [a-z0-9] guard was never load-bearing. I added it believing it was what separated privateKey from hotkey, but the camel alternation is already case-sensitive (Key, not key), so hotkey can never match that pass no matter what precedes the suffix. The guard bought nothing and cost every acronym prefix.
Relaxed the core as suggested, and re-ran the full over-redaction corpus this file already tests plus your donkey:
kept hotkey / HOTKEY / keyword / monkeypatch / tokenizer / secretary / donkey
redacted APIToken / JWTSecret / SSHKey / AWSSecret / OTPKey
Verified end-to-end too — planted all three acronym shapes in an imported config, ran a real session, downloaded the bundle:
APIToken: [REDACTED]
JWTSecret: [REDACTED]
SSHKey: [REDACTED]
hotkey: 'ctrl+shift+k',Two new unit tests lock both directions.
| // the upload path. Re-deriving it at archive time from cwd is the exact bug | ||
| // SDK-5993 fixed in the Node SDK (silently dropped the config on every monorepo / | ||
| // subdir CI run). Best-effort: never blocks the run. | ||
| if (!publishAutoCaptureDisabled(this._options)) { |
There was a problem hiding this comment.
This turns on default collection of customer source (wdio.conf + its imports + package.json), and there is no runtime disclosure anywhere in the run. The only surfaces that mention it are the changeset/CHANGELOG and the JSDoc on disableAutoCaptureLogs — neither is visible to someone who upgrades the service and runs a build. Since the redaction is key-name driven and best-effort by the PR's own framing, the notice is part of the control, not decoration.
Evidence — the Node SDK, which this feature is ported from, does exactly this. browserstack-node-agent/src/helpers/BrowserStackSetup.js:154-155:
if (!this.config.disableAutoCaptureLogs) {
logger.info(constants.AUTOLOGCAPTURE_NOTIFICATION);
}with src/bin/utils/constants.js:111 = "Project and debug logs are captured by default. To disable, set disableAutoCaptureLogs: true in config."
No equivalent BStackLogger.info exists in this service today (the only info-level notices are accessibility/self-heal/build-link lines), so a wdio customer gets strictly less disclosure than a Node SDK customer for a strictly broader capture.
Fix — one info line in this same block, reusing the value you already compute:
if (!publishAutoCaptureDisabled(this._options)) {
BStackLogger.info(AUTOLOGCAPTURE_NOTIFICATION)
initWdioConfigPath(config)
}with the constant naming what is included (config file + package.json) and how to turn it off.
There was a problem hiding this comment.
Valid — added in 5ebc318.
The Node SDK precedent is the right citation and I had not carried it across. Added AUTOLOGCAPTURE_NOTIFICATION to constants.ts and it is emitted from the same block, reusing the value already computed:
if (!publishAutoCaptureDisabled(this._options)) {
BStackLogger.info(AUTOLOGCAPTURE_NOTIFICATION)
initWdioConfigPath(config)
}The text names what is collected (config file, its local imports, package.json), states that values under known credential keys are removed, and gives the opt-out — so it matches the "best-effort, key-name driven" framing rather than implying the capture is fully sanitised.
Agreed on the reasoning: with key-name-driven redaction the notice is part of the control, not decoration.
| // Full archive manifest: the only place the complete entry list is visible, so | ||
| // regression automation can assert package.json and the config files actually | ||
| // made it in rather than inferring it from the config-capture line alone. | ||
| BStackLogger.debug(`Auto-capture archive entries: ${copiedFileNames.join(', ')}`) |
There was a problem hiding this comment.
This manifest never reaches the uploaded archive, which defeats the reason given for adding it.
Evidence — ordering inside uploadLogs: the log files are snapshotted into the staging dir at line 1582 (fs.copyFileSync(f, path.join(tmpDir, entryName))), and everything below that writes to the original log file, after the copy:
- 1593 —
collectConfigFilesForUpload(options.config)(and, on the detached-cleanup path, theResolved wdio config file … (strategy …)line it emits internally) - 1623 —
Auto-captured N config file(s) via <strategy> - 1639 — this manifest line
So the archive that support downloads contains none of them. On the main path the onPrepare-time resolution line does survive, because it is written long before the copy — but the manifest never does, and on the cleanup-rescue path the strategy is absent from the bundle entirely. The config_capture: reasons land only on the SDK_UPLOAD_LOGS event, so a bundle with a missing config carries no in-bundle explanation.
Fix — either build copiedFileNames and log the manifest before the copy loop, or write it as its own archive entry so it is inside the tarball by construction:
copiedFileNames.push(uniqueName('capture-manifest.txt')) // content = entry list + strategy + failuresThe second option also makes the strategy and failure reasons visible to whoever opens the bundle, which is where triage actually starts.
There was a problem hiding this comment.
Valid, and the most important of this round — fixed in 5ebc318.
My first check appeared to contradict you: three previously downloaded bundles all contained the manifest line. So I ran a controlled single run and compared the same run's local log against the archived one:
line local archived
Resolved wdio config file 1 1
Auto-captured N config file(s) 1 0
Auto-capture archive entries 1 0
local log: 237 627 bytes
archived log: 235 449 bytes
You are right, and the older bundles misled me — they came from runs where uploadLogs executed twice, so the second run's snapshot picked up the first run's tail. A single run never contains them.
Took your second option: the manifest is now its own archive entry, so it is in the tarball by construction rather than by ordering luck, and it carries the things triage actually opens the bundle for:
archive entries: bstack-wdio-service.log, sdk-cli-debug.log, wdio.bstack.conf.js, base.conf.js, package.json, capture-manifest.txt
config resolution strategy: cli_config_path
config files captured: wdio.bstack.conf.js, base.conf.js
package.json: /path/to/project
capture failures: none
Verified in a real downloaded bundle (build 3birnn473vhi1devkzf3qgge8mnibr23zz0pngpb).
One consequence worth recording: this also invalidated a regression test. The BStackAutomation coverage asserts the manifest by reading the local log, so it passed happily while the archive lacked it — a test that measured the wrong artifact. Two new unit tests here assert against the gunzipped tarball instead.
| } | ||
| } | ||
| if (configFiles.length > 0) { | ||
| BStackLogger.debug(`Auto-captured ${configFiles.length} config file(s) via ${strategy}: ${configFiles.map(f => f.name).join(', ')}`) |
There was a problem hiding this comment.
Two small things make this line misleading in exactly the case where triage needs it most.
strategy is undefined on every failure path of collectConfigFilesForUpload, and package.json is pushed onto configFiles after this count's semantics were set. So when config resolution fails but the manifest is found, the log reads:
Auto-captured 1 config file(s) via undefined: package.json
which reads as "capture worked" when no config was captured at all.
Fix — log the manifest separately from the config entries, and drop the via clause when there is no strategy:
if (strategy) {
BStackLogger.debug(`Auto-captured ${capturedConfigs.length} config file(s) via ${strategy}: ...`)
}
if (packageJsonPath) {
BStackLogger.debug(`Auto-captured package.json from ${relativeToCwd(packageJsonPath)}`)
}There was a problem hiding this comment.
Valid — fixed in 5ebc318. Both halves reproduce exactly as described: package.json is appended to configFiles before the count is taken, and strategy is undefined on every failure path of collectConfigFilesForUpload, so the line claimed a successful capture in precisely the case where none happened.
Split as suggested, with the config names snapshotted before package.json joins the list:
Auto-captured 2 config file(s) via cli_config_path: wdio.bstack.conf.js, base.conf.js
Auto-captured package.json from /path/to/project
and when nothing resolves it now reads No wdio config captured rather than inventing a count. The same values also go into capture-manifest.txt, so the distinction survives into the bundle rather than living only in the log.
| } | ||
| seen.add(resolved) | ||
|
|
||
| const { content: importedContent } = readCappedFile(resolved) |
There was a problem hiding this comment.
Each imported config is read from disk twice: collectLocalImports reads it here to seed the frontier, discards the content and returns paths only, then the caller re-reads every path at line 491.
Bounded at 5 files × 1 MB so the cost is small, but the two reads can also disagree if a file changes between them — the content used for import discovery would then not be the content archived.
Fix — return what you already have:
found.push({ filePath: resolved, content: importedContent })and have the caller at 491 use content directly instead of calling readCappedFile again. The imported.reason failure accumulation there becomes unnecessary too, since a read failure is already skipped inside this loop.
There was a problem hiding this comment.
Valid — fixed in 5ebc318. collectLocalImports now returns { filePath, content } and the caller uses the content it already has, so each imported config is read once.
The wasted read was the smaller half; the reason I took it is the second point — the two reads could disagree if a file changed between them, which would mean the content scanned for imports is not the content archived. The imported.reason accumulation went away with it, since a read failure is already skipped inside the discovery loop.
…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>
| * rejected: `wdio --spec ./a.js` resolves to the SPEC file under that rung, and to the real | ||
| * `wdio.conf.js` without it. | ||
| */ | ||
| export function resolveWdioConfigPath(config?: Options.Testrunner): ConfigPathResolution { |
There was a problem hiding this comment.
Suggest resolving this the way @wdio/cli resolves it, which collapses the ladder into one function.
What wdio actually does
It never searches. One candidate, one stem, six extensions:
run.ts:71— bare form is normalised intorun:configPath = path.resolve(cwd, params._[0] || 'wdio.conf.js')commands/run.ts:199—const { configPath = 'wdio.conf.js', ...params } = argvcommands/config.ts:181—formatConfigFilePaths()absolutises + strips the extensioncommands/config.ts:193—canAccessConfigPath(stem)probes that one stem across the 6 extensions, firstfs.accesshit wins- no hit →
missingConfigurationPrompt/exit 1 commands/run.ts:275→new Launcher(wdioConf.fullPath, params)→launcher.ts:60→new ConfigParser(...),rootDir = dirname(configFilePath)
There is no directory scan and no upward walk anywhere in that path. launcher.ts:68 then does configParser.initialize(this._args) → merge({...args}), which is exactly why config-path and _ are visible to us.
On 89a8c42 — the premise doesn't reproduce
Measured with real yargs and wdio's own declarations (spec: {type:'array'} at run.ts:99, watch: {type:'boolean'} at run.ts:28):
wdio --spec ./a.js _ = [] spec=["./a.js"]
wdio ./configs/a.conf.ts _ = ["./configs/a.conf.ts"]
wdio run ./configs/a.conf.ts _ = ["run","./configs/a.conf.ts"]
wdio --watch ./configs/a.conf.ts _ = ["./configs/a.conf.ts"] watch=true
wdio run ./a.conf.ts --spec ./t/a.js _ = ["run","./a.conf.ts"] spec=["./t/a.js"]
wdio --spec ./a.js leaves _ empty — yargs consumes the value because spec is a declared array option, so the removed rung could never have resolved a spec file.
The inverse is what bites: wdio --watch ./configs/a.conf.ts puts the real config in _[0], and scanArgvForConfig skips that token by its own rule (previous token starts with - and has no =). At head that invocation now falls through rungs 2-5 and lands on the single_conf_scan guess — or config_ambiguous and captures nothing if the directory holds two configs.
The deeper reason: config._ is not a degraded copy of argv. run.ts:71 resolves the bare form from params._[0] — it is the source wdio itself trusts, after yargs has applied the option declarations. The raw-argv scan is the degraded one, because it re-implements yargs' flag/value logic with a heuristic that cannot know which flags are boolean.
One function
export function resolveWdioConfigPath(config?: Options.Testrunner): ConfigPathResolution {
const record = (config || {}) as Record<string, unknown>
/* accept the path as spelled, else probe its stem: a TS project legally carries a
`.js` spelling for a `.ts` file on disk, which is why the CLI probes too */
const resolve = (value: unknown): string | undefined => {
if (typeof value !== 'string' || !value.trim()) {
return undefined
}
const full = path.resolve(process.cwd(), value.trim())
const ext = path.extname(full)
const stem = SUPPORTED_WDIO_CONFIG_EXTENSIONS.includes(ext) ? full.slice(0, -ext.length) : full
return [full, ...SUPPORTED_WDIO_CONFIG_EXTENSIONS.map(e => `${stem}${e}`)].find((candidate) => {
try {
return fs.statSync(candidate).isFile()
} catch {
return false
}
})
}
try {
const positionals = Array.isArray(record._) ? record._ as unknown[] : []
const positional = positionals.filter(
(entry): entry is string => typeof entry === 'string' && !WDIO_CLI_SUBCOMMANDS.includes(entry)
)[0]
const rootDir = typeof record.rootDir === 'string' ? record.rootDir : undefined
const rungs: Array<[string, unknown]> = [
['env_override', process.env[BROWSERSTACK_WDIO_CONFIG_FILE_PATH]],
['cli_config_path', record['config-path']],
['config_positional', positional],
['root_dir_default', rootDir && path.join(rootDir, DEFAULT_WDIO_CONFIG_BASENAME)],
['cwd_default', DEFAULT_WDIO_CONFIG_BASENAME]
]
for (const [strategy, value] of rungs) {
const configPath = resolve(value)
if (configPath) {
return { configPath, strategy }
}
}
return { reason: 'config_not_found' }
} catch {
return { reason: 'config_resolve_exception' }
}
}Verified against real temp dirs, 7/7:
| Case | Result |
|---|---|
wdio run ./configs/a.conf.ts |
configs/a.conf.ts · cli_config_path |
bare wdio ./configs/a.conf.ts (and the --watch form above, identical _) |
configs/a.conf.ts · config_positional |
wdio run ./configs/a.conf.js, only .ts on disk |
configs/a.conf.ts · cli_config_path — head misses this |
no-arg wdio, TS project |
wdio.conf.ts · root_dir_default |
user overrode rootDir |
wdio.conf.js · cwd_default |
run form with a run.js in cwd |
configs/a.conf.ts — subcommand filter holds |
two *.conf.ts in cwd, no CLI value |
config_not_found |
The third row is the gap worth the change on its own: because commands/run.ts:275 hands Launcher the user's spelling rather than the probed hit, a TS project legally carries configs/a.conf.js in config-path while configs/a.conf.ts is on disk. Rungs 2-3 reject it (resolveCandidate requires the path to exist verbatim), rungs 4-5 probe the wrong stem, and only the guessing rung can save it. Stem-probing the CLI value fixes it deterministically — and makes scanForSingleConfig unnecessary rather than load-bearing.
Deletes isReadableFile, resolveCandidate, probeConfigBasename, scanForSingleConfig, scanArgvForConfig and the current body — ~170 lines to ~45. initWdioConfigPath, relativeToCwd, redaction, import following and the package.json walk are untouched.
Three things to check before taking it:
- Two strategy values disappear (
argv_positional,single_conf_scan) and so does theconfig_ambiguousreason. Update anything downstream that keys on them;argv_positionalfolds intoconfig_positional. - Re-run your dev-test case 2 (bare
wdio ./configs/wdio.bstack.conf.js) against a real wdio and confirm it lands asconfig_positional. The yargs measurement above says it will, but your harness is the ground truth. If it somehow doesn't, keep the argv scan as a rung afterconfig._— not before it. - The subcommand filter is why
WDIO_CLI_SUBCOMMANDSstays; the extension list is still thecreate-wdiomirror.
There was a problem hiding this comment.
Valid on every count, taken in full in 812c85a — and it reverses my previous commit, which was wrong.
On 89a8c42, you are right and my proof was invalid. I removed the config._ rung after "demonstrating" it would resolve a spec file — but I hand-built {_: ["./a.js"]}, which yargs cannot produce. Re-measured with real yargs and wdio's own declarations (spec: {type:"array"} at the spec block, watch: {type:"boolean"}):
wdio --spec ./a.js _ = [] spec=["./a.js"]
wdio ./configs/a.conf.ts _ = ["./configs/a.conf.ts"]
wdio run ./configs/a.conf.ts _ = ["run","./configs/a.conf.ts"]
wdio --watch ./configs/a.conf.ts _ = ["./configs/a.conf.ts"] watch=true
wdio run ./a.conf.ts --spec ./t/a.js _ = ["run","./a.conf.ts"] spec=["./t/a.js"]
A declared array option consumes its value, so _ is empty and the rung was never reachable that way. Meanwhile the inverse bites exactly as you said — --watch <config> puts the real config in _[0], and my argv scan discarded it by its own "previous token is a flag" rule. Your framing is the part I had backwards: config._ is the post-yargs value the CLI itself trusts (run.ts resolves the bare form from params._[0]); the raw-argv scan is the degraded copy, because it re-implements yargs without knowing which flags are boolean.
The TS-spelling gap reproduces too, and it is the one none of my rungs could reach. Live run, configs/ containing only a.conf.ts:
$ wdio run ./configs/a.conf.js
{"config-path":"./configs/a.conf.js","rootDir":".../configs"}
wdio starts fine and reports a path that does not exist, because the CLI hands Launcher the probed hit but leaves config-path as the user typed it. Stem-probing the CLI value resolves it deterministically — and that is what makes the directory scan unnecessary rather than load-bearing, which was the real argument.
Re-ran the live matrix you asked for (point 2), against a real wdio rather than trusting the yargs measurement:
| invocation | strategy |
|---|---|
wdio run ./configs/x.conf.mjs |
cli_config_path |
bare wdio ./configs/x.conf.mjs |
config_positional ✓ |
no-arg wdio |
root_dir_default |
wdio --watch ./configs/x.conf.mjs |
config_positional — broken at head |
So the argv scan is gone entirely rather than kept as a lower rung.
Deleted resolveCandidate's exists-verbatim form, probeConfigBasename, scanForSingleConfig, scanArgvForConfig and the ladder body: 347 → 296 code lines. Per your point 1, argv_positional and single_conf_scan are gone and config_ambiguous with them; nothing downstream keyed on them except tests, which are updated.
One thing worth recording: util.test.ts asserted the upload event carried no failure string, and that only ever 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 — a small but direct demonstration of the over-reach you were removing.
| `archive entries: ${[...copiedFileNames, manifestName].join(', ')}`, | ||
| `config resolution strategy: ${strategy || 'none'}`, | ||
| `config files captured: ${capturedConfigNames.join(', ') || 'none'}`, | ||
| `package.json: ${packageJsonPath ? path.dirname(packageJsonPath) : 'not found'}`, |
There was a problem hiding this comment.
This puts the absolute project directory into the archive, which reverses the decision recorded earlier in this PR.
Both this line and the debug line above it (Auto-captured package.json from ${path.dirname(packageJsonPath)}) emit a full absolute path:
package.json: /Users/jane.doe/work/checkout/project
The manifest is inside the tarball we upload, so this is the same exposure — OS username and directory layout — that the config-path log was deliberately made cwd-relative to avoid (relativeToCwd, configCapture.ts:243). The manifest fix reintroduced it at the same blast radius.
Fix — export relativeToCwd from configCapture.ts (it is module-local today) and use it in both places, or inline path.relative(process.cwd(), …). The diagnostic survives either way: a manifest outside cwd still renders as ../../project, which is exactly the monorepo/subdir signal the field is there for.
There was a problem hiding this comment.
Valid — fixed in 812c85a. You are right that it reverses the decision made earlier in this PR; the manifest work reintroduced the absolute path at the same blast radius, in a file that ships inside the tarball.
relativeToCwd was module-local; it is now exported from configCapture.ts and used for both the manifest field and the debug line above it. The manifest now reads:
package.json: .
for a project-root manifest, and ../../project for one outside cwd — so the monorepo/subdir signal the field exists for still survives, without the OS username.
…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>
| `config files captured: ${capturedConfigNames.join(', ') || 'none'}`, | ||
| // cwd-relative: the manifest ships inside the tarball, so an absolute path would | ||
| // leak the OS username and layout — the same reason the resolution log line is relative | ||
| `package.json: ${packageJsonPath ? relativeToCwd(path.dirname(packageJsonPath)) : 'not found'}`, |
There was a problem hiding this comment.
The relative-path fix is right, but this field doesn't render as . — relativeToCwd falls back to the directory name here, and in one layout that name is the OS username.
relativeToCwd is path.relative(...) || path.basename(filePath), and path.relative(cwd, cwd) returns ''. The || basename fallback was written for file paths, where path.relative is never empty; path.dirname(packageJsonPath) is the first directory passed through it, so the common case — manifest at the project root — is exactly the case that trips it.
Measured against a port of this line:
manifest at cwd -> "package.json: my-e2e-project"
manifest at "home" dir -> "package.json: jane.doe" <- OS username, inside the tarball
manifest above cwd -> "package.json: ../.." <- correct
Row 2 is the narrow case (project checked out directly in $HOME) but it is the same exposure this fix was for, and row 1 means the field silently reports a folder name where the reader expects ..
Fix — keep relativeToCwd's file semantics and handle the directory case at the two call sites (this line and the debug line at 1635):
`package.json: ${packageJsonPath ? relativeToCwd(path.dirname(packageJsonPath)) || '.' : 'not found'}`or add a sibling helper, since both new uses pass a directory:
export const relativeDirToCwd = (dir: string): string => {
try {
return path.relative(process.cwd(), dir) || '.'
} catch {
return '.'
}
}There was a problem hiding this comment.
Valid — fixed in 927c837. And it corrects something I asserted in my previous reply: I said the field "now reads package.json: ." without checking. It did not.
Reproduced exactly as you described:
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
Your diagnosis is precisely right: the || path.basename(...) fallback was written for FILE paths, where path.relative is never empty, and path.dirname(packageJsonPath) is the first directory to go through it — so the most common layout is exactly the one that trips it.
Took the sibling-helper option, since both new call sites pass a directory and the semantics genuinely differ:
export const relativeDirToCwd = (dir: string): string => {
try {
return path.relative(process.cwd(), dir) || '.'
} catch {
return '.'
}
}Empty means "cwd" for a directory, so . is the correct rendering rather than a fallback. relativeToCwd keeps its file semantics and now carries a note not to pass a directory to it.
Live-verified rather than asserted this time — real run with cwd as the project root:
Auto-captured package.json from .
with the cwd basename (devtest) absent from the log. Unit tests added for all three directory cases, plus one asserting the tarball contains package.json: . and not the temp dir's folder name.
Unrelated but in the same commit: the "leaves no staging directory behind" test was flaky and I only noticed because this change made the suite run again. 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. It now spies on mkdtempSync and asserts the directories that call created are gone.
…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>
|
RUN_TESTS |
07souravkunda
left a comment
There was a problem hiding this comment.
Approving. All eight findings from my review are fixed and I have verified each one against the head commit rather than the replies — the redaction closes the acronym-prefix family without new false positives, the manifest is now an archive entry so the strategy and capture failures actually reach the bundle, the resolver mirrors how @wdio/cli resolves the config (verified 9/9 across the invocation matrix, including --watch <config> and the .js-spelling-with-.ts-on-disk case that no previous rung could reach), and no absolute path or folder name reaches the tarball.
Two things worth noting beyond the code. The disclosure notice matters as much as the redaction here: this ships customer source by default, key-name-driven scrubbing is best-effort by design, and the notice plus the opt-out are what make that defensible — please keep them in step if the capture surface grows. And the testing on this PR was the reason several of these were provable: the live-run evidence, and the two cases where a test was measuring the wrong artifact (the manifest asserted against the local log, and util.test.ts passing only because the old directory scan matched vitest.config.ts) are exactly the class of thing that hides a broken feature behind a green suite.
One open item, non-blocking: the "bounded quantifiers fail open on oversized single tokens" thread has no reply yet. It needs a recorded decision rather than code — its own recommendation is to keep the URL userinfo bound as-is, and I agree. Worth resolving the addressed threads so that one is visible.
07souravkunda
left a comment
There was a problem hiding this comment.
Approving. All eight findings from my review are fixed and I have verified each one against the head commit rather than the replies — the redaction closes the acronym-prefix family without new false positives, the manifest is now an archive entry so the strategy and capture failures actually reach the bundle, the resolver mirrors how @wdio/cli resolves the config (verified 9/9 across the invocation matrix, including --watch <config> and the .js-spelling-with-.ts-on-disk case that no previous rung could reach), and no absolute path or folder name reaches the tarball.
Two things worth noting beyond the code. The disclosure notice matters as much as the redaction here: this ships customer source by default, key-name-driven scrubbing is best-effort by design, and the notice plus the opt-out are what make that defensible — please keep them in step if the capture surface grows. And the testing on this PR was the reason several of these were provable: the live-run evidence, and the two cases where a test was measuring the wrong artifact (the manifest asserted against the local log, and util.test.ts passing only because the old directory scan matched vitest.config.ts) are exactly the class of thing that hides a broken feature behind a green suite.
One open item, non-blocking: the "bounded quantifiers fail open on oversized single tokens" thread has no reply yet. It needs a recorded decision rather than code — its own recommendation is to keep the URL userinfo bound as-is, and I agree. Worth resolving the addressed threads so that one is visible.
|
[SDK Wdio Test] TRA build state: failed | Stability 98% — verdict: success. Passed: 83, Failed: 2, Aggregate: 85. TRA: https://observability.browserstack.com/builds/3cdynvtugehuvmqvp8kn5ykvckhdrnva28nrqisn |
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>
22946ba
|
Narrowed the capture scope: imported files are now followed only when they are themselves configs. The follower previously resolved any relative specifier landing on a supported extension, with no name filter — so a config doing That is ordinary application source, not configuration. Capturing the split-config case is the reason for following imports at all; shipping a customer's application modules is not. Imports now resolve only if the file is named like a config (
This also makes the user-facing wording true as written — the changeset, the Full list of what the archive carries after this change: the resolved 1147 tests green, lint clean. |
…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>
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.