Fixes for LOC-6730 & 6729

[rounak610]

No description provided.

[07souravkunda]

Anchoring here because this is the only INF- fix in the diff* — the actual concern is PR-wide.

PR title says "Fixes for LOC-6730 & 6729". After diffing .travis.yml and pom.xml between master and this PR head, both files are identical — i.e. INF-003/INF-004/INF-005 weren't touched. Sub-issue status:

Ticket / sub-issue	Severity	Status
LOC-6729 / CSL-002 — access key in argv (Local.java:152-153)	Medium (5.5)	❌ Not addressed — still command.add("--key"); command.add(options.get("key"));
LOC-6729 / AUZ-002 — != "false" reference comparison	Medium (5.5)	✅ Fixed
LOC-6729 / CSL-004 — exception message leak	Medium (4.3)	✅ Fixed
LOC-6730 / INF-002 — Semgrep digest pin	Medium (6.3)	✅ Fixed (this line)
LOC-6730 / INF-003 — Travis wget Maven, no checksum	Medium (6.8)	❌ Not addressed
LOC-6730 / INF-004 — no Maven lockfile	Medium (5.9)	❌ Not addressed
LOC-6730 / INF-005 — maven-compiler-plugin 2.3.2, maven-surefire-plugin 2.4.2	Low (3.7)	❌ Not addressed

3 of 7 sub-issues. The two highest-CVSS items (INF-003 = 6.8, INF-002 = 6.3) include one still untouched. The PR body is empty, so on merge both tickets will look closed from the ticket-watcher's view.

Questions:

1. Are CSL-002 / INF-003 / INF-004 / INF-005 intentional scope cuts? If so, please call them out in the PR description so the tickets don't auto-close on merge.
2. For CSL-002 specifically — the ticket noted "if binary supports it". Has the binary's BROWSERSTACK_ACCESS_KEY env-var support been confirmed?
3. Will follow-up PRs / sub-tickets be filed for the remaining four? Linking them here would close the loop.

[rounak610]

The "not addressed" fixes marked above are not intended to be fixed, as this has been approved by the Security team.
https://browserstack.atlassian.net/browse/LOC-6729?focusedCommentId=2063323

[07souravkunda]

getClass().getName() closes the path-leak (good), but it throws away nearly all triage signal. The fallback error_message field is what the BrowserStack backend uses to triage why a user's binary download failed (DNS, connectivity, SSL, disk-write, etc.). Before this PR ops saw e.g. Connection refused to local.browserstack.com:443 — informative but PII-leaking. After this PR ops see java.net.ConnectException — safe but no host, no port, no boundary.

The ticket said "send error code, not raw getMessage()" — getClass().getName() is technically a code, but a coarse one.

Suggested alternatives (any of these is better):

1. getClass().getSimpleName() — drops the redundant java.net. package prefix; same safety, shorter logs.
2. Curated mapping — small switch / map from exception class → short code (NETWORK_TIMEOUT, CONNECTION_REFUSED, SSL_HANDSHAKE_FAILED, IO_ERROR, UNKNOWN). Preserves triage value, still no PII.
3. Selective fields — for known-safe exception types, include the non-PII field (TLS alert code, HTTP status, but not file paths). More effort; better diagnostics.

Question: has the team that consumes error_message (ops / fallback funnel watchers) been looped in on losing this signal? They may have a preference between (1), (2), or (3).

[rounak610]

checked local-service code, this errorMessage is only sent to EDS.
https://github.com/browserstack/local-service/blob/380690e52864cc93bde77cc893c16db6a73cdc70/app/controllers/local_api_controller.rb#L54

[07souravkunda]

The fix is correct (!"false".equals(value) is null-safe and idiomatic), but it's a real user-visible behavior change:

• Before: force=false, forcelocal=false, forceproxy=false, onlyAutomate=false, v=false all silently added the flag — because dynamically-constructed strings never == a literal.
• After: those values correctly suppress the flag.

That's the intended fix. But anyone who was passing force=false thinking "I want -force always included" was riding the bug and will now experience different behavior on upgrade.

More importantly: the PR ships this behavior change with no test coverage. There appears to be no test asserting:

• Map.of("force", "false") → -force NOT in command
• Map.of("force", "true") → -force IS in command
• Map.of("force", "FALSE") (case variant) → -force NOT in command (validates the .toLowerCase() step)

