Skip to content

Conversation

@Kwstubbs
Copy link
Contributor

This PR adds javax.servlet.http.Part and org.apache.commons.fileupload.FileItem/Stream support to RemoteFlow Sources.

@Kwstubbs
Copy link
Contributor Author

Kwstubbs commented Sep 25, 2024

I added basic tests and stubs to the library-tests folder.
I have two questions:

  1. I manually added a stub for FileItemHeadersSupport. The java compiler seems to allow the import and private field, but errors out when I attempt to use the variable within the test method. I am not sure what is wrong with my code.
  2. Should I add stubs and tests for jakarta.servlet.http.model.yml?

@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2024

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

  • Changes to framework-coverage-java.rst:
-    Java extensions,"``javax.*``, ``jakarta.*``",87,4185,90,10,4,2,1,1,4
+    Java extensions,"``javax.*``, ``jakarta.*``",101,4185,90,10,4,2,1,1,4
-    Others,"``actions.osgi``, ``antlr``, ``ch.ethz.ssh2``, ``cn.hutool.core.codec``, ``com.alibaba.com.caucho.hessian.io``, ``com.alibaba.druid.sql``, ``com.alibaba.fastjson2``, ``com.amazonaws.auth``, ``com.auth0.jwt.algorithms``, ``com.azure.identity``, ``com.caucho.burlap.io``, ``com.caucho.hessian.io``, ``com.cedarsoftware.util.io``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.esotericsoftware.yamlbeans``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.google.gson``, ``com.hubspot.jinjava``, ``com.jcraft.jsch``, ``com.microsoft.sqlserver.jdbc``, ``com.mitchellbosecke.pebble``, ``com.mongodb``, ``com.opensymphony.xwork2``, ``com.rabbitmq.client``, ``com.sshtools.j2ssh.authentication``, ``com.sun.crypto.provider``, ``com.sun.jndi.ldap``, ``com.sun.net.httpserver``, ``com.sun.net.ssl``, ``com.sun.rowset``, ``com.sun.security.auth.module``, ``com.sun.security.ntlm``, ``com.sun.security.sasl.digest``, ``com.thoughtworks.xstream``, ``com.trilead.ssh2``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``freemarker.cache``, ``freemarker.template``, ``groovy.lang``, ``groovy.text``, ``groovy.util``, ``hudson``, ``io.jsonwebtoken``, ``io.netty.bootstrap``, ``io.netty.buffer``, ``io.netty.channel``, ``io.netty.handler.codec``, ``io.netty.handler.ssl``, ``io.netty.handler.stream``, ``io.netty.resolver``, ``io.netty.util``, ``io.undertow.server.handlers.resource``, ``javafx.scene.web``, ``jenkins``, ``jodd.json``, ``liquibase.database.jvm``, ``liquibase.statement.core``, ``net.lingala.zip4j``, ``net.schmizz.sshj``, ``net.sf.json``, ``net.sf.saxon.s9api``, ``ognl``, ``okhttp3``, ``org.acegisecurity``, ``org.antlr.runtime``, ``org.apache.commons.codec``, ``org.apache.commons.compress.archivers.tar``, ``org.apache.commons.exec``, ``org.apache.commons.httpclient.util``, ``org.apache.commons.jelly``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.lang``, ``org.apache.commons.logging``, ``org.apache.commons.net``, ``org.apache.commons.ognl``, ``org.apache.cxf.catalog``, ``org.apache.cxf.common.classloader``, ``org.apache.cxf.common.jaxb``, ``org.apache.cxf.common.logging``, ``org.apache.cxf.configuration.jsse``, ``org.apache.cxf.helpers``, ``org.apache.cxf.resource``, ``org.apache.cxf.staxutils``, ``org.apache.cxf.tools.corba.utils``, ``org.apache.cxf.tools.util``, ``org.apache.cxf.transform``, ``org.apache.directory.ldap.client.api``, ``org.apache.hadoop.fs``, ``org.apache.hadoop.hive.metastore``, ``org.apache.hadoop.hive.ql.exec``, ``org.apache.hadoop.hive.ql.metadata``, ``org.apache.hc.client5.http.async.methods``, ``org.apache.hc.client5.http.classic.methods``, ``org.apache.hc.client5.http.fluent``, ``org.apache.hive.hcatalog.templeton``, ``org.apache.ibatis.jdbc``, ``org.apache.ibatis.mapping``, ``org.apache.log4j``, ``org.apache.shiro.authc``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.apache.shiro.mgt``, ``org.apache.sshd.client.session``, ``org.apache.struts.beanvalidation.validation.interceptor``, ``org.apache.struts2``, ``org.apache.tools.ant``, ``org.apache.tools.zip``, ``org.apache.velocity.app``, ``org.apache.velocity.runtime``, ``org.codehaus.cargo.container.installer``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.eclipse.jetty.client``, ``org.exolab.castor.xml``, ``org.fusesource.leveldbjni``, ``org.geogebra.web.full.main``, ``org.gradle.api.file``, ``org.hibernate``, ``org.ho.yaml``, ``org.influxdb``, ``org.jabsorb``, ``org.jboss.vfs``, ``org.jdbi.v3.core``, ``org.jenkins.ui.icon``, ``org.jenkins.ui.symbol``, ``org.jooq``, ``org.keycloak.models.map.storage``, ``org.kohsuke.stapler``, ``org.lastaflute.web``, ``org.mvel2``, ``org.openjdk.jmh.runner.options``, ``org.owasp.esapi``, ``org.pac4j.jwt.config.encryption``, ``org.pac4j.jwt.config.signature``, ``org.scijava.log``, ``org.slf4j``, ``org.thymeleaf``, ``org.xml.sax``, ``org.xmlpull.v1``, ``org.yaml.snakeyaml``, ``play.libs.ws``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``, ``retrofit2``, ``software.amazon.awssdk.transfer.s3.model``, ``sun.jvmstat.perfdata.monitor.protocol.local``, ``sun.jvmstat.perfdata.monitor.protocol.rmi``, ``sun.misc``, ``sun.net.ftp``, ``sun.net.www.protocol.http``, ``sun.security.acl``, ``sun.security.jgss.krb5``, ``sun.security.krb5``, ``sun.security.pkcs``, ``sun.security.pkcs11``, ``sun.security.provider``, ``sun.security.ssl``, ``sun.security.x509``, ``sun.tools.jconsole``",133,10525,927,140,6,22,18,,208
+    Others,"``actions.osgi``, ``antlr``, ``ch.ethz.ssh2``, ``cn.hutool.core.codec``, ``com.alibaba.com.caucho.hessian.io``, ``com.alibaba.druid.sql``, ``com.alibaba.fastjson2``, ``com.amazonaws.auth``, ``com.auth0.jwt.algorithms``, ``com.azure.identity``, ``com.caucho.burlap.io``, ``com.caucho.hessian.io``, ``com.cedarsoftware.util.io``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.esotericsoftware.yamlbeans``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.google.gson``, ``com.hubspot.jinjava``, ``com.jcraft.jsch``, ``com.microsoft.sqlserver.jdbc``, ``com.mitchellbosecke.pebble``, ``com.mongodb``, ``com.opensymphony.xwork2``, ``com.rabbitmq.client``, ``com.sshtools.j2ssh.authentication``, ``com.sun.crypto.provider``, ``com.sun.jndi.ldap``, ``com.sun.net.httpserver``, ``com.sun.net.ssl``, ``com.sun.rowset``, ``com.sun.security.auth.module``, ``com.sun.security.ntlm``, ``com.sun.security.sasl.digest``, ``com.thoughtworks.xstream``, ``com.trilead.ssh2``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``freemarker.cache``, ``freemarker.template``, ``groovy.lang``, ``groovy.text``, ``groovy.util``, ``hudson``, ``io.jsonwebtoken``, ``io.netty.bootstrap``, ``io.netty.buffer``, ``io.netty.channel``, ``io.netty.handler.codec``, ``io.netty.handler.ssl``, ``io.netty.handler.stream``, ``io.netty.resolver``, ``io.netty.util``, ``io.undertow.server.handlers.resource``, ``javafx.scene.web``, ``jenkins``, ``jodd.json``, ``liquibase.database.jvm``, ``liquibase.statement.core``, ``net.lingala.zip4j``, ``net.schmizz.sshj``, ``net.sf.json``, ``net.sf.saxon.s9api``, ``ognl``, ``okhttp3``, ``org.acegisecurity``, ``org.antlr.runtime``, ``org.apache.commons.codec``, ``org.apache.commons.compress.archivers.tar``, ``org.apache.commons.exec``, ``org.apache.commons.fileupload``, ``org.apache.commons.httpclient.util``, ``org.apache.commons.jelly``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.lang``, ``org.apache.commons.logging``, ``org.apache.commons.net``, ``org.apache.commons.ognl``, ``org.apache.cxf.catalog``, ``org.apache.cxf.common.classloader``, ``org.apache.cxf.common.jaxb``, ``org.apache.cxf.common.logging``, ``org.apache.cxf.configuration.jsse``, ``org.apache.cxf.helpers``, ``org.apache.cxf.resource``, ``org.apache.cxf.staxutils``, ``org.apache.cxf.tools.corba.utils``, ``org.apache.cxf.tools.util``, ``org.apache.cxf.transform``, ``org.apache.directory.ldap.client.api``, ``org.apache.hadoop.fs``, ``org.apache.hadoop.hive.metastore``, ``org.apache.hadoop.hive.ql.exec``, ``org.apache.hadoop.hive.ql.metadata``, ``org.apache.hc.client5.http.async.methods``, ``org.apache.hc.client5.http.classic.methods``, ``org.apache.hc.client5.http.fluent``, ``org.apache.hive.hcatalog.templeton``, ``org.apache.ibatis.jdbc``, ``org.apache.ibatis.mapping``, ``org.apache.log4j``, ``org.apache.shiro.authc``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.apache.shiro.mgt``, ``org.apache.sshd.client.session``, ``org.apache.struts.beanvalidation.validation.interceptor``, ``org.apache.struts2``, ``org.apache.tools.ant``, ``org.apache.tools.zip``, ``org.apache.velocity.app``, ``org.apache.velocity.runtime``, ``org.codehaus.cargo.container.installer``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.eclipse.jetty.client``, ``org.exolab.castor.xml``, ``org.fusesource.leveldbjni``, ``org.geogebra.web.full.main``, ``org.gradle.api.file``, ``org.hibernate``, ``org.ho.yaml``, ``org.influxdb``, ``org.jabsorb``, ``org.jboss.vfs``, ``org.jdbi.v3.core``, ``org.jenkins.ui.icon``, ``org.jenkins.ui.symbol``, ``org.jooq``, ``org.keycloak.models.map.storage``, ``org.kohsuke.stapler``, ``org.lastaflute.web``, ``org.mvel2``, ``org.openjdk.jmh.runner.options``, ``org.owasp.esapi``, ``org.pac4j.jwt.config.encryption``, ``org.pac4j.jwt.config.signature``, ``org.scijava.log``, ``org.slf4j``, ``org.thymeleaf``, ``org.xml.sax``, ``org.xmlpull.v1``, ``org.yaml.snakeyaml``, ``play.libs.ws``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``, ``retrofit2``, ``software.amazon.awssdk.transfer.s3.model``, ``sun.jvmstat.perfdata.monitor.protocol.local``, ``sun.jvmstat.perfdata.monitor.protocol.rmi``, ``sun.misc``, ``sun.net.ftp``, ``sun.net.www.protocol.http``, ``sun.security.acl``, ``sun.security.jgss.krb5``, ``sun.security.krb5``, ``sun.security.pkcs``, ``sun.security.pkcs11``, ``sun.security.provider``, ``sun.security.ssl``, ``sun.security.x509``, ``sun.tools.jconsole``",144,10529,927,140,6,22,18,,208
-    Totals,,330,26361,2656,404,16,128,33,1,409
+    Totals,,355,26365,2656,404,16,128,33,1,409
  • Changes to framework-coverage-java.csv:
