-
Notifications
You must be signed in to change notification settings - Fork 6
Remove old modules and use react web components. #1986
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: develop
Are you sure you want to change the base?
Conversation
| this.data = getJsonFromScriptTag<{ | ||
| items: ComboBoxItem[] | ||
| selected_id?: string | ||
| }>('branch-data')! |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
The best way to fix this issue is to ensure the JSON data is robustly validated and sanitized after it is retrieved from the DOM, before it is parsed and used in the application. Specifically, after parsing, all fields that can influence what is rendered (such as items, selected_id, etc.) should be validated to ensure they match expected types/values, and any string outputs should be escaped or rendered safely to avoid injection.
The code for parsing JSON should be enhanced to:
- Validate the parsed data structure against the expected shape.
- Optionally strip or encode any unwanted HTML or script in string values.
- Return
undefinedor throw if validation fails.
We should update getJsonFromScriptTag (and/or parseJsonSafely) to perform additional schema validation. This can be achieved with a type-guard or a schema checking function, passing in a validator where we call the util. For type-safe validation, we may use a well-known external library such as zod for runtime type-checking, or we can write a custom utility that validates each field manually.
The changes are fully contained within src/open_inwoner/react/lib/getJsonScriptData.ts, adding schema or type validation and defensive error handling.
-
Copy modified line R7 -
Copy modified lines R13-R18
| @@ -4,13 +4,18 @@ | ||
| * @param id The HTML id of the script element | ||
| * @returns The parsed JSON data or undefined if not found/invalid | ||
| */ | ||
| export function getJsonFromScriptTag<T = unknown>(id: string): T | undefined { | ||
| export function getJsonFromScriptTag<T = unknown>(id: string, validator?: (data: unknown) => data is T): T | undefined { | ||
| const scriptElement = document.getElementById(id) | ||
| if (!scriptElement?.textContent) { | ||
| return undefined | ||
| } | ||
|
|
||
| return parseJsonSafely<T>(scriptElement.textContent) | ||
| const parsed = parseJsonSafely(scriptElement.textContent) | ||
| if (validator && !validator(parsed)) { | ||
| console.error(`[getJsonFromScriptTag] JSON validation failed`, parsed) | ||
| return undefined | ||
| } | ||
| return parsed as T | undefined | ||
| } | ||
|
|
||
| /** |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1986 +/- ##
========================================
Coverage 92.32% 92.32%
========================================
Files 1206 1206
Lines 45299 45309 +10
========================================
+ Hits 41822 41833 +11
+ Misses 3477 3476 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| </a> | ||
| {% endfor %} | ||
| <div class="card-container card-container--columns-1 plugin-card"> | ||
| <action-list> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stefrado can we explore whether we can do away with the entire parse-JSON-from-tag approach, and pass it directly as an attribute?
| <action-list> | |
| <action-list data-items="{{ feed_item_list_json }}"> |
Where feed_item_list_json is json.dumps'ed in the backend.
Then in the web component you could do something like:
const items = JSON.parse(this.getAttribute('data-config'));
I think this would make the code simpler and easier to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Same for the KvK branch selector)
Summary
Remove old module mounting react method and use cleaner web component mounting method.
Issue References
Checklist
docker-compose.ymlenvironment variablesvariables