Bug infinate loading#645
Conversation
Signed-off-by: yashchaud <yashchaudhari908@gmail.com>
Signed-off-by: yashchaud <yashchaudhari908@gmail.com>
Confidence Score: 3/5The core fix is correct, but the error rendering path introduces a hand-rolled HTML sanitizer whose behaviour depends on an unverified assumption about the ErrorMessage component internals. The error display logic is sound and resolves the reported bug. The sanitizedError computed builds HTML from server-supplied data using a custom DOM trick and a narrow regex — if ErrorMessage renders with v-html this is the only XSS barrier; if it renders as plain text the mechanism is a no-op. Neither outcome is clearly correct without knowing the component internals. frontend/src/pages/Setup.vue — specifically the sanitizedError computed and how its output is consumed by ErrorMessage
|
| Filename | Overview |
|---|---|
| frontend/src/pages/Setup.vue | Adds v-if/v-else to surface createTeam errors instead of infinite loading; introduces a bespoke DOM-based HTML sanitizer that is either a security smell (if ErrorMessage uses v-html) or a no-op (if it does not), plus leaves an unused useStore import. |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/pages/Setup.vue:43-48
**Bespoke HTML sanitization passed to component as trusted HTML**
`sanitizedError` constructs a string containing real `<strong>` tags from a server-supplied error message, then passes it to `<ErrorMessage :message="sanitizedError" />`. If `ErrorMessage` from frappe-ui renders the `message` prop with `v-html` (rather than `{{ message }}`), any server-controlled string that happens to embed `<strong>` alongside other payloads could bypass the regex — e.g. a multi-message response or a future server change widening the allowed tags. If `ErrorMessage` does NOT render with `v-html`, the entire sanitization round-trip is wasted and `<strong>` tags will appear as literal text instead of bold. In either case, prefer passing `createTeam.error?.messages?.[0]` directly (plain text), or use a proper sanitizer like DOMPurify if HTML rendering is genuinely required.
### Issue 2 of 2
frontend/src/pages/Setup.vue:38-39
`useStore` is imported but never referenced in the component after the refactor.
```suggestion
import { createTeam } from '@/resources/permissions'
```
Reviews (1): Last reviewed commit: "fix: fixed xss attack posibility with sa..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const sanitizedError = computed(() => { | ||
| const raw = createTeam.error?.messages?.[0] ?? '' | ||
| const el = document.createElement('div') | ||
| el.textContent = raw | ||
| return el.innerHTML.replace(/<(\/?strong)>/g, '<$1>') | ||
| }) |
There was a problem hiding this comment.
sanitizedError constructs a string containing real <strong> tags from a server-supplied error message, then passes it to <ErrorMessage :message="sanitizedError" />. If ErrorMessage from frappe-ui renders the message prop with v-html (rather than {{ message }}), any server-controlled string that happens to embed <strong> alongside other payloads could bypass the regex — e.g. a multi-message response or a future server change widening the allowed tags. If ErrorMessage does NOT render with v-html, the entire sanitization round-trip is wasted and <strong> tags will appear as literal text instead of bold. In either case, prefer passing createTeam.error?.messages?.[0] directly (plain text), or use a proper sanitizer like DOMPurify if HTML rendering is genuinely required.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/pages/Setup.vue
Line: 43-48
Comment:
**Bespoke HTML sanitization passed to component as trusted HTML**
`sanitizedError` constructs a string containing real `<strong>` tags from a server-supplied error message, then passes it to `<ErrorMessage :message="sanitizedError" />`. If `ErrorMessage` from frappe-ui renders the `message` prop with `v-html` (rather than `{{ message }}`), any server-controlled string that happens to embed `<strong>` alongside other payloads could bypass the regex — e.g. a multi-message response or a future server change widening the allowed tags. If `ErrorMessage` does NOT render with `v-html`, the entire sanitization round-trip is wasted and `<strong>` tags will appear as literal text instead of bold. In either case, prefer passing `createTeam.error?.messages?.[0]` directly (plain text), or use a proper sanitizer like DOMPurify if HTML rendering is genuinely required.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
i could add DOMPurify but it is a additional Dependency and just {{}} shows html tags which looks ugly, so this is sort of a middle ground
There was a problem hiding this comment.
Fair point. After checking, ErrorMessage does use v-html internally, so HTML rendering is real and plain text would show literal tags.
Your sanitization logic is actually sound on closer inspection: el.textContent = raw HTML-encodes everything (including any <script>, onerror=, etc.), then you selectively un-encode only <strong> tags. Since <strong> has no src/href/event attributes, there's no XSS vector here. The regex can't be bypassed because all other tags were already encoded by the textContent assignment first.
I withdraw my concern — this is a reasonable middle ground given the constraints. The implementation is safe as written.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
| import { useStore } from 'vuex' | ||
| import { createTeam } from '@/resources/permissions' |
There was a problem hiding this comment.
useStore is imported but never referenced in the component after the refactor.
| import { useStore } from 'vuex' | |
| import { createTeam } from '@/resources/permissions' | |
| import { createTeam } from '@/resources/permissions' |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/pages/Setup.vue
Line: 38-39
Comment:
`useStore` is imported but never referenced in the component after the refactor.
```suggestion
import { createTeam } from '@/resources/permissions'
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
fixes #613