Skip to content

build: reproducible build + retire the unverified Travis Maven download #102

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

Draft
07souravkunda wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

build: reproducible build + retire the unverified Travis Maven download #102

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b9c44c1
build: make the build reproducible and retire the unverified Travis d…
07souravkunda Aug 20, 2026
2fb05a5
ci: skip the credential-gated tests in the build workflow
07souravkunda Aug 20, 2026
d31e688
docs: drop the dead Travis build badge
07souravkunda Aug 20, 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
39 changes: 39 additions & 0 deletions .github/workflows/build.yml
View file
Open in desktop

@07souravkunda 07souravkunda Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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:

  1. 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 — validate passed on every leg before surefire failed, which is how we know requirePluginVersions / banDynamicVersions / requireReleaseDeps and the pinned lifecycle plugins are clean across the matrix, including on the deploy and site lifecycles the rules introspect.
  2. 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.

@07souravkunda 07souravkunda Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Build + unit tests. Replaces the abandoned Travis config, which fetched a Maven
# distribution over the network with no integrity check (CWE-494).
#
# Rules for this file:
# * every third-party action is pinned by full commit SHA, never a mutable tag;
# * Maven comes from the runner image / setup-java, never an ad-hoc download;
# * `-C` makes Maven FAIL (not warn) on a checksum mismatch for any artifact.
name: Build

on:
pull_request:
branches: ["master", "main"]
push:
branches: ["master", "main"]

permissions:
contents: read

jobs:
build:
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
java: ['8', '11', '17']

@07souravkunda 07souravkunda Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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".

@07souravkunda 07souravkunda Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

@07souravkunda 07souravkunda Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

name: build (JDK ${{ matrix.java }})
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1

- uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0
with:
distribution: temurin
java-version: ${{ matrix.java }}
cache: maven

# -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

@07souravkunda 07souravkunda Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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_KEY from 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 -DskipTests plus mvn -B -C test-compile. That still exercises everything this PR actually changes — the three enforcer rules, the nine pinned plugins, compiler 3.14.1, and source/target 1.7 compilation on all three JDKs; or
  • extend the assumeNotNull guard to the other 12 tests so a keyless run skips cleanly instead of erroring, then keep verify as-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.

@07souravkunda 07souravkunda Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

@07souravkunda 07souravkunda Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verified resolved at d31e688. I re-checked rather than taking the commit message for it:

  • build (JDK 8) / (JDK 11) / (JDK 17) are all success on the current head (Build run 32365108197, head d31e6889) — not just on 2fb05a5.
  • The green run is doing real work, not passing vacuously. On every one of the three legs: enforcer:3.6.3 runs and Rule 0: BanDynamicVersions / Rule 1: RequireReleaseDeps / Rule 2: RequirePluginVersions all pass; clean:3.5.0, resources:3.5.0 (main + test), jar:3.5.1 execute — so four of the six newly pinned lifecycle plugins are exercised on the runner, not just declared; and Compiling 3 source files … [debug target 1.7] plus Compiling 1 source file … to target/test-classes confirms -DskipTests compiles the tests as you say. surefire:3.5.6 loads and reports Tests are skipped. BUILD SUCCESS.
  • Root cause corroborated independently: ~/.browserstack/BrowserStackLocal on 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.

23 changes: 0 additions & 23 deletions .travis.yml
View file
Open in desktop

@07souravkunda 07souravkunda Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[nit] Deleting the config leaves its badge behind — README.md:3 on this branch still has:

[![Build Status](https://travis-ci.org/browserstack/browserstack-local-java.svg?branch=master)](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.

@07souravkunda 07souravkunda Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

This file was deleted.

95 changes: 93 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@
</license>
</licenses>

<properties>
<!-- Without this the build uses the platform default encoding, so the same
source produces different bytes on different machines. -->
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
</properties>

<developers>
<developer>
<name>BrowserStack</name>
Expand Down Expand Up @@ -72,6 +79,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-gpg-plugin</artifactId>
<version>3.2.8</version>
<executions>
<execution>
<id>sign-artifacts</id>
Expand All @@ -96,6 +104,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-source-plugin</artifactId>
<version>3.4.0</version>
<executions>
<execution>
<id>attach-sources</id>
Expand All @@ -108,6 +117,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-javadoc-plugin</artifactId>
<version>3.12.0</version>
<executions>
<execution>
<id>attach-javadocs</id>
Expand All @@ -126,6 +136,46 @@
</profiles>

<build>
<!-- Maven binds these six to the lifecycle itself. Left unpinned they take
whatever the running Maven distribution's super-POM happens to default to
(Maven 3.8.6 gives jar 2.4 / resources 2.6 / install 2.4 / deploy 2.7 /
clean 2.5 / site 3.3, all circa 2013) — so the same source builds with a
different plugin set on a different machine. Pinning them makes the build
reproducible and retires the 2013 versions. -->
<pluginManagement>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-clean-plugin</artifactId>
<version>3.5.0</version>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-resources-plugin</artifactId>
<version>3.5.0</version>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<version>3.5.1</version>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-install-plugin</artifactId>
<version>3.1.4</version>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-deploy-plugin</artifactId>
<version>3.1.4</version>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-site-plugin</artifactId>
<version>3.22.0</version>
</plugin>
</plugins>
</pluginManagement>
<plugins>
<plugin>
<groupId>org.sonatype.central</groupId>
Expand All @@ -138,10 +188,51 @@
<autoPublish>false</autoPublish>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-enforcer-plugin</artifactId>
<version>3.6.3</version>
<executions>
<execution>
<id>enforce-deterministic-dependencies</id>
<phase>validate</phase>
<goals>
<goal>enforce</goal>
</goals>
<configuration>
<rules>
<!-- No version ranges / LATEST / RELEASE: every
coordinate resolves to the same artifact on
every build, on every machine. -->
<banDynamicVersions>
<allowSnapshots>false</allowSnapshots>
<allowLatest>false</allowLatest>
<allowRelease>false</allowRelease>
<allowRanges>false</allowRanges>
</banDynamicVersions>
<requireReleaseDeps>
<message>No SNAPSHOT dependencies allowed.</message>
</requireReleaseDeps>
<!-- Every plugin must carry an explicit version, so a new
upstream release cannot silently enter the build (this
is what used to happen on the gpg/source/javadoc
signing path). -->
<requirePluginVersions>
<banLatest>true</banLatest>
<banRelease>true</banRelease>
<banSnapshots>true</banSnapshots>
<phases>clean,deploy,site</phases>
</requirePluginVersions>
</rules>
<fail>true</fail>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>2.3.2</version>
<version>3.14.1</version>
<configuration>
<source>1.7</source>
<target>1.7</target>
Expand All @@ -150,7 +241,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>2.4.2</version>
<version>3.5.6</version>
</plugin>
</plugins>
</build>
Expand Down
Loading