Repository navigation
fix: show stored config that is not a JSON object as text and block unsafe saves - #718
Open
bhaskargurram-ai wants to merge 2 commits into
Open
bhaskargurram-ai wants to merge 2 commits into
bhaskargurram-ai wants to merge 2 commits into
Conversation
…nsafe saves
Plugin config, plugin-handle extObj and API-document ext/document are
stored as strings and were passed straight to JSON.parse inside render.
A record whose string is malformed, or valid JSON that is not an object,
threw during render and blanked the Home page, the Plugin and
NamespacePlugin lists, the plugin edit forms and the API-document form.
Add parseJsonObject and formatJsonObject in src/utils/jsonObject.js.
The read-only views pretty-print a JSON object as before and otherwise
render the original string as text. The Plugin and NamespacePlugin edit
forms keep the stored string: when it is not a JSON object the schema
fields are replaced by a textarea holding the original text with a
validation error, and the form cannot be saved until that text is a
JSON object. Nothing is substituted with {}. A field whose extObj is
not a JSON object renders without its metadata instead of throwing. The
API-document form shows ext and document as text with an error status
when they are not JSON objects.
Fixes apache#612
There was a problem hiding this comment.
🟡 Changes recommended
Both plugin recovery validators accept cleared text, allowing configuration to be overwritten without a valid JSON object.
2 open findings
What changed in this PR
Addresses #612 by safely displaying malformed or non-object stored JSON across dashboard views and adding recovery controls to plugin forms.
Changes:
- Adds shared JSON-object parsing and display helpers.
- Replaces unsafe rendering with raw-text fallbacks and validation feedback.
- Adds regression tests for parsing, rendering, and configuration recovery.
| File | Description |
|---|---|
| src/utils/jsonObject.test.js | Tests parsing and display behavior. |
| src/utils/jsonObject.js | Adds safe parsing and formatting helpers. |
| src/routes/System/Plugin/index.js | Safely displays stored configuration. |
| src/routes/System/Plugin/AddModal.test.js | Tests configuration recovery and metadata handling. |
| src/routes/System/Plugin/AddModal.js | Adds raw-text recovery and guarded metadata parsing. |
| src/routes/System/NamespacePlugin/index.js | Safely displays namespace-plugin configuration. |
| src/routes/System/NamespacePlugin/AddModal.test.js | Tests recovery and malformed metadata. |
| src/routes/System/NamespacePlugin/AddModal.js | Adds recovery validation and guarded parsing. |
| src/routes/Home/index.js | Safely formats configuration popovers. |
| src/routes/Document/components/AddAndUpdateApiDoc.test.js | Tests object rendering and raw-text fallbacks. |
| src/routes/Document/components/AddAndUpdateApiDoc.js | Displays invalid JSON as text with errors. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The recovery text area accepted an empty value, because parseJsonObject treats empty input as "no config" rather than an error, so clearing the field and saving erased the stored configuration. Both plugin forms now require the text to parse as a JSON object. Adds a clear-then-save test for each form.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Plugin config, plugin-handle extObj and API-document ext/document are stored as strings and were passed straight to
JSON.parseinside render. A record whose string is malformed, or is valid JSON that is not an object, threw during render and blanked the Home page, the Plugin and NamespacePlugin lists, the plugin edit forms and the API-document form.src/utils/jsonObject.jsaddsparseJsonObject, which never throws and returns{ value }for a JSON object,{ value: undefined }for null/empty and{ error }for everything else, plusformatJsonObjectfor read-only views. The Home, Plugin and NamespacePlugin popovers pretty-print a JSON object as before and otherwise render the original string as text, which React escapes.The Plugin and NamespacePlugin edit forms keep the stored string. When it is not a JSON object the schema fields are replaced by a textarea holding the original text with a validation error shown on open, and the form cannot be saved until that text is a JSON object; the corrected text is then saved as the config. Nothing is substituted with
{}, so a save can no longer overwrite the stored value with an empty object. A field whose plugin-handleextObjis not a JSON object renders without its required/default/rule metadata instead of throwing. The API-document form showsextanddocumentas text with an error status when they are not JSON objects;isJsonStralready blocks saving a badextand is unchanged.Tests cover null/empty values, malformed strings and valid JSON of an unexpected type as separate cases in
src/utils/jsonObject.test.js,src/routes/System/Plugin/AddModal.test.js,src/routes/System/NamespacePlugin/AddModal.test.jsandsrc/routes/Document/components/AddAndUpdateApiDoc.test.js. Against master, 8 of the new tests fail with the uncaughtSyntaxError; with this changenpm run test:unitpasses 308 tests in 58 suites,npm run lintandprettier --checkare clean andnpm run buildsucceeds.Fixes #612