Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (30)
🧰 Additional context used🪛 ast-grep (0.45.3)Library/Homebrew/api.rb[warning] 98-98: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file. (hardcoded-secret-rsa-passphrase-ruby) [warning] 100-100: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file. (hardcoded-secret-rsa-passphrase-ruby) [warning] 104-104: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file. (hardcoded-secret-rsa-passphrase-ruby) Library/Homebrew/test/api_spec.rb[warning] 135-135: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file. (hardcoded-secret-rsa-passphrase-ruby) [warning] 143-143: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file. (hardcoded-secret-rsa-passphrase-ruby) [warning] 152-152: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file. (hardcoded-secret-rsa-passphrase-ruby) [warning] 196-196: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file. (hardcoded-secret-rsa-passphrase-ruby) 🔇 Additional comments (1)
📝 SummarySummary by CodeRabbit
Walkthrough
ChangesAPI cache revalidation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant FetchJsonApiFile
participant CachedAPIFile
participant Curl
FetchJsonApiFile->>CachedAPIFile: Read cached response and saved ETag
FetchJsonApiFile->>Curl: Send conditional request with ETag or timestamp
Curl-->>FetchJsonApiFile: Return response status and temporary ETag
FetchJsonApiFile->>CachedAPIFile: Update cached ETag and clean up temporary sidecar
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The previously identified stale-ETag case is addressed; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Signed package metadata remains verified, with no new download origins or permissions. However, routine updates discard the new validators, and overlapping cache operations can interfere with their publication. These are bounded freshness and recovery risks that could delay package updates. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
925b6dfc-a6b9-40ef-8a7e-ecdf5826df44
📒 Files selected for processing (2)
Library/Homebrew/api.rbLibrary/Homebrew/test/api_spec.rb
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (20)
- GitHub Check: zizmor
- GitHub Check: upload_sarif
- GitHub Check: docker (arm64 Ubuntu 24.04)
- GitHub Check: docker (x86_64 Ubuntu 26.04)
- GitHub Check: docker (arm64 Ubuntu 26.04)
- GitHub Check: docker (x86_64 Ubuntu 24.04)
- GitHub Check: docs
- GitHub Check: tests (macOS, 1/2)
- GitHub Check: tests (load-only Linux)
- GitHub Check: tests (load-only macOS)
- GitHub Check: tests (macOS, 2/2)
- GitHub Check: tests (Linux, 1/2)
- GitHub Check: tests (no Sorbet)
- GitHub Check: tests (Linux, 2/2)
- GitHub Check: tests (generic OS, 2/2)
- GitHub Check: tests (online, 1/2)
- GitHub Check: Analyze
- GitHub Check: tests (online, 2/2)
- GitHub Check: tests (generic OS, 1/2)
- GitHub Check: syntax
🧰 Additional context used
🪛 ast-grep (0.45.3)
Library/Homebrew/test/api_spec.rb
[warning] 122-122: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(args).to include("--etag-compare", etag_path.to_s)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 153-153: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(args).to include("--time-cond", target.to_s)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures
(hardcoded-secret-rsa-passphrase-ruby)
Library/Homebrew/api.rb
[warning] 101-101: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: args.prepend("--etag-compare", etag_path.to_s)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 103-103: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: args.prepend("--time-cond", target.to_s)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 108-108: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: args.prepend("--etag-save", new_etag_path.to_s)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures
(hardcoded-secret-rsa-passphrase-ruby)
fetch_json_api_file touches the cache file to now after every successful check (it doubles as the staleness clock), then sends --time-cond <mtime> next time. A copy fetched from a stale CDN edge therefore sends an If-Modified-Since newer than the real object's Last-Modified, gets 304 forever, and `brew upgrade` keeps reporting "already installed" until the next publish. Save the ETag beside the cache file and send it with --etag-compare when present; fall back to --time-cond otherwise (mirrors, first run after upgrading). curl blanks the ETag file on a 304, so save to a temp path and only replace the stored ETag on a non-empty result.
fc83866 to
d1c0f1f
Compare
CodeRabbit on Homebrew#24193: curl creates an empty --etag-save file for an untagged 200 as well as for a 304, so the old ETag survived a body it no longer describes. Tell them apart by the HTTP status already written to stderr.
--etag-save and --etag-compare need curl 7.68.0 while Homebrew only requires 7.41, so on older curl every API fetch failed. Add curl_supports_etag? and fall back to --time-cond when it is false. brew cleanup --scrub removed the .etag beside the current packages file, so the next fetch lost its validator; keep it. Unlink the empty .etag.new curl leaves after a 304, and rework the specs around a shared stale-cache context with real SystemCommand::Result stubs.
|
Pushed |
- `fetch_api_file` touches the cached `*.jws.json` to now after every check and revalidates with `curl --time-cond`. A body fetched from a stale CDN edge ends up with an mtime newer than the real object's `Last-Modified`, so the server answers `304` until the next publish. - Save the ETag to `<file>.etag` and send `--etag-compare` when one is saved, as `Homebrew::API.fetch_json_api_file` does in Homebrew#24193. Fall back to `--time-cond` without a saved ETag or on curl older than 7.68.0, which lacks `--etag-save` and `--etag-compare`. - curl blanks the `--etag-save` file on a `304` and on an untagged `200`, so save to `<file>.etag.new`, replace `<file>.etag` only when it is non-empty and delete it after a non-`304` without an ETag. This keeps `.etag` in sync with whatever body `brew update` wrote. - Keep the current OS's `.etag` when removing other OS versions' API files.
|
Stacked follow-up for the |
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks! Might be a nice change. Struggling to understand the cases in which this solves problems though.
brew upgradekeeps saying "already installed" until the next publish
Why is this a problem? It is already installed and we don't want to redownload until the next publish? Missing something here.
Can you provide a step-by-step example of how/if/when the existing flow breaks down and what the worst case scenario is and how this fixes it?
I also think it may be worth considering including the update.sh changes in here too so it's rolled out consistently rather than different parts of the code using different ways to verify it "up to date" or not.
| sig { returns(T::Boolean) } | ||
| def curl_supports_etag? | ||
| @curl_supports_etag ||= T.let(Hash.new do |h, key| | ||
| h[key] = curl_version >= Version.new("7.68.0") |
There was a problem hiding this comment.
What's the minimum version we already require in the entire application? I'm wondering when/if we allow using older versions.
Also, looking at your other PR: we should avoid repeating this logic between Ruby and Bash.
This branch has not been deployed
2 participants
fetch_json_api_filebumps the cache file's mtime to now after every successful check (it doubles as thestale_secondsclock), then revalidates withcurl --time-cond <mtime>(anIf-Modified-Since). If the first fetch comes from a stale CDN edge, the stale body ends up with an mtime newer than the real object'sLast-Modified. The server then answers304indefinitely, andbrew upgradekeeps saying "already installed" until the next publish.This saves the response's ETag beside the cache file and revalidates with
--etag-comparewhen present. It falls back to--time-condotherwise (mirrors, first run after upgrading). curl blanks the ETag file on a304, so the new ETag is saved to a temp path and only replaces the stored one when non-empty. Specs cover all three paths.Repro (the mechanism, without needing a stale edge):
Seen for real on 2026-10-07:
claude-code@latestper-cask JSON said 2.1.293, the local internal index (fetched ~16 min after the server'sLast-Modified) said 2.1.292.brew updateandbrew upgrade --cask claude-code@latestboth kept 2.1.292 until I deleted~/Library/Caches/Homebrew/api/internal/packages.arm64_tahoe.jws.json*.brew benchmarkresults.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?Disclosure: diagnosed and drafted with Claude Code (Sonnet 5.5). I reviewed the diff, the root-cause analysis and the curl behaviour (the
304blanking the--etag-savefile) by hand before opening this.🤖 Generated with Claude Code
https://claude.ai/code/session_01XdswxjrNLFURYY68A6ZUSi