Use a safer function explicitly sharing credentials with same origin. #595
Conversation
Prevent not to share the credentials with the XHR request in old browser versions. Check the compatibility list ... https://developer.mozilla.org/en-US/docs/Web/API/Request/credentials#Browser_compatibility
|
First of all, thanks a lot for you contribution and keen eye, @juazugas !!! We truly appreciate your interest! |
| @@ -57,10 +57,15 @@ document.addEventListener("DOMContentLoaded", function(e) { | |||
| opt.text = val; | |||
| return selectElem.appendChild(opt); | |||
| }; | |||
|
|
|||
| function safe_fetch(url, init) { | |||
There was a problem hiding this comment.
You could call the function customFetch. It seems the init arg is not needed, right?
Also, you could send a second arg named with default value like options = CUSTOM_FETCH_OPTIONS and define that const before like:
const CUSTOM_FETCH_OPTIONS = { credentials: "same-origin" }
I know this will be refactor in the (hopefully near) future, but that way would prepare the fn to a possible minor refactor accepting more request options.
There was a problem hiding this comment.
I think the idea is to hardcode credentials: "same-origin" in the fetch call. Adding this options arg would make the method vulnerable again.
There was a problem hiding this comment.
if we want to make it "closed" we just pass the options in the original fetch call.
| @@ -57,10 +57,15 @@ document.addEventListener("DOMContentLoaded", function(e) { | |||
| opt.text = val; | |||
| return selectElem.appendChild(opt); | |||
| }; | |||
|
|
|||
| function safe_fetch(url, init) { | |||
| const tempRequest = new Request(url, init); | |||
There was a problem hiding this comment.
Same here in the tempRequest, is any specific reason why to use the Request interface of Fetch API and not using fetch directly? I feel I'm missing something.
|
|
||
| function safe_fetch(url, init) { | ||
| const tempRequest = new Request(url, init); | ||
| return fetch(tempRequest, { credentials: "same-origin" }); |
There was a problem hiding this comment.
and here would be return fetch(url, options)
|
|
||
| function fetchData(url, type) { | ||
| if ('fetch' in window) { | ||
| fetch(url) |
There was a problem hiding this comment.
or just pass the options here
There was a problem hiding this comment.
Agree, perhaps the simplest is better here.
There was a problem hiding this comment.
I agree, we can pass the credentials here, is a lot simpler:
fetch(url, { credentials: 'same-origin' })
There was a problem hiding this comment.
Ok, since there is only one point where it is used.
There was a problem hiding this comment.
Thanks @juazugas
In fact, in other parts of the app we are already using the fetch polyfill.
We are incorporating the polyfill step by step, and this file is one of the next ones to be updated.
Codecov Report
@@ Coverage Diff @@
## master #595 +/- ##
=======================================
Coverage 92.74% 92.74%
=======================================
Files 2355 2355
Lines 76252 76252
=======================================
Hits 70718 70718
Misses 5534 5534Continue to review full report at Codecov.
|
app/assets/javascripts/provider/admin/apiconfig/services/service_discovery.js
Outdated
Show resolved
Hide resolved
|
Closed in favor of #750 |
What this PR does / why we need it:
Explicitly declare to share the credentials with the same-origin in the XHR request to the server.
The credentials is not shared by default in older browser versions, ...
check the compatibility list ...
https://developer.mozilla.org/en-US/docs/Web/API/Request/credentials#Browser_compatibility
Which issue(s) this PR fixes
fixes #594
Verification steps
Special notes for your reviewer: