-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add client-response
Threat Model and update JS ClientsRequests
#19656
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: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
Adds a new client-response
threat model for client-side response data and updates the JavaScript QL logic and documentation to use it
- Included
client-response
in the shared threat‐model grouping - Added change notes in both shared and JS QL directories
- Updated the
ClientRequests.qll
module to return"client-response"
instead of"response"
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
shared/threat-models/ext/threat-model-grouping.model.yml | Added client-response under local extensions |
shared/threat-models/change-notes/2025-06-03-client-response-threatmodel.md | Documented addition of the client-response threat model |
javascript/ql/lib/semmle/javascript/frameworks/ClientRequests.qll | Changed getThreatModel() to return "client-response" |
javascript/ql/lib/change-notes/2025-06-03-client-response-threatmodel.md | Added JS QL change note for the new threat model |
Comments suppressed due to low confidence (2)
javascript/ql/lib/semmle/javascript/frameworks/ClientRequests.qll:950
- You’ve introduced a new
client-response
threat model but haven’t added tests for it. Please add unit tests that verify HTTP response data is correctly flagged under theclient-response
model.
override string getThreatModel() { result = "client-response" }
shared/threat-models/ext/threat-model-grouping.model.yml:21
- The indentation for the comment and the new
client-response
entry doesn't match existing list items. Align them with the other entries (8 spaces) to maintain valid YAML structure.
# Client-side threat models for request responses.
Could you say a few words about why the existing |
Correct, right now you can't enable |
Default Setup cannot use I would argue for allowing I haven't seen the data on the testing, but I understand a decision last year was made to favour work on dealing with false negatives over false positives - this decision to make Allowing an explicit |
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.
The reasoning and outline of a solution can be found here.
I understand the desire to quickly get this included in local
, we just need to make sure it's forward compatible with the intended solution.
--- | ||
category: minorAnalysis | ||
--- | ||
* Added support for ClientRequest being part of the `client-response` threat model versus part of `response` threat model. |
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.
* Added support for ClientRequest being part of the `client-response` threat model versus part of `response` threat model. | |
* Responses from outgoing HTTP requests will now be included as taint sources when the `local` threat model is used. |
I'd try to use a change note that's easier to understand for external users.
--- | ||
category: minorAnalysis | ||
--- | ||
* Add support for `client-response` threat model. |
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.
I don't think we need change notes for qlpacks
@@ -18,6 +18,8 @@ extensions: | |||
- ["stdin", "local"] | |||
- ["file", "local"] | |||
- ["windows-registry", "local"] | |||
# Client-side threat models for request responses. | |||
- ["client-response", "local"] |
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.
- ["client-response", "local"] | |
- ["browser-response", "local"] |
@@ -947,7 +947,7 @@ module ClientRequest { | |||
private class ClientRequestThreatModel extends ThreatModelSource::Range { | |||
ClientRequestThreatModel() { this = any(ClientRequest r).getAResponseDataNode() } | |||
|
|||
override string getThreatModel() { result = "response" } | |||
override string getThreatModel() { result = "client-response" } |
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.
override string getThreatModel() { result = "client-response" } | |
override string getThreatModel() { result = ["response", "browser-response"] } |
To be forward compatible with the planned solution. I'll open a PR to make this a bit more accurate.
I've added the
client-response
threat model to the Threat Modelling shared library. This is a new local threat model that includes the sources of client libraries (mainly focuses at JavaScript / Typescript).This is needed to discover XSS or other types of security issues when the source of untrusted data in the response content of REST, GraphQL, Soap, etc. clients.