fix(mobile-api): Swagger UI self-hosted (niente CDN) — /api/v1/docs era vuoto#197
fix(mobile-api): Swagger UI self-hosted (niente CDN) — /api/v1/docs era vuoto#197fabiodalez-dev wants to merge 4 commits into
Conversation
GET /api/v1/docs rendered blank wherever the jsDelivr CDN is blocked (egress firewall): only public/assets/swagger-ui/.gitkeep shipped, so the controller fell back to cdn.jsdelivr.net for swagger-ui-bundle.js/css and the bundle never loaded. Vendor swagger-ui-dist 5.18.2 (bundle + css) into the repo so it ships in the release ZIP, and make SwaggerUiController always serve the local copy — no CDN reference at all. The openapi.json spec was already fine. Verified locally: /api/v1/docs references /assets/swagger-ui/* (no jsdelivr), the assets serve 200, and Swagger UI renders the 29 operations.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSwaggerUiController usa asset Swagger UI locali, aggiorna l’escaping delle URL e aggiunge verifiche di release e test per gli asset vendorizzati. ChangesAsset Swagger UI locali
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
tests/swagger-docs.spec.js asserts /api/v1/docs is self-hosted: openapi.json is a valid OpenAPI 3 spec, the docs page references /assets/swagger-ui/* and never a CDN (jsdelivr/cdnjs/unpkg), the local bundle+css are actually served, and Swagger UI renders the operations from the local bundle. Skips cleanly when the Mobile API plugin is not active.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Code reviewBranch: Revisione automatica a 6 lenti (correttezza, struttura/blast-radius, compliance CLAUDE.md, commenti, UX, sicurezza). Il cuore del PR — rimuovere il fallback CDN jsDelivr e servire Swagger UI solo da asset locali vendored — è corretto. Sotto, i rilievi sul codice già esistente che la rimozione del fallback rende più rilevanti. 🔧 Da sistemare (confermato — escaping/XSS, regola #3)
ℹ️ Da valutare (reali ma sotto la soglia di conferma)
Senza rilievi
|
…red assets in release Two follow-ups from the self-hosted Swagger UI change, both low-risk: - SwaggerUiController::buildHtml() now wraps $cssUrl/$jsUrl (built from the Host-derived $assetsBaseUrl) in htmlspecialchars(ENT_QUOTES,'UTF-8'), matching the sibling $title/$docUrl. The /api/v1/docs route is exempt from the canonical-host redirect, so an arbitrary Host header reaches the page; without escaping a Host containing `"`/`>` could break out of the href/src attribute (reflected injection) and it violated the "escape all HTML attributes" rule. Legitimate hosts are unaffected (htmlspecialchars is identity on a clean URL). - create-release.sh: add public/assets/swagger-ui/swagger-ui-bundle.js and swagger-ui.css to CRITICAL_FILES. With the CDN fallback gone, /api/v1/docs hard- depends on these vendored files; the release gate guarded TinyMCE but not these, so a future export-ignore/removal could ship a blank docs page with a green build. Verified: php -l + PHPStan level 5 clean; bash -n clean; both assets present (1.4 MB / 152 KB) and git-tracked so the new gate entries pass; escaping confirmed to neutralize a malicious Host while leaving normal asset URLs unchanged.
|
Fix applicati (commit
Verificato: Restano aperti, non richiesti in questo giro: |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/swagger-docs.spec.js`:
- Around line 15-26: The skip gate in the swagger-docs Playwright spec currently
depends on probing GET /api/v1/openapi.json in test.beforeAll, which can hide
regressions by skipping the whole spec when that endpoint breaks. Remove this
availability probe from the beforeAll/beforeEach flow and make the guard rely
only on CI-injected E2E environment signals, consistent with the other
Playwright specs using E2E_ADMIN_EMAIL and E2E_ADMIN_PASS; keep the logic in
test.beforeEach and the apiAvailable-related setup aligned with that pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5c54c20-6cf2-4a66-8ca2-241d2abbbf66
📒 Files selected for processing (3)
scripts/create-release.shstorage/plugins/mobile-api/src/Controllers/SwaggerUiController.phptests/swagger-docs.spec.js
| test.beforeAll(async ({ request }) => { | ||
| try { | ||
| const res = await request.get(`${BASE}/api/v1/openapi.json`); | ||
| apiAvailable = res.status() === 200; | ||
| } catch { | ||
| apiAvailable = false; | ||
| } | ||
| }); | ||
|
|
||
| test.beforeEach(() => { | ||
| test.skip(!apiAvailable, 'Mobile API plugin not active / app not installed'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Non usare l’endpoint sotto test come condizione di skip.
Se la regressione rompe GET /api/v1/openapi.json su Line 17, apiAvailable diventa false e da Line 25 in poi l’intero spec viene saltato invece di fallire. Così il test non protegge più proprio il bug che dovrebbe intercettare. Il gate deve dipendere solo dai segnali d’ambiente che CI inietta per gli E2E, oppure questo probe va rimosso del tutto.
Diff proposto
const { test, expect } = require('`@playwright/test`');
const BASE = process.env.E2E_BASE_URL || 'http://localhost:8081';
-
-// Public endpoints (no bearer token needed) but they only exist when the
-// bundled Mobile API plugin is active. Probe once and skip cleanly otherwise.
-let apiAvailable = true;
+const ADMIN_EMAIL = process.env.E2E_ADMIN_EMAIL;
+const ADMIN_PASS = process.env.E2E_ADMIN_PASS;
test.describe.serial('Mobile API — Swagger UI docs (self-hosted, no CDN)', () => {
- test.beforeAll(async ({ request }) => {
- try {
- const res = await request.get(`${BASE}/api/v1/openapi.json`);
- apiAvailable = res.status() === 200;
- } catch {
- apiAvailable = false;
- }
- });
-
test.beforeEach(() => {
- test.skip(!apiAvailable, 'Mobile API plugin not active / app not installed');
+ test.skip(!ADMIN_EMAIL || !ADMIN_PASS, 'Credenziali E2E admin non configurate');
});Based on learnings, i Playwright E2E in questo repo devono essere gated da variabili d’ambiente che CI inietta, e negli UI-only spec il guard deve usare solo E2E_ADMIN_EMAIL e E2E_ADMIN_PASS.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.beforeAll(async ({ request }) => { | |
| try { | |
| const res = await request.get(`${BASE}/api/v1/openapi.json`); | |
| apiAvailable = res.status() === 200; | |
| } catch { | |
| apiAvailable = false; | |
| } | |
| }); | |
| test.beforeEach(() => { | |
| test.skip(!apiAvailable, 'Mobile API plugin not active / app not installed'); | |
| }); | |
| const { test, expect } = require('`@playwright/test`'); | |
| const BASE = process.env.E2E_BASE_URL || 'http://localhost:8081'; | |
| const ADMIN_EMAIL = process.env.E2E_ADMIN_EMAIL; | |
| const ADMIN_PASS = process.env.E2E_ADMIN_PASS; | |
| test.describe.serial('Mobile API — Swagger UI docs (self-hosted, no CDN)', () => { | |
| test.beforeEach(() => { | |
| test.skip(!ADMIN_EMAIL || !ADMIN_PASS, 'Credenziali E2E admin non configurate'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/swagger-docs.spec.js` around lines 15 - 26, The skip gate in the
swagger-docs Playwright spec currently depends on probing GET
/api/v1/openapi.json in test.beforeAll, which can hide regressions by skipping
the whole spec when that endpoint breaks. Remove this availability probe from
the beforeAll/beforeEach flow and make the guard rely only on CI-injected E2E
environment signals, consistent with the other Playwright specs using
E2E_ADMIN_EMAIL and E2E_ADMIN_PASS; keep the logic in test.beforeEach and the
apiAvailable-related setup aligned with that pattern.
Source: Learnings
…asset-load failure
Two remaining Swagger UI docs-page follow-ups, both low-risk:
- buildHtml() now JSON-encodes $openApiUrl with json_encode(JSON_HEX_TAG |
JSON_UNESCAPED_SLASHES) instead of htmlspecialchars(). The value is consumed
inside a JS string literal (SwaggerUIBundle url:), where HTML escaping is the
wrong escaper (it leaves backslashes untouched); json_encode is the correct
JS-context escaper and JSON_HEX_TAG blocks a </script> breakout. It already
emits the surrounding quotes, so the heredoc drops them (url: {$docUrl}).
- The inline script now guards `typeof SwaggerUIBundle === 'undefined'` and
renders a diagnostic (with a link to the raw openapi.json) into #swagger-ui
instead of leaving a blank page. Self-hosted with no CDN fallback, a missing
vendored bundle would otherwise show only the header bar with no explanation.
Verified: php -l + PHPStan level 5 clean; the generated inline JS is well-formed
for normal URLs and neutralizes a malicious Host (\" and < escapes); browser
check on /api/v1/docs — SwaggerUIBundle boots, 29 operation blocks render, no JS
errors.
|
Sistemati anche gli ultimi due (commit
Verificato: Tutti e 4 i rilievi della review sono ora chiusi. |
1 participant
/api/v1/docs(la Swagger UI dell'API mobile) era vuoto sui server dove il CDN jsDelivr è bloccato (egress firewall): nel repo c'era solopublic/assets/swagger-ui/.gitkeep, quindiSwaggerUiControllerripiegava sucdn.jsdelivr.netperswagger-ui-bundle.js/.csse il bundle non veniva mai caricato.Fix
swagger-ui-dist5.18.2 (swagger-ui-bundle.js+swagger-ui.css) inpublic/assets/swagger-ui/→ spediti nello ZIP di release, la pagina funziona offline / dietro firewall.SwaggerUiControllerora serve sempre la copia locale: nessun riferimento CDN, rimosso il fallback jsDelivr.openapi.jsonera già a posto.Verifica
/api/v1/docsreferenzia/assets/swagger-ui/*(niente jsdelivr), gli asset servono 200, Swagger UI renderizza le 29 operazioni.Summary by CodeRabbit
New Features
Bug Fixes
openapie asset) e aggiunta una diagnostica in pagina se il bundle non risulta disponibile.Chores