feat(032): Security Advisories Check – spec, plan, tasks, and config#4263
feat(032): Security Advisories Check – spec, plan, tasks, and config#4263
Conversation
Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com>
Agent-Logs-Url: https://github.com/LycheeOrg/Lychee/sessions/81f90b10-06c7-406e-8ac0-f3ca79f1be39 Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com>
…point, tests Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com>
Agent-Logs-Url: https://github.com/LycheeOrg/Lychee/sessions/9946b46a-03f9-4ed0-bb55-8c1dcba27496 Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com>
…document singleton composable, fix controller trailing blank line Agent-Logs-Url: https://github.com/LycheeOrg/Lychee/sessions/9946b46a-03f9-4ed0-bb55-8c1dcba27496 Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com>
|
@coderabbitai review ? |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughA new security advisories feature is added that fetches GitHub Security Advisories, evaluates which vulnerabilities match the installed Lychee version, and surfaces matching advisories to administrators through a diagnostic pipeline, REST API endpoint, and login-time dismissible modal UI. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/specs/4-architecture/roadmap.md (1)
111-111:⚠️ Potential issue | 🟡 MinorUpdate the "Last updated" date.
The roadmap's "Last updated" timestamp (line 111) still shows
2026-03-18but should be updated to reflect this change (e.g.,2026-04-06). As per coding guidelines, documentation files should have the update date refreshed when modified.
🧹 Nitpick comments (9)
lang/fa/dialogs.php (1)
297-302: Localize the new advisory dialog strings for Persian users.Line 298 through Line 301 are currently English, which creates mixed-language UI in
fa. Please provide Persian translations for these new keys.lang/nl/dialogs.php (1)
296-301: Please translate the new advisory strings in Dutch locale.At Line 297 through Line 300, new values are English in
lang/nl, which will show mixed-language modal content.lang/ru/dialogs.php (1)
296-301: Localize new advisory labels for Russian users.Line 297 through Line 300 are currently English in
lang/ru; translating these will avoid mixed-language UI in the new modal.lang/zh_CN/dialogs.php (1)
299-304: Translate new advisory strings forzh_CN.Line 300 through Line 303 are English; adding Simplified Chinese values here will keep modal UX consistent.
docs/specs/4-architecture/open-questions.md (1)
2719-2719: Update the "Last updated" date to reflect recent changes.The resolved questions Q-032-01 through Q-032-10 were opened and resolved on 2026-04-06, but the "Last updated" line still shows 2026-03-15. As per coding guidelines, documentation files should update this date when modified.
-*Last updated: 2026-03-15* +*Last updated: 2026-04-06*resources/js/views/LoginPage.vue (1)
54-56: Replaceasync/awaitwith.then()in this Vue method.The
goBack()method violates the project's Vue guideline by usingasync/await. Convert to promise chaining per coding guidelines for**/*.vuefiles.♻️ Proposed fix
-async function goBack() { - await Promise.allSettled([lycheeStore.load(), userStore.refresh()]); - advisoryCheck(); - router.push({ name: "gallery" }); -} +function goBack() { + Promise.allSettled([lycheeStore.load(), userStore.refresh()]).then(() => { + advisoryCheck(); + router.push({ name: "gallery" }); + }); +}resources/js/views/gallery-panels/Albums.vue (1)
273-278: Use Promise chaining instead ofawaitin this Vue handler.Line 273 introduces
awaitin a Vue file; switch this handler to.then()to match repo conventions.♻️ Suggested change
-async function onLoggedIn() { - await Promise.allSettled([lycheeStore.load(), userStore.refresh()]); - albumsStore.load(router); - orderManagementStore.refresh(); - advisoryCheck(); -} +function onLoggedIn() { + Promise.allSettled([lycheeStore.load(), userStore.refresh()]).then(() => { + albumsStore.load(router); + orderManagementStore.refresh(); + advisoryCheck(); + }); +}As per coding guidelines:
**/*.vue: “Do not use await async calls in Vue3, use .then() instead”.resources/js/composables/modals/useAdvisoryModal.ts (1)
28-43: MakeadvisoryCheck()idempotent.The singleton comment promises one check per admin login, but every call before dismissal still issues another request. Add an
inFlight/hasCheckedguard so repeated mounts or route re-entries do not keep hitting/Security/Advisories.♻️ One way to guard duplicate checks
const isAdvisoriesVisible = ref(false); const advisories = ref<App.Http.Resources.Models.SecurityAdvisoryResource[]>([]); +let advisory_check_in_flight: Promise<void> | null = null; +let has_checked_advisories = false; @@ function advisoryCheck() { - if (sessionStorage.getItem(DISMISSED_KEY) !== null) { + if ( + sessionStorage.getItem(DISMISSED_KEY) !== null || + has_checked_advisories || + advisory_check_in_flight !== null + ) { return; } - SecurityAdvisoriesService.getAdvisories() + advisory_check_in_flight = SecurityAdvisoriesService.getAdvisories() .then((response) => { + has_checked_advisories = true; if (response.data.length > 0) { advisories.value = response.data; isAdvisoriesVisible.value = true; } }) .catch(() => { + has_checked_advisories = true; // 401/403 for non-admins or network errors: silently ignore. + }) + .finally(() => { + advisory_check_in_flight = null; }); }tests/Unit/Services/SecurityAdvisoriesServiceTest.php (1)
58-60: Guard fixture loading with an explicit assertion.If the fixture path becomes invalid, this currently fails later during decode; adding an immediate assertion makes failures clearer.
Suggested tweak
// Decode fixture JSON. $json = file_get_contents(base_path('tests/Fixtures/github-security-advisories.json')); + $this->assertNotFalse($json, 'Failed to read tests/Fixtures/github-security-advisories.json'); $this->fixture_data = json_decode($json, false, 512, JSON_THROW_ON_ERROR);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af4f02e8-fa45-4166-8ca3-6571f7c7bb57
📒 Files selected for processing (58)
app/Actions/Diagnostics/Errors.phpapp/Actions/Diagnostics/Pipes/Checks/SecurityAdvisoriesCheck.phpapp/DTO/SecurityAdvisory.phpapp/Http/Controllers/Admin/SecurityAdvisoriesController.phpapp/Http/Requests/Admin/SecurityAdvisories/IndexSecurityAdvisoriesRequest.phpapp/Http/Resources/Models/SecurityAdvisoryResource.phpapp/Metadata/Cache/RouteCacheManager.phpapp/Metadata/Json/AdvisoriesRequest.phpapp/Metadata/Json/ExternalRequestFunctions.phpapp/Services/SecurityAdvisoriesService.phpapp/Services/VersionRangeChecker.phpconfig/features.phpconfig/urls.phpdocs/specs/4-architecture/features/032-security-advisories/plan.mddocs/specs/4-architecture/features/032-security-advisories/spec.mddocs/specs/4-architecture/features/032-security-advisories/tasks.mddocs/specs/4-architecture/open-questions.mddocs/specs/4-architecture/roadmap.mdlang/ar/dialogs.phplang/bg/dialogs.phplang/cz/dialogs.phplang/de/dialogs.phplang/el/dialogs.phplang/en/dialogs.phplang/es/dialogs.phplang/fa/dialogs.phplang/fr/dialogs.phplang/hu/dialogs.phplang/it/dialogs.phplang/ja/dialogs.phplang/nl/dialogs.phplang/no/dialogs.phplang/pl/dialogs.phplang/pt/dialogs.phplang/ru/dialogs.phplang/sk/dialogs.phplang/sv/dialogs.phplang/tr/dialogs.phplang/vi/dialogs.phplang/zh_CN/dialogs.phplang/zh_TW/dialogs.phpresources/js/components/modals/SecurityAdvisoriesModal.vueresources/js/composables/modals/useAdvisoryModal.tsresources/js/lychee.d.tsresources/js/services/security-advisories-service.tsresources/js/views/LoginPage.vueresources/js/views/gallery-panels/Album.vueresources/js/views/gallery-panels/Albums.vueresources/js/views/gallery-panels/Tag.vueresources/js/views/gallery-panels/Timeline.vueroutes/api_v2.phptests/Feature_v2/SecurityAdvisories/IndexTest.phptests/Fixtures/github-security-advisories.jsontests/ImageProcessing/Import/ImportFromServerBrowseTest.phptests/Unit/Actions/Diagnostics/SecurityAdvisoriesCheckTest.phptests/Unit/Caching/CachingConfigTest.phptests/Unit/Services/SecurityAdvisoriesServiceTest.phptests/Unit/Services/VersionRangeCheckerTest.php
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
Feature 032 – Security Advisories Check
PHP Backend
urls.advisories.api_url,urls.advisories.cache_ttl,features.vulnerability-check)app/DTO/SecurityAdvisory.phpapp/Services/VersionRangeChecker.php+ 22 unit testsExternalRequestFunctionswitharray $extra_headers = []app/Metadata/Json/AdvisoriesRequest.php(Accept: application/vnd.github+json)tests/Fixtures/github-security-advisories.jsonapp/Services/SecurityAdvisoriesService.php+ 10 unit testsapp/Actions/Diagnostics/Pipes/Checks/SecurityAdvisoriesCheck.php+ 9 unit tests + registered inErrors.phpapp/Http/Resources/Models/SecurityAdvisoryResource.phpIndexSecurityAdvisoriesRequest+ routeGET /api/v2/Security/Advisories+ 6 feature testsFrontend
resources/js/lychee.d.ts–SecurityAdvisoryResourcetyperesources/js/services/security-advisories-service.tsresources/js/components/modals/SecurityAdvisoriesModal.vue(modal only emits update:visible; composable owns sessionStorage)resources/js/composables/modals/useAdvisoryModal.ts(singleton, documented) + integrated inAlbums.vueQuality Gates
npm run check0 errors,npm run lintcleanOriginal prompt
Created from VS Code.
Summary by CodeRabbit
Release Notes
New Features
Tests