Skip to content
This repository has been archived by the owner on Sep 26, 2024. It is now read-only.

[MOC-57] Load modifications on edit mode #66

Merged
merged 18 commits into from
Jun 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion apps/mocksi-lite/content/ContentApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import closeIcon from "../public/close-icon.png";
import mocksiLogo from "../public/mocksi-logo.png";
import { setRootPosition } from "../utils";
import { setEditorMode } from "./EditMode/editMode";
import { ContentHighlighter } from "./EditMode/highlighter";
import { getHighlighter } from "./EditMode/highlighter";
import Popup from "./Popup";
import { RecordButton } from "./RecordButton";

Expand Down Expand Up @@ -40,6 +40,7 @@ export default function ContentApp({ isOpen, email }: ContentProps) {
const [state, setState] = useState<RecordingState>(
RecordingState.UNAUTHORIZED,
);
const ContentHighlighter = getHighlighter();
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Ensure that all references to ContentHighlighter are replaced with getHighlighter() to maintain consistency with the new instantiation pattern.

  • apps/mocksi-lite/content/ContentApp.tsx:
    • Line 43: const ContentHighlighter = getHighlighter();
    • Line 45: ContentHighlighter.showHideHighlights(!prevValue);
Analysis chain

Ensure that all references to ContentHighlighter are replaced with getHighlighter() to maintain consistency with the new instantiation pattern.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that all references to `ContentHighlighter` are replaced with `getHighlighter()`.

# Test: Search for `ContentHighlighter` usage. Expect: No occurrences except in comments or documentation.
rg --type typescript --glob 'apps/mocksi-lite/**/*.tsx' 'ContentHighlighter'

Length of output: 111



Script:

#!/bin/bash
# Description: Verify that all references to `ContentHighlighter` are replaced with `getHighlighter()`.

# Test: Search for `ContentHighlighter` usage. Expect: No occurrences except in comments or documentation.
rg --type ts --glob 'apps/mocksi-lite/**/*.tsx' 'ContentHighlighter'

Length of output: 248


useEffect(() => {
// Load initial state from chrome storage
Expand Down
4 changes: 2 additions & 2 deletions apps/mocksi-lite/content/EditMode/actions.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { DOMManipulator } from "@repo/dodom";
import { saveModification } from "../../utils";
import { ContentHighlighter } from "./highlighter";
import { getHighlighter } from "./highlighter";

export function cancelEditWithoutChanges(nodeWithTextArea: HTMLElement | null) {
if (nodeWithTextArea) {
Expand All @@ -24,7 +24,7 @@ export function applyChanges(
// TODO: check if we should keep the singleton behavior we had before
const domManipulator = new DOMManipulator(
fragmentTextNode,
ContentHighlighter,
getHighlighter(),
saveModification,
);
domManipulator.addPattern(oldValue, newValue);
Expand Down
3 changes: 0 additions & 3 deletions apps/mocksi-lite/content/EditMode/editMode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
import { persistModifications, undoModifications } from "../../utils";
import { cancelEditWithoutChanges } from "./actions";
import { decorate } from "./decorator";
import { ContentHighlighter, initHighlighter } from "./highlighter";

export const setEditorMode = async (turnOn: boolean, recordingId?: string) => {
if (turnOn) {
Expand All @@ -16,7 +15,6 @@ export const setEditorMode = async (turnOn: boolean, recordingId?: string) => {
await chrome.storage.local.set({
[MOCKSI_RECORDING_STATE]: RecordingState.EDITING,
});
initHighlighter();
blockClickableElements();
document.body.addEventListener("dblclick", onDoubleClickText);
return;
Expand All @@ -31,7 +29,6 @@ export const setEditorMode = async (turnOn: boolean, recordingId?: string) => {
await chrome.storage.local.remove(MOCKSI_RECORDING_ID);
document.body.removeEventListener("dblclick", onDoubleClickText);
restoreNodes();
ContentHighlighter.removeHighlightNodes();
cancelEditWithoutChanges(document.getElementById("mocksiSelectedText"));
document.normalize();
};
Expand Down
5 changes: 3 additions & 2 deletions apps/mocksi-lite/content/EditMode/highlighter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@ class Highlighter {
};
}

export let ContentHighlighter: Highlighter;
let ContentHighlighter: Highlighter;

export const initHighlighter = () => {
export const getHighlighter = () => {
if (!ContentHighlighter) {
ContentHighlighter = new Highlighter();
}
return ContentHighlighter;
};

const highlight = ({
Expand Down
3 changes: 2 additions & 1 deletion apps/mocksi-lite/content/Popup/CreateDemo/DemoItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const DemoItem = ({
}: DemoItemProps) => {
const handleEdit = () => {
setEditorMode(true, uuid);
loadAlterations(alterations, true);
setState(RecordingState.EDITING);
};

Expand All @@ -46,7 +47,7 @@ const DemoItem = ({
</Button>
<Button
variant={Variant.icon}
onClick={() => loadAlterations(alterations)}
onClick={() => loadAlterations(alterations, false)}
disabled={
!url.includes(window.location.hostname) ||
!alterations ||
Expand Down
120 changes: 59 additions & 61 deletions apps/mocksi-lite/utils.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { DOMManipulator } from "@repo/dodom";
import sanitizeHtml from "sanitize-html";
import MocksiRollbar from "./MocksiRollbar";
import type { Alteration } from "./background";
Expand All @@ -14,6 +15,8 @@ import {
STORAGE_KEY,
SignupURL,
} from "./consts";
import { fragmentTextNode } from "./content/EditMode/actions";
import { getHighlighter } from "./content/EditMode/highlighter";

type DOMModificationsType = {
[querySelector: string]: { nextText: string; previousText: string };
Expand All @@ -40,7 +43,7 @@ export const logout = () => {

const commandsExecuted: Command[] = [];

const domainModifications: DOMModificationsType = {};
let domainModifications: DOMModificationsType = {};

export const saveModification = (
parentElement: HTMLElement,
Expand Down Expand Up @@ -74,17 +77,35 @@ export const persistModifications = (recordingId: string) => {
export const undoModifications = () => {
loadPreviousModifications();
chrome.storage.local.remove(MOCKSI_MODIFICATIONS);
getHighlighter().removeHighlightNodes();
// clean the domainModifications
domainModifications = {};
};

// v2 of loading alterations, this is from backend
export const loadAlterations = (alterations: Alteration[] | null) => {
export const loadAlterations = (
alterations: Alteration[] | null,
withHighlights: boolean,
) => {
undoModifications();
if (!alterations?.length) {
// FIXME: we should warn the user that there are no alterations for this demo
return [] as Alteration[];
}
const domManipulator = new DOMManipulator(
fragmentTextNode,
getHighlighter(),
saveModification,
);
for (const alteration of alterations) {
const { selector, dom_after, dom_before } = alteration;
modifyElementInnerHTML(selector, dom_before, sanitizeHtml(dom_after));
const elemToModify = getHTMLElementFromSelector(selector);
domManipulator.iterateAndReplace(
elemToModify as Node,
new RegExp(dom_before, "gi"),
sanitizeHtml(dom_after),
withHighlights,
);
}
};

Expand All @@ -93,75 +114,52 @@ export const loadPreviousModifications = () => {
for (const modification of Object.entries(domainModifications)) {
const [querySelector, { previousText, nextText }] = modification;
const sanitizedPreviousText = sanitizeHtml(previousText);
const elemToModify = getHTMLElementFromSelector(querySelector);
// here newText and previous is in altered order because we want to revert the changes
modifyElementInnerHTML(querySelector, nextText, sanitizedPreviousText);
}
};

const modifyElementInnerHTML = (
selector: string,
oldContent: string,
newContent: string,
) => {
// querySelector format {htmlElementType}#{elementId}.{elementClassnames}[${elementIndexIfPresent}]{{newValue}}
const hasIndex = selector.match(/\[[0-9]+\]/);
const valueInQuerySelector = selector.match(/\{[a-zA-Z0-9 ]+\}/); // add spaces to pattern
let elemToModify: Element | null;
// FIXME: this needs to be refactored
if (hasIndex) {
// with all this replaces, we should build a formatter
const filteredQuerySelector = formatQuerySelector(
selector,
valueInQuerySelector,
hasIndex,
);
const index: number = +hasIndex[0].replace("[", "").replace("]", "");
// FIXME: lots of duplicated code here
try {
elemToModify = document.querySelectorAll(filteredQuerySelector)[index];
} catch (e: unknown) {
if (e instanceof Error) {
console.error(`Error querying selector: ${e}`);
}

elemToModify = null;
}
} else {
// FIXME: lots of duplicated code here
try {
elemToModify = document.querySelector(
formatQuerySelector(selector, valueInQuerySelector, null),
if (elemToModify) {
elemToModify.innerHTML = elemToModify.innerHTML.replaceAll(
nextText,
sanitizedPreviousText,
Comment on lines +117 to +122
Copy link

@coderabbitai coderabbitai bot Jun 20, 2024

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The loadPreviousModifications function sanitizes previousText using sanitizeHtml, but nextText is used directly in the innerHTML replacement without sanitization. This could potentially introduce XSS vulnerabilities if nextText contains malicious code.

  • apps/mocksi-lite/utils.ts:108: nextText is extracted from modification.
  • apps/mocksi-lite/utils.ts:114: nextText is used in innerHTML replacement without sanitization.
Analysis chain

The loadPreviousModifications function correctly handles the reversion of DOM changes. However, ensure that the innerHTML replacements do not introduce security vulnerabilities such as XSS.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that `innerHTML` replacements do not introduce XSS vulnerabilities.

# Test: Check for proper sanitization of `nextText` before insertion.
echo "Check sanitization of nextText in loadPreviousModifications"

Length of output: 128



Script:

#!/bin/bash
# Locate the `loadPreviousModifications` function in the utils.ts file.
ast-grep --lang typescript --pattern 'function loadPreviousModifications($_) { $$$ }' apps/mocksi-lite/utils.ts

Length of output: 113



Script:

#!/bin/bash
# Search for the `loadPreviousModifications` function definition in the utils.ts file using ripgrep.
rg 'function loadPreviousModifications' apps/mocksi-lite/utils.ts

Length of output: 67



Script:

#!/bin/bash
# Search for any references to `loadPreviousModifications` in the utils.ts file.
rg 'loadPreviousModifications' apps/mocksi-lite/utils.ts

Length of output: 135



Script:

#!/bin/bash
# Extract the implementation of the `loadPreviousModifications` function from the utils.ts file.
ast-grep --lang typescript --pattern 'const loadPreviousModifications = () => { $$$ }' apps/mocksi-lite/utils.ts

Length of output: 1075

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nextText is only to search and replace with the previousText.
nextText won't end in the innerHTML. The potential vuln is on the previousText only.

Copy link

Choose a reason for hiding this comment

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

@NicoMoreinoSirius, thank you for clarifying the usage of nextText and previousText in the loadPreviousModifications function. You are correct that the current implementation does not insert nextText into the DOM, and thus it doesn't pose a direct XSS risk in this context.

However, as a best practice and to safeguard against potential future changes that might alter how these variables are used, it might be worth considering sanitizing nextText as well. This would ensure that any modifications to the function's logic or usage in the future wouldn't inadvertently introduce vulnerabilities.

Let me know if you would like me to assist further or if there's anything else you'd like to address!

);
} catch (e: unknown) {
if (e instanceof Error) {
console.error(`Error querying selector: ${e}`);
}
elemToModify = null;
}
}
const sanitizedNewContent = sanitizeHtml(newContent);
if (elemToModify?.innerHTML) {
elemToModify.innerHTML =
elemToModify.innerHTML.replaceAll(oldContent, sanitizedNewContent) || "";
}
};

const formatQuerySelector = (
rawSelector: string,
valueInQuerySelector: RegExpMatchArray | null,
hasIndex: RegExpMatchArray | null,
) => {
if (hasIndex) {
return valueInQuerySelector
? rawSelector
.replace(hasIndex[0], "")
.replace(valueInQuerySelector[0], "")
: rawSelector.replace(hasIndex[0], "");
}
// querySelector format {htmlElementType}#{elementId}.{elementClassnames}[${elementIndexIfPresent}]{{newValue}}
const [index] = hasIndex || [""];
const [value] = valueInQuerySelector || [""];
return rawSelector.replace(index, "").replace(value, "");
};

return valueInQuerySelector
? rawSelector.replace(valueInQuerySelector[0], "")
: rawSelector;
const getHTMLElementFromSelector = (
unfomattedSelector: string,
): Element | null => {
const hasIndex = unfomattedSelector.match(/\[[0-9]+\]/);
const valueInQuerySelector = unfomattedSelector.match(/\{.+\}/);
const formattedSelector = formatQuerySelector(
unfomattedSelector,
hasIndex,
valueInQuerySelector,
);
let elemToModify: NodeListOf<Element> | null;
try {
elemToModify = document.querySelectorAll(formattedSelector);
} catch (e: unknown) {
if (e instanceof Error) {
console.error(`Error querying selector: ${e}`);
}
elemToModify = null;
}
if (elemToModify) {
const index = hasIndex ? +hasIndex[0].replace("[", "").replace("]", "") : 0;
return elemToModify[index];
}
return elemToModify;
};

export const sendMessage = (
Expand Down Expand Up @@ -235,7 +233,7 @@ export const getEmail = async (): Promise<string | null> => {
export const getRecordingsStorage = async (): Promise<Recording[]> => {
try {
const results = await chrome.storage.local.get(["recordings"]);
console.log(results.recordings);
console.log("results", results.recordings);
if (results.recordings) {
return JSON.parse(results.recordings);
}
Expand Down
84 changes: 57 additions & 27 deletions packages/dodom/receivers/DOMManipulator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export class DOMManipulator {
addPattern(pattern: string | RegExp, replace: string) {
const replacePattern = { pattern: toRegExpPattern(pattern), replace };
this.patterns.push(replacePattern);
this.seekAndReplace(replacePattern.pattern, replacePattern.replace);
this.seekAndReplaceAllPage(replacePattern.pattern, replacePattern.replace);
}

removePattern(pattern: string | RegExp) {
Expand All @@ -50,47 +50,45 @@ export class DOMManipulator {
}
}

seekAndReplace(pattern: RegExp, newText: string) {
seekAndReplaceAllPage(pattern: RegExp, newText: string) {
const body = document.querySelector("body");
if (!body) {
console.log("Body not found");
return;
}
this.iterateAndReplace(body, pattern, newText, true);
}

iterateAndReplace(
rootNode: Node,
oldValueInPattern: RegExp,
newText: string,
highlightReplacements: boolean,
) {
const fragmentsToHighlight: Node[] = [];
const replacements: { nodeToReplace: Node; replacement: Node }[] = [];
createTreeWalker(body, (textNode) => {
if (!textNode.nodeValue) {
return;
}
const matches = [...textNode.nodeValue.matchAll(pattern)];
if (matches.length > 0) {
const fragmentedTextNode = this.fragmentTextNode(
fragmentsToHighlight,
matches,
textNode,
newText,
);
replacements.push({
nodeToReplace: textNode,
replacement: fragmentedTextNode as Node,
});
this.saveModification(
textNode.parentElement as HTMLElement,
newText,
cleanPattern(pattern),
);
}
createTreeWalker(rootNode, (textNode) => {
fillReplacements(
textNode,
oldValueInPattern,
newText,
fragmentsToHighlight,
replacements,
this.fragmentTextNode,
this.saveModification,
);
});

for (const { nodeToReplace, replacement } of replacements) {
if (nodeToReplace.parentElement == null) {
continue;
}
nodeToReplace.parentElement.replaceChild(replacement, nodeToReplace);
}

for (const fragment of fragmentsToHighlight) {
this.contentHighlighter.highlightNode(fragment);
if (highlightReplacements) {
for (const fragment of fragmentsToHighlight) {
this.contentHighlighter.highlightNode(fragment);
}
}
}

Expand Down Expand Up @@ -184,6 +182,38 @@ const createTreeWalker = (
} while (treeWalker.nextNode());
};

const fillReplacements = (
textNode: Node,
oldTextPattern: RegExp,
newText: string,
fragmentsToHighlight: Node[],
replacements: { nodeToReplace: Node; replacement: Node }[],
fragmentTextNode: FragmentTextNode,
saveModification: SaveModification,
) => {
if (!textNode || !textNode.nodeValue) {
return;
}
const matches = [...textNode.nodeValue.matchAll(oldTextPattern)];
if (matches.length > 0) {
const fragmentedTextNode = fragmentTextNode(
fragmentsToHighlight,
matches,
textNode,
newText,
);
replacements.push({
nodeToReplace: textNode,
replacement: fragmentedTextNode as Node,
});
saveModification(
textNode.parentElement as HTMLElement,
newText,
cleanPattern(oldTextPattern),
);
}
};

export const replaceFirstLetterCase = (value: string) => {
return (match: string) => {
if (match[0]?.toLowerCase() !== match[0]?.toUpperCase()) {
Expand Down
Loading
Loading