-
Notifications
You must be signed in to change notification settings - Fork 40
Docs Renderer: Add Grafana-themed layout with nav and toc #2440
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 5 days ago
In general, to fix log injection when logging user-controlled data, sanitize the data before including it in log messages. For plain-text logs this typically means removing or escaping newline and carriage-return characters (and optionally other control characters). The business logic should continue to work on the original value; only the value sent to the logger needs to be sanitized to avoid changing functionality.
Here, the best fix with minimal behavior change is to keep using the original slug for routing and page lookup, but introduce a sanitized variant solely for logging. Right before the debug('Request for slug: %s', slug); call, define const safeSlugForLog = slug.replace(/[\r\n]/g, ''); and log safeSlugForLog instead of slug. This ensures any line breaks an attacker might inject in the request path cannot create additional log entries or corrupt log structure. No new imports are needed, as we can rely on JavaScript’s built-in String.prototype.replace.
Concretely, in packages/plugin-docs-renderer/src/server/server.ts around line 117, replace the single debug('Request for slug: %s', slug); with two lines: one computing safeSlugForLog by stripping \r and \n from slug, and one logging safeSlugForLog. All other uses of slug remain unchanged.
-
Copy modified lines R117-R118
| @@ -114,7 +114,8 @@ | ||
| return; | ||
| } | ||
|
|
||
| debug('Request for slug: %s', slug); | ||
| const safeSlugForLog = slug.replace(/[\r\n]/g, ''); | ||
| debug('Request for slug: %s', safeSlugForLog); | ||
|
|
||
| // 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 5 days ago
In general, to fix log injection issues, any user-controlled data should be sanitized before being written to logs. For plain text logs, this usually means removing or neutralizing newline (\n), carriage return (\r), and other control characters, and optionally clearly marking the boundaries of user input so that injected text cannot masquerade as a separate log entry.
In this file, the problematic value is slug, derived from req.path. The minimal, non‑functional‑breaking fix is to sanitize slug before passing it to debug. We can do this locally at the log call site by creating a sanitized version of the slug that strips \r and \n. This keeps all existing behavior intact (routing, page lookup, etc.) and only affects how the slug is rendered in the log. A simple approach is:
const safeSlug = slug.replace(/[\r\n]/g, '');
debug('Page not found for slug: %s', safeSlug);We will apply this pattern specifically at line 122 (the location CodeQL flagged). No new imports are necessary because this uses only standard String.prototype.replace with a regular expression. We will keep the rest of the function’s behavior unchanged.
-
Copy modified lines R122-R123
| @@ -119,7 +119,8 @@ | ||
| // find the page for this slug | ||
| const page = findPageBySlug(slug, manifest.pages); | ||
| if (!page) { | ||
| debug('Page not found for slug: %s', slug); | ||
| const safeSlug = slug.replace(/[\r\n]/g, ''); | ||
| debug('Page not found for slug: %s', safeSlug); | ||
| res.status(404).send('Page not found'); | ||
| return; | ||
| } |
What this PR does / why we need it:
This PR adds a Grafana themed user interface to the documentation preview server, replacing the basic HTML rendering with a polished three-column layout. The left sidebar displays a collapsible navigation tree with expand/collapse chevrons, the center column renders markdown content styled with Grafana design tokens and the right sidebar shows an auto-generated table of contents from H2/H3 headings.
The scanner now extracts heading data from markdown files to generate table of contents(TOC) and the renderer uses the official
marked-gfm-heading-idextension for GitHub-compatible heading anchors instead of a custom implementation. Note that besides from this, there are no other components such as classes or ids added to the HTML. All styling is done through CSS design tokens and semantic HTML elements, keeping the markup clean and possible to use any other environments such as the new marketing website.Which issue(s) this PR fixes:
Fixes https://github.com/grafana/grafana-community-team/issues/740
Special notes for your reviewer: