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

feat/567-user-data-dir-config #569

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

feat/567-user-data-dir-config #569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c6f7462
Added #567, PUPPETEER_DIR made configurable.
jszuminski Aug 13, 2024
05c0f93
Update CHANGELOG.md with #567.
jszuminski Aug 13, 2024
e2d3b1c
Fixed a naming conflict puppeteer-puppeteerOptions.
jszuminski Aug 13, 2024
cbc0130
Add env verifier for paths, add PUPPETEER_DIR verifier with a unit test.
jszuminski Aug 13, 2024
86075d2
Changed puppeteer.dir to puppeteer.tempDir.
jszuminski Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
# 4.0.3

_New features:_

- Made the temporary Puppeteer directory (`PUPPETEER_DIR`) (till now, `'./tmp'`) configurable by the user [(#567)](https://github.com/highcharts/node-export-server/issues/567)

# 4.0.2

_Hotfix_:
Expand Down
4 changes: 2 additions & 2 deletions lib/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,14 @@ export function get() {
*/
export async function create(puppeteerArgs) {
// Get debug and other options
const { debug, other } = getOptions();
const { puppeteer: puppeteerOptions, debug, other } = getOptions();

// Get the debug options
const { enable: enabledDebug, ...debugOptions } = debug;

const launchOptions = {
headless: other.browserShellMode ? 'shell' : true,
userDataDir: './tmp/',
userDataDir: puppeteerOptions.tempDir,
args: puppeteerArgs,
handleSIGINT: false,
handleSIGTERM: false,
Expand Down
21 changes: 21 additions & 0 deletions lib/envs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,24 @@
)
.transform((value) => (value !== '' ? value : undefined)),

// Checks if the string is a valid path directory (path format)
path: () =>
z
.string()
.trim()
.refine(
(value) => {
// Simplified regex to match both absolute and relative paths
return /^(\.\/|\.\.\/|\/|[a-zA-Z]:\\|[a-zA-Z]:\/)?([\w-]+[\\/]?)+$/.test(

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings containing many repetitions of '-'.

Copilot Autofix AI about 2 months ago

To fix the problem, we need to modify the regular expression to remove the ambiguity that leads to exponential backtracking. Specifically, we should ensure that the - character is not ambiguously included in the \w character class. One way to achieve this is to explicitly exclude the hyphen from the \w character class and handle it separately.

The best way to fix this without changing the existing functionality is to use a non-ambiguous character class that clearly separates the hyphen from the word characters. We can use a character class that includes word characters and underscores, and then explicitly add the hyphen as an alternative.

Suggested changeset 1
lib/envs.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/lib/envs.js b/lib/envs.js
--- a/lib/envs.js
+++ b/lib/envs.js
@@ -74,3 +74,3 @@
           // Simplified regex to match both absolute and relative paths
-          return /^(\.\/|\.\.\/|\/|[a-zA-Z]:\\|[a-zA-Z]:\/)?([\w-]+[\\/]?)+$/.test(
+          return /^(\.\/|\.\.\/|\/|[a-zA-Z]:\\|[a-zA-Z]:\/)?(([\w]+|-)[\\/]?)+$/.test(
             value
EOF
@@ -74,3 +74,3 @@
// Simplified regex to match both absolute and relative paths
return /^(\.\/|\.\.\/|\/|[a-zA-Z]:\\|[a-zA-Z]:\/)?([\w-]+[\\/]?)+$/.test(
return /^(\.\/|\.\.\/|\/|[a-zA-Z]:\\|[a-zA-Z]:\/)?(([\w]+|-)[\\/]?)+$/.test(
value
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
value
);
},
{},
{
message: 'The string is an invalid path directory string.'
}
),

// Allows positive numbers or no value in which case the returned value will
// be undefined
positiveNum: () =>
Expand Down Expand Up @@ -127,6 +145,9 @@
HIGHCHARTS_CACHE_PATH: v.string(),
HIGHCHARTS_ADMIN_TOKEN: v.string(),

// puppeteer
PUPPETEER_DIR: v.path(),

// export
EXPORT_TYPE: v.enum(['jpeg', 'png', 'pdf', 'svg']),
EXPORT_CONSTR: v.enum(['chart', 'stockChart', 'mapChart', 'ganttChart']),
Expand Down
6 changes: 6 additions & 0 deletions lib/schemas/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,12 @@ export const defaultConfig = {
],
type: 'string[]',
description: 'Arguments array to send to Puppeteer.'
},
tempDir: {
value: './tmp/',
type: 'string',
envLink: 'PUPPETEER_DIR',
description: 'The directory for Puppeteer to store temporary files.'
}
},
highcharts: {
Expand Down
21 changes: 21 additions & 0 deletions tests/unit/envs.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,25 @@ describe('Environment variables should be correctly parsed', () => {
env.HIGHCHARTS_FORCE_FETCH = 'false';
expect(Config.partial().parse(env).HIGHCHARTS_FORCE_FETCH).toEqual(false);
});

test('PUPPETEER_DIR should be a valid path', () => {
const env = { PUPPETEER_DIR: '/path/to/dir' };
expect(Config.partial().parse(env).PUPPETEER_DIR).toEqual('/path/to/dir');

env.PUPPETEER_DIR = '/another/path/to/dir';
expect(Config.partial().parse(env).PUPPETEER_DIR).toEqual(
'/another/path/to/dir'
);

env.PUPPETEER_DIR = '';
expect(() => Config.partial().parse(env)).toThrow();
});

test('PUPPETEER_DIR can be a relative path', () => {
const env = { PUPPETEER_DIR: './tmp/' };
expect(Config.partial().parse(env).PUPPETEER_DIR).toEqual('./tmp/');

env.PUPPETEER_DIR = '../custom-tmp/';
expect(Config.partial().parse(env).PUPPETEER_DIR).toEqual('../custom-tmp/');
});
});
Loading