Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support cancel rendering on client cancellation #588

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Support cancel rendering on client cancellation #588

wants to merge 4 commits into from

Conversation

AgnesToulet
Copy link
Contributor

Currently when client cancels the requests, the rendering request continues to run until it ends.

This PR fixes that by stopping the rendering process.

It has two main benefits:

  • Improve performance when the image renderer is receiving a lot of requests from Grafana, especially when it comes from the alerting feature. In the alerting feature, image renderer requests are stacked when they exceed the concurrent request limit, so they can be made after a few seconds, be cancelled right after that because they exceed the timeout and still use the image renderer CPU and memory to render the image.
  • Remove logs Failed to get render key from cache. This is displayed when the image renderer tries to access Grafana after the request is cancelled because the render key has been deleted but this makes it more complicated to investigate real issues.

Fixes #186

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 17, 2024
View reviewed changes
src/browser/browser.ts
} catch (err) {
this.log.error('Error while trying to prepare page for screenshot', 'url', options.url, 'err', err.stack);
}
await page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000, signal });

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

The
URL
Loading
of this request depends on a
user-provided value
Loading
.

Copilot Autofix AI 4 days ago

To fix the problem, we need to ensure that the user-provided URL is validated against a whitelist of allowed URLs or domains. This will prevent an attacker from making arbitrary requests from the server. We can achieve this by implementing a validation function that checks if the URL belongs to an allowed list of domains.

  1. Create a list of allowed domains.
  2. Implement a function to validate the user-provided URL against this list.
  3. Use the validated URL in the page.goto method.
Suggested changeset 1
src/service/http-server.ts
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/service/http-server.ts b/src/service/http-server.ts
--- a/src/service/http-server.ts
+++ b/src/service/http-server.ts
@@ -24,2 +24,16 @@
 const upload = multer({ storage: multer.memoryStorage() });
+const allowedDomains = ['example.com', 'another-example.com'];
+
+function validateUrl(url) {
+  try {
+    const parsedUrl = new URL(url);
+    if (allowedDomains.includes(parsedUrl.hostname)) {
+      return url;
+    } else {
+      throw boom.badRequest('Invalid URL domain');
+    }
+  } catch (e) {
+    throw boom.badRequest('Invalid URL');
+  }
+}
 
@@ -191,4 +205,5 @@
 
+    const validatedUrl = validateUrl(req.query.url);
     const options: ImageRenderOptions = {
-      url: req.query.url,
+      url: validatedUrl,
       width: req.query.width,
EOF
@@ -24,2 +24,16 @@
const upload = multer({ storage: multer.memoryStorage() });
const allowedDomains = ['example.com', 'another-example.com'];

function validateUrl(url) {
try {
const parsedUrl = new URL(url);
if (allowedDomains.includes(parsedUrl.hostname)) {
return url;
} else {
throw boom.badRequest('Invalid URL domain');
}
} catch (e) {
throw boom.badRequest('Invalid URL');
}
}

@@ -191,4 +205,5 @@

const validatedUrl = validateUrl(req.query.url);
const options: ImageRenderOptions = {
url: req.query.url,
url: validatedUrl,
width: req.query.width,
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
src/browser/browser.ts

await page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000 });
await page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000, signal });

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

The
URL
Loading
of this request depends on a
user-provided value
Loading
.

Copilot Autofix AI 4 days ago

To fix the SSRF vulnerability, we need to validate and sanitize the url parameter before using it in the page.goto function. The best way to do this is to use an allow-list of trusted domains or URLs and ensure that the user-provided URL matches one of these trusted entries. This approach prevents attackers from specifying arbitrary URLs.

  1. Create a list of allowed domains or URLs.
  2. Validate the url parameter against this list.
  3. If the url parameter is not in the allow-list, reject the request with an appropriate error message.
Suggested changeset 1
src/service/http-server.ts
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/service/http-server.ts b/src/service/http-server.ts
--- a/src/service/http-server.ts
+++ b/src/service/http-server.ts
@@ -277,2 +277,8 @@
 
+    const allowedDomains = ['example.com', 'trusted.com']; // Add your trusted domains here
+    const url = new URL(req.query.url);
+    if (!allowedDomains.includes(url.hostname)) {
+      throw boom.badRequest('Invalid url parameter');
+    }
+
     const headers: HTTPHeaders = {};
EOF
@@ -277,2 +277,8 @@

const allowedDomains = ['example.com', 'trusted.com']; // Add your trusted domains here
const url = new URL(req.query.url);
if (!allowedDomains.includes(url.hostname)) {
throw boom.badRequest('Invalid url parameter');
}

const headers: HTTPHeaders = {};
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@AgnesToulet AgnesToulet requested review from a team, juanicabanas and evictorero and removed request for a team December 17, 2024 20:08
juanicabanas
juanicabanas approved these changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@juanicabanas juanicabanas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Tested and works as expected 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanicabanas juanicabanas juanicabanas approved these changes

@evictorero evictorero Awaiting requested review from evictorero evictorero was automatically assigned from grafana/sharing-squad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Server doesn't cancel browser rendering when client cancel
2 participants
@AgnesToulet @juanicabanas