[AUTH-1236] Optionally support accepting HTTPS connections in addition to HTTP #472
Conversation
… mode instead of HTTP mode.
|
|
timdawborn
changed the title
timdawborn
changed the title
| httpsServer = https.createServer(httpsOptions, handler); | ||
| } | ||
|
|
||
| // Copied from https://stackoverflow.com/a/42019773/16286019 |
| @@ -113,6 +169,7 @@ export class RecordReplayServer { | |||
| request.path === "/__proxay" | |||
| ) { | |||
| res.end("Proxay!"); | |||
| return; | |||
There was a problem hiding this comment.
This is a bug fix.
| @@ -154,7 +211,12 @@ export class RecordReplayServer { | |||
| this.unloadTape(); | |||
| res.end(`Unloaded tape`); | |||
| } | |||
| return; | |||
There was a problem hiding this comment.
This is also a bug fix.
|
|
||
| // If we got here, we don't know what to do. Return a 404. | ||
| res.statusCode = 404; | ||
| res.end(`Unhandled proxay request.\n\n${JSON.stringify(request)}`); |
There was a problem hiding this comment.
This is also a bug fix. Currently if a __proxay/blah request is made for an unhandled route, the server hangs indefinitely. This adds a 404 response and logs out the request for debugging.
Holy crap, Batman! G'day, stranger. |
src/cli.ts
Outdated
| .option( | ||
| "--https-ca <filename.pem>", | ||
| "Enable HTTPS server with this CA certificate. Also requires --https-key and --https-cert." | ||
| ) |
There was a problem hiding this comment.
question: Are there any issues around invalid certificates detected by the browser (and throwing the warning screen)? A readme section would be awesome on how to use this feature.
There was a problem hiding this comment.
https://letsencrypt.org/ maybe ?
There was a problem hiding this comment.
Good point. I'll add a README around this.
| // Pause the socket | ||
| socket.pause(); | ||
|
|
||
| // Determine if this is an HTTP(s) request |
There was a problem hiding this comment.
nit: Can we encapsulate this https determination logic to a separate function/module? Makes it easier to understand the intent
| httpsServer = https.createServer(httpsOptions, handler); | ||
| } | ||
|
|
||
| // Copied from https://stackoverflow.com/a/42019773/16286019 |
There was a problem hiding this comment.
question: Why do we have the requirement to server Https and Https over the same port?
There was a problem hiding this comment.
Haha, now that is an excellent question. The short answer is cypress. The slightly longer answer is during web testing, requests are made to proxay directly and via an nginx rewrite. The former (will shortly be) HTTPS and the later is HTTP. The later can't easily be made HTTPS due to docker-compose circular dependency reasons.
There was a problem hiding this comment.
Cheers for the context!
I assume we can't point nginx -> proxay to a http port (using docker), and still have cypress -> proxay using the https port.
There was a problem hiding this comment.
It might be possible, I'd have to check again. I was having issues with that earlier but I may have solved them now.
Under certain situations, we want the
proxayserver to accept connections over HTTPS instead of over HTTP for CORS reasons. This PR adds an option to optionally supply a HTTPS key and cert on the CLI, and if both are provided, HTTPS support is enabled. Both HTTP and HTTPS run side by side in this situation.