Repository navigation
Add nineteen sdkharness conformance checks: large files, copy, URLs, locks, encryption, keys, bucket configuration - #222
sophiecarreras wants to merge 2 commits into
Conversation
…locks, encryption, keys, bucket configuration Slices 4 and 5 of the Java conformance plan, completing one check for each of the 27 capabilities this SDK claims: files.server_side_copy, large.multipart, large.concurrent_parts, large.resume, large.compose_mixed, urls.native_download, client.progress, lock.bucket_default, lock.per_file_retention, lock.legal_hold, lock.bypass_governance, enc.sse_b2, enc.sse_c, keys.crud, bucket.cors, bucket.lifecycle, bucket.notification_rules, bucket.inbound_webhook, bucket.replication_config. Support.java gains an executor helper, an in-flight request counter on the recording client, Object Lock and key cleanup, a second client for a key the check created, and a loopback-only raw GET for share links (covered by GuardSelfTest). The README table lists the new scenarios. Both run-conformance and run-resilience now pass -encoding UTF-8 to javac, so a check source with a non-ASCII literal compiles under LC_ALL=C on a JDK before 18 (the harness's daily run exports LC_ALL=C).
ffumero2003
left a comment
There was a problem hiding this comment.
Thanks for this — it's a big, careful slice, and most of it is exactly what we want: the checks exercise the public API, the concurrency measurement in LargeConcurrentParts is a genuine in-flight count off the recording client (not inferred from wall time), and Support.cleanup() is thorough on the failure path (cancels unfinished large files, releases legal holds and bypasses governance before deleting locked versions, deletes tracked keys, shuts down executors). Requesting changes on two correctness issues plus the red Snyk check before it lands.
Blocking
1. LargeResume turns a real SDK failure into a PASS (masks a bug)
In LargeResume.java (the resume leg, ~L91–104), the check first calls finishUploadingLargeFile(...) with the original request (the large-file-sha1 custom field left null). When the SDK rejects it with a "mismatch" B2LocalException, the check catches it, logs a FINDING ... note, and then re-runs finishUploadingLargeFile with a different request — one that re-supplies sha1 as the custom field — and reports PASS.
That's the problem: the scenario detects that the SDK refuses to resume the same large file with the same request that started it, and then works around that refusal so the cell still reads PASS. Our harness is detect-only — when a scenario hits genuine broken behavior it must report FAIL so the dashboard surfaces the gap, not paper over it with a modified request. As written, large.resume is green while the behavior it exists to prove (resume an interrupted large file with the request you started it with) is actually broken.
Please keep the original request and let the scenario FAIL (surfacing the FINDING text as the failure reason), and track the underlying finishUploadingLargeFile behavior as a separate SDK bug/fix. The workaround branch (the retry that passes sha1 into the custom field) should come out.
2. KeysCrud deleted-key assertion also passes for a valid key
In KeysCrud.java (~L83–84), revocation is asserted by expecting HTTP 401 from:
Support.clientFor(created.getApplicationKeyId(), secret).buckets()But created is a bucket-restricted key (earlier in the check it's verified to list only its one bucket and its authorization is scoped to that bucket id). A bucket-restricted key calling .buckets() — i.e. b2_list_buckets with no bucket named — returns 401 even when the key is perfectly valid, because a restricted key isn't allowed to list all buckets. So this assertion passes whether or not the delete actually revoked anything; it can't establish credential revocation. (Reproduced with a live, un-deleted key: same 401.)
Please assert on authorization itself instead — e.g. call getAccountAuthorization() (a fresh b2_authorize_account) with the deleted key's id/secret and assert that authorization fails. That genuinely distinguishes a revoked key from a valid restricted one.
3. code/snyk is red — two new Code Analysis issues
CI's code/snyk reports 2 new issues; build, license/snyk, and security/snyk are green. I can't see the Snyk Code output directly, so please confirm the specifics there, but reading the diff the two likely hits are:
Support.rawGet— SSRF (openConnection()on a tainted URL). The method guards loopback (throws unless the host is127.0.0.1, andGuardSelfTestnow covers both the pass and block paths), so it's safe at runtime — but it parses the URL twice (URI.create(url)for the host check, thenURI.create(url).toURL()for the connection), so static analysis can't see the value that reachesopenConnection()as sanitized. Parsing once into a singleURI/URL, validatinggetHost()on that object, and connecting on the same object should both clear Snyk and remove the double-parse smell. If it still flags, a scoped suppression with the justification "loopback-only harness code, host asserted127.0.0.1, covered byGuardSelfTest" is fine.- A crypto/secret false positive — most likely the
MD5digest inEncSseC(the...Customer-Key-Md5header is mandated by the B2 SSE-C protocol; it's a protocol checksum of a throwaway test key, not a security hash), or the hardcodedSECRET = "..."literal inBucketInboundWebhook(an offline test fixture). Either is a false positive → keep the value and add a justified suppression with a one-line reason.
Non-blocking (optional, your call)
LargeConcurrentPartsresetsmaxInFlightbetween the one-worker and four-worker legs but not the sharedinFlightcounter; if a request were ever sent without a response, the leftover increment could make the strictserialMax == 1assertion flake. It won't against the fault-free simulator, but aSupport.inFlight.set(0)beside the existingmaxInFlight.set(0)would harden it.Support.cleanup()deletes keys before buckets; forBucketReplicationConfigthe source bucket references the source key id, so on a stricter backend the bucket delete could error (surfaces as a cleanup FAIL, never a silent leak). Fine on the simulator.UrlsNativeDownloaduses a wall-clock token-expiry margin (2s validity + 1.5s). Generous, acceptable.
Once 1 and 2 are addressed and code/snyk is green, this is good to go — the rest is solid.
- large.resume: drop the retry that re-supplied sha1 as the custom field; the original request is kept and the scenario fails with the FINDING as its reason when the SDK refuses it with "mismatch". - keys.crud: assert revocation through a fresh authorization (getAccountAuthorization) with the deleted key; the earlier authorization of the live restricted key is the control. - Support.rawGet: parse the URL once and validate and connect on the same URI. - large.concurrent_parts: reset inFlight beside maxInFlight. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
|
Thanks for the careful review. Pushed 12af861 with the changes below.
Also applied the non-blocking one: All 27 checks were run against the simulator with this checkout: 26 pass, |
Errors fixed, we can't fix the snyk, cause: we don't have access
Context
Adds 19 conformance scenarios to the SDK harness, bringing the total to 27 alongside the existing health check and 16 resilience scenarios.
Changes
Adds coverage for:
Shared helpers provide loopback guards, request recording, concurrency measurement, and cleanup of created resources. Both conformance and resilience runners explicitly compile sources as UTF-8.
Only
.sdkharness/changes; SDK implementation and Gradle configuration remain unchanged.Verification
At commit
12af861:large.resumecorrectly reports FAIL for an existing SDK bug: resuming with the original request is rejected when the content source provides a known SHA-1. The check preserves the original request and does not work around the failure.The reviewer independently reproduced these results using a local simulator revision different from the originally cited pin.
GitHub build, dependency-security, and license checks pass. Snyk Code still reports two issues requiring investigation.