-
Notifications
You must be signed in to change notification settings - Fork 16
add metadata parameter to CodeArtifact api #109
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,7 @@ | |
interface CodeArtifactClient { | ||
|
||
/** | ||
* Transfers a file from the file system to Contrast Scan to create a new code artifact for | ||
* Transfers an artifact from the file system to Contrast Scan to create a new code artifact for | ||
* analysis. | ||
* | ||
* @param projectId ID of the project to which the code artifact belongs | ||
|
@@ -51,4 +51,22 @@ interface CodeArtifactClient { | |
* @throws ServerResponseException when Contrast API returns a response that cannot be understood | ||
*/ | ||
CodeArtifactInner upload(String projectId, Path file) throws IOException; | ||
|
||
/** | ||
* Transfers artifact and prescan metadata from the file system to Contrast Scan to create a new | ||
* code artifact for analysis. | ||
* | ||
* <p>Prescan metadata will allow the scanner to produce more detailed finding reports. | ||
* | ||
* @param projectId ID of the project to which the code artifact belongs | ||
* @param file the file to upload | ||
* @param metadata the prescan metadata to upload with the file artifact. | ||
* @return new {@link CodeArtifactInner} from Contrast API | ||
* @throws IOException when an IO error occurs while making the request to the Contrast API | ||
* @throws UnauthorizedException when Contrast rejects the credentials used to send the request | ||
* @throws ResourceNotFoundException when the requested resource does not exist | ||
* @throws HttpResponseException when Contrast rejects this request with an error code | ||
* @throws ServerResponseException when Contrast API returns a response that cannot be understood | ||
*/ | ||
CodeArtifactInner upload(String projectId, Path file, Path metadata) throws IOException; | ||
seschis marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This API asks that the user provide the metadata JSON as a file. Maybe passing the metadata as a file is the only use case we have now (i.e. the Maven plugin), but it doesn't feel like the most flexible assumption for the SDK. I would expect the SDK to accept the metadata as a Java object that describes the data structure, and I would expect the SDK to marshal that object to JSON for me. Are the fields in the metadata JSON object well known, or is this more of an open-ended bag of key-value pairs? If it's the former, then we should define a new class for it. If it's the latter, then maybe all we need is a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the format of the data is defined here: https://github.com/Contrast-Security-OSS/contrast-scan-prescan/blob/master/src/main/resources/schema/scan-input-metadata-schema-1.0.0.json I don't anticipate it changing from json, but I can't say it never will. I also don't know how the content of the data will be required to change over time. The main purpose of the prescan metadata is to allow the engine to generate physical absolute paths to files in its sarif report which GitHub uses as part of preview renderings when displaying the sarif findings. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand. Knowing that has a well-defined schema, I feel more strongly that we should make a corresponding Java class to hold this data, but I don't fully understand what expectation you have for users who wan to use this API. How do they know how to generate this file? |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,17 @@ final class CodeArtifactClientImpl implements CodeArtifactClient { | |
|
||
@Override | ||
public CodeArtifactInner upload(final String projectId, final Path file) throws IOException { | ||
return sendRequest(projectId, file, null); | ||
} | ||
|
||
@Override | ||
public CodeArtifactInner upload(final String projectId, final Path file, final Path metadata) | ||
throws IOException { | ||
return sendRequest(projectId, file, Objects.requireNonNull(metadata)); | ||
} | ||
|
||
private CodeArtifactInner sendRequest( | ||
final String projectId, final Path file, final Path metadata) throws IOException { | ||
final String uri = | ||
contrast.getRestApiURL() | ||
+ new URIBuilder() | ||
|
@@ -66,9 +77,9 @@ public CodeArtifactInner upload(final String projectId, final Path file) throws | |
"code-artifacts") | ||
.toURIString(); | ||
final String boundary = "ContrastFormBoundary" + ThreadLocalRandom.current().nextLong(); | ||
final String header = | ||
"--" | ||
+ boundary | ||
final String boundaryMarker = CRLF + "--" + boundary; | ||
final String filenameSection = | ||
boundaryMarker | ||
+ CRLF | ||
+ "Content-Disposition: form-data; name=\"filename\"; filename=\"" | ||
+ file.getFileName().toString() | ||
|
@@ -80,8 +91,28 @@ public CodeArtifactInner upload(final String projectId, final Path file) throws | |
+ "Content-Transfer-Encoding: binary" | ||
+ CRLF | ||
+ CRLF; | ||
final String footer = CRLF + "--" + boundary + "--" + CRLF; | ||
final long contentLength = header.length() + Files.size(file) + footer.length(); | ||
final String metadataSection = | ||
metadata != null | ||
? boundaryMarker | ||
+ CRLF | ||
+ "Content-Disposition: form-data; name=\"metadata\"; filename=\"" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we know this form is of type application/json, shouldn't we send it as such instead of sending it as a file? This relates to my question in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this gets back to the API design of the REST endpoint which I did not design. Let me see if I can provide some clarity on a few of your questions for trying to figure the best way to proceed. The JSON spec I linked to (https://github.com/Contrast-Security-OSS/contrast-scan-prescan/blob/master/src/main/resources/schema/scan-input-metadata-schema-1.0.0.json) is an ingest specification for the scanner engine itself. It's saying, that if you want to provide the scanner engine some "prescan data" this is the format it will accept it in. Its published this way because I just wanted a specification for the input format rather than telling someone, "go read the code", and I also wanted to use code generators like jsonschema2pojo, to automatically generate the libraries to handle the data models of the scanner's prescan ingest format.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this SDK an example of such a "contrast tool", or does the SDK user have to use another tool to generate the file first before they can use the SDK to include in their new code artifacts? |
||
+ metadata.getFileName().toString() | ||
+ '"' | ||
+ CRLF | ||
+ "Content-Type: " | ||
+ determineMime(metadata) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we determine the MIME this way if we always know that it is JSON? |
||
+ CRLF | ||
+ "Content-Transfer-Encoding: binary" | ||
+ CRLF | ||
+ CRLF | ||
: ""; | ||
|
||
final String footer = boundaryMarker + "--" + CRLF; | ||
long contentLength = filenameSection.length() + Files.size(file); | ||
if (metadata != null) { | ||
contentLength += metadataSection.length() + Files.size(metadata); | ||
} | ||
contentLength += footer.length(); | ||
|
||
final HttpURLConnection connection = contrast.makeConnection(uri, "POST"); | ||
connection.setDoOutput(true); | ||
|
@@ -91,9 +122,14 @@ public CodeArtifactInner upload(final String projectId, final Path file) throws | |
try (OutputStream os = connection.getOutputStream(); | ||
PrintWriter writer = | ||
new PrintWriter(new OutputStreamWriter(os, StandardCharsets.US_ASCII), true)) { | ||
writer.append(header).flush(); | ||
writer.append(filenameSection).flush(); | ||
Files.copy(file, os); | ||
os.flush(); | ||
if (metadata != null) { | ||
writer.append(metadataSection).flush(); | ||
Files.copy(metadata, os); | ||
os.flush(); | ||
} | ||
writer.append(footer).flush(); | ||
} | ||
final int code = connection.getResponseCode(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,11 @@ public String filename() { | |
return inner.filename(); | ||
} | ||
|
||
@Override | ||
public String metadata() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the metadata is a String, does that imply that the SDK user is not intended to parse this structured data; rather, they should treat it as an opaque box? Specifically, is this JSON-encoded JSON inside this JSON There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, I always considered the generation of prescan data something that was external to the SDK.... but it probably would be more user-friendly to allow the SDK user to call a function that generated prescan data for them.... or perhaps just do it transparently. |
||
return inner.metadata(); | ||
} | ||
|
||
@Override | ||
public Instant createdTime() { | ||
return inner.createdTime(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
* #L% | ||
*/ | ||
|
||
import com.contrastsecurity.sdk.internal.Nullable; | ||
import com.google.auto.value.AutoValue; | ||
import java.time.Instant; | ||
|
||
|
@@ -44,6 +45,10 @@ static Builder builder() { | |
/** @return filename */ | ||
abstract String filename(); | ||
|
||
/** @return metadata filename */ | ||
seschis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
@Nullable | ||
abstract String metadata(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this imply that the user can retrieve the metadata filename, but not the metadata? What would the user do with the filename? |
||
|
||
/** @return time at which the code artifact was uploaded to Contrast Scan */ | ||
abstract Instant createdTime(); | ||
|
||
|
@@ -63,6 +68,9 @@ abstract static class Builder { | |
/** @see CodeArtifactInner#filename() */ | ||
abstract Builder filename(String value); | ||
|
||
/** @see CodeArtifactInner#metadata() */ | ||
abstract Builder metadata(String value); | ||
|
||
/** @see CodeArtifactInner#createdTime() */ | ||
abstract Builder createdTime(Instant value); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,6 +62,23 @@ interface Factory { | |
*/ | ||
CodeArtifact upload(Path file, String name) throws IOException; | ||
|
||
/** | ||
* Transfers a file from the file system to Contrast Scan to create a new code artifact for static | ||
* analysis. | ||
* | ||
* @param file the code artifact to upload | ||
* @param name the name of the code artifact | ||
* @param metadata the path of the prescan data file to upload | ||
* @param metaname the name of the prescan data file | ||
* @return new {@link CodeArtifact} from Contrast | ||
* @throws IOException when an IO error occurs while making the request to the Contrast API | ||
* @throws UnauthorizedException when Contrast rejects the credentials used to send the request | ||
* @throws ResourceNotFoundException when the requested resource does not exist | ||
* @throws HttpResponseException when Contrast rejects this request with an error code | ||
* @throws ServerResponseException when Contrast API returns a response that cannot be understood | ||
*/ | ||
CodeArtifact upload(Path file, String name, Path metadata, String metaname) throws IOException; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iirc the filename of the upload is reflected in the UI, and that's why we give the user the capability to set that. I don't see a use case for allowing the user to set the name of the metadata file. I'd argue we take that capability out. Also, I'm still questioning whether the metadata should be sent as a file vs as JSON, but I'll continue that discussion in another thread. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, I see now. I can take it out. Before I do any work on it yet though I'll wait until we resolve the fundamental API design questions you have in this area. |
||
|
||
/** | ||
* Transfers a file from the file system to Contrast Scan to create a new code artifact for static | ||
* analysis. | ||
|
@@ -75,4 +92,19 @@ interface Factory { | |
* @throws ServerResponseException when Contrast API returns a response that cannot be understood | ||
*/ | ||
CodeArtifact upload(Path file) throws IOException; | ||
|
||
/** | ||
* Transfers a file from the file system to Contrast Scan to create a new code artifact for static | ||
* analysis. | ||
* | ||
* @param file the code artifact to upload | ||
* @param metadata the path of the prescan data file to upload | ||
* @return new {@link CodeArtifact} from Contrast | ||
* @throws IOException when an IO error occurs while making the request to the Contrast API | ||
* @throws UnauthorizedException when Contrast rejects the credentials used to send the request | ||
* @throws ResourceNotFoundException when the requested resource does not exist | ||
* @throws HttpResponseException when Contrast rejects this request with an error code | ||
* @throws ServerResponseException when Contrast API returns a response that cannot be understood | ||
*/ | ||
CodeArtifact upload(Path file, Path metadata) throws IOException; | ||
seschis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Uh oh!
There was an error while loading. Please reload this page.