- jakarta.servlet,2,19,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,,19,,
+ jakarta.servlet,2,26,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,,26,,
- javax.servlet,10,22,3,,,,,,,,,,,,,,1,,,,,,,,,,2,,,,,,,,,,3,,,2,,2,,,,,,,,,22,3,
+ javax.servlet,10,29,3,,,,,,,,,,,,,,1,,,,,,,,,,2,,,,,,,,,,3,,,2,,2,,,,,,,,,29,3,
+ org.apache.commons.fileupload,,11,4,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,11,4,

@smowton
Copy link
Contributor

smowton commented Sep 26, 2024

If you push the stub that isn't working for you I could probably shed some light.

And yes it does seem wrong that the Jakarta version of the models has become out of sync with the Javax version. Syncing the two and adding appropriate tests would be welcome.

@Kwstubbs
Copy link
Contributor Author

Kwstubbs commented Sep 27, 2024

@smowton I have found the solution to my problem. This is ready for review. 🍨

@smowton
Copy link
Contributor

smowton commented Oct 7, 2024

Thanks for this! To check: can most/all of https://github.com/github/codeql/blob/main/java/ql/src/experimental/semmle/code/java/security/FileAndFormRemoteSource.qll be removed as a consequence of this work?

@Kwstubbs
Copy link
Contributor Author

Kwstubbs commented Oct 15, 2024

@smowton I have modeled everything except parseRequest which I assume is already modeled since it returns a java.util.List<FileItem>. If someone with more Java experience could take a look to see if that would need to be modeled that would be great. I just added the value preserving steps summaryModel. I'm not sure if tests are required for those, or where they would go. If they are required, could you please direct me. I'm happy to remove or edit FileAndFormRemoteSource if you would like.

@smowton
Copy link
Contributor

smowton commented Oct 16, 2024

Suggest giving it a test anyway -- it should work; it doesn't matter if it's due to changes here or if existing models are sufficient.

To reiterate, can we move QL models from https://github.com/github/codeql/blob/main/java/ql/src/experimental/semmle/code/java/security/FileAndFormRemoteSource.qll as mentioned above?

@Kwstubbs
Copy link
Contributor Author

Kwstubbs commented Oct 22, 2024

Correct, the models from https://github.com/github/codeql/blob/main/java/ql/src/experimental/semmle/code/java/security/FileAndFormRemoteSource.qll should be modeled here so they are no longer needed. Can you specify which file I should add the tests for the summaryModels to?

@smowton
Copy link
Contributor

smowton commented Nov 12, 2024

Hi, sorry for the slow reponse-- how about adding the summary model tests to https://github.com/github/codeql/tree/main/java/ql/test/library-tests/frameworks similar to the existing test directories there for commons-lang and so on

