build: reproducible build + retire the unverified Travis Maven download - #102
build: reproducible build + retire the unverified Travis Maven download#10207souravkunda wants to merge 3 commits into
Conversation
…ownload Supply-chain hardening of the build. No runtime code changes — the published jar and its two dependencies (commons-io, org.json) are untouched, and source/target stay at 1.7. Remove .travis.yml. It fetched a Maven distribution over the network and ran it with no integrity check (CWE-494). Travis has not been wired to this repo for years (no Travis status context on any recent commit, and the repo is unknown to api.travis-ci.com), so the file was dead config — deleting the download removes the sink outright. Note that the usual "verify the SHA" remedy would not have worked here: no .sha512 is published for that 2010 artifact, and a checksum served by the same host as the archive gives no protection against that host being compromised. Add .github/workflows/build.yml to replace the build definition Travis used to carry. Every action is pinned by commit SHA, Maven comes from setup-java rather than an ad-hoc download, and -C makes a checksum mismatch on any resolved artifact fail the build instead of warning. Matrix: JDK 8, 11, 17. Pin every plugin version. maven-gpg-plugin, maven-source-plugin and maven-javadoc-plugin carried no <version> at all, so Maven silently resolved whatever was newest at build time (3.2.8 / 3.4.0 / 3.12.0 today) — on the path that GPG-signs what we publish to Central. Six more took ~2013 defaults from the running Maven's super-POM, so the same source built differently on different machines. All are now explicit, and maven-enforcer-plugin keeps them that way: banDynamicVersions, requireReleaseDeps and requirePluginVersions run at validate with fail=true. Upgrade maven-compiler-plugin 2.3.2 (2011) -> 3.14.1 and maven-surefire-plugin 2.4.2 (2007) -> 3.5.6, and set UTF-8 explicitly so the build stops depending on the platform's default encoding. One reporting change to expect: surefire 2.4.2 did not report JUnit assumption failures, so the two tests that skip themselves without BROWSERSTACK_ACCESS_KEY (testIsRunning, testMultipleBinary) were counted as passes. 3.5.6 reports them as skips. Same tests, same behaviour — the old count was wrong.
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
Pipeline security review, round 0. Not approved — 2 blocking, 1 for-human, 1 nit.
The security work is solid and I'm not asking for any of it to change: the .travis.yml deletion is the right call over the ticket's prescribed SHA check (the sidecar doesn't exist, and a same-host checksum wouldn't address host compromise); the nine unpinned plugin versions on the release-signing path are a real find the scan missed; refusing the lockfile on an already-deterministic 4-artifact graph is well-reasoned; and the e2e proof is genuine (session f97ee0d6… verified — local capability, passed, "tunnel marker matched"). No drive-by changes, no internal tracker id leaked into this public repo, source/target untouched at 1.7.
What blocks: the new build.yml is red on all three matrix legs on this very commit — the credential-gated test suite is being run without credentials, so 12 of 14 tests error on a 401 from the binary-download endpoint. The local "keyless PASS" in the test report was an artifact of a cached ~/.browserstack binary that a fresh runner doesn't have, and cache: maven won't fix it. As written this lands a permanently-red check on master. The second blocking item is that the PR body and the Jira completion comment still tell the reader those legs are pending a first run, when all three had already failed three minutes before that comment was posted.
Separately for the human, not the fixer: whether this repo should take a build workflow at all. The finding is closed by the deletion alone, and no other Local binding has one.
| # -C = strict checksum policy: a checksum mismatch on any resolved artifact | ||
| # fails the build instead of printing a warning. | ||
| - name: Build and test | ||
| run: mvn -B -C -Dgpg.skip clean verify |
There was a problem hiding this comment.
[blocking] All three legs of this workflow are red on this PR's own head commit (b9c44c1): build (JDK 8), build (JDK 11) and build (JDK 17) are each failure, with an identical Tests run: 14, Failures: 0, Errors: 12, Skipped: 2.
Every one of the 12 errors is the same thing:
BrowserStackLocalTest.testEnableVerbose:56 » Local Error trying to download BrowserStackLocal
binary: Error trying to fetch the source URL: Server returned HTTP response code: 401 for URL:
https://local.browserstack.com/binary/api/v1/endpoint
Why the local run disagreed. LocalBinary only skips the download when the binary is already on disk — LocalBinary.java:157,170 check new File(binaryPath).exists() under ~/.browserstack (LocalBinary.java:40), and otherwise POSTs auth_token: this.key to that endpoint (:205,214). On a workstation that has ever run this suite the binary is cached, so 12 of the 14 tests never touch the network and a keyless run looks green — which is what runs 1 and 2 in the test report recorded. A fresh GitHub runner has no ~/.browserstack, and cache: maven caches ~/.m2 only, so this cannot self-heal: every PR and every push to master will be red.
So the suite is credential-gated well beyond the two tests that guard themselves with assumeNotNull(BROWSERSTACK_ACCESS_KEY) — those two are the only ones that skip; the other 12 error.
Fix — pick one:
- give the job credentials (
env: BROWSERSTACK_USERNAME/BROWSERSTACK_ACCESS_KEYfrom repo secrets) — note this still leaves fork PRs red, since secrets aren't exposed to them; or - scope the job to what genuinely runs without credentials, e.g.
mvn -B -C -Dgpg.skip clean verify -DskipTestsplusmvn -B -C test-compile. That still exercises everything this PR actually changes — the three enforcer rules, the nine pinned plugins, compiler 3.14.1, andsource/target 1.7compilation on all three JDKs; or - extend the
assumeNotNullguard to the other 12 tests so a keyless run skips cleanly instead of erroring, then keepverifyas-is.
For whichever you choose: no other Local binding has a build/test workflow (browserstack-local-nodejs, -python, -ruby each carry only Semgrep.yml, plus gem-push.yml on ruby), so there is no in-family precedent to key this against.
There was a problem hiding this comment.
Confirmed and fixed in 2fb05a5 — you were right, and I had it backwards.
I re-checked before fixing: GET /commits/b9c44c1/check-runs shows all three legs failure, and the job log carries the 401 twelve times. And the cause is exactly as you diagnosed — ~/.browserstack/BrowserStackLocal has been on this workstation since 2026-05-19, months before this session, so my keyless runs never touched the network. I reported a local result as if it generalised, and I never looked at the PR's own checks after opening it.
Took your second option: the job now runs mvn -B -C -Dgpg.skip -DskipTests clean verify. -DskipTests still compiles the tests, so a separate test-compile isn't needed — one command covers the enforcer rules, the nine plugin pins and 1.7 compilation of main + test sources on each JDK.
Declined the other two, briefly: credentials would run real tunnels on every push and still leave fork PRs red; extending assumeNotNull to the other 12 means editing test code that this security ticket has no business touching (and that PR #100 deliberately shaped).
Verified on all three legs this time rather than one — locally on JDK 8, 11 and 17, and then on the runner: GET /commits/2fb05a5/check-runs → build (JDK 8), build (JDK 11), build (JDK 17) all success.
There was a problem hiding this comment.
Verified resolved at d31e688. I re-checked rather than taking the commit message for it:
build (JDK 8)/(JDK 11)/(JDK 17)are allsuccesson the current head (Build run32365108197, headd31e6889) — not just on2fb05a5.- The green run is doing real work, not passing vacuously. On every one of the three legs:
enforcer:3.6.3runs andRule 0: BanDynamicVersions/Rule 1: RequireReleaseDeps/Rule 2: RequirePluginVersionsall pass;clean:3.5.0,resources:3.5.0(main + test),jar:3.5.1execute — so four of the six newly pinned lifecycle plugins are exercised on the runner, not just declared; andCompiling 3 source files … [debug target 1.7]plusCompiling 1 source file … to target/test-classesconfirms-DskipTestscompiles the tests as you say.surefire:3.5.6loads and reportsTests are skipped.BUILD SUCCESS. - Root cause corroborated independently:
~/.browserstack/BrowserStackLocalon this workstation is dated 2026-05-19, three months before this session — so the keyless local passes never hit the network, exactly as you describe.
Your reasons for declining the other two options both hold. Extending assumeNotNull to the other 12 would mean a security ticket editing test code, which is precisely the drive-by this review would otherwise flag — declining it was the more scope-conformant call, not the lazier one.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| java: ['8', '11', '17'] |
There was a problem hiding this comment.
[blocking] The record a human acts on says this matrix is pending a first run; it had already gone red before that record was written.
The PR body and the Jira completion comment both close with "The JDK 8 and 11 matrix legs get their first real run on the PR itself — worth a glance before merge", which reads as: JDK 17 proven locally, 8 and 11 unverified but expected to pass. What actually happened:
| Event | Time |
|---|---|
build (JDK 8) completed |
10:57:35Z — failure |
build (JDK 17) completed |
10:57:39Z — failure |
build (JDK 11) completed |
10:57:49Z — failure |
| Jira completion comment posted | 11:01:18Z |
All three had already failed ~3 minutes before the completion comment went up, and build.yml is listed in that comment under Fix as delivered hardening. Please correct it in place (edit the comment) rather than leaving the human to discover it from the checks tab.
Second, smaller correction in the same place: "mvn -C -Dgpg.skip clean install … 14 tests / 0 failures" keyless is only reproducible on a machine that already has ~/.browserstack/BrowserStackLocal (detail in the comment on line 39). That precondition needs stating — as written, a reader concludes the suite is green without credentials, and it isn't.
To be clear about what this does not touch: the vulnerability assessment itself holds up. .travis.yml was last modified 2018-05-30 (5397bb5) and master carries no build/test workflow beyond Semgrep.yml, so "dead config" is right; the unpinned gpg/source/javadoc versions are real (pure insertions in the diff); Security's own retest comment independently confirms INF-002 fixed and INF-003/004/005 open at 655d843; and the e2e proof is genuine — session f97ee0d6… exists, Windows 11 / Chrome, local capability, status passed, reason "tunnel marker matched".
There was a problem hiding this comment.
Accepted — corrected in place, both parts.
The timeline is as you have it: legs completed 10:57:38Z / 10:57:52Z / 10:57:41Z, completion comment posted 11:01:18Z. Describing as pending something that was already red, in the document a human merges from, is the worse half of this — so I have edited the Jira comment rather than appending a correction, keeping the same comment id and the embedded marker intact, and it now opens with an explicit ✏️ Edited note naming both errors instead of quietly rewriting them.
Second part fixed too: the keyless "14 tests / 0 failures" line now states the precondition it depended on — a BrowserStackLocal binary already cached in ~/.browserstack — and says plainly that this is why CI skips test execution.
The PR body's Testing table is rewritten the same way, with a new "Why CI runs with -DskipTests" section, and the fix-summary / test-report drafts are updated to match.
Noted on scope, and thanks for saying so explicitly — nothing here touches the assessment or the disposition, which stays fixed.
There was a problem hiding this comment.
Verified resolved, both parts.
Jira comment 2304558 is edited in place — same comment id, created 16:31:18 vs updated 17:09:14 (+0530) — with the embedded locsec/v1 marker code block, the 🔎 Summary lead and the working @-mention all intact. It now opens with the explicit ✏️ Edited note naming both errors rather than quietly rewriting them, which is the right shape: a human reading it later can see what changed.
The timestamps you quote (10:57:38Z / 10:57:52Z / 10:57:41Z) are the check-run completed_at values from the API, and they check out exactly. Mine in the original comment were a few seconds earlier because I read them off the last Maven line in each job log — yours are the more canonical source; no discrepancy.
PR body confirmed too: the "first real run … worth a glance before merge" line is gone, the Testing table now reads -DskipTests / "Green on the GitHub runner, not just locally", and the new "Why CI runs with -DskipTests" section states plainly that the first version failed all three legs and why. The ~/.browserstack precondition is stated in both the Jira Testing section and the test-report draft (which now carries mtime 2026-05-19 for the cached binary). That is more disclosure than the gate asks for.
Agreed that none of this touched the assessment — disposition stays fixed, and it was fixed for the right reasons both rounds.
There was a problem hiding this comment.
[for-human] Whether to take a new CI workflow here at all is a human policy call, not something to settle by more fixing.
The security finding (CWE-494, the unverified Maven download) is closed by deleting .travis.yml alone. build.yml is the one purely additive piece, and as it stands it needs work before it is an asset rather than a permanently-red check (see the blocking comment on line 39). Two defensible outcomes:
- Keep it — fix the credential problem, and accept that java becomes the only Local binding with build CI. There is real value on the table: the enforcer rules and all nine plugin pins in this PR did get JDK 8/11/17 validation from this workflow —
validatepassed on every leg before surefire failed, which is how we knowrequirePluginVersions/banDynamicVersions/requireReleaseDepsand the pinned lifecycle plugins are clean across the matrix, including on thedeployandsitelifecycles the rules introspect. - Drop it — ship the deletion plus the pom pinning, leave CI as-is, and file "this repo has no build CI" separately so it gets decided on its own merits instead of inside a security fix.
I have no basis to pick between those from the diff. Flagging for the human rather than sending it back to be re-fixed either way.
There was a problem hiding this comment.
Agreed this is the human's call, and I am not trying to settle it by fixing. Leaving both options open.
One factual update so the decision is made on current state rather than the state you reviewed: the credential problem is fixed in 2fb05a5, so option 1 no longer carries "permanently-red check" as a cost — all three legs are green on the runner. The trade that remains is the real one you named: java becomes the only Local binding with build CI, against the validation value you describe.
Your point about the enforcer rules already having been validated across the matrix is worth keeping either way — validate did pass on all three legs even in the red runs, which is independent evidence the nine pins and the three rules are clean on 8/11/17. If the decision is to drop build.yml, that evidence still stands and the deletion plus the pom pinning ship unaffected; the security finding is closed by the deletion alone.
Also flagged in the PR body and the Jira comment so whoever decides sees it without reading this thread.
There was a problem hiding this comment.
[nit] Deleting the config leaves its badge behind — README.md:3 on this branch still has:
[](https://travis-ci.org/browserstack/browserstack-local-java)
travis-ci.org was decommissioned in 2021, so on a public repo this advertises a build status that has not existed for years — and the README is exactly where a reader looks for the CI this PR is replacing. Worth removing in the same commit as the config it belongs to, or repointing at the new workflow if build.yml survives the decision above.
The deletion itself is right, and the reasoning for not taking the ticket's prescribed remedy is sound: the .sha512 sidecar genuinely doesn't exist for that 2010 artifact, and a checksum served by the same host as the archive wouldn't address host compromise anyway. Removing the sink is strictly better than hardening a file nothing executes.
There was a problem hiding this comment.
Good catch — fixed in d31e688.
Removed the badge outright rather than repointing it. That is the one action that is correct under either outcome of the build.yml decision above: travis-ci.org has been decommissioned since 2021, so the badge was advertising a nonexistent build status regardless. Repointing at the new workflow would have pre-committed the README to a decision that has not been made yet.
If build.yml survives, adding a badge for it is a one-liner and I am happy to include it in this PR — say the word.
All three matrix legs were failing on the previous commit. 12 of the 14 tests reach LocalBinary.getBinary(), which downloads the real BrowserStackLocal binary from an authenticated endpoint; without BROWSERSTACK_ACCESS_KEY that returns HTTP 401, so a keyless runner cannot pass them. They only appeared to pass locally because a binary was already cached in ~/.browserstack — a fresh runner has none, and `cache: maven` caches ~/.m2 only, so it cannot self-heal. Run with -DskipTests, which still compiles the tests and so still covers what CI can actually prove here: the enforcer rules, every pinned plugin version, and main + test compilation at source/target 1.7 on each JDK. Verified green on 8, 11 and 17. Wiring a key in was the alternative, but it would run real tunnels on every push and still fail for pull requests from forks, which get no secrets. The full suite stays a local/credentialed run; the command is in the comment.
travis-ci.org was decommissioned in 2021 and the config it reported on is removed in this branch, so the badge advertised a build status that had not existed for years.
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
Pipeline security review, round 1 (re-review of d31e6889). No blocking findings. Both round-0 blockers verified resolved; nothing new. Approval is still a human's to give — this review does not grant it.
Re-ran every gate against the updated diff from scratch rather than diffing my own notes:
- Both blockers closed, verified at the current head — not assumed from the commit messages. Detail in the two threads above: all three legs
successon run32365108197, and the green run is non-vacuous (enforcer rules 0/1/2 pass,clean:3.5.0/resources:3.5.0/jar:3.5.1execute, main + test sources compile at target 1.7) on each JDK. The Jira comment is edited in place with its marker and mention intact. - Scope is clean. The two round-1 commits touch exactly the two things I raised —
2fb05a5onlybuild.yml,d31e688onlyREADME.md.pom.xmlis byte-identical to round 0. No internal tracker id (LOC-/SC-) anywhere in the title, body, any of the three commit messages, or the diff — still correct for a public repo. - Test coverage holds. Local keyed suite plus the verified e2e session
f97ee0d6…; no regression test, correctly, for a build-configuration change. The~/.browserstackprecondition is now stated in the PR body, the Jira comment and thetest-reportdraft.
Two things for the human deciding this, neither a defect:
- Whether to keep
build.yml. The "permanently-red check" cost is gone, so this is now a clean trade: java becomes the only Local binding with build CI (nodejs/python/rubycarry onlySemgrep.yml), against validation value that is no longer hypothetical — the runner is what proves the nine pins resolve and the three enforcer rules pass on 8/11/17. Either outcome is defensible and the security finding is closed by the.travis.ymldeletion alone. - What this workflow does and does not gate. With
-DskipTestsit is a compile-and-enforcer gate, not a test gate — honestly named, and the reasoning (real tunnels on every push; fork PRs get no secrets) is sound. The durable consequence worth knowing: this binding's test suite will still only ever run on a developer's machine, so CI here will not catch a runtime regression. That is unchanged from before this PR, not caused by it.
Also still outstanding from round 0, unchanged and already disclosed by the author: maven-deploy-plugin 3.1.4 and maven-site-plugin 3.22.0 are the two pins verify never reaches. The ask to run mvn -DperformRelease=true clean deploy against a dry-run target before the next release is the right mitigation and is recorded in both the PR body and the ticket.
1 participant
Supply-chain hardening of the build, from a security review of this repo. No runtime code changes — the published jar, its two dependencies (
commons-io,org.json) andsource/target1.7 are all untouched, so there is no functional impact on consumers.Four findings; they did not all end the same way.
1. Removed
.travis.yml— unverified Maven download (CWE-494).travis.yml:16fetchedapache-maven-3.0-bin.zipover the network and executed it with no integrity check.Travis has not been wired to this repo for years — no Travis status context on any of the last 30 commits on
master, andapi.travis-ci.comdoes not know the repo. So the file was dead config, and deleting it removes the sink outright rather than patching something nothing runs.Worth recording why the obvious fix was not taken: "verify the archive's SHA-512" doesn't work here. No
.sha512is published for that 2010 artifact (…zip.sha512→ 404; only.sha1,.md5,.ascexist), and a checksum served by the same host as the archive gives no protection against that host being compromised — only the GPG.ascagainst Apache's KEYS would. A SHA check would have looked like a fix without being one.2. Added
.github/workflows/build.ymlReplaces the build definition Travis used to carry — this repo otherwise has no build or test CI (only Semgrep and org-level CodeQL).
setup-java, never an ad-hoc downloadmvn -Cmakes a checksum mismatch on any resolved artifact fail the build instead of warningReviewers: this is the one piece that is new surface rather than removal. The security finding is closed by the deletion alone, so if you'd rather not take a new workflow in this PR, it can be dropped without weakening the fix.
3. Pinned every plugin version
maven-gpg-plugin,maven-source-pluginandmaven-javadoc-plugincarried no<version>at all. Maven resolved whatever was newest at build time — todaymvn -DperformRelease=true help:effective-pomgives gpg 3.2.8 / source 3.4.0 / javadoc 3.12.0, none of which any commit here chose. That is on the path that GPG-signs what we publish to Maven Central, so a new upstream release lands in the signing step unreviewed.Six more (
jar,clean,install,site,resources,deploy) took defaults from the running Maven's super-POM — circa-2013 versions under Maven 3.8.6 — so the same source built with a different plugin set on a different machine. All now explicit in<pluginManagement>.maven-enforcer-pluginkeeps it that way:banDynamicVersions+requireReleaseDeps+requirePluginVersions, atvalidate, withfail=true. This is a real gate — before the lifecycle plugins were pinned it failed the build and named all six.Note the report also asked for a Maven lockfile. Not adopted: the dependency graph is 4 artifacts, all at exact versions, no ranges and no SNAPSHOT, resolved from an immutable registry — it is already deterministic. A lockfile plugin would add a third-party build dependency and a standing maintenance obligation for no gain. The plugin non-determinism above is the real instance of that problem and is what got fixed.
4. Upgraded the outdated plugins (CWE-1104)
maven-compiler-plugin2.3.2 (2011) → 3.14.1,maven-surefire-plugin2.4.2 (2007) → 3.5.6. Also setproject.build.sourceEncodingto UTF-8 so the build stops depending on the platform default (all sources are ASCII, so output is unchanged).Testing
mvn -C -Dgpg.skip -DskipTests clean verifyon JDK 8, 11 and 17-DperformRelease=true … packageBROWSERSTACK_ACCESS_KEY, locallymaster— see belowThe end-to-end check started a tunnel via
new Local().start(...)using the jar built from this branch (verified in the process args:--source java-1.1.9), drove a Windows 11 / Chrome session through it to a local origin athttp://bs-local.com:45678/, read the expected marker back, and stopped cleanly viastop()with no leftover processes.Why CI runs with
-DskipTestsThe first version of this PR ran the full suite in CI and all three matrix legs failed. 12 of the 14 tests reach
LocalBinary.getBinary(), which downloads the realBrowserStackLocalbinary from an authenticated endpoint and returnsHTTP 401withoutBROWSERSTACK_ACCESS_KEY. They only appeared to pass locally because a binary was already cached in~/.browserstack; a fresh runner has none, andcache: mavencaches~/.m2only, so it cannot self-heal.-DskipTestsstill compiles the tests, so the job still covers what CI can actually prove here — the enforcer rules, all nine plugin pins, and 1.7 compilation on each JDK. Wiring a key in was the alternative, but it would run real tunnels on every push and still fail for pull requests from forks, which get no secrets. Run the suite locally withBROWSERSTACK_ACCESS_KEY+BROWSERSTACK_USERNAMEset.Two differences you will notice in a local keyed run, neither a regression:
testIsRunningandtestMultipleBinaryguard themselves withassumeNotNull(System.getenv("BROWSERSTACK_ACCESS_KEY")); surefire 2.4.2 did not report assumption failures and counted them as passes. 3.5.6 reports them correctly. Same tests, same behaviour — the old count was wrong.testMultipleBinaryerrors when run with a key (JSONException: A JSONObject text must begin with '{'atLocal.java:81). I ran the same suite onorigin/masterwith the key as a control: it fails identically — same test, same exception, same line. Pre-existing and untouched by this PR, which changes no runtime code. Worth its own issue: starting a second concurrentLocalreturns a non-JSON body thatLocal.startfeeds straight tonew JSONObject(...).Not covered here
maven-gpg-pluginis pinned to 3.2.8, which is the exact version Maven was already resolving — a no-op pin, not an upgrade. But signing needs a release key and deploying would publish, so the release profile was exercised with-Dgpg.skipand nodeploy.maven-deploy-plugin(2.7 → 3.1.4) andmaven-site-plugin(3.3 → 3.22.0) are untested for the same reason. Please runmvn -DperformRelease=true clean deployagainst a dry-run/staging target once before the next release. Central publishing itself is unchanged (central-publishing-maven-plugin0.8.0,autoPublish=false).source/targetstay at 1.7, which JDK 21 rejects — raising the bytecode target would be a functional change for consumers and is deliberately out of scope. Hence the 8/11/17 matrix.