fix(updater): fail-closed patch error handling + redirect-safe releases fetch by fabiodalez-dev · Pull Request #174 · fabiodalez-dev/Pinakes

fabiodalez-dev · 2026-06-15T20:37:05Z

Follow-up to the PR #172 review. Each finding verified against current code; only still-valid ones fixed.

Fixed

applyPreUpdatePatch fail-open → fail-closed. A present, digest-verified pre-update patch that failed to stage (file_put_contents), parsed to a non-array, or threw mid-apply used to return success=true, letting the install proceed half/un-patched. Now success=false (the caller aborts; nothing committed yet). Consistent with the existing invalid handling.
applyPostInstallPatch catch surfaces errors. A runtime exception set only $error but left success=true, hiding it as “no patch applied”. Now success=false, so it surfaces through the caller’s non-fatal warning branch (core update is already done → stays a warning).
fetchReleasesRaw cURL branch redirect-safe. Primary + token-retry contexts now pin FOLLOWLOCATION=false / MAXREDIRS=0 and gate success to 2xx, matching makeGitHubRequest. (Token was already protected by UNRESTRICTED_AUTH=false; this is defense-in-depth/consistency. Asset-download paths keep FOLLOWLOCATION=true on purpose — github.com→CDN.)

Skipped (with reason)

Locale digest keys (it/en/de/fr), sidecar guard removal, digest full-hex regex, scrape-hooks false→null — already addressed in earlier commits.
create-release.sh “draft-safe STEP 9 / cleanup” — false positive: gh release view <tag> (CLI) resolves drafts by tag_name (the 0.7.20 release passed those steps as a draft). Only the REST /releases/tags endpoint 404s on drafts, which is exactly why STEP 9.5 uses the API list.

Validation

PHPStan L5 clean · updater-hardening unit 37/37 (+4 new fail-closed assertions) · all tests/*.unit.php green.

Summary by CodeRabbit

Note di Rilascio

Bug Fixes
- Migliorata la robustezza del sistema di aggiornamento con gestione più rigorosa dei codici di risposta della API
- Rafforzata la gestione degli errori durante il processo di aggiornamento per bloccare operazioni non sicure
- Garantito il fallimento controllato delle operazioni di patching in caso di problemi
Tests
- Aggiunti test di sicurezza per verificare il comportamento del sistema di aggiornamento

…es fetch Follow-ups from the PR #172 review, verified against current code (locale keys, sidecar guard, digest full-hex regex and scrape-hooks read were already done). - applyPreUpdatePatch: a PRESENT, digest-verified patch that then fails to stage, parses to a non-array, or throws mid-apply used to return success=true (fail-open) — letting the install proceed half/un-patched. Now fail-closed (success=false) so the caller aborts; nothing is committed yet, retry is clean. - applyPostInstallPatch catch: a runtime error set only error but left success=true, hiding it as "no patch". Now success=false so it surfaces through the caller's non-fatal warning branch (core update already done → warning). - fetchReleasesRaw cURL branch (primary + token-retry): FOLLOWLOCATION=false / MAXREDIRS=0 and success gated to 2xx, matching makeGitHubRequest's stance. Token was already protected by UNRESTRICTED_AUTH=false; this is defense-in-depth. The asset-download paths keep FOLLOWLOCATION=true on purpose (github.com -> CDN). Skipped (false positive): create-release.sh "draft-safe" finding — gh release view <tag> (CLI) does resolve drafts by tag_name (0.7.20 passed those steps as a draft); only the REST /releases/tags endpoint 404s on drafts (why 9.5 uses the API list). PHPStan L5 clean; updater-hardening unit 37/37; all tests/*.unit.php green.

coderabbitai · 2026-06-15T20:37:19Z

Warning

Review limit reached

@fabiodalez-dev, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 34 minutes and 9 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 422fee27-aba5-4913-a3f2-f51544296827

📥 Commits

Reviewing files that changed from the base of the PR and between e4ae586 and d98af36.

📒 Files selected for processing (2)

app/Support/Updater.php
tests/updater-hardening.unit.php

📝 Walkthrough

Walkthrough

Updater::fetchReleasesRaw() disabilita il follow dei redirect cURL e accetta solo risposte 2xx. applyPreUpdatePatch() imposta success=false su salvataggio fallito, definizione patch non array ed eccezioni. applyPostInstallPatch() imposta result['success']=false sulle eccezioni. I test hardening vengono estesi per coprire tutti questi rami.

Changes

Updater fail-closed hardening

Layer / File(s)	Summary
Disabilitazione redirect cURL in `fetchReleasesRaw` `app/Support/Updater.php`, `tests/updater-hardening.unit.php`	`CURLOPT_FOLLOWLOCATION=false` e `CURLOPT_MAXREDIRS=0` vengono impostati sia sul ramo autenticato sia sul retry senza token; la condizione di successo viene ristretta ai soli 2xx. I test contano le occorrenze di `CURLOPT_FOLLOWLOCATION` per verificare entrambi i rami.
Fail-closed per pre-update e post-install patch `app/Support/Updater.php`, `tests/updater-hardening.unit.php`	`applyPreUpdatePatch()` imposta `success=false` su salvataggio file temporaneo fallito, definizione non array ed eccezioni nel catch. `applyPostInstallPatch()` imposta `result['success']=false` nel catch. I test rimuovono le asserzioni fail-open e verificano via regex che i catch impostino `success=false`.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

fabiodalez-dev/Pinakes#172: Modifica app/Support/Updater.php sugli stessi punti: redirect cURL e gestione fail-closed delle patch pre-update e post-install.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Il titolo descrive accuratamente i due cambiamenti principali: hardening del patch error handling e protezione dei redirect nella fetch delle release.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 60.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/updater-followups

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

fabiodalez-dev · 2026-06-16T05:27:11Z

@coderabbitai full review

coderabbitai · 2026-06-16T05:27:17Z

✅ Action performed

Full review finished.

) - version.json -> 0.7.20.1 (maintenance release, no schema change; the highest migration stays migrate_0.7.20.sql, which is <= 0.7.20.1). - README: add "What's New in v0.7.20.1" (OpenLibrary cover fetch via SSRF-safe per-hop redirect + IP pinning #173; fail-closed updater patches + redirect-safe releases fetch #174). The version badge is dynamic and tracks version.json.

fabiodalez-dev merged commit 50817b8 into main Jun 16, 2026
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(updater): fail-closed patch error handling + redirect-safe releases fetch#174

fix(updater): fail-closed patch error handling + redirect-safe releases fetch#174
fabiodalez-dev merged 1 commit into
mainfrom
fix/updater-followups

fabiodalez-dev commented Jun 15, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 15, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Uh oh!

fabiodalez-dev commented Jun 16, 2026

Uh oh!

coderabbitai Bot commented Jun 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

fabiodalez-dev commented Jun 15, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed

Skipped (with reason)

Validation

Summary by CodeRabbit

Note di Rilascio

Uh oh!

coderabbitai Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Uh oh!

fabiodalez-dev commented Jun 16, 2026

Uh oh!

coderabbitai Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fabiodalez-dev commented Jun 15, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 15, 2026 •

edited

Loading

coderabbitai Bot commented Jun 16, 2026 •

edited

Loading