Skip to content

O3-5739: Skip XSS request wrapping for web-service paths - #265

Closed
wikumChamith wants to merge 1 commit into
openmrs:2.xfrom
wikumChamith:2.x-O3-5739
Closed

O3-5739: Skip XSS request wrapping for web-service paths#265
wikumChamith wants to merge 1 commit into
openmrs:2.xfrom
wikumChamith:2.x-O3-5739

Conversation

@wikumChamith

@wikumChamith wikumChamith commented Jun 16, 2026

Copy link
Copy Markdown
Member

XSSFilter no longer wraps requests whose URI contains /ws/, so REST and FHIR request bodies reach the resource layer as raw bytes.

https://openmrs.atlassian.net/browse/O3-5739

XSSFilter no longer wraps requests whose URI contains /ws/, so REST and FHIR request bodies reach the resource layer as raw bytes.
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@wikumChamith

wikumChamith commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

Quality Gate Failed Quality Gate failed

Failed conditions B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

This feels like an overreach: https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=265&id=openmrs_openmrs-module-legacyui&open=AZ7OwGPqhsNhAUEFGDCj

@ibacher

ibacher commented Jun 16, 2026

Copy link
Copy Markdown
Member

So, while I do think the XSSFilter is the cause of the problem, I don't think that just dropping requests to /ws/ winds up being a complete or even good solution. Basically this just creates an asymmetry between "data entered via Java forms" and "data entered via the REST API", but worse, it makes the REST API a backdoor for loading XSS-style strings that are then incorrectly displayed in the legacy or O2 UI, basically removing the whole reason for having this filter in the first place.

Ultimately, rather than this being something we can just hack-up a quick patch around, I think we need to come up with a principled way to properly re-encode output at the point it's displayed.

@wikumChamith

Copy link
Copy Markdown
Member Author

So isn't it better to fix this in the frontend?

@ibacher

ibacher commented Jun 16, 2026

Copy link
Copy Markdown
Member

So isn't it better to fix this in the frontend?

Yep. And probably also the legacy UI...

@ibacher ibacher closed this Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants