diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 00000000..bce2dc92 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,25 @@ +version: 2 +updates: + # Gradle dependencies. org.json is declared separately in sdk-java, app-java + # and app-javafx; Dependabot opens one PR per module, so check that a bump + # lands everywhere before closing the others. + - package-ecosystem: gradle + directory: "/" + schedule: + interval: weekly + day: monday + open-pull-requests-limit: 5 + labels: + - dependencies + commit-message: + prefix: "chore(deps)" + + - package-ecosystem: github-actions + directory: "/" + schedule: + interval: monthly + labels: + - dependencies + - ci + commit-message: + prefix: "chore(ci)" diff --git a/.github/dependency-scan-allowlist.txt b/.github/dependency-scan-allowlist.txt new file mode 100644 index 00000000..8b301cee --- /dev/null +++ b/.github/dependency-scan-allowlist.txt @@ -0,0 +1,8 @@ +# Vulnerabilities the dependency security scan may ignore, one per line: +# +# +# +# The expiry is mandatory. Once it passes, the finding blocks the scan again and the entry +# has to be re-reviewed — a temporary exception must not become a permanent silence. +# Only add an entry when there is a concrete reason the advisory cannot or need not be acted +# on right now, and say what that reason is. diff --git a/.github/scripts/dependency-report.init.gradle b/.github/scripts/dependency-report.init.gradle new file mode 100644 index 00000000..d2f6c362 --- /dev/null +++ b/.github/scripts/dependency-report.init.gradle @@ -0,0 +1,170 @@ +// Prints every resolved external dependency coordinate, for every module, as +// +// COORD :: +// +// Applied via `--init-script` so no build file in the repo has to change. +// Consumed by .github/scripts/osv_scan.py via .github/workflows/dependency-security.yml. +// +// Resolving the real configurations rather than parsing build files is deliberate: it +// captures transitive dependencies and the version conflict resolution actually picks, +// which is what ends up in the artifact. These projects have no Gradle lockfile, so +// lockfile-based scanners see nothing at all. +// +// THREE SCOPES, because a finding's weight depends entirely on who inherits it: +// +// published — on the runtime classpath of a module we publish to Maven Central. +// Every integrator inherits these. Blocks the scan. +// sample — demo applications and test-only dependencies. Never reaches an +// integrator, but it is still our code and our upgrade to make, so it +// blocks too. (This is the scope that catches a vulnerable JSON library +// left behind in a demo module after the SDK itself was fixed.) +// buildscript — the Gradle plugin classpath. Mostly the Android Gradle Plugin's own +// internals, which cannot be upgraded independently of AGP, so these are +// reported and never block. +// +// The published module list is passed in with -DpublishedModules=:a,:b so this file stays +// identical across repositories. Any module that applies the vanniktech publish plugin is +// added automatically, and one that does so without being declared is reported as an error +// rather than being quietly downgraded to `sample`. +// +// Run locally with: +// ./gradlew -I .github/scripts/dependency-report.init.gradle \ +// -DpublishedModules=:sdk,:sdk-native,:upload-plugin printResolvedDependencies + +import org.gradle.api.artifacts.component.ModuleComponentIdentifier + +// Configure-on-demand leaves subprojects unevaluated unless something asks for them, and an +// unevaluated project reports no configurations — the scan would silently cover part of the +// build and still look clean. +gradle.startParameter.configureOnDemand = false + +// Configurations carrying what a consumer of the published artifact actually gets. +def PUBLISHED_CONFIGS = ["runtimeClasspath", "releaseRuntimeClasspath"] as Set + +// Configurations carrying dependencies we declare for demo apps and tests. +// +// This is a closed list on purpose. "Every resolvable configuration that is not a published +// runtime classpath" looks equivalent and is not: the Android Gradle Plugin hangs its own +// tooling configurations off each project (lintChecks, androidTestUtil, androidJdkImage, +// _internal_aapt2_binary, kotlinCompilerClasspath, ...), which drag in netty, bouncycastle +// and jose4j. Treating those as `sample` makes the scan block on AGP internals we cannot +// upgrade -- the very thing the buildscript scope exists to avoid. +// +// Compile classpaths are included as well as runtime ones. A stale declaration can be +// masked at runtime: if a demo module declares an old version of a library that a project +// dependency also supplies at a newer version, `runtimeClasspath` resolves up and the old +// version disappears, while `compileClasspath` still shows what was actually declared. +def SAMPLE_CONFIGS = [ + "runtimeClasspath", + "compileClasspath", + "releaseRuntimeClasspath", + "releaseCompileClasspath", + "debugRuntimeClasspath", + "debugCompileClasspath", + "debugAndroidTestRuntimeClasspath", + "debugAndroidTestCompileClasspath", + "releaseUnitTestRuntimeClasspath", + "debugUnitTestRuntimeClasspath", + "debugUnitTestCompileClasspath", + "testRuntimeClasspath", + "testCompileClasspath", +] as Set + +// Local project artifacts have no upstream version to look up. +def SKIP_VERSIONS = ["unspecified", ""] as Set + +def PUBLISH_PLUGIN = "com.vanniktech.maven.publish" + +gradle.rootProject { rootProject -> + rootProject.tasks.register("printResolvedDependencies") { + group = "verification" + description = "Print every resolved external dependency coordinate for security scanning." + notCompatibleWithConfigurationCache("Resolves every project's configurations at execution time.") + + doLast { + def declared = (System.getProperty("publishedModules") ?: "") + .split(",").collect { it.trim() }.findAll { it } as Set + + // " " -> scope. Each line reports the scope that module + // reaches the coordinate through; the scanner takes the widest scope across lines. + def found = new TreeMap() + def skipped = [] + def undeclared = [] + + def precedence = ["buildscript": 0, "sample": 1, "published": 2] + + def record = { String coordinate, String scope, String modulePath -> + def key = "${modulePath} ${coordinate}".toString() + def existing = found[key] + if (existing == null || precedence[scope] > precedence[existing]) { + found[key] = scope + } + } + + def collectFrom = { configuration, String scope, String modulePath, String label -> + try { + configuration.incoming.resolutionResult.allComponents.each { component -> + def id = component.id + if (id instanceof ModuleComponentIdentifier && !SKIP_VERSIONS.contains(id.version)) { + record("${id.group}:${id.module}:${id.version}".toString(), scope, modulePath) + } + } + } catch (Exception e) { + // A configuration that cannot resolve here (missing variant, platform-only, needs + // credentials) must not take the whole scan down, but it must be visible: an + // unreported skip reads as "clean" when it is really "unchecked". + skipped << "${label}: ${e.class.simpleName}: ${e.message?.readLines()?.first()}".toString() + } + } + + rootProject.allprojects.each { project -> + def appliesPublishPlugin = project.plugins.hasPlugin(PUBLISH_PLUGIN) + def isPublished = declared.contains(project.path) || appliesPublishPlugin + if (appliesPublishPlugin && !declared.contains(project.path)) { + undeclared << project.path + } + + project.configurations.each { configuration -> + if (!configuration.canBeResolved) { + return + } + def name = configuration.name + def scope + if (isPublished && PUBLISHED_CONFIGS.contains(name)) { + scope = "published" + } else if (SAMPLE_CONFIGS.contains(name)) { + scope = "sample" + } else { + // An AGP/Gradle tooling configuration. Reported, but not ours to upgrade. + scope = "buildscript" + } + collectFrom(configuration, scope, project.path, "${project.path}:${name}") + } + + def buildscriptClasspath = project.buildscript.configurations.findByName("classpath") + if (buildscriptClasspath != null) { + collectFrom(buildscriptClasspath, "buildscript", project.path, + "${project.path}:buildscript.classpath") + } + } + + found.each { key, scope -> + println "COORD ${key} ${scope}" + } + + def distinct = found.keySet().collect { it.split(' ')[1] } as Set + def publishedCoordinates = found.findAll { it.value == "published" } + .keySet().collect { it.split(' ')[1] } as Set + logger.lifecycle("Resolved ${distinct.size()} distinct coordinates " + + "(${publishedCoordinates.size()} published) across ${rootProject.allprojects.size()} modules") + skipped.each { logger.warn("[dependency-report] could not resolve ${it}") } + + if (!undeclared.isEmpty()) { + throw new GradleException( + "These modules apply ${PUBLISH_PLUGIN} but are missing from -DpublishedModules: " + + "${undeclared.join(', ')}. Add them, otherwise their dependencies are scanned as " + + "'sample' and a vulnerability shipped to integrators would not be reported as such.") + } + } + } +} diff --git a/.github/scripts/osv_scan.py b/.github/scripts/osv_scan.py new file mode 100755 index 00000000..adbac0d8 --- /dev/null +++ b/.github/scripts/osv_scan.py @@ -0,0 +1,318 @@ +#!/usr/bin/env python3 +"""Check resolved Gradle dependencies against the OSV vulnerability database. + +Reads "COORD :: " lines (produced by +dependency-report.init.gradle) on stdin and queries https://osv.dev. + +Why this rather than a stock scanner: these projects have no Gradle lockfile, so +lockfile-based scanners see nothing. Scanning the *resolved* graph of every module is +what catches a dependency that only appears transitively, or one that was upgraded in +the SDK but left behind in a demo module. + +Scopes, and which of them fail the run: + + published ships to integrators -> blocks + sample demo apps and test-only deps -> blocks (ours to fix, just not shipped) + buildscript Gradle plugin classpath -> reported, never blocks + +`buildscript` is exempt because it is largely the Android Gradle Plugin's own transitive +internals, which cannot be upgraded independently of AGP. Blocking on them would make the +job permanently red, and a permanently red check is one nobody reads. + + python3 .github/scripts/osv_scan.py < resolved-dependencies.txt + +Options come from the environment so the workflow stays declarative: + OSV_FAIL_ON lowest severity that fails the run (default HIGH) + OSV_ALLOWLIST allowlist file (default .github/dependency-scan-allowlist.txt) + OSV_BLOCKING_SCOPES comma-separated scopes that may fail the run + (default "published,sample") + OSV_REPORT_FILE markdown report for the PR comment (default osv-report.md) + +OSV needs no API key, so this runs on forks and without repository secrets. +""" + +import datetime +import json +import os +import re +import sys +import time +import urllib.error +import urllib.request + +OSV_HOST_PREFIX = "https://api.osv.dev/" +OSV_BATCH_URL = OSV_HOST_PREFIX + "v1/querybatch" +OSV_VULN_URL = OSV_HOST_PREFIX + "v1/vulns/" +BATCH_SIZE = 100 +SEVERITY_ORDER = ["UNKNOWN", "LOW", "MODERATE", "HIGH", "CRITICAL"] +SCOPE_PRECEDENCE = {"buildscript": 0, "sample": 1, "published": 2} +KNOWN_SCOPES = set(SCOPE_PRECEDENCE) + +# Advisory ids arrive inside an OSV response, i.e. from outside this repo, and are +# interpolated into a URL. Accept only the documented shape. +VULN_ID_RE = re.compile(r"^[A-Za-z0-9._-]{1,100}$") + + +def rank(severity): + try: + return SEVERITY_ORDER.index(severity) + except ValueError: + return 0 + + +def _check_url(url): + """Reject any URL that is not a plain https OSV endpoint. + + urlopen would happily accept file:/ or a custom scheme, so the host and scheme are + pinned here rather than trusted from the caller. + """ + if not url.startswith(OSV_HOST_PREFIX): + raise ValueError(f"refusing to fetch a non-OSV URL: {url}") + return url + + +def post_json(url, payload, attempts=4): + """POST with retries. A network failure must abort the scan, never quietly pass it.""" + body = json.dumps(payload).encode() + request = urllib.request.Request(_check_url(url), body, {"Content-Type": "application/json"}) + return _send(request, url, attempts) + + +def get_json(url, attempts=4): + return _send(urllib.request.Request(_check_url(url)), url, attempts) + + +def _send(request, url, attempts): + last_error = None + for attempt in range(attempts): + try: + with urllib.request.urlopen(request, timeout=60) as response: + return json.load(response) + except (urllib.error.URLError, TimeoutError, json.JSONDecodeError) as error: + last_error = error + if attempt < attempts - 1: + time.sleep(2 ** attempt) + raise SystemExit(f"error: could not reach OSV at {url}: {last_error}") + + +def read_coordinates(stream): + """Return {coordinate: {"scope": str, "modules": [str, ...]}}.""" + entries = {} + for line in stream: + parts = line.split() + if len(parts) != 4 or parts[0] != "COORD": + continue + _, module, coordinate, scope = parts + if coordinate.count(":") != 2: + print(f"warning: skipping unparseable coordinate {coordinate!r}", file=sys.stderr) + continue + if scope not in KNOWN_SCOPES: + raise SystemExit(f"error: unknown scope {scope!r} for {coordinate}") + + entry = entries.setdefault(coordinate, {"scope": scope, "modules": set()}) + entry["modules"].add(module) + # The widest-reaching scope wins: an artifact reached both ways still ships. + if SCOPE_PRECEDENCE[scope] > SCOPE_PRECEDENCE[entry["scope"]]: + entry["scope"] = scope + + return {c: {"scope": e["scope"], "modules": sorted(e["modules"])} + for c, e in sorted(entries.items())} + + +def read_allowlist(path): + """Parse " " lines. + + An expiry is mandatory. An entry that has passed its date stops suppressing its + finding, so a temporary exception cannot quietly become permanent. + """ + allowed = {} + if not os.path.exists(path): + return allowed + today = datetime.date.today() + with open(path) as handle: + for number, line in enumerate(handle, 1): + line = line.split("#", 1)[0].strip() + if not line: + continue + parts = line.split(None, 2) + if len(parts) < 3: + raise SystemExit( + f"error: {path}:{number}: expected ' ', " + f"got {line!r}") + vuln_id, expiry_text, reason = parts + try: + expiry = datetime.date.fromisoformat(expiry_text) + except ValueError: + raise SystemExit(f"error: {path}:{number}: {expiry_text!r} is not a YYYY-MM-DD date") + allowed[vuln_id] = {"expiry": expiry, "reason": reason, "expired": expiry < today} + return allowed + + +def query_osv(coordinates): + """Return {coordinate: [vuln id, ...]} for coordinates OSV knows something about.""" + queries = [] + for coordinate in coordinates: + group, name, version = coordinate.split(":") + queries.append({"version": version, + "package": {"name": f"{group}:{name}", "ecosystem": "Maven"}}) + + results = [] + for start in range(0, len(queries), BATCH_SIZE): + chunk = queries[start:start + BATCH_SIZE] + batch = post_json(OSV_BATCH_URL, {"queries": chunk}).get("results", []) + if len(batch) != len(chunk): + raise SystemExit( + f"error: OSV returned {len(batch)} results for {len(chunk)} queries; " + "refusing to report a partial scan as clean") + results.extend(batch) + + hits = {} + for coordinate, result in zip(coordinates, results): + ids = [] + for vuln in result.get("vulns", []): + vuln_id = vuln.get("id", "") + if VULN_ID_RE.match(vuln_id): + ids.append(vuln_id) + else: + print(f"warning: ignoring malformed advisory id {vuln_id!r}", file=sys.stderr) + if ids: + hits[coordinate] = ids + return hits + + +def describe(vuln_id, package_name, cache): + """Fetch severity, summary and fixed versions for one advisory.""" + if vuln_id not in cache: + cache[vuln_id] = get_json(OSV_VULN_URL + vuln_id) + vuln = cache[vuln_id] + + fixed = set() + for affected in vuln.get("affected", []): + if affected.get("package", {}).get("name") != package_name: + continue + for entry in affected.get("ranges", []): + for event in entry.get("events", []): + if "fixed" in event: + fixed.add(event["fixed"]) + + return { + "id": vuln_id, + "severity": (vuln.get("database_specific", {}).get("severity") or "UNKNOWN").upper(), + "summary": (vuln.get("summary") or "").strip(), + "fixed": sorted(fixed), + } + + +def main(): + fail_on = os.environ.get("OSV_FAIL_ON", "HIGH").upper() + if fail_on not in SEVERITY_ORDER: + raise SystemExit(f"error: OSV_FAIL_ON must be one of {', '.join(SEVERITY_ORDER)}") + + blocking_scopes = {s.strip() for s in + os.environ.get("OSV_BLOCKING_SCOPES", "published,sample").split(",") + if s.strip()} + unknown = blocking_scopes - KNOWN_SCOPES + if unknown: + raise SystemExit(f"error: unknown scope(s) in OSV_BLOCKING_SCOPES: {', '.join(sorted(unknown))}") + + allowlist = read_allowlist( + os.environ.get("OSV_ALLOWLIST", ".github/dependency-scan-allowlist.txt")) + + entries = read_coordinates(sys.stdin) + if not entries: + raise SystemExit("error: no dependency coordinates on stdin; the Gradle report step " + "produced nothing, so nothing was actually scanned") + + counts = {scope: sum(1 for e in entries.values() if e["scope"] == scope) + for scope in sorted(KNOWN_SCOPES)} + print(f"Scanning {len(entries)} resolved dependencies " + f"({', '.join(f'{n} {s}' for s, n in counts.items())}), " + f"failing on {fail_on}+ in {'/'.join(sorted(blocking_scopes))}") + + hits = query_osv(list(entries)) + + cache = {} + blocking, suppressed, informational = [], [], [] + for coordinate in sorted(hits): + group, name, _ = coordinate.split(":") + for vuln_id in hits[coordinate]: + finding = describe(vuln_id, f"{group}:{name}", cache) + finding["coordinate"] = coordinate + finding["scope"] = entries[coordinate]["scope"] + finding["modules"] = entries[coordinate]["modules"] + entry = allowlist.get(vuln_id) + + can_block = finding["scope"] in blocking_scopes + + if entry and not entry["expired"]: + finding["reason"] = entry["reason"] + finding["expiry"] = entry["expiry"] + suppressed.append(finding) + elif can_block and rank(finding["severity"]) >= rank(fail_on): + finding["expired_allowlist"] = bool(entry) + blocking.append(finding) + else: + informational.append(finding) + + blocking.sort(key=lambda f: (-rank(f["severity"]), f["coordinate"])) + report(entries, blocking, suppressed, informational, fail_on) + return 1 if blocking else 0 + + +def format_finding(finding): + fixed = ", ".join(finding["fixed"]) if finding["fixed"] else "no fixed version published" + return (f"[{finding['scope']}] {finding['coordinate']} — {finding['id']} " + f"[{finding['severity']}]\n" + f" {finding['summary'] or '(no summary)'}\n" + f" fixed in: {fixed}\n" + f" used by: {', '.join(finding['modules'])}") + + +def report(entries, blocking, suppressed, informational, fail_on): + lines = [] + + if blocking: + lines.append(f"BLOCKING — {len(blocking)} vulnerability(ies) at or above {fail_on}:\n") + for finding in blocking: + lines.append(format_finding(finding)) + if finding.get("expired_allowlist"): + lines.append(" note: this advisory's allowlist entry has expired " + "and needs re-review") + lines.append("") + if suppressed: + lines.append(f"ALLOWLISTED — {len(suppressed)}:\n") + for finding in suppressed: + lines.append(f"[{finding['scope']}] {finding['coordinate']} — {finding['id']} " + f"[{finding['severity']}] until {finding['expiry']}: {finding['reason']}") + lines.append("") + if informational: + lines.append(f"INFORMATIONAL — {len(informational)} " + f"(non-blocking scope, or below the {fail_on} threshold):\n") + for finding in informational: + lines.append(format_finding(finding)) + lines.append("") + if not (blocking or suppressed or informational): + lines.append(f"No known vulnerabilities in {len(entries)} resolved dependencies.") + elif not blocking: + lines.append("No blocking findings.") + + text = "\n".join(lines).strip() + print("\n" + text) + + markdown = ("## Dependency security scan\n\n" + f"Scanned **{len(entries)}** resolved dependencies, failing on **{fail_on}** " + f"and above.\n\n```\n{text}\n```\n") + + summary_path = os.environ.get("GITHUB_STEP_SUMMARY") + if summary_path: + with open(summary_path, "a", encoding="utf-8") as handle: + handle.write(markdown) + + # The job summary only shows on the workflow run page. This file is what the workflow + # posts onto the pull request, where people actually look. + with open(os.environ.get("OSV_REPORT_FILE", "osv-report.md"), "w", encoding="utf-8") as handle: + handle.write(markdown) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/dependency-security.yml b/.github/workflows/dependency-security.yml new file mode 100644 index 00000000..6bb2b948 --- /dev/null +++ b/.github/workflows/dependency-security.yml @@ -0,0 +1,181 @@ +name: "Dependency Security Scan" + +# Runs on pushes/PRs so a bad upgrade is caught immediately, and on a schedule because a +# dependency can become vulnerable without this repo changing at all. +on: + push: + branches: + - master + - staging + pull_request: + branches: + - master + - staging + schedule: + # Mondays 06:00 UTC + - cron: '0 6 * * 1' + workflow_dispatch: + +permissions: + contents: read + +env: + # Modules published to Maven Central. Their runtime dependencies are what every + # integrator inherits, so they are scanned as `published` and always block. + # NOTE: sdk-java only applies the publish plugin when a publish task is requested, so the + # report step cannot auto-detect it -- this list is the only signal. + PUBLISHED_MODULES: ":sdk-java" + +jobs: + osv-scan: + name: OSV scan (all modules) + runs-on: ubuntu-latest + permissions: + contents: read + # Needed to post the scan result as a comment on the pull request. + pull-requests: write + steps: + - name: Checkout code + uses: actions/checkout@v4 + + # NOTE: JDK 17, not the JDK 8 used by gradle.yml. settings.gradle only includes + # :app-javafx on Java 11+, so a JDK 8 run would silently skip that module's + # dependencies -- exactly the blind spot that let a known org.json CVE sit in the demo + # modules after sdk-java had been fixed. + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + java-version: '17' + distribution: 'corretto' + + - name: Set up Gradle + uses: gradle/actions/setup-gradle@v4 + + - name: Resolve dependencies for every module + run: | + set -o pipefail + ./gradlew -q --init-script .github/scripts/dependency-report.init.gradle \ + "-DpublishedModules=$PUBLISHED_MODULES" \ + printResolvedDependencies | tee resolved-dependencies.txt + count=$(grep -c '^COORD' resolved-dependencies.txt || true) + echo "Resolved $count coordinate lines." + if [ "$count" -eq 0 ]; then + echo "::error::Gradle produced no dependency coordinates" + exit 1 + fi + + # Blocks on `published` and `sample` findings -- everything we declare ourselves, + # including the demo modules. `buildscript` is reported only: it is Gradle plugin + # internals, which cannot be upgraded independently of the plugins themselves. + - name: Check resolved dependencies against OSV + id: osv + env: + OSV_FAIL_ON: HIGH + OSV_BLOCKING_SCOPES: published,sample + run: python3 .github/scripts/osv_scan.py < resolved-dependencies.txt + + # The job summary written above only shows on the workflow run page. This is what + # actually puts the result on the pull request. It updates one sticky comment + # instead of adding a new one on every push. + - name: Comment scan result on the pull request + if: always() && github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + const marker = ''; + const outcome = '${{ steps.osv.outcome }}'; + // The report body already carries its own heading. + const status = outcome === 'success' + ? '**Result: passed**' + : '**Result: FAILED — a dependency has a known vulnerability**'; + let report = ''; + try { + report = fs.readFileSync('osv-report.md', 'utf8'); + } catch (e) { + report = `The scan step did not produce a report (outcome: ${outcome}). See the workflow logs.`; + } + const body = [ + marker, + report, + status, + '', + `[Full run log](${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId})`, + ].join('\n'); + + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + per_page: 100, + }); + const existing = comments.find(c => c.body && c.body.includes(marker)); + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + body, + }); + } + + - name: Upload dependency list + if: always() + uses: actions/upload-artifact@v4 + with: + name: resolved-dependencies + path: | + resolved-dependencies.txt + osv-report.md + if-no-files-found: warn + + dependency-submission: + name: Submit dependency graph + # Runs on pushes to the long-lived branches, on the weekly cron, on manual dispatch, + # AND on same-repo pull requests -- the last one is what gives dependency-review a head + # snapshot to diff against. + # Fork PRs are skipped on purpose: `contents: write` is not granted to a workflow + # triggered by a PR from a public fork, so the submit would fail. Covering forks needs + # the two-workflow generate-and-upload + workflow_run pattern from the gradle/actions docs. + if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository + runs-on: ubuntu-latest + permissions: + contents: write + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-java@v4 + with: + java-version: '17' + distribution: 'corretto' + - name: Submit resolved dependency graph to GitHub + uses: gradle/actions/dependency-submission@v4 + + dependency-review: + name: Review dependency changes + # Needs the submission job so the head snapshot exists before the diff. + # `always()` keeps this running on fork PRs, where submission is skipped -- without it, + # a skipped dependency-submission would skip this job too. + # SCOPE: this can only diff Gradle dependencies once BOTH the base branch and the head + # have a submitted snapshot, so expect Actions-only output until master has been through + # dependency-submission at least once. osv-scan is the job that covers Gradle + # dependencies unconditionally. + if: always() && github.event_name == 'pull_request' + needs: dependency-submission + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write + steps: + - uses: actions/checkout@v4 + - name: Fail the PR on newly introduced vulnerable dependencies + uses: actions/dependency-review-action@v4 + with: + fail-on-severity: low + comment-summary-in-pr: on-failure diff --git a/.gitignore b/.gitignore index da46f720..ef03efee 100644 --- a/.gitignore +++ b/.gitignore @@ -3,14 +3,16 @@ /.idea/workspace.xml /.idea/libraries .DS_Store -/build .idea/caches/ -sdkJava/build/ -core/build/ -sdk-java/build/ *.attach_pid* -app-java/out/ data -sdk-java/out/ /.vscode/ -*.class \ No newline at end of file +*.class + +# Gradle build output (all modules, current and future) +build/ +out/ + +# Dependency security scan output +osv-report.md +resolved-dependencies.txt diff --git a/CHANGELOG.md b/CHANGELOG.md index 5df6f63c..b42b9ead 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,6 @@ +## XX.XX.XX +* Updated JSON library version from "20250107" to "20250517". + ## 24.1.6 * Fixed a bug where the request queue would stall after sending the first request, preventing subsequent persisted requests from being sent. diff --git a/app-java/build.gradle b/app-java/build.gradle index 3b90276a..7b39c889 100644 --- a/app-java/build.gradle +++ b/app-java/build.gradle @@ -9,7 +9,7 @@ repositories { } dependencies { - implementation 'org.json:json:20230227' + implementation 'org.json:json:20250517' implementation fileTree(dir: 'libs', include: ['*.jar']) implementation project(path: ':sdk-java') diff --git a/app-javafx/build.gradle b/app-javafx/build.gradle index d175b4a5..9c8be04c 100644 --- a/app-javafx/build.gradle +++ b/app-javafx/build.gradle @@ -10,7 +10,7 @@ java { } javafx { - version = '17.0.10' + version = '17.0.20' modules = [ 'javafx.controls', 'javafx.web' ] } @@ -21,7 +21,7 @@ dependencies { // org.json is used by the demo for pretty-printing widget data; the // SDK also uses it internally (and exposes JSONObject in its API). - implementation 'org.json:json:20230227' + implementation 'org.json:json:20250517' } application { diff --git a/gradle/wrapper/gradle-wrapper.jar b/gradle/wrapper/gradle-wrapper.jar index e708b1c0..1b33c55b 100644 Binary files a/gradle/wrapper/gradle-wrapper.jar and b/gradle/wrapper/gradle-wrapper.jar differ diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index fae08049..4f5eb9dc 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -1,5 +1,7 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionUrl=https\://services.gradle.org/distributions/gradle-8.1.1-bin.zip +distributionUrl=https\://services.gradle.org/distributions/gradle-8.14.5-bin.zip +networkTimeout=10000 +validateDistributionUrl=true zipStoreBase=GRADLE_USER_HOME zipStorePath=wrapper/dists diff --git a/gradlew b/gradlew index 4f906e0c..23d15a93 100755 --- a/gradlew +++ b/gradlew @@ -1,7 +1,7 @@ -#!/usr/bin/env sh +#!/bin/sh # -# Copyright 2015 the original author or authors. +# Copyright © 2015-2021 the original authors. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -15,81 +15,115 @@ # See the License for the specific language governing permissions and # limitations under the License. # +# SPDX-License-Identifier: Apache-2.0 +# ############################################################################## -## -## Gradle start up script for UN*X -## +# +# Gradle start up script for POSIX generated by Gradle. +# +# Important for running: +# +# (1) You need a POSIX-compliant shell to run this script. If your /bin/sh is +# noncompliant, but you have some other compliant shell such as ksh or +# bash, then to run this script, type that shell name before the whole +# command line, like: +# +# ksh Gradle +# +# Busybox and similar reduced shells will NOT work, because this script +# requires all of these POSIX shell features: +# * functions; +# * expansions «$var», «${var}», «${var:-default}», «${var+SET}», +# «${var#prefix}», «${var%suffix}», and «$( cmd )»; +# * compound commands having a testable exit status, especially «case»; +# * various built-in commands including «command», «set», and «ulimit». +# +# Important for patching: +# +# (2) This script targets any POSIX shell, so it avoids extensions provided +# by Bash, Ksh, etc; in particular arrays are avoided. +# +# The "traditional" practice of packing multiple parameters into a +# space-separated string is a well documented source of bugs and security +# problems, so this is (mostly) avoided, by progressively accumulating +# options in "$@", and eventually passing that to Java. +# +# Where the inherited environment variables (DEFAULT_JVM_OPTS, JAVA_OPTS, +# and GRADLE_OPTS) rely on word-splitting, this is performed explicitly; +# see the in-line comments for details. +# +# There are tweaks for specific operating systems such as AIX, CygWin, +# Darwin, MinGW, and NonStop. +# +# (3) This script is generated from the Groovy template +# https://github.com/gradle/gradle/blob/HEAD/platforms/jvm/plugins-application/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt +# within the Gradle project. +# +# You can find Gradle at https://github.com/gradle/gradle/. +# ############################################################################## # Attempt to set APP_HOME + # Resolve links: $0 may be a link -PRG="$0" -# Need this for relative symlinks. -while [ -h "$PRG" ] ; do - ls=`ls -ld "$PRG"` - link=`expr "$ls" : '.*-> \(.*\)$'` - if expr "$link" : '/.*' > /dev/null; then - PRG="$link" - else - PRG=`dirname "$PRG"`"/$link" - fi +app_path=$0 + +# Need this for daisy-chained symlinks. +while + APP_HOME=${app_path%"${app_path##*/}"} # leaves a trailing /; empty if no leading path + [ -h "$app_path" ] +do + ls=$( ls -ld "$app_path" ) + link=${ls#*' -> '} + case $link in #( + /*) app_path=$link ;; #( + *) app_path=$APP_HOME$link ;; + esac done -SAVED="`pwd`" -cd "`dirname \"$PRG\"`/" >/dev/null -APP_HOME="`pwd -P`" -cd "$SAVED" >/dev/null -APP_NAME="Gradle" -APP_BASE_NAME=`basename "$0"` - -# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script. -DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"' +# This is normally unused +# shellcheck disable=SC2034 +APP_BASE_NAME=${0##*/} +# Discard cd standard output in case $CDPATH is set (https://github.com/gradle/gradle/issues/25036) +APP_HOME=$( cd -P "${APP_HOME:-./}" > /dev/null && printf '%s\n' "$PWD" ) || exit # Use the maximum available, or set MAX_FD != -1 to use that value. -MAX_FD="maximum" +MAX_FD=maximum warn () { echo "$*" -} +} >&2 die () { echo echo "$*" echo exit 1 -} +} >&2 # OS specific support (must be 'true' or 'false'). cygwin=false msys=false darwin=false nonstop=false -case "`uname`" in - CYGWIN* ) - cygwin=true - ;; - Darwin* ) - darwin=true - ;; - MINGW* ) - msys=true - ;; - NONSTOP* ) - nonstop=true - ;; +case "$( uname )" in #( + CYGWIN* ) cygwin=true ;; #( + Darwin* ) darwin=true ;; #( + MSYS* | MINGW* ) msys=true ;; #( + NONSTOP* ) nonstop=true ;; esac -CLASSPATH=$APP_HOME/gradle/wrapper/gradle-wrapper.jar +CLASSPATH="\\\"\\\"" # Determine the Java command to use to start the JVM. if [ -n "$JAVA_HOME" ] ; then if [ -x "$JAVA_HOME/jre/sh/java" ] ; then # IBM's JDK on AIX uses strange locations for the executables - JAVACMD="$JAVA_HOME/jre/sh/java" + JAVACMD=$JAVA_HOME/jre/sh/java else - JAVACMD="$JAVA_HOME/bin/java" + JAVACMD=$JAVA_HOME/bin/java fi if [ ! -x "$JAVACMD" ] ; then die "ERROR: JAVA_HOME is set to an invalid directory: $JAVA_HOME @@ -98,88 +132,120 @@ Please set the JAVA_HOME variable in your environment to match the location of your Java installation." fi else - JAVACMD="java" - which java >/dev/null 2>&1 || die "ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH. + JAVACMD=java + if ! command -v java >/dev/null 2>&1 + then + die "ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH. Please set the JAVA_HOME variable in your environment to match the location of your Java installation." + fi fi # Increase the maximum file descriptors if we can. -if [ "$cygwin" = "false" -a "$darwin" = "false" -a "$nonstop" = "false" ] ; then - MAX_FD_LIMIT=`ulimit -H -n` - if [ $? -eq 0 ] ; then - if [ "$MAX_FD" = "maximum" -o "$MAX_FD" = "max" ] ; then - MAX_FD="$MAX_FD_LIMIT" - fi - ulimit -n $MAX_FD - if [ $? -ne 0 ] ; then - warn "Could not set maximum file descriptor limit: $MAX_FD" - fi - else - warn "Could not query maximum file descriptor limit: $MAX_FD_LIMIT" - fi +if ! "$cygwin" && ! "$darwin" && ! "$nonstop" ; then + case $MAX_FD in #( + max*) + # In POSIX sh, ulimit -H is undefined. That's why the result is checked to see if it worked. + # shellcheck disable=SC2039,SC3045 + MAX_FD=$( ulimit -H -n ) || + warn "Could not query maximum file descriptor limit" + esac + case $MAX_FD in #( + '' | soft) :;; #( + *) + # In POSIX sh, ulimit -n is undefined. That's why the result is checked to see if it worked. + # shellcheck disable=SC2039,SC3045 + ulimit -n "$MAX_FD" || + warn "Could not set maximum file descriptor limit to $MAX_FD" + esac fi -# For Darwin, add options to specify how the application appears in the dock -if $darwin; then - GRADLE_OPTS="$GRADLE_OPTS \"-Xdock:name=$APP_NAME\" \"-Xdock:icon=$APP_HOME/media/gradle.icns\"" -fi +# Collect all arguments for the java command, stacking in reverse order: +# * args from the command line +# * the main class name +# * -classpath +# * -D...appname settings +# * --module-path (only if needed) +# * DEFAULT_JVM_OPTS, JAVA_OPTS, and GRADLE_OPTS environment variables. # For Cygwin or MSYS, switch paths to Windows format before running java -if [ "$cygwin" = "true" -o "$msys" = "true" ] ; then - APP_HOME=`cygpath --path --mixed "$APP_HOME"` - CLASSPATH=`cygpath --path --mixed "$CLASSPATH"` - - JAVACMD=`cygpath --unix "$JAVACMD"` - - # We build the pattern for arguments to be converted via cygpath - ROOTDIRSRAW=`find -L / -maxdepth 1 -mindepth 1 -type d 2>/dev/null` - SEP="" - for dir in $ROOTDIRSRAW ; do - ROOTDIRS="$ROOTDIRS$SEP$dir" - SEP="|" - done - OURCYGPATTERN="(^($ROOTDIRS))" - # Add a user-defined pattern to the cygpath arguments - if [ "$GRADLE_CYGPATTERN" != "" ] ; then - OURCYGPATTERN="$OURCYGPATTERN|($GRADLE_CYGPATTERN)" - fi +if "$cygwin" || "$msys" ; then + APP_HOME=$( cygpath --path --mixed "$APP_HOME" ) + CLASSPATH=$( cygpath --path --mixed "$CLASSPATH" ) + + JAVACMD=$( cygpath --unix "$JAVACMD" ) + # Now convert the arguments - kludge to limit ourselves to /bin/sh - i=0 - for arg in "$@" ; do - CHECK=`echo "$arg"|egrep -c "$OURCYGPATTERN" -` - CHECK2=`echo "$arg"|egrep -c "^-"` ### Determine if an option - - if [ $CHECK -ne 0 ] && [ $CHECK2 -eq 0 ] ; then ### Added a condition - eval `echo args$i`=`cygpath --path --ignore --mixed "$arg"` - else - eval `echo args$i`="\"$arg\"" + for arg do + if + case $arg in #( + -*) false ;; # don't mess with options #( + /?*) t=${arg#/} t=/${t%%/*} # looks like a POSIX filepath + [ -e "$t" ] ;; #( + *) false ;; + esac + then + arg=$( cygpath --path --ignore --mixed "$arg" ) fi - i=`expr $i + 1` + # Roll the args list around exactly as many times as the number of + # args, so each arg winds up back in the position where it started, but + # possibly modified. + # + # NB: a `for` loop captures its iteration list before it begins, so + # changing the positional parameters here affects neither the number of + # iterations, nor the values presented in `arg`. + shift # remove old arg + set -- "$@" "$arg" # push replacement arg done - case $i in - 0) set -- ;; - 1) set -- "$args0" ;; - 2) set -- "$args0" "$args1" ;; - 3) set -- "$args0" "$args1" "$args2" ;; - 4) set -- "$args0" "$args1" "$args2" "$args3" ;; - 5) set -- "$args0" "$args1" "$args2" "$args3" "$args4" ;; - 6) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" ;; - 7) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" "$args6" ;; - 8) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" "$args6" "$args7" ;; - 9) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" "$args6" "$args7" "$args8" ;; - esac fi -# Escape application args -save () { - for i do printf %s\\n "$i" | sed "s/'/'\\\\''/g;1s/^/'/;\$s/\$/' \\\\/" ; done - echo " " -} -APP_ARGS=`save "$@"` -# Collect all arguments for the java command, following the shell quoting and substitution rules -eval set -- $DEFAULT_JVM_OPTS $JAVA_OPTS $GRADLE_OPTS "\"-Dorg.gradle.appname=$APP_BASE_NAME\"" -classpath "\"$CLASSPATH\"" org.gradle.wrapper.GradleWrapperMain "$APP_ARGS" +# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script. +DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"' + +# Collect all arguments for the java command: +# * DEFAULT_JVM_OPTS, JAVA_OPTS, and optsEnvironmentVar are not allowed to contain shell fragments, +# and any embedded shellness will be escaped. +# * For example: A user cannot expect ${Hostname} to be expanded, as it is an environment variable and will be +# treated as '${Hostname}' itself on the command line. + +set -- \ + "-Dorg.gradle.appname=$APP_BASE_NAME" \ + -classpath "$CLASSPATH" \ + -jar "$APP_HOME/gradle/wrapper/gradle-wrapper.jar" \ + "$@" + +# Stop when "xargs" is not available. +if ! command -v xargs >/dev/null 2>&1 +then + die "xargs is not available" +fi + +# Use "xargs" to parse quoted args. +# +# With -n1 it outputs one arg per line, with the quotes and backslashes removed. +# +# In Bash we could simply go: +# +# readarray ARGS < <( xargs -n1 <<<"$var" ) && +# set -- "${ARGS[@]}" "$@" +# +# but POSIX shell has neither arrays nor command substitution, so instead we +# post-process each arg (as a line of input to sed) to backslash-escape any +# character that might be a shell metacharacter, then use eval to reverse +# that process (while maintaining the separation between arguments), and wrap +# the whole thing up as a single "set" statement. +# +# This will of course break if any of these variables contains a newline or +# an unmatched quote. +# + +eval "set -- $( + printf '%s\n' "$DEFAULT_JVM_OPTS $JAVA_OPTS $GRADLE_OPTS" | + xargs -n1 | + sed ' s~[^-[:alnum:]+,./:=@_]~\\&~g; ' | + tr '\n' ' ' + )" '"$@"' exec "$JAVACMD" "$@" diff --git a/gradlew.bat b/gradlew.bat index ac1b06f9..5eed7ee8 100644 --- a/gradlew.bat +++ b/gradlew.bat @@ -13,8 +13,10 @@ @rem See the License for the specific language governing permissions and @rem limitations under the License. @rem +@rem SPDX-License-Identifier: Apache-2.0 +@rem -@if "%DEBUG%" == "" @echo off +@if "%DEBUG%"=="" @echo off @rem ########################################################################## @rem @rem Gradle startup script for Windows @@ -25,7 +27,8 @@ if "%OS%"=="Windows_NT" setlocal set DIRNAME=%~dp0 -if "%DIRNAME%" == "" set DIRNAME=. +if "%DIRNAME%"=="" set DIRNAME=. +@rem This is normally unused set APP_BASE_NAME=%~n0 set APP_HOME=%DIRNAME% @@ -40,13 +43,13 @@ if defined JAVA_HOME goto findJavaFromJavaHome set JAVA_EXE=java.exe %JAVA_EXE% -version >NUL 2>&1 -if "%ERRORLEVEL%" == "0" goto execute +if %ERRORLEVEL% equ 0 goto execute -echo. -echo ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH. -echo. -echo Please set the JAVA_HOME variable in your environment to match the -echo location of your Java installation. +echo. 1>&2 +echo ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH. 1>&2 +echo. 1>&2 +echo Please set the JAVA_HOME variable in your environment to match the 1>&2 +echo location of your Java installation. 1>&2 goto fail @@ -56,32 +59,34 @@ set JAVA_EXE=%JAVA_HOME%/bin/java.exe if exist "%JAVA_EXE%" goto execute -echo. -echo ERROR: JAVA_HOME is set to an invalid directory: %JAVA_HOME% -echo. -echo Please set the JAVA_HOME variable in your environment to match the -echo location of your Java installation. +echo. 1>&2 +echo ERROR: JAVA_HOME is set to an invalid directory: %JAVA_HOME% 1>&2 +echo. 1>&2 +echo Please set the JAVA_HOME variable in your environment to match the 1>&2 +echo location of your Java installation. 1>&2 goto fail :execute @rem Setup the command line -set CLASSPATH=%APP_HOME%\gradle\wrapper\gradle-wrapper.jar +set CLASSPATH= @rem Execute Gradle -"%JAVA_EXE%" %DEFAULT_JVM_OPTS% %JAVA_OPTS% %GRADLE_OPTS% "-Dorg.gradle.appname=%APP_BASE_NAME%" -classpath "%CLASSPATH%" org.gradle.wrapper.GradleWrapperMain %* +"%JAVA_EXE%" %DEFAULT_JVM_OPTS% %JAVA_OPTS% %GRADLE_OPTS% "-Dorg.gradle.appname=%APP_BASE_NAME%" -classpath "%CLASSPATH%" -jar "%APP_HOME%\gradle\wrapper\gradle-wrapper.jar" %* :end @rem End local scope for the variables with windows NT shell -if "%ERRORLEVEL%"=="0" goto mainEnd +if %ERRORLEVEL% equ 0 goto mainEnd :fail rem Set variable GRADLE_EXIT_CONSOLE if you need the _script_ return code instead of rem the _cmd.exe /c_ return code! -if not "" == "%GRADLE_EXIT_CONSOLE%" exit 1 -exit /b 1 +set EXIT_CODE=%ERRORLEVEL% +if %EXIT_CODE% equ 0 set EXIT_CODE=1 +if not ""=="%GRADLE_EXIT_CONSOLE%" exit %EXIT_CODE% +exit /b %EXIT_CODE% :mainEnd if "%OS%"=="Windows_NT" endlocal diff --git a/sdk-java/build.gradle b/sdk-java/build.gradle index a7db0009..1480f0e6 100644 --- a/sdk-java/build.gradle +++ b/sdk-java/build.gradle @@ -22,7 +22,7 @@ java { } dependencies { - implementation 'org.json:json:20250107' + implementation 'org.json:json:20250517' implementation 'com.google.code.findbugs:jsr305:3.0.2' testImplementation 'junit:junit:4.13.1' diff --git a/sdk-java/src/test/java/ly/count/sdk/java/internal/ScenarioNetworkDeadlockTests.java b/sdk-java/src/test/java/ly/count/sdk/java/internal/ScenarioNetworkDeadlockTests.java index 5a2cd31a..852ec370 100644 --- a/sdk-java/src/test/java/ly/count/sdk/java/internal/ScenarioNetworkDeadlockTests.java +++ b/sdk-java/src/test/java/ly/count/sdk/java/internal/ScenarioNetworkDeadlockTests.java @@ -134,7 +134,15 @@ public void serverRecovery_sdkResumesSending() throws Exception { Countly.instance().init(configForLocalServer()); Countly.session().begin(); - Thread.sleep(2000); + + // Poll instead of sleeping a fixed 2s and asserting immediately. The + // property under test is "the SDK does not stay stuck sending", so + // waiting longer for isSending() to clear is correct; a genuine deadlock + // still fails the assertion when the budget runs out. + long deadline = System.currentTimeMillis() + 30_000; + while (SDKCore.instance.networking.isSending() && System.currentTimeMillis() < deadline) { + Thread.sleep(100); + } Assert.assertFalse( "SDK should recover from 502 HTML response", diff --git a/sdk-java/src/test/java/ly/count/sdk/java/internal/ScenarioRequestQueueStallTests.java b/sdk-java/src/test/java/ly/count/sdk/java/internal/ScenarioRequestQueueStallTests.java index 5f4c46f4..7dab91b7 100644 --- a/sdk-java/src/test/java/ly/count/sdk/java/internal/ScenarioRequestQueueStallTests.java +++ b/sdk-java/src/test/java/ly/count/sdk/java/internal/ScenarioRequestQueueStallTests.java @@ -1,11 +1,14 @@ package ly.count.sdk.java.internal; import com.sun.net.httpserver.HttpServer; +import java.io.File; import java.io.OutputStream; import java.net.InetSocketAddress; +import java.nio.charset.StandardCharsets; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.BooleanSupplier; import ly.count.sdk.java.Config; import ly.count.sdk.java.Countly; import org.junit.After; @@ -29,12 +32,48 @@ * To reproduce deterministically we hold request #1 open with a CountDownLatch * while we generate a backlog, then release it and assert the backlog drains * without any external trigger. + * + * Timing note: every wait here is a *generous upper bound* on a condition we + * poll for, never a fixed sleep calibrated to one machine. The regression this + * guards against is "the queue stops forever", so a slow CI runner must make + * the test slower, never red. Only the drain assertion is a real assertion — + * the setup waits are preconditions. + * + * Deliberately left on the production session update interval (60s). Shortening + * it makes the SDK's own timer drain the backlog, which would make this test + * pass even with the issue-271 bug present. The callback re-entry path must be + * the only thing that can drain the queue here. */ @RunWith(JUnit4.class) public class ScenarioRequestQueueStallTests { + /** + * Upper bound for a precondition to become true. Not a measurement. + * + * Sized from observed behaviour with a large margin: while a request is in + * flight the synchronous Countly calls in the setup phase have been seen to + * block for ~17s on a developer machine, and the drain runs at roughly one + * request per second. These bounds cost nothing when the test passes -- the + * waits return as soon as their condition holds -- so they are set well + * above the worst case rather than close to the typical case. + */ + private static final long SETUP_TIMEOUT_MS = 60_000; + /** Upper bound for the queue to drain once request #1 is released. */ + private static final long DRAIN_TIMEOUT_MS = 60_000; + /** + * How long the server handler holds request #1 open. Must comfortably + * exceed the whole setup phase: if it expires on its own the request + * completes early and the backlog scenario never happens, which would show + * up as a confusing drain failure rather than an honest timeout. + */ + private static final long HOLD_TIMEOUT_MS = 120_000; + private static final int BACKLOG_SIZE = 5; + private static final int POLL_INTERVAL_MS = 50; + private HttpServer server; private int port; + /** Released in tearDown too, so a failed assertion never leaves the handler thread parked. */ + private final CountDownLatch releaseFirstRequest = new CountDownLatch(1); @Before public void setUp() { @@ -43,6 +82,7 @@ public void setUp() { @After public void tearDown() { + releaseFirstRequest.countDown(); Countly.instance().halt(); if (server != null) { server.stop(0); @@ -57,6 +97,37 @@ private Config configForLocalServer() { .setEventQueueSizeToSend(1); } + /** + * Polls until the condition holds or the budget runs out. + * + * @return true if the condition became true within the budget + */ + private static boolean waitFor(long timeoutMs, BooleanSupplier condition) throws InterruptedException { + long deadline = System.currentTimeMillis() + timeoutMs; + while (System.currentTimeMillis() < deadline) { + if (condition.getAsBoolean()) { + return true; + } + Thread.sleep(POLL_INTERVAL_MS); + } + return condition.getAsBoolean(); + } + + /** Number of persisted request files currently waiting on disk. */ + private static int queuedRequestFileCount() { + File[] files = TestUtils.getTestSDirectory().listFiles(); + if (files == null) { + return 0; + } + int count = 0; + for (File file : files) { + if (file.isFile() && file.getName().startsWith("[CLY]_request_")) { + count++; + } + } + return count; + } + /** * Reproduces the user-reported symptom: with multiple requests piled up * on disk while the network is busy, the queue must drain without @@ -69,7 +140,6 @@ private Config configForLocalServer() { public void backloggedRequests_drainAfterInFlightCompletes() throws Exception { AtomicInteger requestCount = new AtomicInteger(0); CountDownLatch firstRequestArrived = new CountDownLatch(1); - CountDownLatch releaseFirstRequest = new CountDownLatch(1); server = HttpServer.create(new InetSocketAddress(0), 0); port = server.getAddress().getPort(); @@ -79,58 +149,83 @@ public void backloggedRequests_drainAfterInFlightCompletes() throws Exception { // Hold request #1 open until the test has built up a backlog. firstRequestArrived.countDown(); try { - releaseFirstRequest.await(10, TimeUnit.SECONDS); + releaseFirstRequest.await(HOLD_TIMEOUT_MS, TimeUnit.MILLISECONDS); } catch (InterruptedException ignored) { + Thread.currentThread().interrupt(); } } String body = "{\"result\":\"Success\"}"; - exchange.sendResponseHeaders(200, body.length()); - OutputStream os = exchange.getResponseBody(); - os.write(body.getBytes()); - os.close(); + byte[] bytes = body.getBytes(StandardCharsets.UTF_8); + exchange.sendResponseHeaders(200, bytes.length); + try (OutputStream os = exchange.getResponseBody()) { + os.write(bytes); + } }); server.start(); Countly.instance().init(configForLocalServer()); Countly.session().begin(); - // Wait until request #1 has reached the server and is being held. + // Queue the backlog BEFORE waiting for request #1 to land. + // + // Ordering matters here and getting it wrong is what made this test + // flaky. DefaultNetworking.check() no-ops while config.getDeviceId() is + // still null, so the check() triggered by begin_session can lose that + // startup race. Every subsequently queued request calls check() again + // (SDKCore.onRequest -> Signal.Ping -> checkNetworking), so recording + // the events here guarantees the queue head gets dispatched. Waiting + // for request #1 first instead left the 60s session timer as the only + // retry, which is far longer than any sane test timeout. + // + // Each recordEvent flushes a new request to disk; while the server + // holds #1 they all pile up, because check() short-circuits on + // isRunning() == true. + for (int i = 0; i < BACKLOG_SIZE; i++) { + Countly.instance().events().recordEvent("backlog_evt_" + i); + } + + // Precondition: request #1 reached the server and is being held. Assert.assertTrue( - "request #1 should reach the server within 5s", - firstRequestArrived.await(5, TimeUnit.SECONDS) + "request #1 never reached the local server within " + SETUP_TIMEOUT_MS + + "ms - the SDK never sent anything, so the drain scenario could not be set up", + firstRequestArrived.await(SETUP_TIMEOUT_MS, TimeUnit.MILLISECONDS) ); - // Build up a backlog: each recordEvent flushes a new request to disk. - // While the server holds #1, all of these queue up because - // DefaultNetworking.check() short-circuits on isRunning() == true. - final int backlogSize = 5; - for (int i = 0; i < backlogSize; i++) { - Countly.instance().events().recordEvent("backlog_evt_" + i); - } + // Precondition: the backlog is actually persisted. Waiting on the + // observable state beats sleeping a fixed interval and hoping the + // writes landed - that guess is what made this test runner-dependent. + Assert.assertTrue( + "expected " + BACKLOG_SIZE + " queued request files on disk within " + + SETUP_TIMEOUT_MS + "ms, found " + queuedRequestFileCount(), + waitFor(SETUP_TIMEOUT_MS, () -> queuedRequestFileCount() >= BACKLOG_SIZE) + ); - // Give the event flushes time to actually write request files to disk. - Thread.sleep(500); + // Guard against this test quietly becoming vacuous: if the backlog had + // already drained while #1 was held, the assertion below would pass + // without ever exercising the callback re-entry path. + int expectedMinimum = 1 + BACKLOG_SIZE; + Assert.assertTrue( + "the backlog drained before request #1 was released (" + requestCount.get() + + " requests) - the stall scenario was never set up, so this test would " + + "no longer prove anything about issue #271", + requestCount.get() < expectedMinimum + ); // Release #1. From this point on no external code calls check() — // the queue must self-drain via the callback re-entry path that // issue #271 broke. releaseFirstRequest.countDown(); - // Poll for drain. Total expected = 1 (begin_session) + backlogSize. + // The actual assertion. Total expected = 1 (begin_session) + backlog. // Use >= because device-id resolution or merge requests may add extras; - // the regression is "queue stops at 1", so any number > 1 + a generous - // wait is the meaningful signal. - int expectedMinimum = 1 + backlogSize; - long deadline = System.currentTimeMillis() + 10_000; - while (System.currentTimeMillis() < deadline && requestCount.get() < expectedMinimum) { - Thread.sleep(100); - } + // the regression is "queue stops at 1". + boolean drained = waitFor(DRAIN_TIMEOUT_MS, () -> requestCount.get() >= expectedMinimum); Assert.assertTrue( "request queue should drain to >= " + expectedMinimum + " requests " + "without external check() calls — got " + requestCount.get() + " (queue stalled if << expected)", - requestCount.get() >= expectedMinimum + drained ); } } diff --git a/sdk-java/src/test/java/ly/count/sdk/java/internal/TestUtils.java b/sdk-java/src/test/java/ly/count/sdk/java/internal/TestUtils.java index eaaa4849..9f71340d 100644 --- a/sdk-java/src/test/java/ly/count/sdk/java/internal/TestUtils.java +++ b/sdk-java/src/test/java/ly/count/sdk/java/internal/TestUtils.java @@ -309,6 +309,14 @@ public static Map parseQueryParams(String data) { return paramMap; } + /** + * Slack allowed above an expected event duration that was produced by a + * Thread.sleep in a test. Absolute rather than proportional, so it soaks up + * scheduler overrun on a loaded CI runner without meaningfully weakening + * the large, synthetically-set durations some tests assert. + */ + private static final double SLEEP_OVERRUN_TOLERANCE_SECONDS = 2.0; + static void validateEvent(EventImpl gonnaValidate, String key, Map segmentation, int count, Double sum, Double duration, String id, String pvid, String cvid, String peid) { Assert.assertEquals(key, gonnaValidate.key); @@ -324,7 +332,21 @@ static void validateEvent(EventImpl gonnaValidate, String key, Map= duration - delta + && gonnaValidate.duration <= duration + SLEEP_OVERRUN_TOLERANCE_SECONDS); + } } Assert.assertTrue(gonnaValidate.dow >= 0 && gonnaValidate.dow < 7);