-
Notifications
You must be signed in to change notification settings - Fork 9
feat(SDK-7250): capture the wdio config file in auto-captured logs #128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2552924
aa16aa1
e9999dd
97e2f83
894bd10
e593093
45d25d7
e5b649e
5ebc318
89a8c42
812c85a
927c837
02c66b5
22946ba
b8d39c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "@wdio/browserstack-service": minor | ||
| --- | ||
|
|
||
| - The debug logs the service uploads at the end of a run now include a copy of your `wdio.conf` file (and the local config files it imports) plus your `package.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. | ||
| - Set `disableAutoCaptureLogs: true` in the service options, or `BROWSERSTACK_DISABLE_AUTO_CAPTURE_LOGS=true`, to turn this upload off entirely. |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import type { BrowserstackConfig, BrowserstackOptions, App, AppConfig, AppUpload | |
| import { | ||
| BSTACK_SERVICE_VERSION, | ||
| NOT_ALLOWED_KEYS_IN_CAPS, PERF_MEASUREMENT_ENV, RERUN_ENV, RERUN_TESTS_ENV, | ||
| AUTOLOGCAPTURE_NOTIFICATION, | ||
| BROWSERSTACK_TESTHUB_UUID, | ||
| VALID_APP_EXTENSION, | ||
| BROWSERSTACK_PERCY, | ||
|
|
@@ -50,6 +51,7 @@ import { | |
| validateSkipAppOverride | ||
| } from './util.js' | ||
| import CrashReporter from './crash-reporter.js' | ||
| import { initWdioConfigPath, isAutoCaptureLogsDisabled, publishAutoCaptureDisabled } from './configCapture.js' | ||
| import { finalizeOrphanedRuns } from './testOps/openRunsJournal.js' | ||
| import { BStackLogger } from './bstackLogger.js' | ||
| import { PercyLogger } from './Percy/PercyLogger.js' | ||
|
|
@@ -250,6 +252,16 @@ export default class BrowserstackLauncherService implements Services.ServiceInst | |
| async onPrepare (config: Options.Testrunner, capabilities: Capabilities.TestrunnerCapabilities | WebdriverIO.Capabilities) { | ||
| PerformanceTester.start(PERFORMANCE_SDK_EVENTS.FRAMEWORK_EVENTS.INIT) | ||
|
|
||
| // Resolve the user's wdio config path ONCE, here, while the freshly parsed config | ||
| // still carries the CLI's `config-path` positional, and publish it on the env for | ||
| // 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)) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This turns on default collection of customer source ( Evidence — the Node SDK, which this feature is ported from, does exactly this. if (!this.config.disableAutoCaptureLogs) {
logger.info(constants.AUTOLOGCAPTURE_NOTIFICATION);
}with No equivalent 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 +
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid — added in The Node SDK precedent is the right citation and I had not carried it across. Added if (!publishAutoCaptureDisabled(this._options)) {
BStackLogger.info(AUTOLOGCAPTURE_NOTIFICATION)
initWdioConfigPath(config)
}The text names what is collected (config file, its local imports, Agreed on the reasoning: with key-name-driven redaction the notice is part of the control, not decoration. |
||
| BStackLogger.info(AUTOLOGCAPTURE_NOTIFICATION) | ||
| initWdioConfigPath(config) | ||
| } | ||
|
|
||
| // skipAppOverride: emit the fixed warning once + handle the 3 edge cases before anything | ||
| // else. Runs once here in the launcher (main process). Edge-2 (explicit false + no app) is a | ||
| // deliberate pre-session config error, surfaced as SevereServiceError so the run aborts cleanly. | ||
|
|
@@ -838,7 +850,12 @@ export default class BrowserstackLauncherService implements Services.ServiceInst | |
| // return path (no creds, archive failure, upload no-response, exception), so | ||
| // measureWrapper is no longer needed here. | ||
| const clientBuildUuid = this._getClientBuildUuid() | ||
| const response = await uploadLogs(getBrowserStackUser(this._config), getBrowserStackKey(this._config), clientBuildUuid) | ||
| const response = await uploadLogs( | ||
| getBrowserStackUser(this._config), | ||
| getBrowserStackKey(this._config), | ||
| clientBuildUuid, | ||
| { disableAutoCaptureLogs: isAutoCaptureLogsDisabled(this._options), config: this._config } | ||
| ) | ||
| // Treat a truthy response carrying a non-success status as a server-side | ||
| // rejection, not a delivery — a delivered upload must not be repeated by | ||
| // the exit-time cleanup rescue; failed/skipped uploads stay eligible for it. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Suggestion — [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:
@-terminated credential is left entirely un-redacted.BEGINline is scrubbed, and only because it happens to contain the wordKEY).BEGIN/ENDheader lines scrubbed via the coincidentalKEYmatch).All three are reachable inside the 1 MB
MAX_CAPTURED_CONFIG_FILE_BYTEScap.Suggested Fix
Mostly this is worth flagging so it's a conscious decision rather than a fix:
://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).-----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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.