@owen-mc
Copy link
Contributor

owen-mc commented Nov 20, 2024

java/ql/test/library-tests/frameworks/apache-commons-fileupload-1.4/test.ql is failing because the line numbers are wrong.

@Kwstubbs
Copy link
Contributor Author

@smowton @owen-mc Hi I think everything is good on this PR. Ready for official review.

@owen-mc
Copy link
Contributor

owen-mc commented Sep 26, 2025

@Kwstubbs Sorry this slipped through the cracks. If you update it then I will review it.

Copilot AI review requested due to automatic review settings October 7, 2025 05:38
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for file upload sources to Remote Data Flow Sources by adding model definitions for javax.servlet.http.Part, jakarta.servlet.http.Part, org.apache.commons.fileupload.FileItem, and org.apache.commons.fileupload.FileItemStream.

  • Adds comprehensive taint source models for Java file upload APIs
  • Includes taint flow models for Apache Commons FileUpload utility classes
  • Adds extensive test stubs for Jakarta Servlet API and Apache Commons FileUpload

Reviewed Changes

Copilot reviewed 57 out of 57 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
java/ql/lib/ext/javax.servlet.http.model.yml Adds remote source models for javax.servlet.http.Part methods
java/ql/lib/ext/jakarta.servlet.http.model.yml Adds remote source models for jakarta.servlet.http.Part methods
java/ql/lib/ext/org.apache.commons.fileupload.model.yml Adds remote source models for FileItem and FileItemStream
java/ql/lib/ext/org.apache.commons.fileupload.util.model.yml Adds taint flow models for Streams utility methods
java/ql/test/library-tests/dataflow/taintsources/FileUpload.java Adds test cases for file upload taint sources
java/ql/test/library-tests/frameworks/apache-commons-fileupload-1.4/Test.java Adds test cases for Apache Commons FileUpload taint flows
Multiple stub files Adds comprehensive test stubs for Jakarta Servlet API and Apache Commons FileUpload

