feat: httpsRequired flag #301
Conversation
| @@ -25,6 +25,7 @@ const config = { | |||
| privateAPIPort: Number(process.env.PRIVATE_API_PORT) || 8081, | |||
| version: process.env.VERSION ?? Version.V1, | |||
| log_level: process.env.LOG_LEVEL ?? 'INFO', | |||
| httpsRequired: process.env.HTTPS_REQUIRED === 'true', | |||
There was a problem hiding this comment.
Default set to false. Added a comment in this file to indicate why.
| @@ -96,8 +96,8 @@ describe('payIdToUrl', function (): void { | |||
| describe('urlToPayId', function (): void { | |||
| it('throws an error on inputs that clearly are not PayID URLs', function (): void { | |||
| // GIVEN a badly formed PayID URL (no leading https://) | |||
| const url = 'http://hansbergren.example.com' | |||
| const expectedErrorMessage = 'Bad input. PayID URLs must be HTTPS.' | |||
| const url = 'example.com+alice' | |||
There was a problem hiding this comment.
| const url = 'example.com+alice' | |
| const url = 'example.com/alice' |
There was a problem hiding this comment.
This test says: 'throws an error on inputs that clearly are not PayID URLs'
I didn't think that http fell into that category, but I also thought this was a good test
So I kept it by making it something that was clearly not a PayID url
And tested http down below
| * | ||
| * NOTE: The defaults are developer defaults. This configuration is NOT a valid | ||
| * production configuration. Please refer to example.production.env for | ||
| * reference. |
There was a problem hiding this comment.
Should you add httpsRequired to example.production.env?
There was a problem hiding this comment.
Oh, I didn't realize example.production.env already had the HTTPS_REQUIRED env var in it.
test/unit/utils.test.ts
Outdated
| // WHEN we attempt converting it to a PayID | ||
| const actualPayId = urlToPayId(url, false) | ||
|
|
||
| // THEN we get our expected error |
There was a problem hiding this comment.
Updated these test comments.
There was a problem hiding this comment.
I think you may have forgotten to push your new test comments.
There was a problem hiding this comment.
Haha, the comments above that were messed up so I fixed all of those, but it seems the one you actually commented on I didn’t fix
Should be fixed now. Also the comments on other tests that were already there were incorrect, so fixed those too.
| throw new Error('Bad input. PayID URLs must be HTTPS.') | ||
| } | ||
|
|
||
| if (!url.startsWith(HTTPS) && !url.startsWith(HTTP)) { |
There was a problem hiding this comment.
Can we break this into 2 errors indicating to the client if the server is requiring HTTP or HTTPS?
if (!url.startsWith(HTTPS) && httpsRequired) {
...
} else if (!url.startsWith(HTTP) && !httpsRequired) {
...
}
There was a problem hiding this comment.
That if statement is slightly different logic than what Dino has.
In general, if a website requires HTTPS, then you can't connect to it over http. But some websites have a SSL certificate but still allow standard http connections.
So if httpsRequired === false, then both HTTP and HTTPS are valid, right?
There was a problem hiding this comment.
So in the else case, we wouldn't return an error, but rather continue as normal because:
if the client requires https, you can only use https
if the client does not require https, you can use either http or https
There was a problem hiding this comment.
whoops, didn't refresh and re-answered. thanks @hbergren
* feat: add httpsRequired to config * fix: code comment * feat: urlToPayID handles http and https * test: tests for https/http flag * fix: getPaymentInfo uses req.protocol * docs: developer defaults comment * refactor: rename example .env * fix: update test comments * fix: more incorrect code comments
High Level Overview of Change
NOTE: Breaking out my URL validation PR into various self-contained PRs as it
was getting too big.
Adds
httpsRequiredflag to configuration.Updates getPaymentInfo middleware to use
req.protocolto test theimplementation.
Add unit tests for urlToPayId to make sure this works.
Context of Change
Currently, we are hardcoding
httpsfor every PayId, and not actually checkingthe request protocol.
This could be dangerous, as it means the PayID code claims to use
https, butsupports the insecure
http.By adding a flag, and updating the relevant code, we can get secure PayID for
production (
https), and insecure PayID for local development (http).Type of Change
Before / After
Before: All PayIDs automatically "upgraded" to
httpsAfter: Use
req.protocoland check againsthttpsorhttp, depending onconfig
Test Plan
Added unit tests for urlToPayId that covers usage of the new configuration.