fix(mobile-api): self-host Swagger UI (no CDN) — /api/v1/docs was blank#197
Open
fabiodalez-dev wants to merge 4 commits into
Open
fix(mobile-api): self-host Swagger UI (no CDN) — /api/v1/docs was blank#197fabiodalez-dev wants to merge 4 commits into
fabiodalez-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.
📝 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.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
Code reviewBranch: Automated 6-lens review. The core of the PR — dropping the jsDelivr CDN fallback and serving Swagger UI from vendored local assets only — is correct. Below are findings on pre-existing code that the CDN removal makes more relevant. 🔧 To fix (confirmed — escaping/XSS, rule #3)
ℹ️ To consider (real, below the confirmation threshold)
No findings
|
…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.
Owner
Author
|
Fixes applied (commit
Verified: Still open, not requested in this round: |
![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
Comment on lines
+15
to
+26
| 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.
Suggested change
| 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.
Owner
Author
|
Fixed the last two as well (commit
Verified: All four review findings are now closed. |
fabiodalez-dev
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
GET /api/v1/docs(the Mobile API Swagger UI page) rendered blank: only a.gitkeephad shipped underpublic/assets/swagger-ui/, so the page fell back to the jsDelivr CDN — which is blocked behind an egress firewall / offline.Fix
Vendor Swagger UI 5.18.2 locally and drop the CDN fallback entirely:
assetsBaseUrl()now always returns the localpublic/assets/swagger-ui/path. Removed the deadSWAGGER_UI_VERSIONconst anddocRoot()method. Added a Playwright regression test (tests/swagger-docs.spec.js) asserting the page references local assets and never a CDN, and that the bundle actually renders the API operations.Follow-up fixes from the review (commits
2376fea0,e05e8ffb)$cssUrl/$jsUrl(Host-derived) are nowhtmlspecialchars(ENT_QUOTES,'UTF-8')-escaped before going intohref/src, matching$title/$docUrl(the/api/routes are exempt from the canonical-host redirect, so the Host is attacker-influenceable).create-release.shCRITICAL_FILES, so a release can't silently ship a blank docs page.$docUrlnow usesjson_encode(JSON_HEX_TAG | JSON_UNESCAPED_SLASHES)(correct escaper for a JS string), nothtmlspecialchars.openapi.jsoninstead of an empty#swagger-ui.Verification
php -l+ PHPStan level 5 clean; browser check on/api/v1/docs→ SwaggerUIBundle boots, 29 operation blocks render, 0 JS errors;swagger-docs.spec.js4/4. No runtime CDN dependency remains.