Skip to content

Remove eval #21

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

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

Remove eval #21

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37c6428
Defer the eval injection to prepublish-rrweb.js so that it only appea…
eoghanmurray Mar 13, 2026
b74c2fb
Add the environment in as a placeholder that will get post-processed,…
eoghanmurray Mar 13, 2026
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
16 changes: 8 additions & 8 deletions packages/ws-client/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ export function stop(resetRecordingId: boolean) {
if (resetRecordingId) {
removeRecordingId();
}
const i = document.getElementById('rrwebcloud-recording-indicator');
if (i) {
i.remove();
Copy link
Copy Markdown

@kilo-code-bot kilo-code-bot Bot Mar 13, 2026

Choose a reason for hiding this comment

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

WARNING: Element.remove() method is not supported in IE or older browsers. Consider using element.parentNode?.removeChild(element) for broader compatibility, or ensure this library only targets modern browsers.

}
}

function removeRecordingId(): void {
Expand Down Expand Up @@ -291,6 +295,7 @@ export function start(
recordVersion: __PKG_VERSION__,
recordCommitHash: __COMMIT_HASH__,
};
initialPayload.jsSource = 'esm'; // this line get's replaced by prepublish-rrweb.sh

// the expected replacement of recording id
serverUrl = serverUrl.replace('{recordingId}', recordingId);
Expand Down Expand Up @@ -482,11 +487,6 @@ if (document && document.currentScript) {
.replace(/,(\s*[\]}])/g, '$1'), // allow trailing commas
);
} catch (e) {
/* this allows bare prop names and single quoted values:
{
blockSelector: '.my-block-selector',
}
*/
config = looseJsonParse(self.innerText);
}
config = normalizeKeys(config, defaultClientConfig);
Expand Down Expand Up @@ -522,10 +522,10 @@ if (document && document.currentScript) {
}
}
function looseJsonParse(obj: string) {
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval#never_use_direct_eval!
return eval?.(`"use strict";(${obj})`);
// this is replaced by an eval in prepublish-rrweb.sh
console.log(`couldn't parse config as JSON: ${obj}`);
Copy link
Copy Markdown

@kilo-code-bot kilo-code-bot Bot Mar 13, 2026

Choose a reason for hiding this comment

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

CRITICAL: Config parsing is completely broken - this function now always returns an empty object {} instead of parsing the config. When JSON.parse fails (line 484-490), it falls back to looseJsonParse which returns {}, meaning users' custom configurations will be lost.

The original code used eval to parse JavaScript-style config objects (e.g., {blockSelector: '.my-block-selector'}). While removing eval is good for security, this replacement doesn't actually parse anything - it just silently fails.

Suggested fix: Either:

  1. Actually implement JSON5 parsing (there are libraries like json5 npm package)
  2. Or keep a safe eval alternative that doesn't use direct eval (e.g., using Function constructor with limited scope)

return {} as recordOptions<eventWithTime> & clientConfig;
}

export default {
start,
stop,
Expand Down
Loading