Jax-RS: implement content-type tracking #6162
Merged
Conversation
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
smowton
force-pushed
the
smowton/feature/jax-rs-content-type-sensitivity-fixes
branch
from
506780a to
5849e8b
Compare
Also extend the routine slightly to expose multiple content types given with array notation
This follows content-type specifications across Variant-related functions and the ResponseBuilder class in order to sanitize or sink entities as appropriate.
* Expose getContentTypeString for use by tests * Use it to get constant arguments to @produces annotations * Note that text/html is xss-vulnerable (I have no idea how it ever came to expect exactly text/plain)
smowton
force-pushed
the
smowton/feature/jax-rs-content-type-sensitivity-fixes
branch
from
7d76e55 to
7f556de
Compare
Contributor
Author
|
Rebased now that the corresponding tests have landed. |
aschackmull
reviewed
Aug 2, 2021
| // e.g. MediaType.TEXT_PLAIN => text/plain | ||
| result = jaxMediaType.getName().toLowerCase().replaceAll("_", "/") | ||
| Expr getADeclaredContentTypeExpr() { | ||
| ( |
Contributor
There was a problem hiding this comment.
Drop superfluous parens.
aschackmull
reviewed
Aug 2, 2021
| * This could be an instance of `Response.ResponseBuilder`, `Variant`, `Variant.VariantListBuilder` or | ||
| * a `List<Variant>`. | ||
| * | ||
| * This routine is used to search forwards for response entities set after the content-type is configured. |
aschackmull
reviewed
Aug 2, 2021
| ) | ||
| or | ||
| // Recursive case: ordinary local dataflow | ||
| DataFlow::localFlow(getABuilderWithExplicitContentType(contentType), result) |
Contributor
There was a problem hiding this comment.
Replacing this with DataFlow::localFlowStep might yield better performance and the semantics should be the same, since this is already being transitively closed. Could you perhaps check whether you can observe a difference?
aschackmull
reviewed
Aug 2, 2021
Contributor
aschackmull
left a comment
aschackmull
left a comment
There was a problem hiding this comment.
Some inline comments, otherwise looks ok to me.
smowton
commented
Aug 3, 2021
Contributor
Author
|
@aschackmull suggestions applied |
aschackmull
approved these changes
Aug 3, 2021
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 matches https://github.com/github/codeql-securitylab/blob/main/java/ql/src/pwntester/security/RestXSS.ql in terms of tracking content-types in order to sanitize or sink entity bodies: