JS: polish for #132#300
Merged
Merged
Conversation
added 14 commits
October 10, 2018 12:12
ghost
self-requested a review
as a 
asger-semmle
suggested changes
Oct 10, 2018
Contributor
asger-semmle
left a comment
asger-semmle
left a comment
There was a problem hiding this comment.
Looks great!
Apart from a few typos I have two bikeshedding topics, but feel free to ignore these if you don't agree:
- The phrase "remote request" sounds weird to me. I'd say HTTP request or network request or just outbound request (dropping the "remote" part).
- As mentioned inline, "user-controlled data in file" sounds like you're flagging files with user-controlled data, as opposed to places where user-controlled data is written to a file.
| /** | ||
| * @name Http response data flows to File Access | ||
| * @description Writing data from an HTTP request directly to the file system allows arbitrary file upload and might indicate a backdoor. | ||
| * @name User-controlled data in file |
Contributor
There was a problem hiding this comment.
User-controlled data written to file?
| /** | ||
| * A call that performs a request to a URL. | ||
| * | ||
| * Example: An HTTP POST request is client request that sends some |
| } | ||
|
|
||
| override DataFlow::Node getADataNode() { | ||
| none() |
Contributor
There was a problem hiding this comment.
These are none() because they're just GET requests?
| /** | ||
| * Provides taint tracking configuration for reasoning about files created from untrusted http downloads. | ||
| /** | ||
| * Provides a taint tracking configuration for reasoning about user-controlled data in files. |
Contributor
There was a problem hiding this comment.
Again, I'd emphasize that it's written to files.
Otherwise it sounds like it's looking for files in the codebase that have "user-controlled data" (which doesn't really make sense).
|
|
||
| /** | ||
| * A write to the file system using a stream. | ||
| * A read from the file system. |
Contributor
There was a problem hiding this comment.
This change looks wrong. It is indeed a write operation.
| | **Query** | **Tags** | **Purpose** | | ||
| |-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094 | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. | | ||
| | File data in outbound remote request | security, external/cwe/cwe-200 | Highligts locations where file data is sent in a remote request. Results are not shown on LGTM by default. | |
Contributor
There was a problem hiding this comment.
Highligts -> Highlights
Author
|
All comments addressed. |
asger-semmle
approved these changes
Oct 10, 2018
Contributor
asger-semmle
left a comment
asger-semmle
left a comment
There was a problem hiding this comment.
Thanks! Everything LGTM 👍
ghost
mentioned this pull request
smowton
pushed a commit
to smowton/codeql
that referenced
this pull request
Apr 16, 2022
Kotlin: Improve error catching and logging
MathiasVP
added a commit
to MathiasVP/ql
that referenced
this pull request
Dec 16, 2025
…olate-args-in-paths PS: Never hide string interpolate argument in path graphs
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
2 participants
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.
This PR polishes #132, the changes have been discussed here.
Evaluation shows that this polishing did not break performance. I plan on giving the results a brief look later.