Skip to content

Fixes for LOC-6730 & 6729 #98

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.

Sign up for GitHub

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

Open
rounak610 wants to merge 2 commits into
base: master
Choose a base branch
from
+4 −3
Open

Fixes for LOC-6730 & 6729 #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d60c44
fixes for LOC-6730
rounak610 May 25, 2026
6ed9b48
fixes for LOC-6729
rounak610 May 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/Semgrep.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ jobs:

container:
# A Docker image with Semgrep installed. Do not change this.
image: returntocorp/semgrep
# Pinned by digest (LOC-6730 / INF-002) — tag-mutation is a supply-chain vector.
image: returntocorp/semgrep@sha256:9349edbadf90c3f3c0c3f55867625354e89680e6fa10d9034042af52fdb0e0d0
Copy link
Copy Markdown
Collaborator

@07souravkunda 07souravkunda May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@rounak610 rounak610 May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

@07souravkunda 07souravkunda May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

@rounak610 rounak610 May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope


# Skip any PR created by dependabot to avoid permission issues:
if: (github.actor != 'dependabot[bot]')
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/browserstack/local/Local.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ private void makeCommand(Map<String, String> options, String opCode) {
if (IGNORE_KEYS.contains(parameter)) {
continue;
}
if (avoidValueParameters.get(parameter) != null && opt.getValue().trim().toLowerCase() != "false") {
if (avoidValueParameters.get(parameter) != null && !"false".equals(opt.getValue().trim().toLowerCase())) {
Copy link
Copy Markdown
Collaborator

@07souravkunda 07souravkunda May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@rounak610 rounak610 May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not required

command.add(avoidValueParameters.get(parameter));
} else {
if (parameters.get(parameter) != null) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/browserstack/local/LocalBinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ private void fetchSourceUrl() throws LocalException {
inputParams.put("auth_token", this.key);
if (fallbackEnabled) {
connection.setRequestProperty("X-Local-Fallback-Cloudflare", "true");
inputParams.put("error_message", downloadFailureThrowable.getMessage());
inputParams.put("error_message", downloadFailureThrowable.getClass().getName());
Copy link
Copy Markdown
Collaborator

@07souravkunda 07souravkunda May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Collaborator Author

@rounak610 rounak610 May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

@07souravkunda 07souravkunda May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@rounak610 rounak610 May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not relevant

}
String jsonInputParams = inputParams.toString();

Expand Down
Loading