Without these, a future refactor of makeCommand can silently re-introduce the reference-comparison bug (or its inverse) and CI won't catch it.

Suggested fix: add ~3 unit tests on makeCommand covering the boolean-flag matrix for at least one of the avoidValueParameters keys (force is the natural choice). Lock the contract in test.

[rounak610]

not required

[07souravkunda]

Pin works — verified the digest sha256:9349edba… is the current latest tag digest on Docker Hub (last_pushed: 2026-05-14), so this is immutable and safe. Same digest as the Python sibling PR (browserstack-local-python#61) — convergent supply-chain story across SDKs, which is great.

Two improvements worth considering:

1. The pin gives no human-readable handle to know which Semgrep version CI is running, and the next bump will look like an opaque digest change. A version-tagged digest such as returntocorp/semgrep:1.163.0-nonroot@sha256:... is equivalent in security and clearer to review.
2. LOC-6730 specifically called out adding Dependabot for Docker digest updates — without that, a digest pin in isolation goes stale.

Question: is a .github/dependabot.yml (with package-ecosystem: docker) landing in a separate PR, or should it go in here?

[rounak610]

nope

[07souravkunda]

Adjacent — not about this line, but flagging since I have the whole LocalBinary.java open during this review:

The Python sibling PR (#61) just landed LOC-6717 (shell-injection via logfile) and LOC-6718 (unverified binarypath execution). I checked whether equivalent issues exist in the Java SDK:

Concern	Python (before fix)	Java (today)
Logfile shell-injection	os.system('echo "" > ' + path)	No equivalent — Java doesn't truncate a logfile itself, only passes -logFile <path> through to the binary. Safe.
binarypath unverified execution	self.binary_path = options['binarypath'] → no verify	Partially safer — Java already runs --version regex verification at LocalBinary.java:133-143. ✅
binarypath non-existent → SDK downloads to user-supplied path	(Python doesn't have this)	LocalBinary.java:157-158 — if !new File(binaryPath).exists(), the SDK calls downloadBinary(binaryPath, true), writing to the attacker-supplied path. Potential path-traversal-on-write (e.g., binarypath='/etc/cron.d/x').

Question: is there a sibling ticket for the "download to user-supplied path" case (LocalBinary.java:157-158)? Not in scope for this PR — but worth filing while the cross-SDK threat-model review is fresh.

[rounak610]

not relevant

[07souravkunda]

Thanks for the PR. The three changes that are in scope (Semgrep digest, AUZ-002 boolean-flag fix, CSL-004 exception sanitization) all land in the right shape.

The main thing I'd like aligned before merge is scope vs PR title — by my read, this PR addresses 3 of the 7 sub-issues across LOC-6729 and LOC-6730 (CSL-002 + INF-003 + INF-004 + INF-005 are not touched). PR body is empty, so on merge both tickets will look fully closed. Could you either expand the PR or update the description / file follow-up tickets for the remaining four?

Posted 5 inline comments:

• One on scope (the 3-of-7 breakdown, anchored to Semgrep.yml)
• One on CSL-004 going from getMessage() → getClass().getName() — safe but coarse; suggesting .getSimpleName() or a curated error-code map
• One on AUZ-002 shipping a behavior change with no tests
• One on the Semgrep pin being latest's digest rather than a version-tagged one + Dependabot for Docker
• One adjacent — confirming Python's LOC-6717 has no Java equivalent and Python's LOC-6718 is partially closed in Java already, but flagging the "download binary to user-supplied path" case at LocalBinary.java:157-158 as worth a separate ticket

Same Semgrep digest as the Python sibling PR (#61) — nice cross-SDK consistency.

[07souravkunda]

Thanks for the detailed responses — addressed my concerns:

• Scope: deferrals are per the Security plan in LOC-6729 comment 2063323 (CSL-002 → stdin-based handoff next sprint alongside LOC-6719; AUZ-002 + CSL-004 land now as standalone one-liners). Matches what shipped here.
• CSL-004: verified error_message is consumed at local_api_controller.rb:40-75 and only feeds the LocalCloudflareFallback EDS event. getClass().getName() is appropriate for that path; my "lost triage signal" concern was over-stated.

The three substantive fixes (Semgrep digest pin, AUZ-002 .equals(), CSL-004 sanitization) are correct.