Filter out url parameter when a non-relative path is provided. Also a…

[Ryan-Koch]

…dds line to copy_swagger_ui.sh to copy the public/swagger-ui directory over to the build/swagger-ui directory for builds.

Description

Filter out non-relative paths provided as the url parmeter within Swagger UI. This tool has a behavior will it will try to fetch the parameter for render, so we don't want to be doing this with external resources. There is a non-trivial risk that a malicious actor may take advantage.

Setup

Go here: https://my.move.mil/api/v1/docs?url=[some external link]
See that it is loading external content.

In your local environment you can compare the same by going here while your environment is running:
http://milmovelocal:3000/api/v1/docs?url=[some external link]).

Code Review Verification Steps

• Code follows the guidelines for Logging
• The requirements listed in
  Querying the Database Safely
  have been satisfied.
• Any new migrations/schema changes:
  • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
  • Have been communicated to #dp3-engineering
  • Secure migrations have been tested using bin/run-prod-migrations
• There are no aXe warnings for UI.
• This works in IE.
• Any new client dependencies (Google Analytics, hosted libraries, CDNs, etc) have been:
  • Communicated to @willowbl00
  • Added to the list of network dependencies
• Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
• Request review from a member of a different team.
• Have the Pivotal acceptance criteria been met for this change?

References

• Pivotal story for this change

Screenshots

Bad situation:

Not bad situation:

[Ryan-Koch]

This is merely a convenience thing for future editing in local dev.

[akostibas]

I don't think this is doing what you'd hope. The -f test is for files, and you're testing a directory. Try if [ -d build/swagger-ui/ ]. This test will always evaluate to false.

https://www.tldp.org/LDP/abs/html/fto.html

[akostibas]

Also, I believe files make their way into the build/ directory by way of make client_build, which in turn runs yarn build. That copies all the files from public/ into build/.

While probably harmless, I am concerned that adding this step in here could confuse the expectations around the build process (what puts things into build/) in the future. I'd probably not add this unless the time savings of not needing to use make client_build for this purpose is significant.

[akostibas]

I don't think this is doing what you'd hope. The -f test is for files, and you're testing a directory. Try if [ -d build/swagger-ui/ ]. This test will always evaluate to false.

https://www.tldp.org/LDP/abs/html/fto.html

[akostibas]

Also, I believe files make their way into the build/ directory by way of make client_build, which in turn runs yarn build. That copies all the files from public/ into build/.

While probably harmless, I am concerned that adding this step in here could confuse the expectations around the build process (what puts things into build/) in the future. I'd probably not add this unless the time savings of not needing to use make client_build for this purpose is significant.

[akostibas]

Instead of silently redirecting to a known good URL, it might be better to set URL to null.

Since nobody should be passing in a ?url=... query string in the first place, the only way someone would get here is if they were either 1) tricked, or 2) specifically trying to use the query string. In either case it's probably better to have it result in an indication that things aren't working.

[Ryan-Koch]

That seems reasonable. Using null yields an error state that looks like the attached screenshot.

I'll push this commit up. Does this reflect the sort of 'something is wrong' reaction that you were thinking of? I'm also going to go ahead and remove the convenience line mentioned in the prior comments.

[akostibas]

Yep, that is what I was hoping for!

[akostibas]

LGTM!