@owen-mc
Copy link
Contributor

owen-mc commented Oct 15, 2025

 WARNING: A terminally deprecated method in sun.misc.Unsafe has been called
WARNING: sun.misc.Unsafe::objectFieldOffset has been called by lombok.permit.Permit
WARNING: Please consider reporting this to the maintainers of class lombok.permit.Permit
WARNING: sun.misc.Unsafe::objectFieldOffset will be removed in a future release
Exception in thread "main" Compilation errors were reported by javac.
	at com.semmle.extractor.java.JavaExtractor.main(JavaExtractor.java:999)

@Kwstubbs
Copy link
Contributor Author

@owen-mc Not sure what I can do here. Have any other of your Java PRs had this problem? If you could, could you rerun the tests just to make sure this is not a one time fluke? Thanks

@owen-mc
Copy link
Contributor

owen-mc commented Oct 29, 2025

There are two sources of test failures. I've made #20714 to fix one of them, which is unrelated to this PR. The rest are caused by an extractor crash relating to Lombok. It does seem to be related to the changes in this PR - it's when extracting the test file that you added that it crashes. I've asked if anyone knows what might be causing it.

@owen-mc
Copy link
Contributor

owen-mc commented Oct 29, 2025

I've fixed the language test problems. We can now see the integration test problems that were previously hidden 😆. They seem to have also failed on main recently, so perhaps they're flaky.

Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete the appropriate parts of https://github.com/github/codeql/blob/main/java/ql/src/experimental/semmle/code/java/security/FileAndFormRemoteSource.qll . If you add a test for parseRequest and it works then then I think you can delete the whole file.

Comment on lines +22 to +28
- ["javax.servlet.http", "Part", False, "getInputStream", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getName", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getContentType", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getHeader", "(String)", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getSubmittedFileName", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getHeaders", "(String)", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getHeaderNames", "()", "", "ReturnValue", "remote", "manual"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort in alphabetical order.

Suggested change
- ["javax.servlet.http", "Part", False, "getInputStream", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getName", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getContentType", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getHeader", "(String)", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getSubmittedFileName", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getHeaders", "(String)", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getHeaderNames", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getContentType", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getHeader", "(String)", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getHeaderNames", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getHeaders", "(String)", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getInputStream", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getName", "()", "", "ReturnValue", "remote", "manual"]
- ["javax.servlet.http", "Part", False, "getSubmittedFileName", "()", "", "ReturnValue", "remote", "manual"]

Comment on lines +11 to +12
- ["jakarta.servlet.http", "Part", True, "getHeaders", "(String)", "", "ReturnValue", "remote", "manual"]
- ["jakarta.servlet.http", "Part", True, "getHeaderNames", "()", "", "ReturnValue", "remote", "manual"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's arguable if we're calling toLower before sorting, but this is the order that the java docs has them in.

Suggested change
- ["jakarta.servlet.http", "Part", True, "getHeaders", "(String)", "", "ReturnValue", "remote", "manual"]
- ["jakarta.servlet.http", "Part", True, "getHeaderNames", "()", "", "ReturnValue", "remote", "manual"]
- ["jakarta.servlet.http", "Part", True, "getHeaderNames", "()", "", "ReturnValue", "remote", "manual"]
- ["jakarta.servlet.http", "Part", True, "getHeaders", "(String)", "", "ReturnValue", "remote", "manual"]

- ["org.apache.commons.fileupload", "FileItem", True, "getContentType", "()", "", "ReturnValue", "remote", "manual"]
- ["org.apache.commons.fileupload", "FileItem", True, "getString", "()", "", "ReturnValue", "remote", "manual"]
- ["org.apache.commons.fileupload", "FileItem", True, "getName", "()", "", "ReturnValue", "remote", "manual"]
- ["org.apache.commons.fileupload", "FileItem", True, "getName", "(String)", "", "ReturnValue", "remote", "manual"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API doesn't seem to exist. Maybe it's a typo for getString(String)? If so please add a test for it.

Suggested change
- ["org.apache.commons.fileupload", "FileItem", True, "getName", "(String)", "", "ReturnValue", "remote", "manual"]
- ["org.apache.commons.fileupload", "FileItem", True, "getString "(String)", "", "ReturnValue", "remote", "manual"]

Comment on lines +6 to +12
- ["org.apache.commons.fileupload", "FileItem", True, "getInputStream", "()", "", "ReturnValue", "remote", "manual"]
- ["org.apache.commons.fileupload", "FileItem", True, "getFieldName", "()", "", "ReturnValue", "remote", "manual"]
- ["org.apache.commons.fileupload", "FileItem", True, "getContentType", "()", "", "ReturnValue", "remote", "manual"]
- ["org.apache.commons.fileupload", "FileItem", True, "getString", "()", "", "ReturnValue", "remote", "manual"]
- ["org.apache.commons.fileupload", "FileItem", True, "getName", "()", "", "ReturnValue", "remote", "manual"]
- ["org.apache.commons.fileupload", "FileItem", True, "getName", "(String)", "", "ReturnValue", "remote", "manual"]
- ["org.apache.commons.fileupload", "FileItem", True, "get", "()", "", "ReturnValue", "remote", "manual"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sort alphabetically.

Comment on lines +6 to +9
- ["org.apache.commons.fileupload.util", "Streams", True, "copy", "(InputStream,OutputStream,boolean)", "", "Argument[0]", "Argument[1]", "taint", "manual"]
- ["org.apache.commons.fileupload.util", "Streams", True, "copy", "(InputStream,OutputStream,boolean,byte[])", "", "Argument[0]", "Argument[1]", "taint", "manual"]
- ["org.apache.commons.fileupload.util", "Streams", True, "asString", "(InputStream)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
- ["org.apache.commons.fileupload.util", "Streams", True, "asString", "(InputStream,String)", "", "Argument[0]", "ReturnValue", "taint", "manual"] No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alphabetical order please.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can't you just leave the signature column empty and not have to duplicate the models? That matches what the QL is doing in java/ql/src/experimental/semmle/code/java/security/FileAndFormRemoteSource.qll .

sink(fileItem.get()); // $ hasRemoteValueFlow
sink(fileItem.getString()); // $ hasRemoteValueFlow
sink(fileItem.getContentType()); // $ hasRemoteValueFlow
sink(fileItem.getName()); // $ hasRemoteValueFlow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you don't have tests for getInputStream and getFieldName?

Comment on lines +19 to +25
sink(filePart.getContentType()); // $ hasRemoteValueFlow
sink(filePart.getHeader("test")); // $ hasRemoteValueFlow
sink(filePart.getInputStream()); // $ hasRemoteValueFlow
sink(filePart.getHeaders("test")); // $ hasRemoteValueFlow
sink(filePart.getHeaderNames()); // $ hasRemoteValueFlow
sink(filePart.getSubmittedFileName()); // $ hasRemoteValueFlow
sink(filePart.getName()); // $ hasRemoteValueFlow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sort in the same order as the models (alphabetically). Apply this below too.


sink(fileItemStream.getFieldName()); // $ hasRemoteValueFlow
sink(fileItemStream.getName()); // $ hasRemoteValueFlow
sink(fileItemStream.openStream()); // $ hasRemoteValueFlow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you don't have a test for getContentType?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants