-
Notifications
You must be signed in to change notification settings - Fork 40
Plugin docs: Add cli package #2445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Resolved conflicts by: - Taking main's enhanced versions with debug logging, error handling, and DOMPurify sanitization for parser.ts and loader.ts - Keeping branch's server implementation in run.ts - Merging dependencies from both branches in package.json All tests passing (18/18)
…mentation manifest
… manifest generation
…ate server to serve styles
| return; | ||
| } | ||
|
|
||
| debug('Request for slug: %s', slug); |
Check warning
Code scanning / CodeQL
Log injection Medium
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, to fix log injection issues, any value derived from user input should be sanitized before being written to logs. For plain-text logging, the main concern is control characters (notably \n and \r) that can break log line boundaries. A common and sufficient mitigation is to strip these characters or replace them with safe representations before logging, while keeping the original value for application logic.
In this specific case, slug is derived from req.path and used in application logic (finding the page) as well as logging. To avoid changing existing functionality, we should leave slug unchanged for routing/lookup, and introduce a separate, sanitized version only for logging. The simplest approach is:
- Right before each
debug(..., slug)usage, computeconst safeSlug = slug.replace(/[\r\n]/g, '');(or similar). - Use
safeSlugin the debug statements instead ofslug.
We only need to adjust the log line(s) that directly include user input. In the provided snippet, that is line 117. No additional imports or libraries are required; we can use String.prototype.replace with a small regular expression.
Concretely, in packages/plugin-docs-cli/src/server/server.ts, inside the app.get('*', ...) handler, after determining slug and verifying it is non-empty, we will introduce a safeSlug variable that strips CR and LF characters. We will then change the debug('Request for slug: %s', slug); call to use safeSlug. All other logic remains intact.
-
Copy modified lines R117-R118
| @@ -114,7 +114,8 @@ | ||
| return; | ||
| } | ||
|
|
||
| debug('Request for slug: %s', slug); | ||
| const safeSlug = slug.replace(/[\r\n]/g, ''); | ||
| debug('Request for slug: %s', safeSlug); | ||
|
|
||
| // find the page for this slug | ||
| const page = findPageBySlug(slug, manifest.pages); |
| // find the page for this slug | ||
| const page = findPageBySlug(slug, manifest.pages); | ||
| if (!page) { | ||
| debug('Page not found for slug: %s', slug); |
Check warning
Code scanning / CodeQL
Log injection Medium
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, to fix log injection you must sanitize any user-controlled value before logging it. For line-oriented text logs, the key step is to remove or neutralize newline (\n) and carriage return (\r) characters (and optionally other control characters) from user input before it is written to the log. It is also good practice to clearly label user-controlled segments in log messages, but here the message template already does that.
For this specific case, the best minimal-impact fix is to sanitize the slug immediately before logging it, so the rest of the application can continue to use the original slug logic without alteration. Since we already compute slug once and reuse it, we can derive a sanitized copy for logging only. We can do this inline in the debug call using slug.replace(/\r|\n/g, ''), which strips any line breaks from user input. This avoids adding new helpers or changing behavior elsewhere in the function.
Concretely:
- In
packages/plugin-docs-cli/src/server/server.ts, within theapp.get('*', ...)handler, update the log line where a missing page is reported so that it logs a sanitized version ofslug. - Replace
debug('Page not found for slug: %s', slug);withdebug('Page not found for slug: %s', slug.replace(/\r|\n/g, ''));. - No additional imports or methods are required;
String.prototype.replacewith a regular expression is part of standard JavaScript/TypeScript.
This keeps existing functionality intact and only modifies how the user-controlled value is emitted to logs.
-
Copy modified line R122
| @@ -119,7 +119,7 @@ | ||
| // find the page for this slug | ||
| const page = findPageBySlug(slug, manifest.pages); | ||
| if (!page) { | ||
| debug('Page not found for slug: %s', slug); | ||
| debug('Page not found for slug: %s', slug.replace(/\r|\n/g, '')); | ||
| res.status(404).send('Page not found'); | ||
| return; | ||
| } |
What this PR does / why we need it:
This PR splits
grafana/plugin-docs-rendererinto two focused packages: a minimal rendering library (grafana/plugin-docs-renderer) and a separate CLI tool (grafana/plugin-docs-cli). The renderer package is now streamlined to only handle markdown-to-HTML parsing with just 4 core dependencies, minimizing the security surface area for use in the Grafana website. All CLI functionality, including the development server, documentation scanner, manifest generator and TOC utilities, has been moved to the new@grafana/plugin-docs-clipackage, which depends on the renderer for markdown parsing. This separation enables independent versioning and allows the website to consume only the lightweight rendering library while plugin developers get the full CLI tooling.Which issue(s) this PR fixes:
Fixes https://github.com/grafana/grafana-community-team/issues/746
Special notes for your reviewer: